Merged
Conversation
Walkthrough발주 아이템 관련 엔드포인트를 재구성하여 Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ReceiptController
participant Service as ReceiptService
participant Repo as Repository
rect rgb(220, 240, 250)
Note over Client,Repo: 이전 방식 (receiptId 없음)
Client->>Controller: GET /receipt/{receiptItemId}
Controller->>Service: getReceiptItem(receiptItemId, userId)
Service->>Repo: Store 기반 아이템 조회
Repo-->>Service: ReceiptItem
end
rect rgb(240, 250, 240)
Note over Client,Repo: 개선된 방식 (receiptId 포함)
Client->>Controller: GET /{receiptId}/items/{receiptItemId}
Controller->>Service: getReceiptItem(receiptId, receiptItemId, userId)
Service->>Repo: Receipt 조회 및 접근 검증
Repo-->>Service: Receipt
Service->>Repo: Receipt 내 아이템 검증
Repo-->>Service: ReceiptItem
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25분 추가 검토 필요 영역:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (7)
src/main/java/com/almang/inventory/receipt/service/ReceiptService.java (2)
162-175: 입고 아이템 단건 조회/수정/삭제가 Receipt 기반으로 잘 재설계되었습니다.
- 기존 버그 원인(아이템을 receiptId 없이 itemId만으로 조회/조작 가능하고
/receipt/receipt...같은 애매한 경로 구조)을,findReceiptByIdAndValidateAccess(receiptId, store)→findReceiptItemByIdAndValidateAccess(itemId, receipt)순서로 풀어서 명확히 차단한 구조입니다.- 사용자 → Store → Receipt → ReceiptItem 순서로 권한을 검증하므로,
- 다른 상점의 receiptId:
RECEIPT_ACCESS_DENIED- 같은 상점이지만 다른 Receipt의 아이템:
RECEIPT_ITEM_ACCESS_DENIED- 존재하지 않는 아이템:
RECEIPT_ITEM_NOT_FOUND
로 케이스가 잘 분리됩니다. 서비스 테스트와도 일관적입니다.개선 제안(선택 사항):
- 세 메서드(get/update/delete)의 공통 패턴(유저 조회 + store 추출 + receipt 검증 + item 검증 + 로그)이 거의 동일하니, 나중에
executeReceiptItemAction(...)같은 private 템플릿 메서드로 묶으면 중복 제거와 테스트 유지보수에 도움이 됩니다.
(예: 공통 pre-check + 람다로 아이템 조작 로직 전달).Also applies to: 178-203, 205-220
339-354: updateReceiptItems에서도 Receipt 단위 접근 검증을 재사용한 점이 좋습니다.
- 루프 안에서
findReceiptItemByIdAndValidateAccess(receiptItemRequest.receiptItemId(), receipt)를 쓰도록 바뀌어, 단건 API와 동일하게 “해당 Receipt 소속 아이템만 수정 가능”이라는 규칙이 유지됩니다.findReceiptItemByIdAndValidateAccess가 Receipt로 한 번 더 교차 검증을 하기 때문에, 잘못된 receiptItemId가 섞여 들어와도RECEIPT_ITEM_ACCESS_DENIED로 안전하게 막힙니다.개선 제안(추천 수준):
DTO의 receiptId 활용 여부 정리
UpdateReceiptItemRequest에receiptId필드가 있지만, 서비스에서는 사용하지 않고receiptItemId + Receipt 엔티티만 신뢰하고 있습니다.- 유지보수자의 혼동을 줄이려면 아래 둘 중 하나를 고려해볼 수 있습니다.
- (A) 이 메서드에서
receiptItemRequest.receiptId()와receipt.getId()가 일치하는지 검증해 일종의 이중 방어를 추가.- (B) API 설계상 필요 없다면
receiptId필드를 deprecate 후 제거(다음 버전에서)하고, 식별은 path + receiptItemId에만 의존.쿼리 최적화(필요시)
- 현재는
receiptItemRepository.findById후getReceipt().getId()를 다시 접근하는 구조라 최소 1번, lazy인 경우 2번의 쿼리가 발생할 수 있습니다.- 성능이 이 경로에서 민감하다면 Spring Data JPA의 파생 쿼리 메서드(
findByIdAndReceiptId(...))를 리포지토리에 추가해 한 번에 검증하는 것도 좋습니다.Also applies to: 356-364
src/test/java/com/almang/inventory/receipt/service/ReceiptServiceTest.java (2)
983-1064: 입고 아이템 조회 테스트가 새 시그니처와 예외 케이스를 잘 커버하고 있습니다.
- 버그 원인이었던 “itemId만으로 조회 +
/receipt/receipt...구조”를 반영해,
- 정상 조회:
getReceiptItem(savedReceipt.getId(), targetItem.getId(), userId)- USER_NOT_FOUND, RECEIPT_ITEM_NOT_FOUND, RECEIPT_ACCESS_DENIED
각 케이스를 명확히 분리한 점이 좋습니다.- 특히
입고_아이템_조회시_아이템이_존재하지_않으면_예외에서 실제 Receipt 엔티티를 생성해 유효한receiptId를 넘긴 뒤, itemId만 잘못 주는 패턴은 서비스의 실제 동작과 잘 맞습니다.개선 제안(선택 사항):
- 현재 조회 테스트에서는 “같은 상점이지만 다른 Receipt에 속한 아이템” 시나리오는 없고, 이 케이스는 수정/삭제 테스트에서만 다룹니다.
- 조회에도 같은 패턴의 테스트를 하나 추가하면(
RECEIPT_ITEM_ACCESS_DENIED기대), 단건 API 3종(GET/PATCH/DELETE)의 접근 제어 행태를 완전히 대칭적으로 검증할 수 있습니다.
1107-1430: 입고 아이템 수정/삭제 테스트가 경계 조건을 촘촘하게 잡고 있습니다.
- 수정 성공/실패 케이스에서 새 시그니처
updateReceiptItem(receiptId, itemId, req, userId)를 모두 반영했고,
- 다른 Receipt의 아이템 수정 시
RECEIPT_ITEM_ACCESS_DENIED,- 잘못된 receiptId(존재하지 않는 ID) 사용 시
RECEIPT_NOT_FOUND
등 실제 서비스 로직과 예외 타입을 정확히 매칭하고 있습니다.- 삭제 쪽도
- 성공 시 boxCount 재계산 검증,
- USER_NOT_FOUND / RECEIPT_ITEM_NOT_FOUND / RECEIPT_ACCESS_DENIED
케이스를 별도로 가져가서 회귀 가능성을 잘 줄였습니다.개선 제안(선택 사항):
- 테스트 전반에 걸쳐 Receipt/Order/Item fixture 생성 코드가 거의 동일하게 반복되는데, JUnit helper 메서드나 별도 TestFixture 클래스로 묶어두면 이후 시그니처 변화 시 수정 범위를 줄이는 데 도움이 됩니다.
src/test/java/com/almang/inventory/receipt/controller/ReceiptControllerTest.java (2)
68-81: ReceiptItemResponse 생성자 사용은 맞지만, 장기적으로는 빌더/팩토리 패턴이 더 안전해 보입니다.
- 현재 테스트가 DTO 생성자 시그니처 변경(예:
receiptItemId, receiptId, productId, ...)을 정확히 반영하고 있고, 다른 테스트들과 인자 순서도 일관적입니다. 동기화 자체는 잘 되어 있습니다.- 다만 파라미터 수가 많다 보니, 향후 필드 추가/순서 변경 시 테스트가 “컴파일은 되지만 잘못된 값”을 줄 가능성이 큽니다.
개선 제안:
- DTO에도 적용 가능하다면
ReceiptItemResponse.of(...)정적 팩토리나 빌더를 도입하고, 테스트에서는 의미 있는 파라미터명을 가진 팩토리/빌더를 사용하면 가독성과 안전성이 크게 올라갈 것입니다.
- 예:
ReceiptItemResponse.of(receiptItemId, receiptId, productId, boxCount, ...)- 혹은 Lombok @builder 등을 사용해
ReceiptItemResponse.builder().receiptId(...).productId(...).build()형태로 작성.
787-821: 입고 아이템 컨트롤러 테스트가 새 경로와 시그니처를 잘 검증하고 있습니다. 다만 에러 코드 시나리오 네이밍은 살짝 정리가 필요해 보입니다.좋은 점:
- 모든 아이템 관련 엔드포인트가
/api/v1/receipt/{receiptId}/items/{receiptItemId}형태로 바뀐 것을 테스트에서 명확히 검증하고 있습니다.
- 성공 케이스에서
jsonPath("$.data.receiptId").value(receiptId)까지 확인하는 부분이 특히 좋습니다.- 서비스 mock 역시 새 시그니처
getReceiptItem(anyLong(), anyLong(), anyLong())updateReceiptItem(anyLong(), anyLong(), any(UpdateReceiptItemRequest.class), anyLong())deleteReceiptItem(anyLong(), anyLong(), anyLong())
로 모두 통일되어 있어 컨트롤러와 서비스 인터페이스가 잘 맞습니다.- 각 예외 케이스(USER_NOT_FOUND, RECEIPT_NOT_FOUND, RECEIPT_ACCESS_DENIED, RECEIPT_ITEM_NOT_FOUND, RECEIPT_ITEM_ACCESS_DENIED)에 대한 HTTP status 및 응답 바디 구조를 세밀하게 검증하고 있어, 글로벌 예외 핸들러 변경 시에도 회귀를 잘 잡아줄 수 있습니다.
개선 제안(추천):
“다른 상점” 시나리오의 ErrorCode 정합성
- 서비스 레벨 테스트에서는
- “다른 상점의 아이템” →
RECEIPT_ACCESS_DENIED
로 검증하고 있는 반면, 이 컨트롤러 테스트에서는 같은 한글 설명을 쓰면서 mock이RECEIPT_ITEM_ACCESS_DENIED를 던지는 경우가 있습니다(조회/삭제 모두).- 실제 동작은 ReceiptService 쪽 구현을 따를 것이므로, 다음 중 하나로 정리하면 읽는 사람이 덜 헷갈립니다.
- (A) 이 컨트롤러 테스트의 mock도
RECEIPT_ACCESS_DENIED로 맞추기.- (B) 테스트 메서드 이름을 “아이템 접근 권한 없음” 등으로 일반화해서 ErrorCode와 시나리오를 1:1로 묶지 않기.
중복 상수 정리(선택 사항)
Long receiptId = 1L; Long receiptItemId = 1000L;패턴이 여러 테스트에서 반복되는데, 클래스 상단에 상수로 빼거나 helper 메서드에서 공통으로 만드는 식으로 정리하면 테스트가 더 간결해집니다.관련 개념은 Spring MVC의
@PathVariable활용과 REST 리소스 계층 구조(컬렉션/하위 리소스) 설계 가이드를 한번쯤 같이 참고하시면 좋겠습니다.Also applies to: 823-878, 881-931, 933-1096, 1098-1150
src/main/java/com/almang/inventory/receipt/controller/ReceiptController.java (1)
141-155: 입고 아이템 관련 엔드포인트가 REST스럽게 정리되었고, 서비스 호출도 일관적입니다.
- 버그 원인이었던
/receipt/receipt스타일 중복/모호 경로 대신,
- 조회:
GET /api/v1/receipt/{receiptId}/items/{receiptItemId}- 수정:
PATCH /api/v1/receipt/{receiptId}/items/{receiptItemId}- 삭제:
DELETE /api/v1/receipt/{receiptId}/items/{receiptItemId}
형태로 Receipt → Item 계층 구조가 명확해졌습니다.- 각 메서드가 공통적으로
userPrincipal에서 userId 추출- 로그에
userId, receiptId, receiptItemId모두 포함receiptService.*ReceiptItem(receiptId, receiptItemId, userId/req)호출
로 작성되어 있어, 서비스 레이어 변경사항과도 정확히 맞습니다.개선 제안(추천):
- 현재 API에서는 path variable로
receiptId를 받고, bodyUpdateReceiptItemRequest에도receiptId필드가 중복으로 존재합니다.
- 실제 서비스 로직은 path의
receiptId만 신뢰하고 body 쪽은 사용하지 않기 때문에, Swagger/OpenAPI 문서를 볼 때 “둘 중 무엇이 기준인지” 혼란이 생길 수 있습니다.- 향후 버전에서
- (A) body의
receiptId를 요청 스펙에서 제거하거나(deprecate → remove), 또는- (B) controller에서 path의
receiptId와 body의request.receiptId()가 일치하는지 간단히 검증해 주는
방향을 고려하면 API 일관성이 더 좋아질 것 같습니다.Also applies to: 156-171, 173-190, 192-207
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/almang/inventory/receipt/controller/ReceiptController.java(2 hunks)src/main/java/com/almang/inventory/receipt/service/ReceiptService.java(3 hunks)src/test/java/com/almang/inventory/receipt/controller/ReceiptControllerTest.java(21 hunks)src/test/java/com/almang/inventory/receipt/service/ReceiptServiceTest.java(13 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-22T18:12:13.161Z
Learnt from: JoonKyoLee
Repo: almang2/inventory-server PR: 66
File: src/main/java/com/almang/inventory/order/domain/OrderItem.java:41-53
Timestamp: 2025-11-22T18:12:13.161Z
Learning: In the almang2/inventory-server repository, OrderItem entity update methods (updateQuantity, updatePrice in src/main/java/com/almang/inventory/order/domain/OrderItem.java) do not require null checks because OrderService will validate parameters before calling these update methods, following the same pattern as Product entity updates.
<!--
Applied to files:
src/test/java/com/almang/inventory/receipt/controller/ReceiptControllerTest.javasrc/main/java/com/almang/inventory/receipt/service/ReceiptService.java
📚 Learning: 2025-11-20T10:43:47.489Z
Learnt from: JoonKyoLee
Repo: almang2/inventory-server PR: 33
File: src/main/java/com/almang/inventory/product/domain/Product.java:62-66
Timestamp: 2025-11-20T10:43:47.489Z
Learning: In the almang2/inventory-server repository, Product entity update methods (e.g., updateVendor in src/main/java/com/almang/inventory/product/domain/Product.java) do not require null checks on vendor parameters because ProductService validates vendor existence via findVendorByIdAndValidateAccess before calling update methods, and the vendor field has nullable=false constraint ensuring this.vendor is never null for persisted entities.
Applied to files:
src/test/java/com/almang/inventory/receipt/service/ReceiptServiceTest.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.
✨ 작업 내용
📝 적용 범위
/receipt📌 참고 사항
Summary by CodeRabbit
릴리스 노트
개선 사항
테스트
✏️ Tip: You can customize this high-level summary in your review settings.