Feature: menu sorting 구현 #273
Merged
Conversation
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
|
Caution Review failedThe pull request is closed. Walkthrough메뉴의 정렬 순서를 관리하기 위해 Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Controller as MenuController
participant Service as MenuService
participant Repo as MenuRepository
participant Entity as Menu
Admin->>Controller: PATCH /update-sort (List<MenuSortUpdateRequest>)
Controller->>Service: updateMenuSortOrder(requests, memberDetails)
Service->>Service: 권한 체크 (SUPER_ADMIN)
loop 각 MenuSortUpdateRequest
Service->>Repo: findById(menuId)
Repo-->>Service: Menu entity
Service->>Entity: updateSortOrder(sortOrder)
Service->>Repo: save(Menu)
end
Service-->>Controller: 성공 메시지 반환
Controller-->>Admin: 200 OK + API 응답
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (2)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/menu/service/MenuService.java (1)
35-43: N+1 쿼리 제거: 이미지 일괄 로딩으로 변경 권장현재 매 메뉴마다
menuImageRepository.findByMenu(menu)호출로 N+1이 발생합니다. 한 번에 조회 후 매핑하면 성능이 크게 개선됩니다.
- MenuImageRepository에 다음 메서드 추가:
List<MenuImage> findByMenuIn(List<Menu> menus);
- 서비스 예시(스트림 매핑):
List<MenuImage> images = menuImageRepository.findByMenuIn(menus); Map<Long, List<MenuImageUploadResponse>> imageDtosByMenuId = images.stream() .collect(Collectors.groupingBy(img -> img.getMenu().getId(), Collectors.mapping(MenuImageUploadResponse::fromEntity, Collectors.toList()))); List<MenuReadDto> menuReadResponse = menus.stream() .map(menu -> MenuReadDto.fromEntity(menu, imageDtosByMenuId.getOrDefault(menu.getId(), List.of()))) .toList();원하시면 레포지토리/서비스 패치 전체를 준비해 드립니다.
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java (1)
64-74: N+1 쿼리 발생: 이미지 로딩 방식 개선 필요현재 각 메뉴마다
menuImageRepository.findByMenu(menu)호출로 N+1이 발생합니다. 다음 중 하나로 개선을 권장합니다.
- MenuRepository에 fetch join 쿼리 추가(정렬 포함) 후 한 번에 조회
- menuImageRepository에
findByMenuIn(List<Menu>)추가 후 일괄 조회하여 메뉴별 그룹핑- @entitygraph 또는 @batchsize 활용
대량 매장/메뉴 환경에서 응답 지연을 크게 줄일 수 있습니다.
🧹 Nitpick comments (6)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/menu/dto/MenuReadDto.java (1)
32-32: null/기본값 보장 검토 필요Menu.sortOrder가 NotNull 제약이라면 DTO도 primitive(long) 사용 또는 생성 시 0L 기본값 보장을 고려해 주세요. API 문서(Swagger)에 필수 여부도 명시하면 좋습니다.
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuCreateResponse.java (1)
26-38: 빌더 중복 설정 제거(adminDisplayName가 두 번 설정됨)같은 필드를 중복 지정하고 있어 가독성 저하 및 유지보수에 불리합니다. 아래처럼 한 줄만 남겨 주세요.
return MenuCreateResponse.builder() .createdAt(menu.getCreatedAt()) - .adminDisplayName(menu.getAdminDisplayName()) .menuId(menu.getId()) .storeId(menu.getStoreId()) .adminDisplayName(menu.getAdminDisplayName()) .name(menu.getName())nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java (2)
64-64: 정렬 안정성 및 인덱스동일 sortOrder 충돌 시 보조 정렬(createdAt 등) 추가를 고려해 주세요. 또한 (store_id, sort_order) 복합 인덱스를 생성하면 정렬 조회 성능이 안정적으로 나옵니다.
127-145: 권한 정책 재확인 요청다른 API들은 SUPER_ADMIN 또는 동일 storeId 소유자에게 권한을 부여하는데, 본 정렬 API만 SUPER_ADMIN으로 제한되어 있습니다. 의도된 정책인지 확인 부탁드립니다(사업 규칙에 따라 조정 필요).
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/controller/MenuController.java (1)
132-146: 메서드 수준 권한 부여 제안서비스에서만 권한을 검사하는 대신 컨트롤러에
@PreAuthorize("hasRole('SUPER_ADMIN')")를 추가하면 403 응답이 일찍 반환되고 문서화에도 도움이 됩니다.@PatchMapping("/update-sort") @Operation(summary = "메뉴 순서 수정", description = "메뉴의 순서를 수정합니다.") @ApiResponse(responseCode = "200", description = "메뉴 순서 수정") +@org.springframework.security.access.prepost.PreAuthorize("hasRole('SUPER_ADMIN')") public ResponseEntity<?> updateMenuSortOrder( - @Valid @RequestBody List<MenuSortUpdateRequest> request, + @RequestBody @jakarta.validation.constraints.NotEmpty @Valid List<MenuSortUpdateRequest> request, @AuthenticationPrincipal MemberDetails memberDetails ) {nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/order/dto/OrderResponseDto.java (1)
4-4: HashMap import는 불필요해질 수 있음 — Map 인터페이스 중심으로 회귀 권장아래 코멘트대로 필드/지역변수 타입을 Map으로 되돌리면 이 import는 제거 가능합니다.
-import java.util.HashMap;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/controller/MenuController.java(3 hunks)nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuCreateRequest.java(1 hunks)nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuCreateResponse.java(2 hunks)nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuReadDto.java(2 hunks)nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuSortUpdateRequest.java(1 hunks)nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java(3 hunks)nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/order/dto/OrderResponseDto.java(2 hunks)nowait-app-user-api/src/main/java/com/nowait/applicationuser/menu/dto/MenuReadDto.java(2 hunks)nowait-app-user-api/src/main/java/com/nowait/applicationuser/menu/service/MenuService.java(1 hunks)nowait-domain/domain-core-rdb/src/main/java/com/nowait/domaincorerdb/menu/entity/Menu.java(2 hunks)nowait-domain/domain-core-rdb/src/main/java/com/nowait/domaincorerdb/menu/repository/MenuRepository.java(1 hunks)
🔇 Additional comments (4)
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuReadDto.java (1)
21-21: LGTM: 정렬 정보 노출 추가가 일관성 있게 반영되었습니다
sortOrder필드 추가 및 매핑이 적절합니다. 프런트엔드에서 정렬 재배치 UI와 연계 시 활용 가능해 보입니다.Also applies to: 34-34
nowait-app-user-api/src/main/java/com/nowait/applicationuser/menu/dto/MenuReadDto.java (1)
20-20: DTO에 sortOrder 필드 추가 — 잘 반영되었습니다클라이언트가 정렬 상태를 인지할 수 있게 되어 일관성 좋아졌습니다.
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuCreateResponse.java (1)
22-22: sortOrder 응답 포함 — OK생성 응답에 정렬 정보 포함된 점 좋습니다. 클라이언트 동기화에 유용합니다.
Also applies to: 37-37
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/order/dto/OrderResponseDto.java (1)
35-35: 주문 시점 단가 보존 필요 — Menu 현재 단가(getPrice) 대신 OrderItem의 단가 사용 검토Line 35에서 메뉴 엔티티의 현재 단가를 읽어오고 있는데, 메뉴 가격이 변경된 이후 과거 주문을 조회할 때 단가가 왜곡될 수 있습니다. OrderItem에 주문 시점 단가(예: unitPrice/orderPrice 등)가 있다면 그 값을 사용해 주세요. 없을 경우 스키마/엔티티 수준에서 보강을 고려 바랍니다.
...-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/controller/MenuController.java
Show resolved
Hide resolved
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuCreateRequest.java
Show resolved
Hide resolved
...-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuSortUpdateRequest.java
Show resolved
Hide resolved
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java
Show resolved
Hide resolved
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/order/dto/OrderResponseDto.java
Show resolved
Hide resolved
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/order/dto/OrderResponseDto.java
Outdated
Show resolved
Hide resolved
nowait-app-user-api/src/main/java/com/nowait/applicationuser/menu/service/MenuService.java
Show resolved
Hide resolved
nowait-domain/domain-core-rdb/src/main/java/com/nowait/domaincorerdb/menu/entity/Menu.java
Show resolved
Hide resolved
nowait-domain/domain-core-rdb/src/main/java/com/nowait/domaincorerdb/menu/entity/Menu.java
Show resolved
Hide resolved
...n/domain-core-rdb/src/main/java/com/nowait/domaincorerdb/menu/repository/MenuRepository.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java (1)
65-75: 정렬 안정성 보강 + N+1 이미지 조회 최적화 제안
- 동일한
sortOrder값이 존재할 때 결과 순서가 비결정적일 수 있습니다. 보조 정렬 키(예:id)를 추가해 안정성을 확보하세요.- 현재
menuImageRepository.findByMenu(menu)를 메뉴 수만큼 호출해 N+1이 발생합니다. 한 번에 이미지 목록을 조회하고 그룹핑하는 방식으로 최적화하세요.두 가지 접근 중 택1 또는 병행:
Option A) DB 정렬 키 확장(선호)
- Repository 메서드를
findAllByStoreIdAndDeletedFalseOrderBySortOrderAscIdAsc(또는...OrderBySortOrderAscIdAsc)로 확장.Option B) In-memory 보조 정렬 + 이미지 일괄 로딩
- List<Menu> menus = menuRepository.findAllByStoreIdAndDeletedFalseOrderBySortOrder(storeId); + List<Menu> menus = menuRepository.findAllByStoreIdAndDeletedFalseOrderBySortOrder(storeId) + .stream() + .sorted(java.util.Comparator.comparing(Menu::getSortOrder).thenComparing(Menu::getId)) + .toList(); - List<MenuReadDto> menuReadResponse = menus.stream() - .map(menu -> { - List<MenuImage> images = menuImageRepository.findByMenu(menu); - List<MenuImageUploadResponse> imageDto = images.stream() - .map(MenuImageUploadResponse::fromEntity) - .toList(); - return MenuReadDto.fromEntity(menu, imageDto); - }) - .toList(); + // N+1 제거: 이미지 일괄 조회 후 메뉴별 그룹핑 + List<MenuImage> allImages = menuImageRepository.findByMenuIn(menus); // 없으면 메서드 추가 필요 + java.util.Map<Long, List<MenuImageUploadResponse>> imagesByMenuId = + allImages.stream().collect( + java.util.stream.Collectors.groupingBy(img -> img.getMenu().getId(), + java.util.stream.Collectors.mapping(MenuImageUploadResponse::fromEntity, + java.util.stream.Collectors.toList()))); + + List<MenuReadDto> menuReadResponse = menus.stream() + .map(menu -> MenuReadDto.fromEntity(menu, + imagesByMenuId.getOrDefault(menu.getId(), java.util.List.of()))) + .toList();추가로, 기존 데이터에
sortOrder가 null일 가능성이 있다면 DB 컬럼을NOT NULL DEFAULT 0으로 보강하거나, 조회 시 nulls last/fist 정책을 명확히 해주세요.
♻️ Duplicate comments (1)
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java (1)
128-168: 일괄 정렬 업데이트 전반 구현 훌륭함 (과거 리뷰 반영됨)
- 권한 체크, 빈 요청/중복 menuId/음수 방지, 동일 매장 검증, 일괄 조회/업데이트/저장 등 핵심 포인트가 잘 반영되었습니다.
🧹 Nitpick comments (3)
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java (3)
4-4: Collectors import 일관성(가독성) 제안
Mapimport 추가는 좋습니다.Collectors는 FQCN(java.util.stream.Collectors)으로 사용 중이라 가독성이 떨어집니다. 일반 import 또는 static import로 일관성을 맞추는 것을 권장합니다.아래처럼 import를 추가하고, 본문에서는
Collectors.toMap을 사용하세요:// 파일 상단 import java.util.stream.Collectors;- Map<Long, Long> idToSort = requests.stream() - .collect(java.util.stream.Collectors.toMap(MenuSortUpdateRequest::getMenuId, - MenuSortUpdateRequest::getSortOrder)); + Map<Long, Long> idToSort = requests.stream() + .collect(Collectors.toMap(MenuSortUpdateRequest::getMenuId, + MenuSortUpdateRequest::getSortOrder));
144-146: 정렬 순서 값 중복 허용 여부 확인 및 검증 추가 제안동일 매장 내에서
sortOrder가 중복되면 정렬 결과가 모호해집니다. 정책상 불허라면 페이로드 내 중복을 차단하고(최소한 요청 내), 가능하면 매장 단위 유니크 제약까지 고려하세요.if (requests.stream().anyMatch(r -> r.getSortOrder() == null || r.getSortOrder() < 0)) { throw new IllegalArgumentException("잘못된 정렬 순서가 포함되어 있습니다. 정렬 순서는 0 이상의 정수여야 합니다."); } +// 요청 내 sortOrder 중복 검증 (정책상 필요 시) +if (requests.stream().map(MenuSortUpdateRequest::getSortOrder).distinct().count() != requests.size()) { + throw new IllegalArgumentException("중복된 정렬 순서가 포함되어 있습니다."); +}DB 측면:
- 매장 단위로
(store_id, sort_order)유니크 인덱스 고려(운영 정책에 부합할 경우).
165-165: @transactional 하에서 saveAll 생략 가능동일 트랜잭션 내에서 조회된 엔티티는 관리 상태이므로
m.updateSortOrder(...)만으로 dirty checking이 작동합니다.saveAll(menus)는 생략해도 됩니다. 성능상 큰 차이는 없지만 코드 단순화 관점에서 권장합니다.- menuRepository.saveAll(menus);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuSortUpdateRequest.java(1 hunks)nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java(4 hunks)nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/order/dto/OrderResponseDto.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/order/dto/OrderResponseDto.java
- nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/dto/MenuSortUpdateRequest.java
🔇 Additional comments (2)
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/menu/service/MenuService.java (2)
14-14: 필요 import 추가 LGTM
MenuSortUpdateRequest도입에 따른 import 추가 적절합니다.
132-135: 권한 정책 확인 요청: SUPER_ADMIN 전용이 맞나요?다른 API들은
SUPER_ADMIN또는 동일storeId소유자에게 권한을 부여합니다. 메뉴 정렬 변경은 점주/매장 관리자도 수행해야 할 가능성이 높습니다. 의도적으로SUPER_ADMIN전용이라면 OK, 아니라면 동일 매장 소유자에게도 허용하도록 완화하는 것이 자연스럽습니다.
Comment on lines
+149
to
+156
| List<Menu> menus = menuRepository.findAllById(ids); | ||
| if (menus.size() != ids.size()) { | ||
| throw new MenuNotFoundException(); | ||
| } | ||
|
|
||
| Long storeId = menus.get(0).getStoreId(); | ||
| if (!menus.stream().allMatch(m -> storeId.equals(m.getStoreId()))) { | ||
| throw new IllegalArgumentException("다른 매장의 메뉴가 있습니다."); |
There was a problem hiding this comment.
소프트 삭제 메뉴까지 업데이트될 수 있는 위험
findAllById(ids)는 soft-delete 여부를 고려하지 않아 삭제된 메뉴의 sortOrder가 갱신될 수 있습니다. 기존 서비스 전반이 ...DeletedFalse 패턴을 사용하므로 여기에서도 동일하게 제한해야 합니다.
권장 수정:
- Repository에
findAllByIdInAndDeletedFalse(Collection<Long> ids)(또는findAllByIdInAndDeletedFalseOrderById)를 추가하고 아래처럼 호출부를 변경하세요.
- List<Menu> menus = menuRepository.findAllById(ids);
+ List<Menu> menus = menuRepository.findAllByIdInAndDeletedFalse(ids);
if (menus.size() != ids.size()) {
throw new MenuNotFoundException();
}대안:
- 당장 Repository 확장이 어렵다면, 조회 후 소프트 삭제된 엔티티를 필터링하고 발견 시 예외를 던지도록 방어 로직을 추가하세요(엔티티의 deleted 플래그 접근자에 맞춰 구현).
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
작업 요약
Issue Link
#272
문제점 및 어려움
주문 조회 시 N + 1 발생하여 추후 해결 필요
해결 방안
Reference
Summary by CodeRabbit
신규 기능
기능 개선
버그 수정