Conversation
워크스루상점 정보를 부분적으로 수정할 수 있는 기능을 구현합니다. REST PATCH 엔드포인트에서 상점명, 활성화 상태, 기본 재고 확인 임계값에 대한 선택적 업데이트를 수용하며, 입력값 검증 후 서비스 계층을 통해 변경사항을 데이터베이스에 반영합니다. 변경사항
시퀀스 다이어그램sequenceDiagram
participant Client
participant Controller as StoreController
participant Service as StoreService
participant Repo as Store/UserRepository
participant Domain as Store Entity
Client->>Controller: PATCH /api/v1/store<br/>(UpdateStoreRequest, userId)
activate Controller
Note over Controller: 요청 검증 & 로깅
Controller->>Service: updateStore(request, userId)
activate Service
Service->>Repo: findUserById(userId)
activate Repo
Repo-->>Service: User found
deactivate Repo
Service->>Repo: findByUserId(user)
activate Repo
Repo-->>Service: Store entity
deactivate Repo
Note over Service: validateStoreName()<br/>validateThreshold()
alt Validation Failed
Service-->>Client: BaseException
else Validation Passed
Service->>Domain: updateName(name)
Service->>Domain: updateActivation(isActivate)
Service->>Domain: updateThreshold(threshold)
Service->>Repo: save(Store)
activate Repo
Repo-->>Service: Updated Store
deactivate Repo
Note over Service: UpdateStoreResponse.from(store)
Service-->>Controller: UpdateStoreResponse
end
deactivate Service
Controller-->>Client: ResponseEntity<br/>ApiResponse<UpdateStoreResponse>
deactivate Controller
리뷰 예상 소요 시간🎯 3 (보통) | ⏱️ ~25분 주의 깊게 검토할 영역:
관련된 PR
축시
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: 1
🧹 Nitpick comments (6)
src/main/java/com/almang/inventory/store/domain/Store.java (1)
47-57: 도메인 엔티티에 업데이트 메서드를 둔 선택이 좋습니다필드별 업데이트 메서드를 열어두어 서비스에서 부분 수정 로직을 깔끔하게 표현할 수 있는 점이 좋습니다.
다만 도메인 규칙(예: 이름 공백/길이 제한, 임계치 범위 등)을 한 곳에서 보장하려면, 현재 DTO·서비스에 흩어져 있을 검증 일부를 점진적으로 이 메서드들 안으로 옮겨오는 것도 고려해 볼 만합니다. 이미defaultCountCheckThreshold는 Bean Validation 어노테이션으로 0~1 범위를 보장하고 있으니, 이름 정규화(trim)나 추가 길이 체크도 여기서 함께 다루면 Store 상태 변경 진입점이 더 명확해집니다. 이런 도메인 불변식 캡슐화 패턴은 DDD 자료와 Bean Validation 공식 문서를 같이 보시면 설계 방향에 도움이 됩니다.src/main/java/com/almang/inventory/store/dto/response/UpdateStoreResponse.java (1)
1-20: UpdateStoreResponse 레코드 설계가 단순하고 명확합니다도메인 엔티티를 직접 노출하지 않고 응답 전용 레코드로 감싸고, 정적 팩터리
from(Store)로 매핑 책임을 분리한 구조가 깔끔합니다.
장기적으로 Store 필드 구조가 바뀌거나 응답 요구사항이 늘어날 수 있으니, 전용 매퍼(예: MapStruct 기반 매퍼나 수동 매퍼 클래스)를 하나 두고 이 레코드를 그 매퍼를 통해 생성하도록 추상화를 한 단계 더 두면 컨트롤러·서비스가 도메인 변경에 덜 민감해집니다. Mapper 패턴 관련 예시는 MapStruct 공식 문서를 참고해 보셔도 좋겠습니다.src/test/java/com/almang/inventory/store/service/StoreServiceTest.java (2)
31-155: 부분 수정·동시 수정 시나리오까지 잘 커버한 통합 테스트입니다name/임계치/활성화 여부를 각각·조합으로 수정하는 케이스를 분리해 둬서, “수정하고 싶은 부분만 수정”이라는 요구사항을 테스트 이름만 봐도 이해할 수 있는 점이 좋습니다.
다만 현재는 DB에서 다시 조회한Store만 상세히 검증하고UpdateStoreResponse는isNotNull()만 확인하고 있는데, 응답 DTO 필드(storeId,name,isActivate,defaultCountCheckThreshold)도 함께 검증해 두면 서비스 로직뿐 아니라 API 계약과 매핑까지 한 번에 보호할 수 있습니다. 예를 들어assertThat(response.name()).isEqualTo("수정된 상점 이름")처럼 응답 값과 엔티티 값을 나란히 비교해 두면, 매핑 코드가 변경되었을 때 회귀를 더 빨리 잡을 수 있습니다.
157-226: 예외 케이스 테스트도 충실하지만, 검증 단위를 조금 더 정교하게 다듬을 수 있습니다USER_NOT_FOUND, 상점 이름 길이 초과, 임계치 범위 초과(<0, >1) 같은 주요 실패 경로를 개별 테스트로 잘 분리해 두신 점 좋습니다.
다만.hasMessageContaining(ErrorCode.XXX.getMessage())에 의존하면 나중에 에러 메시지 문구만 바뀌어도 테스트가 깨질 수 있어 유지보수성이 떨어질 수 있습니다.BaseException이ErrorCode를 필드로 노출한다면, AssertJ의extracting("errorCode").isEqualTo(ErrorCode.XXX)처럼 코드 자체를 검증하는 방식으로 바꾸는 것을 추천드립니다. 또 임계치 범위 테스트 두 개(-0.1,1.1)는 JUnit@ParameterizedTest로 하나의 테스트로 묶으면 중복을 줄이면서도 의도는 그대로 드러낼 수 있습니다.src/main/java/com/almang/inventory/store/dto/request/UpdateStoreRequest.java (1)
9-9: 빈 문자열 검증 추가를 권장합니다.
name필드에@Size(max = 20)만 있어서 빈 문자열("")이나 공백 문자열(" ")이 허용됩니다. 상점명을 수정할 때 의미 없는 값이 저장되지 않도록@NotBlank어노테이션을 추가하는 것이 좋습니다.다음과 같이 수정하세요:
- @Size(max = 20) String name, + @NotBlank @Size(max = 20) String name,참고: Bean Validation 스펙에서
@NotBlank는 null이 아니면서 공백이 아닌 문자를 최소 하나 포함해야 합니다.src/main/java/com/almang/inventory/store/service/StoreService.java (1)
30-42: 최소 하나의 필드 제공 여부를 검증하는 것이 좋습니다.현재는 모든 필드가 null인 요청도 허용되어 실제로 아무것도 변경하지 않고 성공 응답을 반환합니다. 사용자 경험과 API 명확성을 위해 최소 하나의 필드가 제공되었는지 검증하는 것을 권장합니다.
메서드 시작 부분에 검증 로직을 추가하세요:
public UpdateStoreResponse updateStore(UpdateStoreRequest request, Long userId) { User user = findUserById(userId); Store store = user.getStore(); + if (request.name() == null + && request.defaultCountCheckThreshold() == null + && request.isActivate() == null) { + throw new BaseException(ErrorCode.NO_FIELD_TO_UPDATE); + } + log.info("[StoreService] 상점 정보 수정 요청 - userId: {}, storeId: {}", userId, store.getId());참고: 이런 검증은 비즈니스 규칙이므로 서비스 레이어에서 처리하는 것이 적절합니다.
📜 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/store/controller/StoreController.java(1 hunks)src/main/java/com/almang/inventory/store/domain/Store.java(1 hunks)src/main/java/com/almang/inventory/store/dto/request/UpdateStoreRequest.java(1 hunks)src/main/java/com/almang/inventory/store/dto/response/UpdateStoreResponse.java(1 hunks)src/main/java/com/almang/inventory/store/service/StoreService.java(1 hunks)src/test/java/com/almang/inventory/store/controller/StoreControllerTest.java(1 hunks)src/test/java/com/almang/inventory/store/service/StoreServiceTest.java(1 hunks)
🔇 Additional comments (6)
src/main/java/com/almang/inventory/global/api/SuccessMessage.java (1)
10-22: 상점 수정 성공 메시지 상수 추가 👍STORE 섹션의 기존 패턴(
CREATE_STORE_SUCCESS)과 이름·문구가 잘 맞아서 응답 메시지 일관성이 유지됩니다.
향후 국제화(i18n)가 필요해지면, 이 enum 값을 메시지 키처럼 사용하고 실제 문자열은 MessageSource/리소스 번들로 분리하는 구조도 한 번 고려해 보시면 좋겠습니다. 스프링 메시지 국제화 가이드를 같이 보면 설계 방향을 잡는 데 도움이 됩니다.src/main/java/com/almang/inventory/store/controller/StoreController.java (1)
30-42: PATCH 상점 수정 엔드포인트 구성이 명확합니다요청 DTO에
@Valid를 적용하고 인증 정보에서userId만 꺼내 서비스에 위임한 구조라, 책임 분리가 잘 되어 있고 유지보수하기 좋아 보입니다. 응답도 공통ApiResponse+SuccessMessage.UPDATE_STORE_SUCCESS패턴을 그대로 따라가서 클라이언트 입장에서도 예측 가능하겠습니다.
실서비스 관점에서 한 가지만 더 점검해 볼 부분은userPrincipal이 null일 수 있는지 보안 설정입니다. 익명 접근이 완전히 차단되어 있다면 지금도 충분하지만, 혹시 모를 설정 변경에 대비해 메서드 초반에 방어적 체크나 Spring Security의@PreAuthorize를 추가하는 것도 선택지입니다. 또한 OpenAPI/Swagger 설정 쪽에서 이 엔드포인트가 인증이 필요한 API임을 명시(security섹션)해 두면 프론트엔드/외부 소비자들이 스펙을 읽기 더 편해집니다.src/main/java/com/almang/inventory/store/dto/request/UpdateStoreRequest.java (1)
8-13: 부분 수정 기능 구현이 깔끔합니다! 👍nullable 필드를 활용한 부분 업데이트 구조가 요구사항을 잘 반영하고 있습니다.
src/main/java/com/almang/inventory/store/service/StoreService.java (1)
23-46: 트랜잭션 처리와 로깅이 잘 구현되었습니다.JPA의 더티 체킹(dirty checking)을 활용한 엔티티 업데이트 방식이 적절하며, 로깅도 요청/성공 시점에 명확하게 남기고 있습니다.
src/test/java/com/almang/inventory/store/controller/StoreControllerTest.java (2)
51-125: 테스트 커버리지가 훌륭합니다! 🎯성공 케이스, 사용자 미존재 에러, 그리고 입력값 검증 실패 시나리오를 모두 커버하고 있습니다. 특히 Line 111에서 21자 이름으로 경계값 테스트를 수행한 점이 좋습니다.
43-49: 인증 헬퍼 메서드 재사용이 깔끔합니다.테스트 인증 토큰 생성 로직을 별도 메서드로 분리해 중복을 제거한 점이 좋습니다.
| private void validateStoreName(String storeName) { | ||
| if (storeName.length() > 20) { | ||
| throw new BaseException(ErrorCode.STORE_NAME_IS_LONG); | ||
| } | ||
| } | ||
|
|
||
| private void validateDefaultCountCheckThreshold(BigDecimal defaultCountCheckThreshold) { | ||
| if (defaultCountCheckThreshold.compareTo(BigDecimal.ZERO) < 0 | ||
| || defaultCountCheckThreshold.compareTo(BigDecimal.ONE) > 0) { | ||
| throw new BaseException(ErrorCode.DEFAULT_COUNT_CHECK_THRESHOLD_NOT_IN_RANGE); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
중복 검증 로직을 제거하세요.
Lines 54-56의 상점명 길이 검증과 Lines 60-62의 threshold 범위 검증은 UpdateStoreRequest의 Bean Validation 어노테이션(@SiZe, @DecimalMin, @DecimalMax)과 중복됩니다. Controller에서 @Valid 어노테이션으로 DTO 검증이 이미 수행되므로, 서비스 레이어의 이 메서드들은 불필요합니다.
중복 검증 메서드를 제거하고 간결하게 리팩토링하세요:
if (request.name() != null) {
- validateStoreName(request.name());
store.updateName(request.name());
}
if (request.defaultCountCheckThreshold() != null) {
- validateDefaultCountCheckThreshold(request.defaultCountCheckThreshold());
store.updateThreshold(request.defaultCountCheckThreshold());
}
- private void validateStoreName(String storeName) {
- if (storeName.length() > 20) {
- throw new BaseException(ErrorCode.STORE_NAME_IS_LONG);
- }
- }
-
- private void validateDefaultCountCheckThreshold(BigDecimal defaultCountCheckThreshold) {
- if (defaultCountCheckThreshold.compareTo(BigDecimal.ZERO) < 0
- || defaultCountCheckThreshold.compareTo(BigDecimal.ONE) > 0) {
- throw new BaseException(ErrorCode.DEFAULT_COUNT_CHECK_THRESHOLD_NOT_IN_RANGE);
- }
- }참고: Spring의 @Valid 어노테이션은 컨트롤러 레벨에서 자동으로 검증하며, 실패 시 MethodArgumentNotValidException을 발생시킵니다. 서비스 레이어에서 다시 검증하는 것은 불필요한 중복입니다.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/almang/inventory/store/service/StoreService.java around
lines 53 to 64, remove the two private validation methods validateStoreName and
validateDefaultCountCheckThreshold because those checks are already enforced by
Bean Validation annotations on UpdateStoreRequest and handled via @Valid in the
controller; delete their declarations and any internal calls to them so the
service no longer duplicates DTO validation, keeping the service focused on
business logic only.
1 participant
✨ 작업 내용
📝 적용 범위
/store📌 참고 사항
Summary by CodeRabbit
릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings.