Merged
Conversation
개요발주처 목록 조회 기능을 구현하였으며, 페이지네이션과 필터링(활성화 상태, 이름) 지원을 추가하였습니다. PaginationUtil 유틸리티 클래스를 신규 도입하여 페이지 요청 생성을 표준화하였고, ProductService도 함께 마이그레이션되었습니다. 변경사항
시퀀스 다이어그램sequenceDiagram
participant Client
participant VendorController
participant VendorService
participant VendorRepository
participant Database
Client->>VendorController: GET /api/v1/vendor?page=1&size=20&isActivate=true&name=keyword
VendorController->>VendorService: getVendorList(userId, page, size, isActivate, nameKeyword)
VendorService->>VendorService: PaginationUtil.createPageRequest(page, size, "name")
VendorService->>VendorService: findVendorsByFilter(storeId, isActivate, nameKeyword, pageable)
VendorService->>VendorRepository: 조건에 맞는 메서드 호출<br/>(활성화/이름 필터 조합)
VendorRepository->>Database: SELECT * FROM vendor WHERE ...
Database-->>VendorRepository: 페이지네이션된 결과 반환
VendorRepository-->>VendorService: Page<Vendor>
VendorService->>VendorService: VendorResponse로 매핑
VendorService-->>VendorController: PageResponse<VendorResponse>
VendorController-->>Client: ApiResponse<PageResponse<VendorResponse>>
예상 코드 리뷰 노력🎯 3 (중간) | ⏱️ ~20-25분 추가 주의 사항
관련 Pull Request
축시
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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: 2
🧹 Nitpick comments (4)
src/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.java (2)
294-385: 테스트 커버리지 개선 제안현재 개별 필터 테스트는 잘 작성되었습니다. 하지만
isActivate와name필터를 동시에 사용하는 케이스에 대한 컨트롤러 테스트가 누락되어 있습니다. 서비스 레이어에는 해당 테스트(활성여부와_이름_검색이_둘다_적용되어_조회된다)가 존재하지만, 컨트롤러 레이어에서도 통합 시나리오 검증이 필요합니다.다음과 같은 테스트 추가를 고려해보세요:
@Test void 발주처_목록_조회_시_활성_필터와_이름검색이_함께_적용된다() throws Exception { // given PageResponse<VendorResponse> response = new PageResponse<>( List.of( new VendorResponse(1L, "사과 공장", VendorChannel.KAKAO, "010-1111-1111", "메모", true, 1L) ), 1, 20, 1, 1, true ); when(vendorService.getVendorList(anyLong(), any(), any(), any(), any())) .thenReturn(response); // when & then mockMvc.perform(get("/api/v1/vendor") .with(authentication(auth())) .param("isActivate", "true") .param("name", "사과") .contentType(MediaType.APPLICATION_JSON)) .andExpect(status().isOk()) .andExpect(jsonPath("$.data.content.length()").value(1)) .andExpect(jsonPath("$.data.content[0].activated").value(true)) .andExpect(jsonPath("$.data.content[0].name").value("사과 공장")); }
309-310: Mock 매처를 더 구체적으로 작성하는 것을 고려해보세요현재
any()매처를 모든 파라미터에 사용하고 있어 테스트가 느슨합니다. 실제 전달되는 값과 일치하는지 검증하면 테스트의 정확도가 높아집니다.예시:
when(vendorService.getVendorList(eq(1L), isNull(), isNull(), isNull(), isNull())) .thenReturn(response);src/main/java/com/almang/inventory/global/util/PaginationUtil.java (2)
11-16: 유틸리티 클래스 구조는 좋습니다만, 확장성 개선 여지가 있습니다현재 구현은 오름차순(ASC) 정렬만 지원합니다. 향후 내림차순 정렬이 필요한 경우를 대비해 정렬 방향을 파라미터로 받는 오버로드 메서드를 고려해보세요.
예시:
public static PageRequest createPageRequest(Integer page, Integer size, String sortBy) { return createPageRequest(page, size, sortBy, Direction.ASC); } public static PageRequest createPageRequest(Integer page, Integer size, String sortBy, Direction direction) { int pageIndex = (page == null || page < 1) ? 0 : page - 1; int pageSize = (size == null || size < 1) ? 20 : size; return PageRequest.of(pageIndex, pageSize, Sort.by(direction, sortBy)); }참고: 현재 요구사항에는 ASC만으로 충분하다면 나중에 필요할 때 추가하는 것도 좋은 접근입니다(YAGNI 원칙).
11-16: sortBy 필드 검증 고려사항
sortBy파라미터에 유효하지 않은 필드명이 전달되면 런타임에 JPA 예외가 발생할 수 있습니다. 현재는 호출하는 쪽에서 하드코딩된 상수("name")를 사용하므로 문제없지만, 향후 확장 시 참고하세요.가능한 개선 방안:
- 허용된 정렬 필드 목록을 검증하는 로직 추가
- Enum으로 정렬 필드를 정의하여 타입 안전성 확보
- 또는 현재처럼 서비스 레이어에서 하드코딩 유지 (더 간단함)
현 시점에서는 3번이 실용적인 선택입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/com/almang/inventory/global/api/SuccessMessage.java(1 hunks)src/main/java/com/almang/inventory/global/util/PaginationUtil.java(1 hunks)src/main/java/com/almang/inventory/product/service/ProductService.java(2 hunks)src/main/java/com/almang/inventory/vendor/controller/VendorController.java(3 hunks)src/main/java/com/almang/inventory/vendor/repository/VendorRepository.java(1 hunks)src/main/java/com/almang/inventory/vendor/service/VendorService.java(4 hunks)src/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.java(2 hunks)src/test/java/com/almang/inventory/vendor/service/VendorServiceTest.java(2 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/product/service/ProductService.javasrc/main/java/com/almang/inventory/vendor/repository/VendorRepository.javasrc/main/java/com/almang/inventory/vendor/service/VendorService.javasrc/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.javasrc/test/java/com/almang/inventory/vendor/service/VendorServiceTest.java
🧬 Code graph analysis (2)
src/main/java/com/almang/inventory/product/service/ProductService.java (1)
src/main/java/com/almang/inventory/global/util/PaginationUtil.java (1)
PaginationUtil(7-17)
src/main/java/com/almang/inventory/vendor/service/VendorService.java (1)
src/main/java/com/almang/inventory/global/util/PaginationUtil.java (1)
PaginationUtil(7-17)
🔇 Additional comments (6)
src/main/java/com/almang/inventory/global/api/SuccessMessage.java (1)
34-34: 깔끔한 추가입니다! 👍기존 패턴과 일관성 있게 잘 작성되었습니다.
src/main/java/com/almang/inventory/product/service/ProductService.java (1)
84-84: 중복 제거 리팩토링 잘하셨습니다! 🎯공통 로직을
PaginationUtil로 중앙화하여 코드 중복을 제거하고 유지보수성을 높였습니다. DRY 원칙을 잘 따른 좋은 사례입니다.src/test/java/com/almang/inventory/vendor/service/VendorServiceTest.java (1)
312-455: 통합 테스트 설계가 훌륭합니다! 💯다양한 필터 조합을 모두 검증하는 포괄적인 테스트 케이스를 작성하셨네요. 특히 결합 필터 테스트까지 포함되어 있어 엣지 케이스 대응이 완벽합니다.
src/main/java/com/almang/inventory/vendor/controller/VendorController.java (2)
82-100: 컨트롤러 구현이 깔끔합니다! 👏기존 패턴을 잘 따르고 있으며, 로깅과 문서화도 적절히 추가되었습니다.
88-88: 네이밍 일관성 문제 확인됨 — 선택적 개선 권장조사 결과, 지적사항이 정확합니다. VendorController의
isActivate파라미터는 Vendor 엔티티 필드activated와 명칭이 일치하지 않습니다.프로젝트 현황:
- Vendor 엔티티: 필드명
activated↔ 파라미터명isActivate(불일치)- Store 엔티티: 필드명
isActivate(일관성 있음)- Product 엔티티: 필드명
activated(Vendor와 동일 패턴)주목할 점:
isActivate파라미터는 값에 상관없이 존재 여부만으로 필터링을 결정합니다. 서비스 로직에서isActivate != null체크 후 항상findAllByStoreIdAndActivatedTrue()를 호출하므로, 이는 "활성 항목만 보기" 필터 토글로 설계되었습니다. false 값을 지원하지 않는 이유가 여기에 있습니다.개선 방향 (선택사항):
- Store와의 일관성을 위해
activated→isActivate로 통일- 또는 모든 엔티티를
activated로 통일하되, 파라미터명을onlyActive또는activeOnly로 명확히 하여 필터 토글 의도를 드러내기현재 구조도 기능상 문제는 없으나, 엔티티 필드와 DTO 필드명을 일치시키면 유지보수성이 향상될 것입니다.
src/main/java/com/almang/inventory/vendor/service/VendorService.java (1)
108-134: 필터 로직 재설계 제안현재 4개의 if 분기로 구현되어 있는데,
isActivate의 true/false를 구분하면 더 많은 케이스가 필요합니다. 로직을 더 명확하게 표현할 수 있습니다.다음과 같이 개선할 수 있습니다:
private Page<Vendor> findVendorsByFilter( Long storeId, Boolean isActivate, String nameKeyword, PageRequest pageable ) { boolean hasName = nameKeyword != null && !nameKeyword.isBlank(); boolean hasActivateFilter = isActivate != null; if (!hasActivateFilter && !hasName) { return vendorRepository.findAllByStoreId(storeId, pageable); } if (!hasActivateFilter) { return vendorRepository.findAllByStoreIdAndNameContainingIgnoreCase( storeId, nameKeyword, pageable ); } if (!hasName) { return isActivate ? vendorRepository.findAllByStoreIdAndActivatedTrue(storeId, pageable) : vendorRepository.findAllByStoreId(storeId, pageable); } // Both filters present if (isActivate) { return vendorRepository.findAllByStoreIdAndActivatedTrueAndNameContainingIgnoreCase( storeId, nameKeyword, pageable ); } else { return vendorRepository.findAllByStoreIdAndNameContainingIgnoreCase( storeId, nameKeyword, pageable ); } }이렇게 하면
isActivate=false케이스도 올바르게 처리됩니다.⛔ Skipped due to learnings
Learnt from: JoonKyoLee Repo: almang2/inventory-server PR: 35 File: src/main/java/com/almang/inventory/product/service/ProductService.java:144-170 Timestamp: 2025-11-20T16:57:53.876Z Learning: almang2/inventory-server 저장소의 품목 목록 조회 API에서 isActivate 파라미터는 활성화된 품목만 조회하는 용도로 설계되었으며, false 값을 지원하지 않습니다.
Comment on lines
+10
to
+22
| Page<Vendor> findAllByStoreId(Long storeId, Pageable pageable); | ||
|
|
||
| Page<Vendor> findAllByStoreIdAndActivatedTrue( | ||
| Long storeId, Pageable pageable | ||
| ); | ||
|
|
||
| Page<Vendor> findAllByStoreIdAndNameContainingIgnoreCase( | ||
| Long storeId, String name, Pageable pageable | ||
| ); | ||
|
|
||
| Page<Vendor> findAllByStoreIdAndActivatedTrueAndNameContainingIgnoreCase( | ||
| Long storeId, String name, Pageable pageable | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Repository 메서드가 불완전합니다
현재 activated=true인 경우만 필터링하는 메서드가 있지만, activated=false를 조회하는 메서드가 없습니다. VendorService의 필터 로직 버그와 연관된 이슈입니다.
완전한 필터링 지원을 위해 다음 메서드들을 추가하세요:
Page<Vendor> findAllByStoreIdAndActivatedFalse(
Long storeId, Pageable pageable
);
Page<Vendor> findAllByStoreIdAndActivatedFalseAndNameContainingIgnoreCase(
Long storeId, String name, Pageable pageable
);또는, Spring Data JPA Specification을 사용하여 동적 쿼리를 구성하는 것도 좋은 대안입니다:
참고: Spring Data JPA Specifications
🤖 Prompt for AI Agents
In src/main/java/com/almang/inventory/vendor/repository/VendorRepository.java
around lines 10 to 22, repository methods only support filtering activated=true
but lack counterparts for activated=false which breaks service-level filtering;
add two methods to support inactive vendors: declare Page<Vendor>
findAllByStoreIdAndActivatedFalse(Long storeId, Pageable pageable) and
Page<Vendor> findAllByStoreIdAndActivatedFalseAndNameContainingIgnoreCase(Long
storeId, String name, Pageable pageable), or alternatively refactor to use
Spring Data JPA Specifications to build dynamic queries for activated true/false
and name filtering.
Comment on lines
+118
to
+121
| // 활성 여부 | ||
| if (isActivate != null && !hasName) { | ||
| return vendorRepository.findAllByStoreIdAndActivatedTrue(storeId, pageable); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🚨 필터 로직에 버그가 있습니다
Line 119에서 isActivate != null만 체크하고 있어, isActivate=false로 요청해도 활성화된 발주처만 조회됩니다. isActivate의 실제 값을 확인해야 합니다.
다음과 같이 수정이 필요합니다:
// 활성 여부
- if (isActivate != null && !hasName) {
- return vendorRepository.findAllByStoreIdAndActivatedTrue(storeId, pageable);
+ if (isActivate != null && isActivate && !hasName) {
+ return vendorRepository.findAllByStoreIdAndActivatedTrue(storeId, pageable);
}또한 Line 130의 결합 필터 로직도 동일하게 수정해야 합니다:
// 활성 여부 + 이름 검색
- return vendorRepository.findAllByStoreIdAndActivatedTrueAndNameContainingIgnoreCase(
- storeId, nameKeyword, pageable
- );
+ if (isActivate) {
+ return vendorRepository.findAllByStoreIdAndActivatedTrueAndNameContainingIgnoreCase(
+ storeId, nameKeyword, pageable
+ );
+ } else {
+ return vendorRepository.findAllByStoreIdAndNameContainingIgnoreCase(
+ storeId, nameKeyword, pageable
+ );
+ }동일한 버그가 ProductService에도 존재하는지 확인이 필요합니다.
🏁 Script executed:
#!/bin/bash
# ProductService의 필터 로직 확인
echo "=== ProductService filter logic ==="
rg -n -A5 "if \(isActivate != null" --type=java src/main/java/com/almang/inventory/product/service/ProductService.javaLength of output: 257
VendorService의 설계 의도를 확인하기 위해 추가 검증이 필요합니다.
추가 검증을 위해 VendorService 컨트롤러 레이어와 메서드 문서를 확인해야 합니다.
VendorService의 컨트롤러 엔드포인트를 확인해야 합니다. 현재까지의 분석으로는 isActivate 파라미터의 설계 의도를 완전히 파악하기 어렵습니다.
또한 학습된 내용을 참고하면, ProductService의 `isActivate` 파라미터가 false 값을 지원하지 않는다고 명시되어 있습니다. 이를 바탕으로 최종 검증을 진행하겠습니다.
**원본 리뷰 코멘트가 부정확합니다.**
학습 기록에 따르면, ProductService의 `isActivate` 파라미터는 **false 값을 지원하지 않도록 설계**되었습니다. VendorService도 동일한 설계 패턴을 따르고 있으므로:
- `isActivate == null`: 활성화 여부로 필터링하지 않음
- `isActivate == true`(null이 아닐 때): 활성화된 항목만 조회
- `isActivate == false`: 지원하지 않음
현재 코드는 `isActivate != null`을 확인하는 것이 올바른 구현입니다. 이는 버그가 아니라 **의도된 설계**입니다. Spring Data JPA의 `ActivatedTrue` 메서드 명명 규칙도 이를 뒷받침합니다.
리뷰 코멘트를 제시한 제안은 false 값을 지원한다는 잘못된 가정에 기반하고 있습니다.
[incorrect_review_comment]
---
🏁 Script executed:
```shell
#!/bin/bash
# VendorService 컨트롤러 찾기 및 getVendorList 엔드포인트 확인
echo "=== Looking for VendorController ==="
find . -name "*VendorController*" -type f
echo -e "\n=== Searching for endpoint calling getVendorList ==="
rg -n "getVendorList|isActivate" --type=java -A5 -B5 | head -100
Length of output: 10730
VendorController 엔드포인트를 확인하여 API 계약을 명확히 하겠습니다.
---
🏁 Script executed:
```shell
#!/bin/bash
# VendorController에서 getVendorList 엔드포인트 확인
echo "=== VendorController getVendorList endpoint ==="
rg -n "getVendorList|@RequestParam" -A10 -B5 git/src/main/java/com/almang/inventory/vendor/controller/VendorController.java | head -60
Length of output: 166
---
🏁 Script executed:
```shell
#!/bin/bash
# VendorController에서 getVendorList 엔드포인트 확인 (경로 수정)
echo "=== VendorController getVendorList endpoint ==="
rg -n "getVendorList|@RequestParam" -A10 -B5 src/main/java/com/almang/inventory/vendor/controller/VendorController.java | head -80
Length of output: 1236
VendorRepository의 쿼리 메서드를 확인하여 false 케이스 처리 가능성을 검증하겠습니다.
---
🏁 Script executed:
```shell
#!/bin/bash
# VendorRepository 메서드 확인
echo "=== VendorRepository methods ==="
rg -n "findAllByStoreId" src/main/java/com/almang/inventory/vendor/repository/VendorRepository.java -A2
Length of output: 510
필터 로직에 실제 버그가 있습니다 - 원본 리뷰가 정확합니다.
VendorRepository에 사용 가능한 메서드를 확인한 결과:
findAllByStoreId(필터 없음)findAllByStoreIdAndActivatedTrue(활성화만)findAllByStoreIdAndNameContainingIgnoreCase(필터 없음 + 이름)findAllByStoreIdAndActivatedTrueAndNameContainingIgnoreCase(활성화 + 이름)
문제점:
- 컨트롤러는
Boolean isActivate를 수락합니다 (true, false, null 모두 가능) - 서비스 라인 119-121:
isActivate != null만 체크하므로,isActivate=false전송 시에도findAllByStoreIdAndActivatedTrue를 호출합니다 - 결과: false와 true가 동일한 결과(활성화된 발주처만) 반환 → 데이터 무결성 문제
- 테스트도 false 케이스를 테스트하지 않음
설계 의도:
ProductService 학습 내용에 따르면 isActivate는 false를 지원하지 않는 설계입니다. 하지만 API가 false를 수락하고 서비스에서 제대로 처리하지 않으므로 버그입니다.
수정 방안:
-
즉시 수정 - 원본 리뷰의 제안대로 isActivate 값을 확인:
- 라인 119:
if (isActivate != null && isActivate && !hasName) - 라인 130+: isActivate 값에 따른 분기 처리
- 라인 119:
-
근본 해결 - false를 지원하지 않으려면:
- 컨트롤러에서
@Range또는 커스텀 검증으로 false 값 거부 - 또는
Boolean대신 Enum 사용하여 API 계약 명확히 하기
- 컨트롤러에서
원본 리뷰 코멘트의 수정 제안이 타당합니다.
🤖 Prompt for AI Agents
In src/main/java/com/almang/inventory/vendor/service/VendorService.java around
lines 118-121, the current check only tests isActivate != null causing
isActivate=false to be treated like true; change the branching to explicitly
check for true (e.g., if (isActivate != null && isActivate && !hasName)) and
update subsequent conditional branches to handle all three cases
(isActivate==true, isActivate==false, isActivate==null) combined with hasName to
call the correct repository methods (no filter, activated-only, name-only, both
activated and name); also add or update unit tests to cover the false case to
prevent regression.
This was referenced Nov 22, 2025
Merged
Merged
Merged
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
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.