Conversation
Walkthrough제품 soft delete 기능을 구현하는 변경입니다. Product 도메인에 soft delete 처리를 추가하고, ProductController에 DELETE 엔드포인트를 신설한 후, ProductService에 삭제 로직을 포함하며, 여러 테스트 클래스에 DiscordErrorNotifier 의존성을 추가합니다. Changes
🎯 코드 리뷰 평가🎯 3 (Moderate) | ⏱️ ~30분 추가 검토 권장 사항
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 (10)
src/main/java/com/almang/inventory/product/domain/Product.java (3)
19-20: Soft delete 전략 일관성 정리가 필요해 보입니다 (@SQLDelete vs delete 메서드).현재
@SQLDelete(sql = "UPDATE products SET deleted_at = NOW() WHERE product_id = ?")와@Where(clause = "deleted_at IS NULL")로 Hibernate 레벨 soft delete를 쓰면서, 동시에delete()메서드에서this.deletedAt = LocalDateTime.now();로 애플리케이션 레벨에서도 값을 세팅하고 있습니다.
이렇게 두 가지 경로가 섞이면 “실제 삭제 시점·주체가 어디인지”를 읽는 사람이 헷갈리기 쉽고, DB 서버 시간(NOW())과 애플리케이션 서버 시간(LocalDateTime.now())이 어긋날 여지도 있습니다.권장 방향 예시는 아래 둘 중 하나로 정리하는 것입니다.
Hibernate 기반 전략으로 통일
@SQLDelete+@Where만 사용하고, 서비스에서는productRepository.delete(product)만 호출.- 이 경우
delete()메서드는 제거하거나, 단순히repository.delete(this)를 감싸는 헬퍼 정도로 사용.- DB 시간이 진실(source of truth)이므로
deleted_at컬럼을 DB가 관리하게 하고 싶다면,@Column(insertable = false, updatable = false)도 고려할 수 있습니다.엔티티 메서드 기반 soft delete로 통일
@SQLDelete를 제거하고, 서비스에서product.delete();후save를 호출하는 방식으로 일관.- 이 경우
deletedAt은 항상 애플리케이션에서 관리되며, DB 함수 의존을 줄일 수 있습니다.또한,
@Where("deleted_at IS NULL")는 Hibernate가 생성하는 JPQL에는 자동으로 붙지만,nativeQuery = true인 쿼리에는 적용되지 않습니다. 나중에 Product 관련 native 쿼리를 추가할 경우deleted_at IS NULL조건을 수동으로 붙여주는 것을 팀 규칙으로 정해두면 좋겠습니다.Hibernate soft delete 관련 개념은 공식 문서의
@SQLDelete,@Where섹션을 함께 참고해 두시면 팀원 교육에도 도움이 됩니다.
67-68:deletedAt필드 매핑 자체는 적절합니다.
@Column(name = "deleted_at")+LocalDateTime deletedAt조합은 일반적인 soft delete 컬럼 매핑으로 문제 없어 보입니다.
다만 위에서 언급한 것처럼 soft delete를 DB에서만 관리할지, 엔티티에서 세팅할지 전략을 정한 뒤에insertable/updatable여부나 인덱스 추가(deleted_at+store_id등)를 함께 정리하면, 대량 데이터 환경에서도 조회 성능과 유지보수성이 좋아집니다.
118-120:delete()메서드의 역할을 명확히 정의해 두는 것이 좋겠습니다.현재
delete()는 단순히deletedAt에LocalDateTime.now()를 세팅만 하고 있어, 호출자가 이 메서드 후에save를 할지,delete를 할지 코드만 보고는 알기 어렵습니다. 위의 soft delete 전략을 정리한 뒤에:
- 만약
repository.save(product)기반 soft delete를 표준으로 삼는다면, 서비스 레이어에서 반드시delete()를 호출하는 규칙을 문서화하고,- 반대로
repository.delete(product)만 쓰는 구조라면,delete()는 제거하거나 테스트용/도메인 이벤트용 등 분명한 용도를 주는 편이 좋습니다.이렇게 해두면 삭제 플로우를 추적할 때 “삭제 진입점이 어디인지”가 훨씬 명확해집니다.
src/test/java/com/almang/inventory/admin/controller/AdminControllerTest.java (1)
14-14: DiscordErrorNotifier 목킹 추가 방향 적절
DiscordErrorNotifier를@MockitoBean으로 주입해서 컨트롤러 테스트에서 실제 Bean 주입 오류를 막는 패턴은 다른 컨트롤러 테스트들과도 일관되고, 현재 테스트 시나리오에 부작용도 없어 보입니다.
추후 Discord 알림 동작 자체를 검증해야 할 때는 이 목 객체에 대한 상호작용 검증을 추가하는 정도만 고민하시면 될 것 같습니다.Also applies to: 37-37
src/main/java/com/almang/inventory/product/service/ProductService.java (2)
72-84:deleteProduct트랜잭션 흐름은 깔끔하지만, soft delete 정책을 주석/테스트로 명시해 두면 더 좋겠습니다
UserStoreContext조회 →findProductById→validateStoreAccess→product.delete()순서가 명확해서, 삭제 권한 체크와 soft delete 적용 흐름이 잘 드러납니다.@Transactional컨텍스트에서 엔티티 메서드product.delete()만 호출하고save를 호출하지 않는 것은 JPA dirty checking 전제를 따르는 구조라서 문제 없습니다. 다만delete()가 단순히deletedAt만 세팅한다는 사실을 유지보수자가 바로 이해하기 어렵기 때문에,
Product.delete()내부에 짧은 Javadoc/주석으로 “soft delete 전용. 영속성 컨텍스트 내에서만 사용” 정도를 남기거나- 이 메서드를 검증하는 단위 테스트를 Product 도메인 레벨에 추가해 두면 정책 이해에 도움이 됩니다.
현재 설계상 삭제 실패 케이스는 모두 예외로 처리하고 성공 시에만
success=true를 반환하므로, API 관점에서도 일관성 있습니다.
129-129:deletedAt(null)명시와findProductById의 soft delete 가드는 정책상 문제 없지만, 책임 위치를 정리해 둘 여지가 있습니다
toEntity에서.deletedAt(null)을 명시하는 부분은 새로 생성되는 품목에 대해 “항상 삭제되지 않은 상태로 시작한다”는 의도를 드러내는 장점이 있지만, 필드 기본값이null이라면 기능상으로는 중복입니다. 팀에서 “빌더에 모든 초기 상태를 명시한다”는 합의가 없다면 제거해도 무방합니다.findProductById에서product.getDeletedAt() != null이면PRODUCT_NOT_FOUND를 던지는 것은 soft delete 정책을 서비스 계층에서 한 번 더 방어하는 코드입니다. Product 엔티티에 이미@Where(deleted_at IS NULL)가 붙어 있다면 이 체크는 실질적으로는 도달하지 않겠지만,
- “삭제된 레코드는 모든 조회에서 항상 숨긴다”는 정책을 명확히 표현한다는 장점이 있고,
- 반대로 “삭제 필터링 책임을 엔티티/리포지토리에서만 지게 한다”는 팀 규칙이라면 이 부분은 정리 대상이 될 수 있습니다.
개인적으로는 메서드명을
findActiveProductById처럼 바꾸어 “삭제되지 않은 품목 전용 조회”라는 의미를 드러내면 의도가 더 명확해질 것 같습니다. (기존 호출부가 모두 활성 품목 조회 용도이므로 시그니처 영향은 크지 않습니다.)Also applies to: 144-150
src/test/java/com/almang/inventory/receipt/controller/ReceiptControllerTest.java (1)
18-18: Receipt 컨트롤러 테스트에서도 DiscordErrorNotifier 목킹 패턴이 잘 적용되었습니다전역 예외 처리나 AOP에서
DiscordErrorNotifier를 주입받는 구조라면, 각 WebMvcTest마다@MockitoBean으로 등록해 두는 지금 방식이 가장 단순하고 안정적인 해결책입니다.
다른 컨트롤러 테스트들과 일관된 구성이어서, 추후 공통 설정(예: 테스트용 설정 클래스)으로 묶고 싶을 때도 마이그레이션이 수월해 보입니다.Also applies to: 54-54
src/test/java/com/almang/inventory/order/controller/OrderControllerTest.java (1)
32-32: 테스트에서LocalDateTime.now()사용은 현재 맥락에서는 안전하지만, 향후에는 고정 Clock 고려도 좋습니다여러
OrderResponse스텁에서 4번째 인자를LocalDateTime.now()로 채우신 부분은, 해당 필드를 별도로 검증하지 않기 때문에 테스트를 불안정하게 만들지는 않습니다. 다만:
- 시간이 요구사항에 직접적인 의미를 갖는 필드를 나중에 검증해야 할 경우,
- 또는 시간 기반 정렬/필터 로직을 테스트해야 할 경우에는
Clock을 주입받아서 테스트에서는Clock.fixed(...)로 고정된 시간을 사용하는 패턴이 유지보수에 유리합니다 (Spring@TestConfiguration등으로 쉽게 교체 가능).현 상태에서는 과하지 않은 선택이라 그대로 유지하셔도 무방하되, “시간 의존 로직이 생기면 Clock 주입으로 전환” 정도만 팀 합의로 공유해 두시면 좋겠습니다.
Also applies to: 84-84, 269-269, 337-337, 353-353, 457-457
src/test/java/com/almang/inventory/product/service/ProductServiceTest.java (1)
14-14: 삭제 시나리오 테스트가 soft delete 정책을 잘 커버하고 있습니다새로 추가된 네 가지 테스트가 각각 다른 관점에서 soft delete 동작을 검증하고 있어서 서비스 변경에 대한 안전망이 잘 갖춰졌습니다.
품목_삭제에_성공한다
DeleteProductResponse.success()를 검증하고,- 같은 ID로 상세 조회 시
PRODUCT_NOT_FOUND,- 목록 조회에서도
totalElements == 0을 확인해서 “삭제된 품목은 모든 조회에서 숨긴다”는 정책을 명확히 보장하고 있습니다.존재하지_않는_품목_삭제시_예외가_발생한다
- 존재하지 않는 ID에 대해
PRODUCT_NOT_FOUND예외를 기대해, 서비스가 조용히 무시하거나 idempotent 삭제로 처리하지 않음을 명시합니다.다른_상점의_품목을_삭제하려고_하면_예외가_발생한다
- 교차 상점 삭제 시
STORE_ACCESS_DENIED를 검증해 접근 제어 정책을 잘 커버합니다. 이는 기존 업데이트/조회 관련 테스트 패턴과도 일관됩니다. (Based on learnings, 서비스 레벨에서 접근 검증 후 도메인 메서드를 호출하는 구조를 유지하고 있습니다.)삭제된_품목은_목록_및_상세에서_조회되지_않는다
- 앞선 성공/예외 케이스와 약간 중복되긴 하지만, “삭제 → 상세/목록 두 경로 모두에서 숨겨짐”을 한 테스트 안에 통합적으로 검증해 시나리오 가독성이 좋습니다.
추가로, 만약 나중에 “삭제된 품목을 특정 관리자 화면에서만 조회” 같은 요구가 생긴다면, 지금 테스트들을 기준으로 별도 메서드(예:
getDeletedProductList)를 설계할 수 있어 방향성이 분명한 상태입니다. 지금 구조 자체는 아주 탄탄합니다.Also applies to: 1092-1226
src/main/java/com/almang/inventory/product/controller/ProductController.java (1)
9-9: DELETE 엔드포인트 설계가 기존 컨트롤러 패턴과 잘 정렬되어 있습니다
@DeleteMapping("/{productId}")+ResponseEntity<ApiResponse<DeleteProductResponse>>조합이 CREATE/UPDATE/GET과 동일한 ApiResponse 래핑 패턴을 그대로 따르고 있어, 클라이언트 입장에서 일관된 응답 포맷을 기대할 수 있습니다.CustomUserPrincipal에서userId를 꺼내 로그에 남긴 뒤productService.deleteProduct(productId, userId)를 호출하는 흐름도 다른 메서드들과 동일한 컨벤션입니다.SuccessMessage.DELETE_PRODUCT_SUCCESS를 사용해 메시지 상수 관리도 잘 되어 있어, i18n/문구 변경 시 한 곳에서 관리하기 좋습니다.추가로, Swagger 문서에 삭제 후 응답 예시(
DeleteProductResponse)를@ApiResponse로 명시해 두면 프론트/외부 연동팀이 스펙을 이해하기 더 수월할 것 같습니다.Also applies to: 19-19, 84-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/main/java/com/almang/inventory/global/api/SuccessMessage.java(1 hunks)src/main/java/com/almang/inventory/product/controller/ProductController.java(3 hunks)src/main/java/com/almang/inventory/product/domain/Product.java(3 hunks)src/main/java/com/almang/inventory/product/dto/response/DeleteProductResponse.java(1 hunks)src/main/java/com/almang/inventory/product/service/ProductService.java(4 hunks)src/test/java/com/almang/inventory/admin/controller/AdminControllerTest.java(2 hunks)src/test/java/com/almang/inventory/inventory/controller/InventoryControllerTest.java(2 hunks)src/test/java/com/almang/inventory/order/controller/OrderControllerTest.java(8 hunks)src/test/java/com/almang/inventory/order/template/controller/OrderTemplateControllerTest.java(2 hunks)src/test/java/com/almang/inventory/product/controller/ProductControllerTest.java(4 hunks)src/test/java/com/almang/inventory/product/service/ProductServiceTest.java(2 hunks)src/test/java/com/almang/inventory/receipt/controller/ReceiptControllerTest.java(2 hunks)src/test/java/com/almang/inventory/store/admin/controller/StoreAdminControllerTest.java(2 hunks)src/test/java/com/almang/inventory/store/controller/StoreControllerTest.java(2 hunks)src/test/java/com/almang/inventory/user/auth/controller/AuthControllerTest.java(2 hunks)src/test/java/com/almang/inventory/user/controller/UserControllerTest.java(2 hunks)src/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.java(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-20T10:43:47.502Z
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.502Z
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/product/controller/ProductControllerTest.javasrc/main/java/com/almang/inventory/product/service/ProductService.javasrc/main/java/com/almang/inventory/product/controller/ProductController.javasrc/test/java/com/almang/inventory/product/service/ProductServiceTest.javasrc/main/java/com/almang/inventory/product/domain/Product.java
📚 Learning: 2025-11-22T18:12:13.172Z
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.172Z
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/main/java/com/almang/inventory/product/service/ProductService.javasrc/test/java/com/almang/inventory/product/service/ProductServiceTest.javasrc/test/java/com/almang/inventory/store/controller/StoreControllerTest.javasrc/main/java/com/almang/inventory/product/domain/Product.java
🧬 Code graph analysis (1)
src/main/java/com/almang/inventory/product/domain/Product.java (1)
src/main/java/com/almang/inventory/vendor/domain/Vendor.java (1)
Entity(8-60)
🔇 Additional comments (13)
src/test/java/com/almang/inventory/store/admin/controller/StoreAdminControllerTest.java (1)
11-12: DiscordErrorNotifier를 WebMvcTest 컨텍스트에 주입한 선택이 적절합니다.
@WebMvcTest(StoreAdminController.class)에서 실제DiscordErrorNotifier빈 의존성을 만족시키기 위해@MockitoBean private DiscordErrorNotifier discordErrorNotifier;를 추가한 흐름이 자연스럽습니다.
현재 테스트 로직에서 직접 스텁이 필요하지 않기 때문에 별도when(...)없이 mock만 주입된 상태도 괜찮습니다. 동일 패턴을 다른 컨트롤러 테스트에도 일관되게 적용하신 점도 좋네요. 😄Also applies to: 35-36
src/test/java/com/almang/inventory/store/controller/StoreControllerTest.java (1)
15-16: StoreControllerTest에서도 모니터링 의존성 주입 패턴이 잘 맞습니다.
DiscordErrorNotifier를@MockitoBean으로 주입해서@WebMvcTest(StoreController.class)슬라이스가 필요한 빈을 모두 갖추도록 한 구성은 적절합니다.
추후 알림 전송 여부를 검증하고 싶다면 이 mock에 대해verify(discordErrorNotifier)...를 추가하는 식으로 확장도 가능하니, 지금 구조를 유지하되 필요 시 테스트 범위만 넓히면 될 것 같습니다.Also applies to: 46-47
src/test/java/com/almang/inventory/order/template/controller/OrderTemplateControllerTest.java (1)
15-16: OrderTemplateControllerTest의 DiscordErrorNotifier mock 추가도 일관적입니다.다른 컨트롤러 테스트와 동일하게
DiscordErrorNotifier를@MockitoBean으로 등록해 WebMvcTest 컨텍스트를 맞춰준 점이 좋습니다.
현재 테스트 시나리오가 에러 알림 자체를 검증하지는 않으므로, 별도의 stubbing 없이 단순 의존성 해소용 mock으로 두는 지금 구성이 충분해 보입니다.Also applies to: 43-44
src/test/java/com/almang/inventory/user/controller/UserControllerTest.java (1)
15-16: UserControllerTest에서도 모니터링 의존성 Mock 주입이 잘 반영되었습니다.
UserController에 새로 추가된DiscordErrorNotifier의존성을 테스트에서@MockitoBean으로 해결한 방식이 깔끔합니다.
테스트 코드에서 알림 전송까지 검증할 계획이 없다면, 지금처럼 “컨텍스트 부트스트랩용 mock” 수준으로 두는 것이 과하지도 부족하지도 않은 적절한 선택입니다.Also applies to: 49-50
src/test/java/com/almang/inventory/user/auth/controller/AuthControllerTest.java (1)
14-15: AuthControllerTest의 DiscordErrorNotifier mock 추가도 패턴에 잘 맞습니다.인증/인가 관련 컨트롤러에서도
DiscordErrorNotifier가 새 의존성으로 들어왔을 때,@WebMvcTest환경을 깨지 않도록@MockitoBean으로 mock을 등록한 점이 좋습니다.
이 mock은 현재 테스트 시나리오에 부수 효과를 주지 않으므로, 불필요한 설정 없이도 컨텍스트 안정성을 확보하는 실용적인 방식입니다.Also applies to: 52-53
src/test/java/com/almang/inventory/vendor/controller/VendorControllerTest.java (1)
18-19: VendorControllerTest에서도 DiscordErrorNotifier mock 주입으로 일관성을 유지했습니다.
VendorController테스트에@MockitoBean private DiscordErrorNotifier discordErrorNotifier;를 추가해, 모니터링 의존성을 가진 컨트롤러 테스트가 모두 동일한 패턴을 따르도록 한 점이 좋습니다.
추후 에러 상황에서 디스코드 알림 트리거를 검증해야 할 경우 이 mock을 활용해verify만 추가하면 되므로, 현재 설계는 확장 여지가 충분합니다.Also applies to: 50-51
src/main/java/com/almang/inventory/global/api/SuccessMessage.java (1)
30-31: PRODUCT 섹션에 DELETE_PRODUCT_SUCCESS 추가가 잘 정리되어 있습니다.
DELETE_PRODUCT_SUCCESS("품목 삭제 성공")상수를 PRODUCT 블록 내부에 추가한 구조가 기존 네이밍/정렬 규칙과 잘 맞습니다.
컨트롤러에서 응답 메시지를 하드코딩하지 않고 이 enum을 재사용하면, 이후 문구 변경이나 다국어 지원 시에도 한 곳에서 관리하기 수월하니 지금 방향을 그대로 유지하시면 좋겠습니다.src/main/java/com/almang/inventory/product/dto/response/DeleteProductResponse.java (1)
1-5: 삭제 응답 DTO 설계가 기존 패턴과 잘 맞습니다
record DeleteProductResponse(boolean success)형태는DeleteOrderResponse,DeleteReceiptResponse와 동일한 미니멀 패턴으로, 삭제 성공 여부만 필요할 때 과하지 않은 설계입니다.
추후 삭제 시 부가 정보를 내려야 할 때(예: 삭제 일시, soft delete 여부 플래그 등) 필드를 확장하기도 쉬운 구조입니다.src/test/java/com/almang/inventory/order/controller/OrderControllerTest.java (1)
18-18: Order 컨트롤러 테스트의 DiscordErrorNotifier 목킹도 구성 일관성이 좋습니다
OrderControllerTest에서도DiscordErrorNotifier를@MockitoBean으로 등록해, 실제 Bean 의존성으로 인한 테스트 실패를 방지하는 패턴이 잘 적용되어 있습니다.
여타 컨트롤러 테스트들과 동일한 형태이므로, 나중에 Discord 알림 로직을 공통 필터/Advice로 옮기더라도 테스트 구조는 그대로 재사용 가능해 보입니다.Also applies to: 56-56
src/test/java/com/almang/inventory/inventory/controller/InventoryControllerTest.java (1)
18-18: Inventory 컨트롤러 테스트에서도 에러 노티파이어 목킹이 잘 정비되었습니다
InventoryControllerTest에DiscordErrorNotifier를@MockitoBean으로 추가해, 실제 모니터링 인프라와 분리된 순수 컨트롤러 단위 테스트로 유지하신 점이 좋습니다.
전역 예외 핸들러/필터에서 이 Bean을 사용하더라도, 현재처럼 목 객체만 주입해 두면 테스트가 인프라 변경에 영향을 덜 받게 됩니다.Also applies to: 52-52
src/test/java/com/almang/inventory/product/controller/ProductControllerTest.java (3)
7-7: 새로운 import 구문들이 적절하게 추가되었습니다! 👍삭제 기능 테스트에 필요한 의존성들이 정확하게 import되었습니다.
Also applies to: 18-18, 23-23
563-634: 품목 삭제 테스트가 체계적으로 작성되었습니다! 🎯네 가지 시나리오를 모두 커버하고 있으며, 기존 테스트 패턴을 일관되게 따르고 있습니다:
- ✅ 정상 삭제 케이스
- ✅ 품목 미존재 케이스 (404)
- ✅ 접근 권한 검증 케이스 (403)
- ✅ 사용자 미존재 케이스 (404)
테스트 구조도 명확한 given-when-then 패턴을 따르고 있어 가독성이 우수합니다.
참고: Soft delete가 구현된 경우, "이미 삭제된 품목을 다시 삭제"하는 케이스도 고려해볼 수 있습니다. 다만 이는 서비스 레이어에서 처리될 수 있으므로, 필요 시 통합 테스트나 서비스 테스트에서 검증하는 것을 권장합니다.
50-50: DiscordErrorNotifier mock bean is necessary and correctly added.This dependency is required because
GlobalExceptionHandler(a@RestControllerAdvicecomponent) declaresDiscordErrorNotifieras a dependency. Since@WebMvcTestloads the entire web layer including global exception handlers, the application context cannot be created without this mock bean.
1 participant
✨ 작업 내용
📝 적용 범위
/product📌 참고 사항
Summary by CodeRabbit
릴리즈 노트
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.