[REFACTOR]: TableLayout 엔티티 컬럼 및 테이블 배치도 재생성 시 미래 예약 검증 로직 추가#132
[REFACTOR]: TableLayout 엔티티 컬럼 및 테이블 배치도 재생성 시 미래 예약 검증 로직 추가#132
Conversation
📝 WalkthroughWalkthrough활성 배치도를 대체할 때 해당 배치도에 미래 예약이 있으면 오류(LAYOUT400_2)를 반환하도록 검증을 추가하고, 엔티티에 활성 기반 생성 칼럼 및 유니크 제약을 추가했습니다. 또한 BookingRepository에 미래 예약 조회 쿼리가 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as Controller
participant Service as Service
participant LayoutRepo as Repository\n(TableLayout)
participant BookingRepo as Repository\n(Booking)
Client->>Controller: POST /layouts (createLayout 요청)
Controller->>Service: createLayout(request)
Service->>LayoutRepo: findActiveLayoutByStore(storeId)
LayoutRepo-->>Service: activeLayout (존재/부재)
alt 활성 배치도 존재
Service->>BookingRepo: findTableIdsWithFutureBookings(tableIds, today, now)
BookingRepo-->>Service: tableIdsWithFutureBookings
alt 결과 비어있지 않음
Service-->>Controller: throw TableLayoutException(LAYOUT400_2)
Controller-->>Client: 400 Bad Request
else 결과 비어있음
Service->>LayoutRepo: deactivate(existingLayout)
LayoutRepo-->>Service: success
Service->>LayoutRepo: save(newLayout)
LayoutRepo-->>Service: createdLayout
Service-->>Controller: createdLayout
Controller-->>Client: 201 Created
end
else 활성 배치도 없음
Service->>LayoutRepo: save(newLayout)
LayoutRepo-->>Service: createdLayout
Service-->>Controller: createdLayout
Controller-->>Client: 201 Created
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/eatsfine/eatsfine/domain/table_layout/controller/TableLayoutControllerDocs.java (1)
25-29: 🧹 Nitpick | 🔵 Trivial
@ApiResponses에 LAYOUT400_2 케이스가 누락되었습니다.Line 21에서 LAYOUT400_2 에러를 설명하고 있지만,
@ApiResponses목록의 400 응답 설명은 여전히 "그리드 크기 범위 초과 등"만 언급합니다. 미래 예약 존재 시의 400 응답도 명시하면 API 문서의 정확성이 높아집니다.💡 제안
- `@io.swagger.v3.oas.annotations.responses.ApiResponse`(responseCode = "400", description = "잘못된 요청 (그리드 크기 범위 초과 등)"), + `@io.swagger.v3.oas.annotations.responses.ApiResponse`(responseCode = "400", description = "잘못된 요청 (그리드 크기 범위 초과, 미래 예약이 존재하는 배치도 등)"),
🤖 Fix all issues with AI agents
In
`@src/main/java/com/eatsfine/eatsfine/domain/table_layout/exception/status/TableLayoutErrorStatus.java`:
- Line 17: Update the enum constant _CANNOT_DELETE_LAYOUT_WITH_FUTURE_BOOKINGS
in TableLayoutErrorStatus so the user-facing message matches the user action
(regenerate) instead of saying "cannot delete"; replace the current Korean
message with one that says the layout cannot be regenerated due to future
bookings (e.g., "미래 예약이 있는 배치도는 재생성할 수 없습니다. 모든 예약이 완료된 후 다시 시도해주세요.") so the
error reflects the "재생성" flow.
In
`@src/main/java/com/eatsfine/eatsfine/domain/table_layout/service/TableLayoutCommandServiceImpl.java`:
- Around line 75-76: The code calls LocalDate.now() and LocalTime.now()
separately which can cause race conditions at midnight and makes testing hard;
replace both calls by a single LocalDateTime snapshot (e.g. LocalDateTime now =
LocalDateTime.now()) and derive currentDate = now.toLocalDate() and currentTime
= now.toLocalTime(), or better inject a java.time.Clock into the class and use
LocalDateTime.now(clock) (then derive date/time) so you can control time in
tests; update any usages of the variables created from LocalDate/LocalTime
accordingly (look for the LocalDate currentDate and LocalTime currentTime
declarations in TableLayoutCommandServiceImpl).
- Around line 39-51: You are re-querying the same active layout twice: once into
existingLayout via findByStoreIdAndIsActiveTrue and again inside
deactivateExistingLayout; instead pass the already-fetched TableLayout
(existingLayout.get()) into a revised deactivateExistingLayout(TableLayout
layout) or inline the deactivation logic to avoid the duplicate repository call,
then remove the old no-arg deactivateExistingLayout(store) if it becomes unused;
update references to deactivateExistingLayout(...) accordingly and ensure the
future-booking check still uses the original existingLayout instance.
- Around line 74-82: The current checkFutureBookingsInLayout in
TableLayoutCommandServiceImpl causes an N+1 DB query because it calls
BookingRepository.existsFutureBookingByTable for each table; fix it by adding a
bulk repository method (e.g.,
BookingRepository.existsFutureBookingByTableIds(List<Long> tableIds, LocalDate
currentDate, LocalTime currentTime)) and update checkFutureBookingsInLayout to
collect table IDs from layout.getTables() and call the new
existsFutureBookingByTableIds once, returning its boolean result.
- Line 12: You're importing and throwing TableBlockException from the wrong
domain and causing extra DB work; replace the import/usage of
TableBlockException with TableLayoutException in TableLayoutCommandServiceImpl,
stop re-querying the active layout by passing the already-fetched layout (from
findByStoreIdAndIsActiveTrue(storeId)) into deactivateExistingLayout(layout)
instead of calling it again, and eliminate the N+1 in
checkFutureBookingsInLayout(layout) by collecting all table IDs from
layout.getTables() and invoking a single repository query (e.g., a new
exists/count method that accepts a collection of table IDs) instead of calling
existsFutureBookingByTable(tableId) per table.
|
|
||
| _LAYOUT_FORBIDDEN(HttpStatus.FORBIDDEN, "LAYOUT403", "해당 가게의 소유자만 접근 가능합니다."), | ||
|
|
||
| _CANNOT_DELETE_LAYOUT_WITH_FUTURE_BOOKINGS(HttpStatus.BAD_REQUEST, "LAYOUT400_2", "미래 예약이 있는 배치도는 삭제할 수 없습니다. 모든 예약이 완료된 후 재생성해주세요.") |
There was a problem hiding this comment.
에러 메시지가 사용자 동작과 불일치합니다.
사용자는 배치도를 "재생성"하는 동작을 수행하지만, 에러 메시지는 "삭제할 수 없습니다"로 표현되어 있어 혼동을 줄 수 있습니다. 사용자 관점의 동작에 맞게 메시지를 수정하는 것을 권장합니다.
💡 제안
- _CANNOT_DELETE_LAYOUT_WITH_FUTURE_BOOKINGS(HttpStatus.BAD_REQUEST, "LAYOUT400_2", "미래 예약이 있는 배치도는 삭제할 수 없습니다. 모든 예약이 완료된 후 재생성해주세요.")
+ _CANNOT_DELETE_LAYOUT_WITH_FUTURE_BOOKINGS(HttpStatus.BAD_REQUEST, "LAYOUT400_2", "미래 예약이 존재하여 배치도를 재생성할 수 없습니다. 모든 예약이 완료된 후 다시 시도해주세요.")📝 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.
| _CANNOT_DELETE_LAYOUT_WITH_FUTURE_BOOKINGS(HttpStatus.BAD_REQUEST, "LAYOUT400_2", "미래 예약이 있는 배치도는 삭제할 수 없습니다. 모든 예약이 완료된 후 재생성해주세요.") | |
| _CANNOT_DELETE_LAYOUT_WITH_FUTURE_BOOKINGS(HttpStatus.BAD_REQUEST, "LAYOUT400_2", "미래 예약이 존재하여 배치도를 재생성할 수 없습니다. 모든 예약이 완료된 후 다시 시도해주세요.") |
🤖 Prompt for AI Agents
In
`@src/main/java/com/eatsfine/eatsfine/domain/table_layout/exception/status/TableLayoutErrorStatus.java`
at line 17, Update the enum constant _CANNOT_DELETE_LAYOUT_WITH_FUTURE_BOOKINGS
in TableLayoutErrorStatus so the user-facing message matches the user action
(regenerate) instead of saying "cannot delete"; replace the current Korean
message with one that says the layout cannot be regenerated due to future
bookings (e.g., "미래 예약이 있는 배치도는 재생성할 수 없습니다. 모든 예약이 완료된 후 다시 시도해주세요.") so the
error reflects the "재생성" flow.
...in/java/com/eatsfine/eatsfine/domain/table_layout/service/TableLayoutCommandServiceImpl.java
Outdated
Show resolved
Hide resolved
...in/java/com/eatsfine/eatsfine/domain/table_layout/service/TableLayoutCommandServiceImpl.java
Show resolved
Hide resolved
| LocalDate currentDate = LocalDate.now(); | ||
| LocalTime currentTime = LocalTime.now(); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LocalDate.now()/LocalTime.now() 사용은 테스트 어려움과 경계값 문제를 유발합니다.
날짜와 시간을 별도로 비교하면 자정 경계에서 경합 조건이 발생할 수 있습니다 (예: now() 호출 사이에 날짜가 변경). LocalDateTime.now()를 한 번 호출하여 사용하거나, Clock을 주입하여 테스트 용이성을 확보하는 것을 권장합니다.
🤖 Prompt for AI Agents
In
`@src/main/java/com/eatsfine/eatsfine/domain/table_layout/service/TableLayoutCommandServiceImpl.java`
around lines 75 - 76, The code calls LocalDate.now() and LocalTime.now()
separately which can cause race conditions at midnight and makes testing hard;
replace both calls by a single LocalDateTime snapshot (e.g. LocalDateTime now =
LocalDateTime.now()) and derive currentDate = now.toLocalDate() and currentTime
= now.toLocalTime(), or better inject a java.time.Clock into the class and use
LocalDateTime.now(clock) (then derive date/time) so you can control time in
tests; update any usages of the variables created from LocalDate/LocalTime
accordingly (look for the LocalDate currentDate and LocalTime currentTime
declarations in TableLayoutCommandServiceImpl).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/eatsfine/eatsfine/domain/table_layout/service/TableLayoutCommandServiceImpl.java (1)
31-67: 🛠️ Refactor suggestion | 🟠 Major
createLayout메서드의 새로운 미래 예약 검증 로직에 대한 테스트가 필요합니다.이번 PR에서 추가된 핵심 비즈니스 로직(미래 예약 존재 시 배치도 재생성 차단)에 대한 단위 테스트가 포함되어 있지 않습니다. 최소한 다음 케이스에 대한 테스트를 권장합니다:
- 기존 배치도 없이 새 배치도 생성 (정상)
- 기존 배치도가 있고 미래 예약이 없는 경우 (삭제 후 재생성)
- 기존 배치도가 있고 미래 예약이 존재하는 경우 (
TableLayoutException발생)As per coding guidelines, "단위 테스트가 충분히 작성되었는지, 핵심 로직의 검증이 누락되지 않았는지."
이 테스트 코드를 생성해 드릴까요, 아니면 이슈를 만들어 추적할까요?
- 기존 배치도 조회 중복 쿼리 수정 - 테이블 미래 예약 내역 조회 시 기존 N+1문제 수정
b46529d to
38eed42
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@src/main/java/com/eatsfine/eatsfine/domain/booking/repository/BookingRepository.java`:
- Around line 106-116: The current bulk query in
BookingRepository.findTableIdsWithFutureBookings returns List<Long> but callers
(TableLayoutCommandServiceImpl) only check isEmpty(), so replace or add a more
efficient boolean method: implement a JPQL method like
existsFutureBookingByTableIds (or modify findTableIdsWithFutureBookings) that
uses SELECT (COUNT(bt) > 0) or an EXISTS subquery and returns boolean, update
the repository signature accordingly and call that boolean method from
TableLayoutCommandServiceImpl instead of fetching the full List<Long> to reduce
DB load.
In
`@src/main/java/com/eatsfine/eatsfine/domain/table_layout/entity/TableLayout.java`:
- Around line 22-25: The `@Table` declaration in TableLayout currently relies on a
MySQL-only GENERATED ALWAYS AS ... STORED virtual column plus the
uk_table_layout_store_active unique constraint to enforce one active layout per
store; update the design to (1) remove the DB-specific virtual column approach
from automatic Hibernate DDL and instead add an explicit migration script
(Flyway/Liquibase) that creates the virtual/generated column and unique
constraint in production, and (2) simplify the generated expression to use
CAST(store_id AS CHAR) rather than CONCAT('store_', store_id) when authoring
that migration; locate the TableLayout entity and the
uk_table_layout_store_active constraint to ensure the migration matches the
entity intent and disable relying on ddl-auto:update for production schema
changes.
In
`@src/main/java/com/eatsfine/eatsfine/domain/table_layout/service/TableLayoutCommandServiceImpl.java`:
- Around line 41-53: The delete+save sequence risks a unique constraint
violation due to Hibernate flush ordering; after verifying future bookings with
checkFutureBookingsInLayout and before creating/saving a new TableLayout, either
(A) call tableLayoutRepository.delete(existingLayout.get()) followed immediately
by an explicit flush on the EntityManager/Repository to force the UPDATE
(SQLDelete) to execute before inserting the new row, or (B) avoid delete()
entirely and instead set existingLayout.get().setIsActive(false) (or equivalent
field setter) and save/update that entity so the deactivation UPDATE occurs
deterministically before persisting the new TableLayout; keep the existing
TableLayoutException/TableLayoutErrorStatus logic unchanged.
| @Query("SELECT DISTINCT bt.storeTable.id FROM BookingTable bt " + | ||
| "JOIN bt.booking b " + | ||
| "WHERE bt.storeTable.id IN :tableIds " + | ||
| "AND (b.bookingDate > :currentDate " + | ||
| " OR (b.bookingDate = :currentDate AND b.bookingTime >= :currentTime)) " + | ||
| "AND b.status IN ('CONFIRMED', 'PENDING')") | ||
| List<Long> findTableIdsWithFutureBookings( | ||
| @Param("tableIds") List<Long> tableIds, | ||
| @Param("currentDate") LocalDate currentDate, | ||
| @Param("currentTime") LocalTime currentTime | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
이전 N+1 문제를 잘 해결한 벌크 쿼리입니다.
쿼리 로직이 정확하고, 기존 existsFutureBookingByTable 단건 호출의 N+1 문제를 해결합니다.
한 가지 고려사항: 반환 타입이 List<Long>이지만, 호출부(TableLayoutCommandServiceImpl)에서는 isEmpty() 여부만 확인합니다. 존재 여부만 필요하다면 SELECT COUNT(...) > 0 또는 JPQL의 EXISTS 서브쿼리를 사용하는 boolean 반환 메서드가 DB 부하를 줄일 수 있습니다.
🤖 Prompt for AI Agents
In
`@src/main/java/com/eatsfine/eatsfine/domain/booking/repository/BookingRepository.java`
around lines 106 - 116, The current bulk query in
BookingRepository.findTableIdsWithFutureBookings returns List<Long> but callers
(TableLayoutCommandServiceImpl) only check isEmpty(), so replace or add a more
efficient boolean method: implement a JPQL method like
existsFutureBookingByTableIds (or modify findTableIdsWithFutureBookings) that
uses SELECT (COUNT(bt) > 0) or an EXISTS subquery and returns boolean, update
the repository signature accordingly and call that boolean method from
TableLayoutCommandServiceImpl instead of fetching the full List<Long> to reduce
DB load.
| @Table( | ||
| name = "table_layout", | ||
| uniqueConstraints = {@UniqueConstraint(name = "uk_table_layout_store_active", columnNames = {"store_active_key"})} | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for migration files and DDL configuration
echo "=== Checking for migration files ==="
find . -type f \( -name "*.sql" -o -name "*.xml" -o -name "*.yaml" -o -name "*.yml" \) 2>/dev/null | grep -iE '(migration|flyway|liquibase|changelog|db)' | head -30
echo ""
echo "=== Checking application configuration files ==="
find . -type f \( -name "application*.properties" -o -name "application*.yml" -o -name "application*.yaml" \) 2>/dev/null | head -10
echo ""
echo "=== Checking for DDL auto settings ==="
find . -type f \( -name "application*.properties" -o -name "application*.yml" -o -name "application*.yaml" \) 2>/dev/null -exec grep -H -i 'ddl-auto\|hibernate.hbm2ddl\|flyway\|liquibase' {} \;
echo ""
echo "=== Checking pom.xml or build.gradle for migration dependencies ==="
fd -e pom.xml -e gradle | head -5 | xargs grep -l -i 'flyway\|liquibase' 2>/dev/nullRepository: Eatsfine/BE
Length of output: 542
🏁 Script executed:
find . -type f -name "TableLayout.java" | head -1 | xargs cat -nRepository: Eatsfine/BE
Length of output: 2668
DB 레벨에서 활성 배치도 중복 방지 — 접근 방식은 적절하나 DDL 관리 방식 재검토 필요합니다.
GENERATED ALWAYS AS ... STORED 가상 컬럼 + UNIQUE 제약조건을 통해 가게당 활성 배치도가 1개만 존재하도록 강제하는 패턴은 적절합니다. is_active = false일 때 NULL이 되어 UNIQUE 제약조건에 걸리지 않는 점도 정확합니다.
확인/개선 사항:
GENERATED ALWAYS AS ... STORED는 MySQL 전용 문법입니다. 현재 프로젝트는 Hibernateddl-auto: update를 사용하고 있어 스키마 변경이 자동 적용되지만, 프로덕션 환경에서는 Flyway/Liquibase 같은 마이그레이션 도구 도입 또는 수동 마이그레이션 스크립트 작성을 강력히 권장합니다. (auto-DDL은 롤백 안전성이 없음)CONCAT('store_', store_id)대신CAST(store_id AS CHAR)만으로도 동일한 유니크 효과를 얻을 수 있어, 'store_' 접두사는 불필요한 문자열 결합입니다.
🤖 Prompt for AI Agents
In
`@src/main/java/com/eatsfine/eatsfine/domain/table_layout/entity/TableLayout.java`
around lines 22 - 25, The `@Table` declaration in TableLayout currently relies on
a MySQL-only GENERATED ALWAYS AS ... STORED virtual column plus the
uk_table_layout_store_active unique constraint to enforce one active layout per
store; update the design to (1) remove the DB-specific virtual column approach
from automatic Hibernate DDL and instead add an explicit migration script
(Flyway/Liquibase) that creates the virtual/generated column and unique
constraint in production, and (2) simplify the generated expression to use
CAST(store_id AS CHAR) rather than CONCAT('store_', store_id) when authoring
that migration; locate the TableLayout entity and the
uk_table_layout_store_active constraint to ensure the migration matches the
entity intent and disable relying on ddl-auto:update for production schema
changes.
| Optional<TableLayout> existingLayout = tableLayoutRepository.findByStoreIdAndIsActiveTrue(storeId); | ||
|
|
||
| if (existingLayout.isPresent()) { | ||
| // 미래 예약 확인 | ||
| boolean hasFutureBookings = checkFutureBookingsInLayout(existingLayout.get()); | ||
|
|
||
| if (hasFutureBookings) { | ||
| throw new TableLayoutException(TableLayoutErrorStatus._CANNOT_DELETE_LAYOUT_WITH_FUTURE_BOOKINGS); | ||
| } | ||
|
|
||
| // 미래 예약이 없으면 배치도 비활성화 후 재생성 | ||
| tableLayoutRepository.delete(existingLayout.get()); | ||
| } |
There was a problem hiding this comment.
delete() 후 save() 실행 시 Hibernate 플러시 순서로 인한 Unique Constraint 위반 가능성이 있습니다.
Hibernate의 기본 플러시 순서는 INSERT → UPDATE → DELETE입니다. @SQLDelete를 사용하면 delete()가 내부적으로 UPDATE로 변환되지만, 엔티티 라이프사이클상 DELETE 단계에서 실행됩니다. 따라서:
- 새 배치도
INSERT(is_active=true, store_active_key='store_X') — 먼저 실행 - 기존 배치도
@SQLDeleteUPDATE (is_active=false → store_active_key=NULL) — 나중 실행
새 배치도가 먼저 INSERT되는 시점에 기존 배치도의 store_active_key가 아직 동일한 값을 가지고 있으므로 uk_table_layout_store_active 유니크 제약조건 위반이 발생할 수 있습니다.
해결 방안: delete() 후 명시적으로 flush()를 호출하거나, delete() 대신 기존 배치도의 isActive를 직접 false로 변경 후 save하는 방식으로 처리하세요.
🐛 제안: flush를 명시적으로 호출
// 미래 예약이 없으면 배치도 비활성화 후 재생성
tableLayoutRepository.delete(existingLayout.get());
+ tableLayoutRepository.flush();
}As per coding guidelines, "DB 일관성, 롤백 정책이 올바른지 검토".
🤖 Prompt for AI Agents
In
`@src/main/java/com/eatsfine/eatsfine/domain/table_layout/service/TableLayoutCommandServiceImpl.java`
around lines 41 - 53, The delete+save sequence risks a unique constraint
violation due to Hibernate flush ordering; after verifying future bookings with
checkFutureBookingsInLayout and before creating/saving a new TableLayout, either
(A) call tableLayoutRepository.delete(existingLayout.get()) followed immediately
by an explicit flush on the EntityManager/Repository to force the UPDATE
(SQLDelete) to execute before inserting the new row, or (B) avoid delete()
entirely and instead set existingLayout.get().setIsActive(false) (or equivalent
field setter) and save/update that entity so the deactivation UPDATE occurs
deterministically before persisting the new TableLayout; keep the existing
TableLayoutException/TableLayoutErrorStatus logic unchanged.
💡 작업 개요
1. TableLayout 엔티티 클래스에 Unique 제약 조건, storeActiveKey 가상 컬럼을 추가
isActive = true인 배치도가 2개가 생기는 것을 방지2. 테이블 배치도 재생성 시 미래 예약 검증 로직을 추가
@SQLRestriction(is_deleted = false)인 테이블만 조회가 가능하기 때문에 기존에 예약 내역을 조회할 수 없게됨.✅ 작업 내용
🧪 테스트 내용
1.
storeId = 2, tableId = 18, date = "2026-02-20", time = "18:00"인 예약 데이터2. 예약 존재 시 테이블 배치도를 재생성하려고 할 때
📝 기타 참고 사항
Summary by CodeRabbit
새로운 기능
버그 수정/동작 변경
문서화