Merged
Conversation
Walkthrough발주처 상세 조회 기능을 구현합니다. API 엔드포인트, 서비스 로직, 성공 메시지 상수를 추가하고 단위 테스트를 작성했습니다. 다만 테스트 코드에 중복 삽입이 발견됩니다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Service
participant UserRepo
participant VendorRepo
Client->>Controller: GET /api/v1/vendor/{vendorId}
Controller->>Service: getVendorDetail(vendorId, userId)
Service->>UserRepo: findById(userId)
UserRepo-->>Service: User
Service->>VendorRepo: findByIdAndUserId(vendorId, userId)
alt 접근 권한 있음
VendorRepo-->>Service: Vendor
Service->>Service: 로그 기록
Service-->>Controller: VendorResponse
Controller-->>Client: ApiResponse (GET_VENDOR_DETAIL_SUCCESS)
else 접근 권한 없음
VendorRepo-->>Service: Optional.empty()
Service-->>Controller: ✗ VENDOR_NOT_FOUND
Controller-->>Client: ApiResponse (Error)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes 주의 필요한 영역:
Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 (4)
src/main/java/com/almang/inventory/vendor/service/VendorService.java (1)
51-58: 조회 로직/트랜잭션 설정 모두 자연스럽습니다
getVendorDetail이findUserById와findVendorByIdAndValidateAccess를 재사용해서 권한 검증 플로우가updateVendor와 동일하게 맞춰진 점이 좋습니다.@Transactional(readOnly = true)설정도 조회 용도에 적합합니다.선택 사항으로, 추후 로그 모니터링을 강화하고 싶다면 아래처럼 userId도 같이 남기는 형태를 고려해볼 수 있습니다.
log.info("[VendorService] 발주처 상세 조회 성공 - vendorId: {}, userId: {}", vendor.getId(), user.getId());지금 구조로도 기능적으로는 문제 없습니다.
src/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.java (1)
239-291: 상세 조회 컨트롤러 테스트 구성이 탄탄합니다 (권한 오류 케이스만 더 있으면 좋겠습니다)
- 성공 케이스에서 HTTP status,
SuccessMessage.GET_VENDOR_DETAIL_SUCCESS, 그리고data의 각 필드를 모두 검증하고 있어서 회귀 방지에 도움이 됩니다.- 존재하지 않는 발주처 케이스에서
ErrorCode.VENDOR_NOT_FOUND의httpStatus,message,data부재까지 체크하는 것도 전역 예외 핸들러 계약을 잘 검증하고 있습니다.개선 제안(선택 사항):
- 서비스 레벨에서는 이미
VENDOR_ACCESS_DENIED를 검증하고 있으니, 컨트롤러에서도 다른 상점 발주처 상세 조회 시 권한 예외(아마 403) 가 적절히 매핑되는지 검증하는 테스트를 하나 더 추가하면 API 레벨 계약이 더 명확해집니다.
- 예시:
vendorService.getVendorDetail이VENDOR_ACCESS_DENIED를 던지도록 stubbing 하고, 컨트롤러 응답이 기대하는 HTTP 코드와 JSON 형태로 변환되는지 확인.- 관련 개념은 Spring MVC Test의 예외 처리 테스트 부분(Spring 공식 문서의 “Testing Spring MVC controllers” 장)을 함께 참고해 보시면 좋습니다.
기능 자체는 현재 테스트만으로도 잘 커버되고 있습니다.
src/test/java/com/almang/inventory/vendor/service/VendorServiceTest.java (1)
242-309: 상세 조회 서비스 테스트 3종 세트 구성이 좋습니다
- 정상 조회 테스트에서
VendorResponse의 각 필드를 엔티티 초기값과 비교해 검증하고 있어, 매핑 로직이 변경되었을 때 바로 감지할 수 있습니다.- 존재하지 않는 발주처/다른 상점 발주처 케이스에서 각각
VENDOR_NOT_FOUND,VENDOR_ACCESS_DENIED메시지를 검증하는 것도 도메인 규칙을 명확히 테스트하고 있습니다.- 기존 생성/수정 테스트와 스타일과도 일관돼서 읽기 편합니다.
추가로, 추후 검증 범위를 확장하고 싶다면 AssertJ의
extracting을 사용해 여러 필드를 한 번에 비교하는 패턴도 고려해 볼 수 있습니다. (예:assertThat(response).extracting("name", "channel", ...)형태 — AssertJ 공식 문서의 “Extracting values” 섹션 참고)지금 상태로도 테스트 품질은 충분히 좋습니다.
src/main/java/com/almang/inventory/vendor/controller/VendorController.java (1)
17-23: 상세 조회 엔드포인트 구현이 기존 패턴과 잘 맞습니다
@GetMapping("/{vendorId}")+@AuthenticationPrincipal CustomUserPrincipal조합이 기존 POST/PATCH 메서드와 동일한 스타일이라, 클라이언트/보안 설정 모두에서 예측 가능성이 높습니다.- 컨트롤러에서 userId만 추출하고 나머지 검증은 Service(
getVendorDetail)에 위임하는 구조도 계층 분리가 잘 되어 있습니다.- 응답은
ApiResponse.success(SuccessMessage.GET_VENDOR_DETAIL_SUCCESS.getMessage(), response)로 다른 API들과 동일한 포맷을 유지하고 있어 프런트엔드 입장에서도 사용성이 좋겠습니다.선택 사항으로, Swagger 문서에서 “발주처 상세 조회”라는 summary에 맞춰 description을 조금 더 구체적으로(예: “발주처 상세 정보를 조회하고, 현재 로그인한 사용자의 상점에 속한 발주처만 조회 가능합니다.”) 적어 두면 권한 제약 사항을 API 문서만 보고도 이해할 수 있어 도움이 됩니다. 이는 OpenAPI 문서화 가이드(예: summary/description 역할 분리)에 잘 맞는 패턴입니다.
기능/구현 자체는 그대로 사용해도 무방해 보입니다.
Also applies to: 65-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/almang/inventory/global/api/SuccessMessage.java(1 hunks)src/main/java/com/almang/inventory/vendor/controller/VendorController.java(2 hunks)src/main/java/com/almang/inventory/vendor/service/VendorService.java(1 hunks)src/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.java(2 hunks)src/test/java/com/almang/inventory/vendor/service/VendorServiceTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/main/java/com/almang/inventory/vendor/controller/VendorController.javasrc/main/java/com/almang/inventory/vendor/service/VendorService.javasrc/test/java/com/almang/inventory/vendor/service/VendorServiceTest.javasrc/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.java
🔇 Additional comments (2)
src/main/java/com/almang/inventory/global/api/SuccessMessage.java (1)
30-37: 발주처 상세 조회 성공 메시지 추가, 일관성 좋습니다
GET_VENDOR_DETAIL_SUCCESS("발주처 상세 조회 성공")상수가 기존 패턴(도메인별 구분 + 한글 메시지)과 잘 맞고, 컨트롤러/테스트에서도 동일하게 참조되고 있어 유지보수성 측면에서 깔끔합니다. 별도 수정 필요는 없어 보입니다.src/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.java (1)
9-10: GET 테스트용 import 추가 적절합니다
MockMvcRequestBuilders.get정적 import 추가가 아래 상세 조회 테스트에서 자연스럽게 사용되고 있고, 기존post,patch와 동일한 패턴이라 괜찮습니다.
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.
✨ 작업 내용
📝 적용 범위
/vendor📌 참고 사항
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.