[Refactor] 장소 운영시간 데이터 이전 - #362
Open
ImHyungsuk wants to merge 19 commits into
Open
ImHyungsuk wants to merge 19 commits into
ImHyungsuk wants to merge 19 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough장소의 자유형 운영시간을 요일별 슬롯으로 파싱하고 Changes운영시간 정규화 및 이관
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Places as places
participant Migration as V20__Normalize_Operation_Hours
participant Parser as OperationHourParser
participant Slots as operation_time_slots
Migration->>Places: opening_hours 조회
Places-->>Migration: 장소별 운영시간 반환
Migration->>Parser: parse(rawText)
Parser-->>Migration: 운영시간 슬롯 목록 반환
Migration->>Slots: 슬롯 배치 삽입
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.java (1)
27-36: LAZY 연관관계는toString에서 제외하세요.
@ToString이place까지 출력하면 LAZY 로딩을 유발하거나 양방향 연관이 추가될 때 순환 출력 위험이 생깁니다.place를 제외하거나 엔티티의@ToString을 제거해 주세요.🧹 제안 수정
-@ToString +@ToString(exclude = "place")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.java` around lines 27 - 36, The class-level `@ToString` on OperationTimeSlot currently includes the LAZY-loaded field place which can trigger lazy loading or cause recursive toString cycles; remove the risk by excluding place from the generated toString (e.g., remove `@ToString` from the class or use Lombok exclusion such as `@ToString.Exclude` on the place field or `@ToString`(exclude="place")) so the place association is not printed.src/main/java/db/migration/V17__Normalize_Operation_Hours.java (1)
15-16: Flyway 마이그레이션 로직을 고정된 구현으로 분리하는 것을 권장합니다.
V17이 애플리케이션의OperationHourParser/DTO에 직접 의존하면, 나중에 파서가 리팩터링될 때 새 환경의 과거 마이그레이션 결과가 바뀌거나 깨질 수 있습니다. 마이그레이션 내부 전용 파서로 복사하거나 변경 불가능한 테스트를 붙여 주세요.Also applies to: 31-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/db/migration/V17__Normalize_Operation_Hours.java` around lines 15 - 16, The migration class V17__Normalize_Operation_Hours currently depends on runtime classes OperationHourParser and OperationTimeDto; copy a self-contained, immutable parser and DTO implementation into the migration file (e.g., private static classes within V17__Normalize_Operation_Hours) and update the migration to use those local symbols instead of org.sopt.solply_server.domain.* so the migration is stable if those runtime classes change; also replicate the same approach for the other referenced imports on lines 31-32 and add a small migration-focused test that validates the parser behavior against known inputs to lock the expected transformation.src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java (1)
153-153: 파서 내부 디버그 로그 레벨을 낮춰주세요.마이그레이션 중 모든 라인마다
INFO로그가 남습니다. 운영 배포 로그를 오염시키지 않도록 제거하거나debug로 낮추는 편이 좋습니다.🧹 제안 수정
- log.info("cleanLine:{}", cleanLine); + log.debug("cleanLine:{}", cleanLine);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java` at line 153, The parser currently logs every processed line at INFO level (log.info("cleanLine:{}", cleanLine)) which floods production logs; change that call to a lower level (e.g., log.debug("cleanLine:{}", cleanLine)) or remove it from OperationHourParser so line-by-line parsing only emits debug-level diagnostics.src/main/java/org/sopt/solply_server/domain/place/dto/OperationTimeDto.java (1)
24-28:endTime변경 시endNextDay가 함께 갱신되도록 묶어주세요.
setEndTime()만 호출하면endNextDay가 이전 값으로 남을 수 있습니다. 실제로 브레이크타임 분리 로직이endTime을 바꾸므로, 두 필드를 한 메서드에서 함께 갱신하는 방식이 안전합니다.🛠️ 제안 수정 방향
- `@Setter` private LocalTime endTime; - `@Setter` private boolean endNextDay; + + public void changeEndTime(LocalTime endTime) { + this.endTime = endTime; + this.endNextDay = startTime != null && !startTime.isBefore(endTime); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/place/dto/OperationTimeDto.java` around lines 24 - 28, OperationTimeDto currently uses Lombok-generated setters for endTime and endNextDay which allows endTime to change without updating endNextDay; remove the Lombok `@Setter` for endTime (and optionally endNextDay) and implement a custom setEndTime method in OperationTimeDto that accepts the new LocalTime (or new LocalTime plus boolean) and updates both the endTime and endNextDay fields together (or recomputes endNextDay using the same logic your break-time separation uses), ensuring any call to setEndTime(...) also sets endNextDay consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/db/migration/V17__Normalize_Operation_Hours.java`:
- Around line 26-27: The query in V17__Normalize_Operation_Hours limits
migration by id (the SQL string in the try-with-resources that calls
executeQuery("SELECT id, opening_hours FROM places WHERE id <= 320")), which can
skip rows in production; change the SELECT to pick rows based on the original
column instead (e.g. WHERE opening_hours IS NOT NULL or WHERE opening_hours <>
'') so all rows with opening_hours are processed, and update the SQL string used
in the Statement in V17__Normalize_Operation_Hours accordingly.
- Around line 73-74: Remove the temporary per-place debug log: delete the
conditional block that checks placeId == 59 and the log.info call (the code
referencing placeId, log.info and slot) from V17__Normalize_Operation_Hours (the
migration class), so no per-place debug output remains in the migration; if any
logging is needed keep only a general, correctly formatted log (no hard-coded
placeId or malformed format string).
In
`@src/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.java`:
- Around line 29-80: The factory methods createDayOn and createDayOff produce
entities missing required NOT NULL fields and timestamp inheritance; update the
class to extend BaseTimeEntity (to provide createdAt/updatedAt), make the place
field non-nullable, and change the factory signatures to accept a Place place
parameter and an endNextDay boolean (or default false) so createDayOn sets
s.place = place and s.endNextDay = endNextDay (and likewise createDayOff sets
s.place and s.endNextDay), ensuring OperationTimeSlot instances have place and
endNextDay populated before persisting.
In
`@src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java`:
- Around line 110-122: splitExistingSlots currently splits every active slot
into morning and afternoon pieces regardless of whether bStart/bEnd fall inside
that slot; change it to first check that bStart is after slot.getStartTime() and
bEnd is before the slot's originalEnd (taking into account next-day flags)
before mutating the slot. Only when the break interval lies strictly within the
existing slot create the morning piece by setting slot.setEndTime(bStart) and
also update the slot's endNextDay flag appropriately, then add the afternoon
piece via OperationTimeDto.createDayOn(slot.getDayOfWeek(), bEnd, originalEnd)
ensuring the new slot's endNextDay is set to match whether originalEnd crossed
midnight. Keep all other checks (days.contains(slot.getDayOfWeek()) &&
!slot.isDayOff()) but add the in-slot boundary checks and endNextDay updates to
prevent inverted or next-day ranges.
- Around line 125-135: The applyLastOrder method currently checks last-order
times by simple isAfter/isBefore against slot.getStartTime() and
slot.getEndTime(), which misses cases where endNextDay is true (overnight slots
like 18:00–02:00); update applyLastOrder (and use or add a time-in-range helper,
e.g., isTimeWithinSlotConsideringEndNextDay) to test each LocalTime lo against
the slot using startTime, endTime and slot.isEndNextDay() (or equivalent) so
that lo is considered inside the slot even when endNextDay is true; locate
OperationTimeDto getters (getStartTime, getEndTime, isDayOff, isEndNextDay) and
replace the current lo.isAfter/lo.isBefore checks with the helper call so
slot.setLastOrderTime(lo) runs for overnight ranges as well.
- Around line 179-188: In extractTimes, the current 24:XX handling drops minutes
by forcing any "24:" start to "00:00"; update the conversion so that "24:00"
becomes "00:00" but other "24:mm" keep the minute portion (e.g., "24:30" ->
"00:30") before parsing with LocalTime.parse (preserving the existing
zero-padding logic that prepends "0" when timeStr.length() == 4); ensure this
change is made inside the extractTimes method where the Matcher and timeStr are
handled.
In `@src/main/java/org/sopt/solply_server/global/entity/DayOfWeek.java`:
- Around line 12-18: Replace the throw of IllegalArgumentException in
DayOfWeek.from(int) with the project's BusinessException using an appropriate
ErrorCode; specifically, throw new
BusinessException(ErrorCode.INVALID_DAY_OF_WEEK) (add the INVALID_DAY_OF_WEEK
enum constant to ErrorCode if it doesn't exist) and update imports/usages
accordingly so the method consistently uses the project's
BusinessException/ErrorCode flow that integrates with GlobalExceptionHandler.
In `@src/main/java/org/sopt/solply_server/global/entity/DayOfWeekConverter.java`:
- Around line 9-15: The converter methods in DayOfWeekConverter are not
null-safe and will NPE when autoApply passes null; update
convertToDatabaseColumn(DayOfWeek dayOfWeek) to return null if dayOfWeek is null
(otherwise return dayOfWeek.getValue()) and update
convertToEntityAttribute(Integer integer) to return null if integer is null
(otherwise return DayOfWeek.from(integer)), so both conversions defensively
handle null inputs.
In
`@src/main/resources/db/migration/V16__create_operation_hour_slot_from_place.sql`:
- Line 21: The UNIQUE constraint uk_place_day currently on (place_id,
day_of_week, start_time) allows multiple NULL start_time rows (day-off slots);
update the schema so day-off rows are deduplicated by including is_day_off in
the uniqueness rule or by adding a generated column that normalizes NULL
start_time and using that in the UNIQUE key. Concretely, modify the constraint
referenced as uk_place_day (or create a new UNIQUE key) to include is_day_off
(e.g. UNIQUE(place_id, day_of_week, is_day_off, start_time) or use a generated
column like normalized_start_time = COALESCE(start_time, <sentinel>) and enforce
UNIQUE(place_id, day_of_week, is_day_off, normalized_start_time) so that rows
with start_time = NULL cannot be inserted multiple times for the same place/day.
---
Nitpick comments:
In `@src/main/java/db/migration/V17__Normalize_Operation_Hours.java`:
- Around line 15-16: The migration class V17__Normalize_Operation_Hours
currently depends on runtime classes OperationHourParser and OperationTimeDto;
copy a self-contained, immutable parser and DTO implementation into the
migration file (e.g., private static classes within
V17__Normalize_Operation_Hours) and update the migration to use those local
symbols instead of org.sopt.solply_server.domain.* so the migration is stable if
those runtime classes change; also replicate the same approach for the other
referenced imports on lines 31-32 and add a small migration-focused test that
validates the parser behavior against known inputs to lock the expected
transformation.
In `@src/main/java/org/sopt/solply_server/domain/place/dto/OperationTimeDto.java`:
- Around line 24-28: OperationTimeDto currently uses Lombok-generated setters
for endTime and endNextDay which allows endTime to change without updating
endNextDay; remove the Lombok `@Setter` for endTime (and optionally endNextDay)
and implement a custom setEndTime method in OperationTimeDto that accepts the
new LocalTime (or new LocalTime plus boolean) and updates both the endTime and
endNextDay fields together (or recomputes endNextDay using the same logic your
break-time separation uses), ensuring any call to setEndTime(...) also sets
endNextDay consistently.
In
`@src/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.java`:
- Around line 27-36: The class-level `@ToString` on OperationTimeSlot currently
includes the LAZY-loaded field place which can trigger lazy loading or cause
recursive toString cycles; remove the risk by excluding place from the generated
toString (e.g., remove `@ToString` from the class or use Lombok exclusion such as
`@ToString.Exclude` on the place field or `@ToString`(exclude="place")) so the place
association is not printed.
In
`@src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java`:
- Line 153: The parser currently logs every processed line at INFO level
(log.info("cleanLine:{}", cleanLine)) which floods production logs; change that
call to a lower level (e.g., log.debug("cleanLine:{}", cleanLine)) or remove it
from OperationHourParser so line-by-line parsing only emits debug-level
diagnostics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fd4770ea-e1f0-4fa3-88cc-7d81db10a4d8
📒 Files selected for processing (8)
src/main/java/db/migration/V17__Normalize_Operation_Hours.javasrc/main/java/org/sopt/solply_server/domain/place/dto/OperationTimeDto.javasrc/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.javasrc/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.javasrc/main/java/org/sopt/solply_server/global/entity/DayOfWeek.javasrc/main/java/org/sopt/solply_server/global/entity/DayOfWeekConverter.javasrc/main/resources/db/migration/V15__fix_invalid_opening_hours_places.sqlsrc/main/resources/db/migration/V16__create_operation_hour_slot_from_place.sql
Comment on lines
+26
to
+27
| try (Statement select = connection.createStatement(); | ||
| ResultSet rs = select.executeQuery("SELECT id, opening_hours FROM places WHERE id <= 320")) { |
There was a problem hiding this comment.
마이그레이션 대상을 ID로 제한하지 마세요.
id <= 320은 운영 DB에 이미 더 큰 ID의 장소가 있으면 운영시간 정규화에서 누락시킵니다. 원본 컬럼 기준으로 대상 행을 고르는 쪽이 안전합니다.
🛠️ 제안 수정
try (Statement select = connection.createStatement();
- ResultSet rs = select.executeQuery("SELECT id, opening_hours FROM places WHERE id <= 320")) {
+ ResultSet rs = select.executeQuery(
+ "SELECT id, opening_hours FROM places WHERE opening_hours IS NOT NULL AND TRIM(opening_hours) <> ''"
+ )) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try (Statement select = connection.createStatement(); | |
| ResultSet rs = select.executeQuery("SELECT id, opening_hours FROM places WHERE id <= 320")) { | |
| try (Statement select = connection.createStatement(); | |
| ResultSet rs = select.executeQuery( | |
| "SELECT id, opening_hours FROM places WHERE opening_hours IS NOT NULL AND TRIM(opening_hours) <> ''" | |
| )) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/db/migration/V17__Normalize_Operation_Hours.java` around lines
26 - 27, The query in V17__Normalize_Operation_Hours limits migration by id (the
SQL string in the try-with-resources that calls executeQuery("SELECT id,
opening_hours FROM places WHERE id <= 320")), which can skip rows in production;
change the SELECT to pick rows based on the original column instead (e.g. WHERE
opening_hours IS NOT NULL or WHERE opening_hours <> '') so all rows with
opening_hours are processed, and update the SQL string used in the Statement in
V17__Normalize_Operation_Hours accordingly.
There was a problem hiding this comment.
@ImHyungsuk, 이해했습니다. id <= 320 제한이 초기 데이터셋에 해당하는 장소만을 대상으로 의도적으로 마이그레이션하는 것임을 확인했습니다. 제안을 철회하겠습니다.
✏️ Learnings added
Learnt from: ImHyungsuk
Repo: SOLPLY/SOLPLY-SERVER PR: 362
File: src/main/java/db/migration/V17__Normalize_Operation_Hours.java:26-27
Timestamp: 2026-04-18T09:28:56.268Z
Learning: In `src/main/java/db/migration/V17__Normalize_Operation_Hours.java`, the SQL query `WHERE id <= 320` is intentional. The migration is designed to only process the initial dataset of places (IDs up to 320) and is not meant to cover all rows with non-null opening_hours.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+110
to
+122
| private void splitExistingSlots(List<OperationTimeDto> result, List<Integer> days, LocalTime bStart, | ||
| LocalTime bEnd) { | ||
| List<OperationTimeDto> newSlots = new ArrayList<>(); | ||
| Iterator<OperationTimeDto> it = result.iterator(); | ||
| while (it.hasNext()) { | ||
| OperationTimeDto slot = it.next(); | ||
| if (days.contains(slot.getDayOfWeek()) && !slot.isDayOff()) { | ||
| LocalTime originalEnd = slot.getEndTime(); | ||
| slot.setEndTime(bStart); // 오전 슬롯으로 변경 | ||
| newSlots.add(OperationTimeDto.createDayOn(slot.getDayOfWeek(), bEnd, originalEnd)); // 오후 슬롯 추가 | ||
| } | ||
| } | ||
| result.addAll(newSlots); |
There was a problem hiding this comment.
브레이크타임이 실제 영업 구간 안에 있을 때만 슬롯을 분리하세요.
현재는 활성 요일의 모든 슬롯을 무조건 start~breakStart, breakEnd~originalEnd로 쪼갭니다. 브레이크타임이 해당 슬롯 밖에 있으면 17:00~15:00 같은 잘못된 구간이나 의도치 않은 익일 종료 슬롯이 생성될 수 있습니다.
분리 전에 breakStart/breakEnd가 원래 영업 구간 내부인지 검사하고, endTime 변경 후 endNextDay도 함께 갱신해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java`
around lines 110 - 122, splitExistingSlots currently splits every active slot
into morning and afternoon pieces regardless of whether bStart/bEnd fall inside
that slot; change it to first check that bStart is after slot.getStartTime() and
bEnd is before the slot's originalEnd (taking into account next-day flags)
before mutating the slot. Only when the break interval lies strictly within the
existing slot create the morning piece by setting slot.setEndTime(bStart) and
also update the slot's endNextDay flag appropriately, then add the afternoon
piece via OperationTimeDto.createDayOn(slot.getDayOfWeek(), bEnd, originalEnd)
ensuring the new slot's endNextDay is set to match whether originalEnd crossed
midnight. Keep all other checks (days.contains(slot.getDayOfWeek()) &&
!slot.isDayOff()) but add the in-slot boundary checks and endNextDay updates to
prevent inverted or next-day ranges.
| CONSTRAINT fk_operation_time_place | ||
| FOREIGN KEY (place_id) REFERENCES places(id) ON DELETE CASCADE, | ||
|
|
||
| UNIQUE KEY uk_place_day (place_id, day_of_week, start_time) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MySQL UNIQUE index multiple NULL values same indexed columns
💡 Result:
Yes, MySQL UNIQUE indexes permit multiple rows with NULL values in the indexed columns, even if all indexed columns are NULL. This applies to both single-column and multi-column (composite) UNIQUE indexes. This behavior is standard across MySQL versions, including the latest (8.4 as of 2026), and all storage engines except the deprecated BDB. NULL values are not considered equal to each other (NULL != NULL), so they do not violate uniqueness. Example: CREATE TABLE test (col1 INT NULL, col2 INT NULL, UNIQUE KEY uniq (col1, col2)); INSERT INTO test VALUES (NULL, NULL); -- Succeeds INSERT INTO test VALUES (NULL, NULL); -- Succeeds again INSERT INTO test VALUES (1, NULL); -- Succeeds INSERT INTO test VALUES (1, NULL); -- Fails: Duplicate entry '1-NULL' If you need to prevent multiple NULLs (e.g., treat NULL as a unique value), use a virtual generated column with COALESCE or IFNULL and index that: ALTER TABLE test ADD COLUMN col1_nonnull INT GENERATED ALWAYS AS (COALESCE(col1, -1)) VIRTUAL, ADD UNIQUE KEY uniq_nonnull (col1_nonnull, col2);
Citations:
- 1: https://stackoverflow.com/questions/58267590/how-to-make-unique-index-constraints-on-multiple-nullable-columns
- 2: https://stackoverflow.com/questions/25844786/unique-multiple-columns-and-null-in-one-column
- 3: https://stackoverflow.com/questions/3712222/does-mysql-ignore-null-values-on-unique-constraints
- 4: https://dba.stackexchange.com/questions/132518/is-it-possible-to-have-unique-null
- 5: https://dev.mysql.com/doc/refman/en/create-index.html
- 6: https://dev.mysql.com/doc/en/create-index.html
- 7: https://dev.mysql.com/doc/refman/8.0/en/create-index.html
🏁 Script executed:
# Find the migration files referenced in the review
find . -path "*db/migration*" -name "V16*" -o -name "V17*" | head -20Repository: SOLPLY/SOLPLY-SERVER
Length of output: 209
🏁 Script executed:
# Check the actual content of V16 migration file
cat "src/main/resources/db/migration/V16__create_operation_hour_slot_from_place.sql"Repository: SOLPLY/SOLPLY-SERVER
Length of output: 1169
🏁 Script executed:
# Look for V17 migration files (could be .sql or .java)
find . -path "*db/migration*" \( -name "V17*" -o -name "*V17*" \) | head -20Repository: SOLPLY/SOLPLY-SERVER
Length of output: 128
🏁 Script executed:
# Check the content of V17 migration file
cat "src/main/java/db/migration/V17__Normalize_Operation_Hours.java"Repository: SOLPLY/SOLPLY-SERVER
Length of output: 2947
🏁 Script executed:
# Search for code that inserts into operation_time_slots to see if there's duplicate prevention
rg -A 5 -B 5 "operation_time_slots" --type java | head -100Repository: SOLPLY/SOLPLY-SERVER
Length of output: 2509
🏁 Script executed:
# Look for any other constraints or indexes on operation_time_slots
rg "uk_place_day|operation_time_slots" --type sqlRepository: SOLPLY/SOLPLY-SERVER
Length of output: 349
🏁 Script executed:
# Check if there's any service-layer logic preventing duplicate day-off insertions
rg -A 10 "isDayOff|is_day_off" --type java | head -150Repository: SOLPLY/SOLPLY-SERVER
Length of output: 11056
🏁 Script executed:
# Check if there are any additional constraints or unique indexes beyond uk_place_day
rg "UNIQUE|INDEX" src/main/resources/db/migration/V16*.sqlRepository: SOLPLY/SOLPLY-SERVER
Length of output: 155
🏁 Script executed:
# Search for any application-level duplicate checks or updates after V17
rg -A 20 "operation_time_slots" src/main/resources/db/migration/ | grep -i "delete\|update\|constraint" | head -20Repository: SOLPLY/SOLPLY-SERVER
Length of output: 541
중복 방지 제약 필요
현재 uk_place_day(place_id, day_of_week, start_time) 제약만으로는 휴무일 중복을 방지할 수 없습니다. V17에서 휴무 슬롯을 start_time = NULL로 삽입하는데, MySQL에서는 UNIQUE 제약이 NULL 값에 대해 중복을 허용하므로 같은 장소/요일의 휴무 행이 여러 개 생길 수 있습니다.
is_day_off 컬럼을 포함한 UNIQUE 제약, 생성 컬럼(generated column), 또는 서비스 계층 검증으로 보강해주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/resources/db/migration/V16__create_operation_hour_slot_from_place.sql`
at line 21, The UNIQUE constraint uk_place_day currently on (place_id,
day_of_week, start_time) allows multiple NULL start_time rows (day-off slots);
update the schema so day-off rows are deduplicated by including is_day_off in
the uniqueness rule or by adding a generated column that normalizes NULL
start_time and using that in the UNIQUE key. Concretely, modify the constraint
referenced as uk_place_day (or create a new UNIQUE key) to include is_day_off
(e.g. UNIQUE(place_id, day_of_week, is_day_off, start_time) or use a generated
column like normalized_start_time = COALESCE(start_time, <sentinel>) and enforce
UNIQUE(place_id, day_of_week, is_day_off, normalized_start_time) so that rows
with start_time = NULL cannot be inserted multiple times for the same place/day.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java (2)
125-138:⚠️ Potential issue | 🟠 Major자정 넘기는 슬롯의 라스트오더 판정이 여전히 불완전합니다. (부분 해결)
|| slot.isEndNextDay()로 한쪽 조건은 보완됐지만,lo.isBefore(slot.getEndTime())조건은 그대로라서18:00~02:00슬롯에서lo=22:00(자정 이전) 같은 값은22:00.isBefore(02:00)가false이므로 여전히 누락됩니다.endNextDay를 고려한 포함 판정 헬퍼(예:isWithin(start, end, endNextDay, lo))를 도입해 양쪽 경계를 모두 처리해 주세요.🛠️ 제안 수정 방향
- for (LocalTime lo : loTimes) { - if (lo.isBefore(slot.getEndTime()) && - (lo.isAfter(slot.getStartTime()) || slot.isEndNextDay())) { - slot.setLastOrderTime(lo); - } - } + for (LocalTime lo : loTimes) { + if (isWithinSlot(slot, lo)) { + slot.setLastOrderTime(lo); + } + }private boolean isWithinSlot(OperationTimeDto slot, LocalTime t) { LocalTime s = slot.getStartTime(); LocalTime e = slot.getEndTime(); if (slot.isEndNextDay()) { // s <= t < 24:00 또는 00:00 <= t < e return !t.isBefore(s) || t.isBefore(e); } return !t.isBefore(s) && t.isBefore(e); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java` around lines 125 - 138, The last-order inclusion logic in applyLastOrder skips times before midnight for slots that end the next day; introduce a helper (e.g., isWithinSlot(OperationTimeDto slot, LocalTime t)) that handles endNextDay correctly (for endNextDay: return t >= start OR t < end; otherwise return start <= t < end) and replace the current compound condition inside applyLastOrder with a call to that helper before calling slot.setLastOrderTime(lo); reference applyLastOrder, OperationTimeDto.isEndNextDay, and setLastOrderTime when implementing the change.
110-123:⚠️ Potential issue | 🟠 Major브레이크타임이 슬롯 범위 밖일 때에도 여전히 무조건 쪼개집니다. (미해결)
이전 리뷰에서 지적된 내용이 반영되지 않았습니다. 현재 로직은
days.contains(...)와!slot.isDayOff()만 확인한 뒤bStart/bEnd를 그대로 대입하므로, 브레이크 시간이 해당 슬롯의[startTime, originalEnd]밖에 있으면17:00~15:00같은 역전 구간이나 의도치 않은 익일 종료 슬롯이 만들어집니다. 또한originalEnd가 자정을 넘어가는 경우(endNextDay=true) 포함 여부 판정이 단순isBefore로는 맞지 않아 경계 검사도endNextDay를 고려해야 합니다.분리 전에
bStart가 슬롯 시작 이후이고bEnd가 원래 종료 이전인지 확인하고, 쪼갠 뒤 앞 구간/뒤 구간 각각의endNextDay도 재계산해 주세요.
🧹 Nitpick comments (3)
src/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.java (1)
43-54:endNextDay를 primitiveboolean으로 두는 편이 안전합니다.DB 컬럼은
NOT NULL인데 필드는Boolean래퍼여서@Setter경로로null이 세팅될 여지가 있습니다.isDayOff처럼 primitive로 바꾸면 default(false) 보장과 getter 네이밍(isEndNextDay())의 일관성도 함께 얻을 수 있습니다.♻️ 제안 수정
- `@Setter` - `@Column` - private Boolean endNextDay; + `@Setter` + `@Column`(nullable = false) + private boolean endNextDay;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.java` around lines 43 - 54, The field endNextDay is declared as the nullable wrapper Boolean with `@Setter` which allows null to be set despite the DB column being NOT NULL; change the field declaration to primitive boolean (like isDayOff) and keep or adjust the `@Setter` so the generated setter/getter use primitive boolean (resulting in isEndNextDay()), ensuring default false and avoiding null assignments via the setter for the field endNextDay.src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java (1)
76-87: 주석 처리된 죽은 코드와 디버그 로그 정리를 권장합니다.
- Lines 76–87:
parseOperationTime으로 이관 완료된 블록이 주석으로 남아 있습니다.- Line 154:
log.info("cleanLine:{}", ...)는 디버그용으로 보이는 흔적입니다. 마이그레이션 실행 시 매 줄마다 찍혀 노이즈가 큽니다.- Lines 192–196: 조건 분기가 죽은 주석으로 남아 있습니다.
삭제하거나 필요시
log.debug로 낮춰 주세요.Also applies to: 154-154, 192-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java` around lines 76 - 87, The file contains dead commented code and noisy info-level logging that should be cleaned up: remove the commented-out block that was migrated to parseOperationTime (the fragment using parseDays, extractTimes, OperationTimeDto.createDayOn and lastActiveDays) and drop the other commented conditional branches left as dead-code in OperationHourParser; also change the noisy log.info("cleanLine:{}" ...) to log.debug(...) or remove it if not needed so migration runs don't log every line. Ensure you only delete/comment-clear the obsolete snippets and preserve real logic in parseOperationTime and any live conditionals.src/main/java/org/sopt/solply_server/global/exception/ErrorCode.java (1)
125-125: 코드 컨벤션 불일치:DAY_001→DAY-001권장.파일 내 대부분의 도메인 코드는
COMMON-001,AUTH-001,PLACE-001처럼 하이픈(-) + 3자리 숫자 형식을 따릅니다. 본 PR에서 새로 추가되는INVALID_DAY_VALUE는 언더스코어를 사용하고 있어 컨벤션에서 벗어납니다(기존PLACE_REVIEW_xxx도 동일한 문제가 있으나 본 PR 스코프는 아니므로 별건).♻️ 제안 수정
- INVALID_DAY_VALUE(HttpStatus.BAD_REQUEST, "DAY_001", "잘못된 요일 값입니다."), + INVALID_DAY_VALUE(HttpStatus.BAD_REQUEST, "DAY-001", "잘못된 요일 값입니다."),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/global/exception/ErrorCode.java` at line 125, The new enum constant INVALID_DAY_VALUE in ErrorCode uses the code string "DAY_001" which breaks the project's convention; change the code string to "DAY-001" inside the ErrorCode enum (update the constructor argument for INVALID_DAY_VALUE) and scan for any usages of ErrorCode.INVALID_DAY_VALUE to ensure they still match the updated code string if they compare by string; leave the enum constant name as-is but update only the error code literal to follow the "XXX-###" convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/sopt/solply_server/global/exception/ErrorCode.java`:
- Around line 34-35: The two enum entries EXPIRED_ACCESS_TOKEN and
EXPIRED_REFRESH_TOKEN currently share the same error code "AUTH-004", causing
clients to be unable to distinguish access vs refresh expiry; change
EXPIRED_ACCESS_TOKEN to keep "AUTH-004" and assign a distinct code to
EXPIRED_REFRESH_TOKEN (e.g., "AUTH-005") in ErrorCode so clients can handle
reissue vs re-login flows, and rename the DAY_001 entry to use the hyphenated
form "DAY-001" (and shift any following DAY codes if your numbering requires
reordering) so it matches the DOMAIN-NNN style used elsewhere.
---
Duplicate comments:
In
`@src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java`:
- Around line 125-138: The last-order inclusion logic in applyLastOrder skips
times before midnight for slots that end the next day; introduce a helper (e.g.,
isWithinSlot(OperationTimeDto slot, LocalTime t)) that handles endNextDay
correctly (for endNextDay: return t >= start OR t < end; otherwise return start
<= t < end) and replace the current compound condition inside applyLastOrder
with a call to that helper before calling slot.setLastOrderTime(lo); reference
applyLastOrder, OperationTimeDto.isEndNextDay, and setLastOrderTime when
implementing the change.
---
Nitpick comments:
In
`@src/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.java`:
- Around line 43-54: The field endNextDay is declared as the nullable wrapper
Boolean with `@Setter` which allows null to be set despite the DB column being NOT
NULL; change the field declaration to primitive boolean (like isDayOff) and keep
or adjust the `@Setter` so the generated setter/getter use primitive boolean
(resulting in isEndNextDay()), ensuring default false and avoiding null
assignments via the setter for the field endNextDay.
In
`@src/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.java`:
- Around line 76-87: The file contains dead commented code and noisy info-level
logging that should be cleaned up: remove the commented-out block that was
migrated to parseOperationTime (the fragment using parseDays, extractTimes,
OperationTimeDto.createDayOn and lastActiveDays) and drop the other commented
conditional branches left as dead-code in OperationHourParser; also change the
noisy log.info("cleanLine:{}" ...) to log.debug(...) or remove it if not needed
so migration runs don't log every line. Ensure you only delete/comment-clear the
obsolete snippets and preserve real logic in parseOperationTime and any live
conditionals.
In `@src/main/java/org/sopt/solply_server/global/exception/ErrorCode.java`:
- Line 125: The new enum constant INVALID_DAY_VALUE in ErrorCode uses the code
string "DAY_001" which breaks the project's convention; change the code string
to "DAY-001" inside the ErrorCode enum (update the constructor argument for
INVALID_DAY_VALUE) and scan for any usages of ErrorCode.INVALID_DAY_VALUE to
ensure they still match the updated code string if they compare by string; leave
the enum constant name as-is but update only the error code literal to follow
the "XXX-###" convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f8b1c670-4980-49c4-99c8-a8c6e8742eb6
📒 Files selected for processing (5)
src/main/java/db/migration/V17__Normalize_Operation_Hours.javasrc/main/java/org/sopt/solply_server/domain/place/entity/OperationTimeSlot.javasrc/main/java/org/sopt/solply_server/domain/place/util/OperationHourParser.javasrc/main/java/org/sopt/solply_server/global/entity/DayOfWeek.javasrc/main/java/org/sopt/solply_server/global/exception/ErrorCode.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/sopt/solply_server/global/entity/DayOfWeek.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/db/migration/V17__Normalize_Operation_Hours.java
# Conflicts: # src/main/java/org/sopt/solply_server/global/exception/ErrorCode.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🌳이슈 번호
resolves #341
☀️어떻게 이슈를 해결했나요?
🗯️ PR 포인트
Summary by CodeRabbit
릴리스 노트