[volume-4] 쿠폰 도메인 추가, 동시성 제어 적용 - 이주옥#154
Open
juoklee wants to merge 19 commits intoLoopers-dev-lab:juokleefrom
Open
[volume-4] 쿠폰 도메인 추가, 동시성 제어 적용 - 이주옥#154juoklee wants to merge 19 commits intoLoopers-dev-lab:juokleefrom
juoklee wants to merge 19 commits intoLoopers-dev-lab:juokleefrom
Conversation
- 재고: 비관적 락 (SELECT FOR UPDATE) 적용 - 좋아요: Atomic Update (@Modifying JPQL) 적용 - 주문 취소: 비관적 락 적용 - 쿠폰 사용: 낙관적 락 예외 핸들러 추가 (409 CONFLICT) - DataIntegrityViolationException 핸들러 추가 (좋아요 토글 동시성) - 데드락 방지: productId 정렬 후 락 획득 - 동시성 E2E 테스트 4건 추가 (재고, 재고 부족, 쿠폰, 좋아요)
📝 WalkthroughWalkthrough이번 변경사항은 쿠폰 도메인을 새로이 도입하고, 주문 생성 시 쿠폰 적용을 지원하도록 확장했다. 동시에 주소/브랜드/상품의 업데이트 메소드를 리네이밍하고, 좋아요 기능을 저수준 저장소 쿼리로 개선했으며, 동시성 제어를 위한 비관적 락을 추가했다. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Controller as CouponV1Controller
participant Facade as CouponFacade
participant CouponService as CouponService
participant MemberRepo as MemberCouponRepository
participant Reader as CouponReader
User->>Controller: POST /api/v1/coupons/{couponId}/issue
Controller->>Facade: issueCoupon(loginId, couponId)
Facade->>Facade: getMemberId(loginId)
Facade->>CouponService: issueCoupon(couponId, memberId)
CouponService->>Reader: findById(couponId)
Reader-->>CouponService: Coupon
CouponService->>CouponService: validate(expired, already issued)
CouponService->>MemberRepo: save(MemberCoupon)
MemberRepo-->>CouponService: MemberCoupon
CouponService-->>Facade: MemberCoupon
Facade->>Reader: findById(couponId)
Reader-->>Facade: Coupon
Facade->>Facade: map to MemberCouponInfo
Facade-->>Controller: MemberCouponInfo
Controller-->>User: 201 Created with MemberCouponResponse
sequenceDiagram
participant User as User
participant Controller as OrderV1Controller
participant OrderFacade as OrderFacade
participant CouponService as CouponService
participant ProductService as ProductService
participant OrderService as OrderService
participant StockRepo as ProductRepository
User->>Controller: POST /api/v1/orders (with memberCouponId)
Controller->>OrderFacade: createOrder(loginId, addressId, memberCouponId, items)
alt memberCouponId provided
OrderFacade->>CouponService: useCoupon(memberCouponId, memberId, orderAmount)
CouponService-->>OrderFacade: CouponApplyResult(discountAmount)
OrderFacade->>OrderFacade: calculate totalAmount = originalAmount - discountAmount
else memberCouponId null
OrderFacade->>OrderFacade: totalAmount = originalAmount
end
OrderFacade->>ProductService: getProductForUpdate(productId)
ProductService-->>OrderFacade: Product with pessimistic lock
OrderFacade->>StockRepo: decreaseStock(productId, qty)
StockRepo-->>OrderFacade: updated stock
OrderFacade->>OrderService: createOrder(..., memberCouponId, originalAmount, discountAmount)
OrderService-->>OrderFacade: Order
OrderFacade-->>Controller: OrderInfo
Controller-->>User: 201 Created with OrderResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 검토 포인트:
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.java (1)
3-10:⚠️ Potential issue | 🟡 Minor중복 import 제거 필요
Line 3과 Line 10에
import com.loopers.domain.PageResult;가 중복 선언되어 있다.수정안
import com.loopers.domain.PageResult; import com.loopers.support.error.CoreException; import com.loopers.support.error.ErrorType; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import com.loopers.domain.PageResult;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.java` around lines 3 - 10, Remove the duplicate import of com.loopers.domain.PageResult in BrandServiceTest by keeping a single import declaration; locate the two identical lines importing PageResult and delete one of them (ensure the remaining imports for the class including PageResult, CoreException, ErrorType, and JUnit annotations remain intact).apps/commerce-api/src/main/java/com/loopers/domain/order/OrderService.java (1)
108-110:⚠️ Potential issue | 🟠 Major이미 취소된 주문을 404로 처리하면 재시도 제어가 흐려진다.
운영에서는 중복 클릭이나 네트워크 재시도로 같은 취소 요청이 다시 들어오는 경우가 흔한데, 이를
NOT_FOUND로 숨기면 "주문 없음"과 "이미 취소됨"을 구분하지 못해 클라이언트 보상 로직과 장애 분석이 어려워진다. 여기서는CoreException을 유지하되 상태 코드는CONFLICT에 가깝게 바꾸고, 사용자 메시지도 현재 상태를 드러내도록 분리하는 편이 낫다. 추가로 동일 주문을 두 번 취소할 때 첫 호출은 성공하고 두 번째 호출은 기대한 에러 코드와 메시지를 반환하는 테스트가 필요하다. Based on learnings, 'In the loop-pack-be-l2-vol3-java project, enforce unified error handling by routing errors through CoreException to ApiControllerAdvice to ensure a consistent response format.'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/order/OrderService.java` around lines 108 - 110, The current check in OrderService that throws CoreException with ErrorType.NOT_FOUND when order.getStatus() == OrderStatus.CANCELLED should be changed to throw CoreException with ErrorType.CONFLICT and a clear message like "주문이 이미 취소되었습니다" so clients can distinguish "not found" vs "already cancelled"; update the cancel flow in OrderService (referencing order.getStatus() and the cancel method) to use ErrorType.CONFLICT and a specific message, keep using CoreException so ApiControllerAdvice still normalizes responses, and add/modify tests for the cancel endpoint to assert the first cancel request succeeds and a repeated cancel returns a CONFLICT error with the new message.
🟡 Minor comments (6)
apps/commerce-api/src/main/java/com/loopers/interfaces/security/MemberAuthFilter.java-98-101 (1)
98-101:⚠️ Potential issue | 🟡 Minor경로 매칭 패턴에 trailing slash 누락으로 인한 범위 과다 매칭 문제가 있다.
path.startsWith()사용 시 trailing slash 없이 비교하면 의도하지 않은 경로까지 매칭된다. 현재 코드에서 두 곳의 문제를 확인했다:
99번 줄:
path.startsWith("/api/v1/users/me")
/api/v1/users/member,/api/v1/users/meow등 의도하지 않은 경로 매칭89번 줄:
path.startsWith("/api/v1/orders")
/api/v1/orders-history,/api/v1/ordersomething등 의도하지 않은 경로 매칭운영 관점에서 향후 새로운 엔드포인트 추가 시 의도하지 않은 인증 요구로 401 응답 발생 가능성이 있다. 파일 내 다른 경로들(lines 78, 84, 94)은 이미 trailing slash를 포함하고 있으므로 일관성 있게 수정해야 한다.
🛡️ 수정 제안
89번 줄:
- if (path.startsWith("/api/v1/orders")) { + if (path.equals("/api/v1/orders") || path.startsWith("/api/v1/orders/")) {99번 줄:
- if (path.startsWith("/api/v1/users/me")) { + if (path.equals("/api/v1/users/me") || path.startsWith("/api/v1/users/me/")) {추가 테스트:
/api/v1/orders-history,/api/v1/users/member같은 경로가 인증 대상에서 제외되는지 단위 테스트로 검증해야 한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/interfaces/security/MemberAuthFilter.java` around lines 98 - 101, The path matching in MemberAuthFilter is too broad because path.startsWith("/api/v1/users/me") and path.startsWith("/api/v1/orders") will match unintended routes; update those checks to include the trailing slash (e.g., use "/api/v1/users/me/" and "/api/v1/orders/") so only the intended subtree is matched, keep the other existing patterns consistent, and add unit tests that assert routes like "/api/v1/users/member" and "/api/v1/orders-history" are not treated as protected.apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java-252-259 (1)
252-259:⚠️ Potential issue | 🟡 MinorHelper 메서드에서 응답 상태 검증 누락
createLimitedCoupon,createValidDaysCoupon,createCoupon헬퍼 메서드들이 응답 상태 코드를 검증하지 않고 바로response.getBody().data().coupon().id()를 호출한다. 쿠폰 생성이 실패하면NullPointerException이 발생하여 실제 원인 파악이 어렵다.🔧 응답 검증 추가 제안
private Long createLimitedCoupon(String name, String type, Long value, LocalDateTime expiredAt, Integer totalQuantity) { var request = new CouponV1Dto.CreateCouponRequest(name, type, value, null, expiredAt, null, totalQuantity); ResponseEntity<ApiResponse<CouponV1Dto.CouponResponse>> response = testRestTemplate.exchange( COUPON_ADMIN, HttpMethod.POST, adminEntity(request), new ParameterizedTypeReference<>() {} ); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.CREATED); return response.getBody().data().coupon().id(); }모든 헬퍼 메서드에 동일한 검증을 추가하면 테스트 실패 시 원인을 빠르게 파악할 수 있다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java` around lines 252 - 259, The helper methods createLimitedCoupon, createValidDaysCoupon, and createCoupon call response.getBody().data().coupon().id() without checking the ResponseEntity status, causing NPEs on failures; update each helper to assert/verify the ResponseEntity status (e.g., equals expected HttpStatus like OK/CREATED) immediately after the testRestTemplate.exchange call and, if not as expected, fail the test or throw a clear exception that includes response.getStatusCode() and response.getBody() for debugging; use the existing ResponseEntity variable from the exchange and place this check before any call to response.getBody() so failures show the HTTP status and body content.apps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponV1Controller.java-38-39 (1)
38-39:⚠️ Potential issue | 🟡 Minor페이지 크기 제한 부재
size파라미터에 기본값(20)만 설정되어 있고 상한이 없다. 악의적인 요청자가size=10000을 요청하면 대량의 데이터를 한 번에 조회하여 DB 부하 및 메모리 문제를 유발할 수 있다.🛡️ 최대 크기 제한 추가 제안
`@GetMapping`("/api/v1/users/me/coupons") public ApiResponse<CouponV1Dto.MemberCouponListResponse> getMyCoupons( HttpServletRequest request, `@RequestParam`(defaultValue = "0") int page, - `@RequestParam`(defaultValue = "20") int size + `@RequestParam`(defaultValue = "20") int size ) { String loginId = AuthUtils.getAuthenticatedLoginId(request); + int limitedSize = Math.min(size, 100); - PagedInfo<MemberCouponInfo> result = couponFacade.getMyCoupons(loginId, page, size); + PagedInfo<MemberCouponInfo> result = couponFacade.getMyCoupons(loginId, page, limitedSize); return ApiResponse.success(CouponV1Dto.MemberCouponListResponse.from(result)); }또는 Facade/Service 계층에서 일괄적으로 제한하는 방안도 가능하다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponV1Controller.java` around lines 38 - 39, The controller currently accepts a page size via the request param `size` in `CouponV1Controller` with only a default but no upper bound; add a maximum page-size guard (e.g. define a constant MAX_PAGE_SIZE and clamp the incoming `size` using Math.min or validate and throw/bad-request) before passing it to the Facade/Service so requests like size=10000 cannot load huge result sets; alternatively annotate/validate the parameter (e.g. with javax.validation `@Max`) or enforce the same clamp in the Facade/Service method that receives the page/size to ensure a safe upper limit across layers.apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/MemberCouponJpaRepository.java-13-14 (1)
13-14:⚠️ Potential issue | 🟡 Minor페이징 쿼리에 명시적 정렬 조건 부재
findAllByMemberIdAndDeletedAtIsNull과findAllByCouponIdAndDeletedAtIsNull에 정렬 조건이 없다. DB 옵티마이저에 따라 페이지 간 데이터 중복 또는 누락이 발생할 수 있다. 특히 동시 발급 시 일관성 없는 결과가 사용자에게 노출될 수 있다.🔧 정렬 조건 추가 제안
-Page<MemberCoupon> findAllByMemberIdAndDeletedAtIsNull(Long memberId, Pageable pageable); -Page<MemberCoupon> findAllByCouponIdAndDeletedAtIsNull(Long couponId, Pageable pageable); +Page<MemberCoupon> findAllByMemberIdAndDeletedAtIsNullOrderByIdDesc(Long memberId, Pageable pageable); +Page<MemberCoupon> findAllByCouponIdAndDeletedAtIsNullOrderByIdDesc(Long couponId, Pageable pageable);또는 호출부에서
PageRequest.of(page, size, Sort.by("id").descending())을 전달하는 방안도 가능하다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/MemberCouponJpaRepository.java` around lines 13 - 14, The repository methods findAllByMemberIdAndDeletedAtIsNull and findAllByCouponIdAndDeletedAtIsNull lack an explicit ordering, which can cause inconsistent paging; update each method to use a JPQL/@Query with an ORDER BY (e.g., "order by m.id desc") to guarantee deterministic sort (or require callers to pass a Pageable with Sort.by("id").descending()); specifically add `@Query` annotations for findAllByMemberIdAndDeletedAtIsNull(...) and findAllByCouponIdAndDeletedAtIsNull(...) that include "order by m.id desc" to enforce consistent page results.apps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponAdminV1Controller.java-41-43 (1)
41-43:⚠️ Potential issue | 🟡 Minor페이지 크기 상한 미설정으로 인한 리소스 과다 사용 가능성
size파라미터에 상한이 없어 악의적 사용자가 매우 큰 값(예: 100000)을 요청하여 DB 및 메모리 부하를 유발할 수 있다. 운영 환경에서 서비스 장애로 이어질 수 있다.Facade 또는 Service 계층에서 최대값을 제한하거나, Controller에서 검증하라:
`@RequestParam`(defaultValue = "20") int size // size = Math.min(size, 100); // 상한 적용테스트 케이스 추가 필요: 과도한
size요청 시 적절히 제한되는지 검증.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponAdminV1Controller.java` around lines 41 - 43, The page-size parameter in CouponAdminV1Controller (method handling the paginated coupon list) has no upper bound and can be abused to request huge sizes; enforce a maximum page size (e.g., define a constant MAX_PAGE_SIZE and apply size = Math.min(size, MAX_PAGE_SIZE) inside the controller method or validate and cap it in the Facade/Service layer before querying), return a sensible error or default when size <= 0, and add unit/integration tests verifying that an excessively large size is capped to MAX_PAGE_SIZE and does not cause large DB/memory loads.apps/commerce-api/src/main/java/com/loopers/domain/coupon/Coupon.java-99-104 (1)
99-104:⚠️ Potential issue | 🟡 Minor정률 할인 계산 시 소수점 이하 절사로 인한 할인 손실
orderAmount * this.value / 100은 정수 나눗셈으로 소수점 이하가 절사된다. 소액 주문 시 할인이 0원이 될 수 있다 (예: 주문금액 1원, 50% 할인 → 0원). 비즈니스 요구사항에 따라 반올림 정책을 명시적으로 적용해야 한다.🔧 수정 제안 (반올림 적용)
public long calculateDiscount(long orderAmount) { if (this.type == CouponType.FIXED) { return Math.min(this.value, orderAmount); } - return orderAmount * this.value / 100; + // 반올림 적용 (비즈니스 요구사항에 따라 조정) + return Math.round(orderAmount * this.value / 100.0); }테스트 케이스 추가 필요: 소액 주문에 대한 정률 할인 계산 결과 검증.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/coupon/Coupon.java` around lines 99 - 104, calculateDiscount in Coupon currently does integer division for percentage discounts causing truncation (e.g., 1 * 50 / 100 == 0); change the percentage branch in Coupon.calculateDiscount to perform rounding (e.g., use BigDecimal or long arithmetic with half-up adjustment) so the result is rounded instead of floored, preserve return type long, and add unit tests for small order amounts (e.g., orderAmount=1, value=50) to assert the expected rounded discount; refer to Coupon.calculateDiscount, CouponType, and fields orderAmount/value when making changes.
🧹 Nitpick comments (28)
apps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.java (1)
118-122: DataIntegrityViolationException 핸들러의 응답 메시지가 실제 예외 원인과 불일치할 수 있다.운영 관점에서 문제가 되는 이유:
DataIntegrityViolationException은 UNIQUE 제약조건 위반뿐 아니라 FK 위반, NOT NULL 위반, CHECK 제약조건 위반 등 다양한 상황에서 발생한다.- "이미 처리된 요청입니다"라는 메시지는 UNIQUE 제약조건 위반에만 적합하며, 다른 제약조건 위반 시 사용자에게 오해를 유발한다.
- 장애 대응 시 실제 원인과 다른 메시지로 인해 초기 판단이 지연될 수 있다.
수정안:
- 제약조건 종류별로 분기 처리하거나, 최소한 메시지를 "데이터 처리 중 충돌이 발생했습니다"와 같이 범용적으로 변경하는 것을 권장한다.
- 또는 쿠폰 중복 발급 방지 등 특정 시나리오에만 적용하려면, 해당 상황에 맞는 커스텀 예외를 정의하여
CoreException으로 처리하는 것이 일관성 측면에서 적합하다.추가 테스트:
- FK 위반, NOT NULL 위반 등 다양한
DataIntegrityViolationException시나리오에서 사용자 응답 메시지의 적합성을 검증하는 테스트 추가를 권장한다.♻️ 범용 메시지로 변경하는 예시
`@ExceptionHandler` public ResponseEntity<ApiResponse<?>> handleDataIntegrity(DataIntegrityViolationException e) { log.warn("DataIntegrityViolationException : {}", e.getMessage(), e); - return failureResponse(ErrorType.CONFLICT, "이미 처리된 요청입니다. 다시 시도해주세요."); + return failureResponse(ErrorType.CONFLICT, "데이터 처리 중 충돌이 발생했습니다. 다시 시도해주세요."); }Based on learnings: 이 프로젝트에서는 통합된 에러 처리를 위해
CoreException을 통해ApiControllerAdvice로 라우팅하여 일관된 응답 형식을 보장하도록 권장하고 있다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.java` around lines 118 - 122, The DataIntegrityViolationException handler in ApiControllerAdvice (method handleDataIntegrity) returns a misleading specific message; update it to return a generic conflict message such as "데이터 처리 중 충돌이 발생했습니다" via failureResponse(ErrorType.CONFLICT, ...) or implement simple branching by inspecting e.getCause() / instanceof ConstraintViolationException to map UNIQUE -> specific duplicate message and all others -> generic conflict; for scenario-specific handling (e.g., coupon duplication) replace callers to throw a domain CoreException so ApiControllerAdvice can handle it consistently.apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandRepository.java (1)
5-6: 도메인 포트에 영향 행 수를 그대로 노출하지 않는 편이 안전하다
int반환은 JPQL bulk update의 구현 상세라서 상위 계층이 0/1의 의미를 제각각 해석하게 만든다. 운영에서는 존재하지 않거나 soft-delete된 브랜드에 대해 "성공했지만 아무 일도 일어나지 않은" 요청을 놓치기 쉽다. 도메인 계약은void/boolean또는 예외 중심으로 정리하고, 0건 갱신은 어댑터나 서비스에서CoreException으로 변환하는 편이 안전하다. 추가로 존재하지 않는 ID와 soft-delete된 ID에서 좋아요 증가/감소가 일관되게 실패하는 테스트를 넣어 두는 것이 좋다. Based on learnings "In the loop-pack-be-l2-vol3-java project, enforce unified error handling by routing errors through CoreException to ApiControllerAdvice to ensure a consistent response format." As per coding guidelines "도메인 규칙과 인프라 관심사가 섞이면 분리하도록 제안한다."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandRepository.java` around lines 5 - 6, Change the BrandRepository port methods increaseLikeCount and decreaseLikeCount so they do not expose JPQL bulk-update row counts: replace the int return with a domain-level contract (void or boolean) in BrandRepository, update the adapter/implementation that currently performs the JPQL bulk update to detect a 0-update result and convert it into a CoreException (or return false if using boolean) instead of returning 0, and ensure the service layer does not interpret raw int counts; also add unit/integration tests that assert operations fail (via CoreException or false) for non-existent and soft-deleted brand IDs so behavior is consistent and routed through ApiControllerAdvice.apps/commerce-api/src/main/java/com/loopers/domain/product/ProductReader.java (1)
9-10:findByIdForUpdate가 락 전략을 도메인 포트 밖으로 새게 만든다
FOR UPDATE여부는 인프라 구현 선택에 가깝기 때문에 이 이름이 도메인 포트에 들어오면 서비스와 테스트 전반이 DB 락 전략에 결합된다. 운영에서는 같은 유스케이스에 일반 조회와 잠금 조회가 혼재하면서 잘못된 경로를 선택해 동시성 회귀가 생기기 쉽다. 재고 변경/주문 처리용 command-side port로 분리하거나, 서비스 내부에서 잠금 조회를 캡슐화해서 호출자가 락 메서드를 직접 고르지 않게 만드는 편이 안전하다. 추가로 동시성 E2E 테스트는 퍼사드 또는 유스케이스 경로만 호출해도 동일하게 통과하는지 확인해 두는 것이 좋다. As per coding guidelines "도메인 규칙과 인프라 관심사가 섞이면 분리하도록 제안한다."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/product/ProductReader.java` around lines 9 - 10, ProductReader exposes a DB locking concern via findByIdForUpdate which leaks infra strategy into the domain port; remove findByIdForUpdate from ProductReader and either (a) create a separate command-side port/interface (e.g., ProductCommandPort or ProductLockingReader) that declares findByIdForUpdate(Long id) for use by mutating use-cases (inventory/order commands), or (b) encapsulate the lock inside the service/use-case by calling the existing ProductReader.findById and delegating locking to an infra-only repository implementation used privately by the service (e.g., a ProductLockingRepository injected into the service), then update services and tests to call the command-side port or service façade so callers never choose the lock method directly and add E2E tests that exercise only the façade/use-case paths.apps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.java (1)
232-241: Fake 구현체의 반환값이 실제 동작을 반영하지 않음
FakeBrandRepository의increaseLikeCount/decreaseLikeCount가 항상1을 반환한다. 실제 구현에서는 해당 ID의 엔티티가 없으면0을 반환하지만, Fake는 이를 구분하지 않는다.테스트 관점 문제: 서비스 레이어에서 반환값 검증 로직을 누락해도 테스트가 통과하여, 존재하지 않는 엔티티에 대한 좋아요 연산 시 예외 미발생 버그를 놓칠 수 있다.
추가 테스트 제안: 존재하지 않는 브랜드 ID로
increaseLikeCount호출 시 적절한 예외가 발생하는지 검증하는 테스트 추가 고려.Fake 개선안
static class FakeBrandRepository implements BrandRepository { private final Map<Long, Brand> brands = new HashMap<>(); private long idSequence = 1L; `@Override` public Brand save(Brand brand) { brands.put(idSequence++, brand); return brand; } `@Override` public int increaseLikeCount(Long id) { - return 1; + return brands.containsKey(id) ? 1 : 0; } `@Override` public int decreaseLikeCount(Long id) { - return 1; + return brands.containsKey(id) ? 1 : 0; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.java` around lines 232 - 241, FakeBrandRepository's increaseLikeCount and decreaseLikeCount always return 1 which hides missing-entity behavior; update the FakeBrandRepository implementations of increaseLikeCount(Long id) and decreaseLikeCount(Long id) to check the fake store (e.g., the in-memory map or list used by FakeBrandRepository) for the given id and return 1 only when the brand exists (and mutate its like count), otherwise return 0; ensure the methods mirror the real repository contract so service-layer tests can detect non-existent-ID behavior and add/adjust tests to call increaseLikeCount/decreaseLikeCount with a missing id to assert the 0-return -> service throws or handles accordingly.apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java (1)
33-35: 중복 조회 최적화 가능Line 35의
productService.getProduct(productId)는 존재 여부 확인용이며, Line 45에서 동일한 조회가 재수행된다. 불필요한 DB 조회를 줄이려면 Line 35의 호출을 제거하고 Line 45의 결과로 존재 여부를 암묵적으로 검증하거나,likeService.toggleLike에서 NOT_FOUND 예외를 발생시키는 방식을 고려할 수 있다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java` around lines 33 - 35, toggleProductLike 메서드에서 중복 조회인 productService.getProduct(productId) 호출을 제거하세요; 대신 현재 메서드의 이후 호출인 likeService.toggleLike(memberId, productId) 결과로 제품 존재 여부를 암묵적으로 검증하거나, 없을 때 예외를 던지도록 likeService.toggleLike를 수정해 NOT_FOUND를 발생시키도록 하여 불필요한 DB 조회를 없애고 단일 조회로 처리되게 만드세요.apps/commerce-api/src/test/java/com/loopers/interfaces/api/ConcurrencyE2ETest.java (1)
288-345: 준비용 helper도 응답을 즉시 검증해야 원인 위치에서 실패한다.Line 288-345는 대부분 응답 상태 확인 없이
body를 바로 꺼내고,registerMember는 결과를 아예 확인하지 않는다. 운영 관점에서는 픽스처 생성이 깨졌을 때 이후 주문/조회 단계에서 NPE나 인증 실패로 터져 장애 원인 추적 시간이 늘어난다. 공통assertSuccess(response, apiName, expectedStatus)helper를 두고 상태 코드와body/data존재 여부를 먼저 검증한 뒤 ID를 추출하도록 바꾸는 편이 좋다. 추가 테스트로는 잘못된 준비 요청 하나를 넣어 준비 단계에서 바로 실패 메시지가 나오고, 이후 단계까지 오염되지 않는지 확인해야 한다. As per coding guidelines, "**/*Test*.java: 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/ConcurrencyE2ETest.java` around lines 288 - 345, The helpers (registerMember, registerBrand, registerProduct, registerAddress, createCoupon, issueCouponAndGetId) currently extract response.body() or ignore responses without asserting status/body; change each to call a common assertSuccess(response, apiName, expectedStatus) helper right after testRestTemplate.exchange to verify HTTP status and non-null body.data before extracting IDs (and update registerMember to capture and assert its response rather than ignoring it); also add one negative prep test that issues a bad request and asserts the failure occurs immediately to prevent downstream NPE/auth failures.apps/commerce-api/src/main/java/com/loopers/interfaces/api/order/OrderV1Dto.java (1)
34-40: 쿠폰 미사용 주문의 금액 필드는 null 대신 계산 가능한 기본값으로 정규화하는 편이 안전하다.운영에서는 프런트와 배치가 금액 필드를 그대로 합산하거나 표시하는 경우가 많아서
originalAmount와discountAmount가null이면 분기 누락으로 오류나 잘못된 표기가 생기기 쉽다. 응답 매핑에서 쿠폰이 없을 때originalAmount = totalAmount,discountAmount = 0으로 정규화해 API 계약을 숫자형으로 고정하는 편이 낫다. 추가로 쿠폰 미적용 주문 응답에서 두 필드가 각각 총액과0으로 내려오는 API 테스트를 추가해 달라.예시 수정안
return new OrderResponse(new OrderDto( info.id(), info.memberId(), info.recipientName(), info.recipientPhone(), info.zipCode(), info.address1(), info.address2(), info.totalAmount(), info.memberCouponId(), - info.originalAmount(), info.discountAmount(), + info.originalAmount() != null ? info.originalAmount() : info.totalAmount(), + info.discountAmount() != null ? info.discountAmount() : 0L, info.status(), items, info.createdAt() ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/interfaces/api/order/OrderV1Dto.java` around lines 34 - 40, The mapping in OrderV1Dto that constructs OrderResponse/OrderDto currently passes info.originalAmount() and info.discountAmount() which may be null for orders without coupons; change the mapper to normalize these fields so originalAmount = info.originalAmount() != null ? info.originalAmount() : info.totalAmount() and discountAmount = info.discountAmount() != null ? info.discountAmount() : 0, updating the OrderV1Dto mapping where new OrderDto(...) is created; also add a unit/integration test asserting that when an Order (used by the mapper) has no coupon (originalAmount and discountAmount null) the API response contains originalAmount equal to totalAmount and discountAmount equal to 0.apps/commerce-api/src/main/java/com/loopers/domain/order/OrderReader.java (1)
11-11: 도메인 Reader 계약에 락 구현 세부사항을 직접 노출하지 않는 편이 안전하다.운영에서는
ForUpdate같은 이름이 서비스 계약에 들어오면 테스트 대역과 다른 저장소 구현까지 DB 잠금 방식에 묶여 변경 비용이 커진다.findCancellableByIdAndMemberId처럼 유스케이스 의미만 드러내고, 실제 비관적 락 선택은 인프라 구현이나 전용 포트 뒤로 숨기는 편이 낫다. 추가로 주문 취소 경로가 동일하게 동작하는 서비스 테스트와 동시성 E2E 테스트를 유지해 달라. As per coding guidelines, '도메인 규칙과 인프라 관심사가 섞이면 분리하도록 제안한다.'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/order/OrderReader.java` at line 11, The public OrderReader contract currently exposes a DB-locking detail via the method findByIdAndMemberIdForUpdate; rename this method to a use-case semantic name such as findCancellableByIdAndMemberId on the OrderReader interface and move pessimistic-lock handling into the infra/repository implementation (or a dedicated port) so the domain contract hides locking concerns; update callers (services/tests) to use the new method name and ensure the order-cancellation service tests and concurrency E2E tests still exercise the same behavior against the infra implementation that applies the FOR UPDATE lock.apps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponV1Dto.java (1)
47-115: 응답 DTO 매핑을 한 곳으로 모아 두는 편이 안전하다.
CouponDto조립 코드가 상세·목록·회원쿠폰 응답에 반복돼 있어 필드가 늘어나면 한 경로만 누락돼 엔드포인트별 응답이 달라질 가능성이 크다. 운영에서는 동일 자원인데 API마다 값이 다르게 나가면 클라이언트 장애 원인 추적이 어려워진다.CouponResponse.CouponDto.from(CouponInfo)와MemberCouponDto.from(MemberCouponInfo)같은 정적 팩터리로 매핑을 중앙화해 달라. 추가 테스트로 동일한CouponInfo/MemberCouponInfofixture를 각 응답 빌더에 넣었을 때 필드 값이 동일한지 검증하는 케이스를 넣는 편이 좋다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponV1Dto.java` around lines 47 - 115, Create centralized static factory methods to map domain -> DTO: add CouponResponse.CouponDto.from(CouponInfo) and MemberCouponResponse.MemberCouponDto.from(MemberCouponInfo) that build DTOs from the source objects, then replace all inline constructions in CouponResponse.from, CouponListResponse.from, MemberCouponResponse.from, and MemberCouponListResponse.from to call these new .from(...) methods; finally add a unit test that feeds the same CouponInfo/MemberCouponInfo fixture into each response builder and asserts the resulting CouponDto fields are identical.apps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceTest.java (1)
322-325: 락 전용 조회 경로를 테스트 더블에서 구분하는 편이 안전하다.지금 구현은
findByIdAndMemberIdForUpdate가 일반 조회에 바로 위임돼cancelOrder가 잠금 없는 조회로 회귀해도 테스트가 계속 통과한다. 주문 취소는 재고·쿠폰 복원과 연결돼 있어 이 회귀를 놓치면 운영에서 경쟁 조건을 늦게 발견하게 된다. fake에 호출 플래그를 두거나 일반 조회와 다른 동작을 주어cancelOrder가 반드시 for-update 경로를 타는지 검증해 달라. 추가 테스트로 취소 시findByIdAndMemberIdForUpdate호출 여부를 단언하는 케이스를 넣는 편이 좋다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceTest.java` around lines 322 - 325, The test double currently has findByIdAndMemberIdForUpdate delegating to findByIdAndMemberId so cancelOrder tests will pass even if the service stops using the FOR UPDATE path; change the fake so findByIdAndMemberIdForUpdate does not delegate but instead records a call flag (e.g., set a boolean like findByIdAndMemberIdForUpdateCalled) or returns a distinct behavior/result, then update/add the test to assert that OrderService.cancelOrder invoked findByIdAndMemberIdForUpdate (use the flag) rather than the regular findByIdAndMemberId; ensure the fake exposes the flag and reset it between tests.apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponReader.java (1)
7-10:ForUpdate가 도메인 포트에 노출돼 락 구현이 새고 있다.
findByIdForUpdate는 DB 락 전략을 도메인 계약에 고정해 호출부가 영속화 세부사항을 알게 만든다. 운영에서는 락 전략을 바꾸는 순간 포트·서비스·테스트를 함께 수정해야 해서 회귀 범위가 커진다. 비즈니스 의미가 드러나는 메서드로 추상화하거나, 락 선택은 애플리케이션/인프라 계층에서 감추는 편이 안전하다. 추가 테스트로 동일 서비스 시나리오가 락 구현 변경과 무관하게 유지되는 포트 계약 테스트를 두는 편이 좋다. As per coding guidelines**/domain/**/*.java: 도메인 규칙과 인프라 관심사가 섞이면 분리하도록 제안한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponReader.java` around lines 7 - 10, The domain port CouponReader currently exposes a persistence-specific lock method findByIdForUpdate; remove this DB-lock-specific method from the interface and replace it with a business-intent method (e.g., acquireForProcessing or getForRedemption) or keep only findById and let application/infrastructure layers handle locking via transactional/locking adapters; update callers (services) to call the new business-intent method (or use an application service that wraps findById plus infrastructure locking), move any lock-specific logic into the infrastructure implementation (repository/adapter) and add port-level contract tests that exercise the service flow to prove behavior remains correct when the underlying lock strategy changes (reference: CouponReader, findByIdForUpdate, findById).apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderV1ApiE2ETest.java (1)
704-763: 성공 케이스에서 쿠폰 상태 전이를 함께 검증해야 한다.현재는 할인 금액만 확인하므로 주문 할인은 적용됐지만
MemberCoupon이USED로 전환되지 않아도 테스트가 통과한다. 운영에서는 이 회귀가 쿠폰 중복 사용이나 취소 복원 불일치로 이어질 수 있다. 주문 생성 직후 쿠폰 조회 API로 해당memberCouponId의status == USED와usedAt != null까지 검증하도록 보강해 달라. 추가 테스트로 정액/정률 쿠폰 각각에 대해 상태 전이까지 확인하는 케이스를 넣는 편이 좋다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderV1ApiE2ETest.java` around lines 704 - 763, After creating the order in both createsOrder_withFixedCoupon and createsOrder_withRateCoupon (in OrderV1ApiE2ETest), immediately call the member-coupon lookup API for the returned memberCouponId (use the same testRestTemplate.exchange pattern with authHeaders and the memberCouponId) and assert that the returned MemberCoupon has status == USED and usedAt != null; locate the memberCouponId variable produced by issueCouponAndGetId(...) and add these assertions after the existing order assertions to verify coupon state transition.apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java (3)
286-313: 발급 내역 조회 테스트에서 페이징 동작 검증이 없다.
GetIssuedCoupons테스트에서 발급 내역이 2건일 때 목록 반환만 검증하고 있다.findAllByCouponId는 페이징을 지원하는데, 페이징 파라미터 전달 및 페이징 응답(totalElements, totalPages 등) 검증이 누락되어 있다.대량 발급 시 페이징 동작이 정상인지 확인하는 테스트가 필요하다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 286 - 313, Update the GetIssuedCoupons.returnsIssuedCoupons_whenIssuesExist test to exercise pagination: call the admin endpoint constructed from COUPON_ADMIN + "/" + couponId + "/issues with query params for page and size (e.g., ?page=0&size=1) instead of the current unpaged request, then assert paging metadata in the ApiResponse (totalElements, totalPages, current page size) and that memberCoupons list size matches the requested page size; reference the findAllByCouponId paging behavior when adding these assertions so the test verifies both content and pagination fields.
328-335: Helper 메서드에서 null 체크가 없어 테스트 실패 원인 파악이 어렵다.
createCoupon메서드에서response.getBody().data().coupon().id()를 직접 호출하고 있다. 쿠폰 생성이 실패하면getBody()나data()가 null이 되어 NullPointerException이 발생하고, 실제 실패 원인(예: 400 Bad Request)이 가려진다.테스트 실패 시 장애 원인 분석을 위해 응답 상태 검증 후 데이터에 접근하는 것이 좋다.
🔧 개선 예시
private Long createCoupon(String name, String type, Long value) { var request = new CouponV1Dto.CreateCouponRequest(name, type, value, null, FUTURE, null, null); ResponseEntity<ApiResponse<CouponV1Dto.CouponResponse>> response = testRestTemplate.exchange( COUPON_ADMIN, HttpMethod.POST, adminEntity(request), new ParameterizedTypeReference<>() {} ); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.CREATED); return response.getBody().data().coupon().id(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 328 - 335, The createCoupon helper currently calls response.getBody().data().coupon().id() directly, which masks failures; update createCoupon (and its use of CouponV1Dto.CreateCouponRequest, ResponseEntity<ApiResponse<CouponV1Dto.CouponResponse>>, testRestTemplate.exchange, COUPON_ADMIN, adminEntity) to first assert/verify the HTTP status (e.g., is2xxSuccessful) and that response.getBody() and response.getBody().data() are non-null, and if not, fail the test with a clear message including response.getStatusCode() and response.getBody() (or response.getHeaders()) so the real error (400/500 and body) is visible instead of a NullPointerException, then return the coupon id after those checks.
48-159: Admin API의 인증/권한 테스트가 누락되어 있다.모든 테스트가
adminHeaders()를 포함한 요청만 수행하고 있다. 운영 환경에서 Admin API는 반드시 인증이 필요하므로, 다음 시나리오에 대한 테스트가 추가되어야 한다:
- 인증 헤더(
X-Loopers-Ldap) 없이 요청 시 401 또는 403 응답- 잘못된 인증 정보로 요청 시 적절한 에러 응답
보안 취약점이 있어도 테스트에서 발견되지 않을 수 있으므로 권한 검증 테스트 추가를 권장한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 48 - 159, Add missing auth/authorization E2E tests in the CreateCoupon nested test class: add tests that call testRestTemplate.exchange(COUPON_ADMIN, HttpMethod.POST, entityWithoutAdminHeader(request), ...) and another that uses entityWithInvalidHeader(request) (or construct headers directly instead of adminEntity) and assert the response status is 401 or 403 as appropriate; update or add test methods named returnsUnauthorized_whenNoAuthHeader and returnsForbidden_whenInvalidAuth to exercise the admin authentication flow and reuse the same request payloads used in existing tests (referencing CreateCoupon, testRestTemplate.exchange, COUPON_ADMIN, and adminEntity to locate where to hook the new tests).apps/commerce-api/src/test/java/com/loopers/domain/coupon/MemberCouponTest.java (2)
17-18: 시간 기반 테스트의 플래키(flaky) 가능성이 있다.
FUTURE와PAST가 클래스 로딩 시점의LocalDateTime.now()를 기준으로 계산된다. 테스트가 자정 직전에 시작되어 자정을 넘어 실행되는 경우, 또는 CI 환경에서 시간대(timezone) 차이로 인해 예상치 못한 결과가 발생할 수 있다.운영 안정성을 위해
Clock을 주입받거나, 테스트 메서드 내에서 명시적으로 시간 값을 생성하는 것이 더 안전하다.🔧 개선 예시
// 테스트 메서드 내에서 시간 생성 `@Test` void returnsTrue_whenExpiredAtIsPast() { LocalDateTime past = LocalDateTime.now().minusDays(1); MemberCoupon memberCoupon = MemberCoupon.create(1L, 100L, past); assertThat(memberCoupon.isExpired()).isTrue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/coupon/MemberCouponTest.java` around lines 17 - 18, The static FUTURE and PAST fields in MemberCouponTest are computed at class-load time using LocalDateTime.now(), which can make the tests flaky; change the tests to create explicit time values inside each test (or accept and use an injected Clock) instead of using the static FUTURE/PAST fields — for example, remove or stop referencing the class-level FUTURE and PAST and instead construct LocalDateTime instances inside the test methods that call MemberCoupon.create(...) and assert on memberCoupon.isExpired() (or modify MemberCouponTest to inject a Clock into MemberCoupon creation and use fixed Clock instances per test).
68-85: 만료일 경계값 테스트가 누락되어 있다.만료일이 정확히 현재 시간과 동일한 경우(
expiredAt == now())의 동작이 테스트되지 않았다.isExpired()메서드가isBefore(now())인지!isAfter(now())인지에 따라 결과가 달라진다.운영 환경에서 쿠폰 만료 시점에 사용자가 쿠폰을 사용하려 할 때 예상치 못한 동작이 발생할 수 있으므로 경계값 테스트 추가를 권장한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/coupon/MemberCouponTest.java` around lines 68 - 85, Add a boundary test for MemberCoupon.isExpired(): create a new test (e.g., returnsX_whenExpiredAtIsNow) that constructs a MemberCoupon via MemberCoupon.create(…) with expiredAt set to a fixed NOW value (use a deterministic/fixed clock or a constant LocalDateTime.now() captured into NOW) and assert the expected boolean from isExpired(); if your intended contract is “expired when expiredAt <= now” assert true, otherwise assert false. Reference MemberCoupon, isExpired(), and create() and reuse the existing FUTURE/PAST pattern but add the NOW case for completeness.apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponInfo.java (1)
15-16: totalQuantity와 issuedQuantity의 타입 일관성이 없다.
totalQuantity는Integer(nullable)이고issuedQuantity는int(primitive, non-null)이다. API 응답에서 수량 관련 필드의 nullable 여부가 일관되지 않으면 클라이언트에서 혼란이 발생할 수 있다.수량 제한이 없는 쿠폰의 경우
totalQuantity가 null이라면,issuedQuantity도 0이 아닌 null로 반환되어야 의미적으로 일관된다. 또는 둘 다 non-null로 통일하는 것이 좋다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponInfo.java` around lines 15 - 16, The CouponInfo class has inconsistent types for quantity fields: totalQuantity is Integer (nullable) while issuedQuantity is int (primitive); make them consistent by changing issuedQuantity to Integer (or change totalQuantity to int) across the class (fields, constructors, getters/setters, builders, equals/hashCode/toString if present) so both quantity fields have the same nullability semantics; update any JSON serialization annotations or DTO mappings that create CouponInfo to handle nulls appropriately and adjust downstream code that assumes issuedQuantity is non-null.apps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.java (1)
80-98: 쿠폰 적용 시 경계값 및 실패 케이스 테스트가 부족하다.현재 정상 케이스만 테스트하고 있다. 운영 환경에서 발생할 수 있는 다음 시나리오에 대한 테스트가 필요하다:
discountAmount가 음수인 경우discountAmount가originalAmount보다 큰 경우memberCouponId만 null이고 다른 쿠폰 필드는 값이 있는 경우 (불완전한 쿠폰 데이터)totalAmount와originalAmount - discountAmount불일치 시 검증 로직 존재 여부이러한 케이스에서 Order.create가 어떻게 동작하는지 명시적으로 테스트해야 장애 시 원인 파악이 용이하다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.java` around lines 80 - 98, Add focused unit tests in OrderTest that exercise Order.create for the edge and failure cases listed: (1) a test that supplies a negative discountAmount and asserts that creation fails or validation is triggered (name e.g., createsOrder_withNegativeDiscount_throws), (2) a test where discountAmount > originalAmount and asserts failure/validation (createsOrder_withExcessiveDiscount_throws), (3) a test where memberCouponId is null but originalAmount/discountAmount are present and assert either rejection or that coupon-related fields are cleared/normalized (createsOrder_withNullMemberCouponId_andCouponFields), and (4) a test that sets totalAmount inconsistent with originalAmount - discountAmount and asserts validation/failure (createsOrder_withMismatchedTotal_throws); in each test call Order.create with the appropriate inputs and assert the expected exception or post-condition using the existing assertion style to match other tests.apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.java (1)
119-124: restoreCoupon에서 소유권 검증 누락
useCoupon에서는mc.validateOwner(memberId)를 호출하여 소유권을 검증하지만,restoreCoupon에서는memberCouponId만으로 복원을 수행한다. 주문 취소 시OrderFacade에서 호출되므로 현재는 문제가 없으나, 이 메서드가 다른 컨텍스트에서 호출될 경우 타인의 쿠폰을 복원할 수 있는 보안 취약점이 발생할 수 있다.♻️ 소유권 검증 또는 내부 전용 메서드 분리 제안
`@Transactional` -public void restoreCoupon(Long memberCouponId) { +public void restoreCoupon(Long memberCouponId, Long memberId) { MemberCoupon mc = memberCouponReader.findById(memberCouponId) .orElseThrow(() -> new CoreException(ErrorType.NOT_FOUND, "사용자 쿠폰을 찾을 수 없습니다.")); + mc.validateOwner(memberId); mc.restore(); }또는 내부 전용(
package-private)으로 분리하여 외부 노출을 방지하는 방안도 고려할 수 있다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.java` around lines 119 - 124, restoreCoupon currently restores a MemberCoupon by id without verifying ownership, allowing potential cross-user restores; update restoreCoupon in CouponService to accept the caller's memberId (or otherwise ensure caller is internal) and after retrieving MemberCoupon via memberCouponReader.findById(...) call mc.validateOwner(memberId) before mc.restore(); alternatively, if this API should never be exposed, make restoreCoupon package-private/internal and document that only OrderFacade may call it to prevent external use.apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponTest.java (2)
259-276: 만료 경계값 테스트 누락
isExpired()테스트에서expiredAt이 정확히 현재 시간과 같은 경우의 경계값 테스트가 없다. 이 경계 조건에서의 동작이 명확히 정의되어야 운영 중 예상치 못한 버그를 방지할 수 있다.`@DisplayName`("만료일이 현재와 정확히 같으면, 만료로 처리한다.") `@Test` void returnsTrue_whenExpiredAtEqualsNow() { Coupon coupon = Coupon.create("쿠폰", CouponType.FIXED, 5000L, null, LocalDateTime.now()); assertThat(coupon.isExpired()).isTrue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponTest.java` around lines 259 - 276, Add a boundary test for expiredAt == now in the IsExpired nested class: create a fixed LocalDateTime now variable (e.g., LocalDateTime now = LocalDateTime.now()), call Coupon.create("쿠폰", CouponType.FIXED, 5000L, null, now), and assert that coupon.isExpired() returns true; name the test returnsTrue_whenExpiredAtEqualsNow and place it alongside the existing returnsFalse_whenNotExpired and returnsTrue_whenExpired tests to ensure deterministic verification of the isExpired() boundary behavior.
17-18: 테스트 시간 상수의 잠재적 불안정성
FUTURE와PAST가 클래스 로딩 시점에 고정되므로, 테스트 실행 시간이 길어지거나 자정을 넘기면 예상치 못한 실패가 발생할 수 있다. 현재 구현에서는 30일/1일 차이로 충분한 여유가 있어 실질적 문제는 적으나, CI 환경에서 장시간 실행 시 주의가 필요하다.♻️ `@BeforeEach에서` 시간 초기화 제안
-private static final LocalDateTime FUTURE = LocalDateTime.now().plusDays(30); -private static final LocalDateTime PAST = LocalDateTime.now().minusDays(1); +private LocalDateTime FUTURE; +private LocalDateTime PAST; + +@BeforeEach +void setUp() { + FUTURE = LocalDateTime.now().plusDays(30); + PAST = LocalDateTime.now().minusDays(1); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponTest.java` around lines 17 - 18, The FUTURE and PAST constants in CouponTest are computed at class load time which can lead to flaky tests if runs span midnight or long durations; change them from static finals to instance fields and initialize them inside a `@BeforeEach` method (or compute them inside each test) using LocalDateTime.now().plusDays(30) and LocalDateTime.now().minusDays(1) so each test gets fresh timestamps; update references to FUTURE and PAST in tests accordingly (look for symbols FUTURE, PAST, and class CouponTest).apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java (1)
140-146: 시간 기반 검증의 불안정성Lines 143-146에서
expiredAt이 특정 시간 범위 내에 있는지 검증한다. 테스트 실행 속도에 따라 경계 조건에서 실패할 가능성이 있다. 현재 6일 이상의 여유를 두고 있어 실질적 문제는 적으나, CI 환경의 부하 상황에서 간헐적 실패가 발생할 수 있다.더 넓은 시간 범위(예: 5일~8일)를 사용하거나, 시간 고정 유틸리티를 도입하는 방안을 고려할 수 있다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java` around lines 140 - 146, The time-based assertion in CouponV1ApiE2ETest (asserting response.getBody().data().memberCoupon().expiredAt() isBefore FUTURE and isAfter(LocalDateTime.now().plusDays(6))) is flaky; either widen the accepted window (e.g., assert expiredAt isAfter(LocalDateTime.now().plusDays(5)) and isBefore(LocalDateTime.now().plusDays(8))) or replace LocalDateTime.now() with a fixed clock injected into the code under test (use a Clock or TimeUtil that the test can freeze) and assert against deterministic values; update assertions that reference expiredAt, FUTURE, and the LocalDateTime.now().plusDays(...) call accordingly.apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponServiceTest.java (1)
491-496: Fake 구현체의 ID 할당 불일치
FakeCouponRepository.save()에서idSequence로 ID를 생성하여fakeCouponReader에 저장하지만, 실제Coupon엔티티의id필드는 설정되지 않는다. 이로 인해 테스트에서coupon.getId()를 호출하면null이 반환되어, 테스트가 하드코딩된 ID(1L)에 의존하게 된다.운영 환경에서는 JPA가 ID를 자동 설정하므로 이 차이로 인해 테스트가 실제 동작을 정확히 검증하지 못할 수 있다.
♻️ 리플렉션을 사용한 ID 설정 또는 테스트용 setter 추가 제안
`@Override` public Coupon save(Coupon coupon) { long id = idSequence++; + // 리플렉션으로 ID 설정 (테스트 전용) + try { + java.lang.reflect.Field idField = Coupon.class.getDeclaredField("id"); + idField.setAccessible(true); + idField.set(coupon, id); + } catch (Exception e) { + throw new RuntimeException(e); + } fakeCouponReader.addCoupon(id, coupon); return coupon; }또는
@DataJpaTest를 사용한 통합 테스트로 전환하여 실제 JPA 동작을 검증하는 방안도 고려할 수 있다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponServiceTest.java` around lines 491 - 496, FakeCouponRepository.save에서 새 id를 생성해 fakeCouponReader에만 기록하고 실제 Coupon 엔티티의 id 필드를 설정하지 않아 테스트가 null id에 의존하고 있습니다; 수정하려면 FakeCouponRepository.save(…)에서 생성한 idSequence 값을 Coupon 객체에 반영하도록 설정한 뒤(fakeCoupon.setId(id) 또는 리플렉션으로 private id 설정) fakeCouponReader.addCoupon(id, coupon)을 호출해 저장하고 그 Coupon을 반환하도록 변경하세요; 필요한 경우 Coupon 클래스에 테스트 전용 setId(long) 메서드를 추가하거나 리플렉션 유틸을 사용해 id를 설정하도록 구현(메서드명: save, 변수: idSequence, fakeCouponReader)하세요.apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java (1)
113-116: cancelOrder에서 Order 중복 조회
orderService.cancelOrder(orderId, memberId)를 호출한 후 쿠폰 복원을 위해orderService.getOrder(orderId)를 다시 호출한다.cancelOrder가 이미 Order를 조회하고 있으므로, 해당 메서드에서 Order를 반환하도록 수정하면 불필요한 DB 쿼리를 제거할 수 있다.♻️ 중복 조회 제거 제안
-// 재고 복원 (cross-domain orchestration) — productId 순 정렬로 데드락 방지 -List<OrderItem> sortedItems = items.stream() +// cancelOrder에서 Order도 함께 반환받도록 수정 +OrderService.CancelResult cancelResult = orderService.cancelOrder(orderId, memberId); +List<OrderItem> sortedItems = cancelResult.items().stream() .sorted(Comparator.comparingLong(OrderItem::getProductId)) .toList(); for (OrderItem item : sortedItems) { Product product = productService.getProductForUpdate(item.getProductId()); product.increaseStock(item.getQuantity()); } // 쿠폰 복원 -Order order = orderService.getOrder(orderId); +Order order = cancelResult.order(); if (order.getMemberCouponId() != null) { couponService.restoreCoupon(order.getMemberCouponId()); }
OrderService.cancelOrder의 반환 타입을record CancelResult(Order order, List<OrderItem> items)로 변경하면 된다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java` around lines 113 - 116, Change cancelOrder to return the cancelled Order (and items) instead of void to avoid re-querying: update OrderService.cancelOrder signature to return a new record CancelResult(Order order, List<OrderItem> items), update its implementation to build and return CancelResult, then update OrderFacade to use the returned CancelResult.order (remove the extra orderService.getOrder(orderId) call) before calling couponService.restoreCoupon(order.getMemberCouponId()); also update any other callers of OrderService.cancelOrder to handle the new return type.apps/commerce-api/src/main/java/com/loopers/domain/coupon/MemberCoupon.java (1)
84-86:LocalDateTime.now()직접 사용으로 인한 테스트 어려움
isExpired()메서드가LocalDateTime.now()를 직접 호출하여 시간 기반 테스트가 어렵다.Clock을 주입하거나 시간을 파라미터로 받는 방식을 고려하라.♻️ 리팩토링 제안 (파라미터 방식)
-public boolean isExpired() { - return LocalDateTime.now().isAfter(this.expiredAt); -} +public boolean isExpired() { + return isExpiredAt(LocalDateTime.now()); +} + +public boolean isExpiredAt(LocalDateTime referenceTime) { + return referenceTime.isAfter(this.expiredAt); +}테스트에서
isExpiredAt(fixedTime)을 사용하여 시간 제어가 가능해진다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/coupon/MemberCoupon.java` around lines 84 - 86, The isExpired() method in MemberCoupon calls LocalDateTime.now() directly which makes time-based testing hard; update MemberCoupon to accept a Clock (e.g., via constructor/field) or add an overload isExpired(Clock clock) / isExpiredAt(LocalDateTime now) so tests can supply a fixed time; change the implementation of isExpired() to delegate to the new time-aware method (using the injected Clock or provided now) and compare expiredAt against the controlled time instead of calling LocalDateTime.now() inline.apps/commerce-api/src/main/java/com/loopers/domain/coupon/Coupon.java (1)
106-108:LocalDateTime.now()직접 사용으로 인한 테스트 어려움
MemberCoupon.isExpired()와 동일하게LocalDateTime.now()직접 호출로 시간 기반 테스트가 어렵다. 일관성을 위해 동일한 리팩토링 패턴을 적용하라.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/coupon/Coupon.java` around lines 106 - 108, The isExpired() method in Coupon currently calls LocalDateTime.now() directly, making time-based tests brittle; refactor Coupon.isExpired() to use the same time abstraction pattern used in MemberCoupon (e.g., rely on an injected Clock or a shared ClockProvider.now() method) instead of LocalDateTime.now(), update the Coupon class to accept or access that Clock abstraction (with a sensible default like Clock.systemDefaultZone()), and adjust any constructors/usage and tests to use a controllable clock for deterministic testing while keeping the expiredAt comparison logic unchanged.apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/MemberCouponReaderImpl.java (1)
30-56: 페이지네이션 매핑 로직 중복 제거 고려
findAllByMemberId와findAllByCouponId메서드에서Page<MemberCoupon>을PageResult<MemberCoupon>로 변환하는 로직이 동일하게 반복된다. 유지보수 시 변경 누락 가능성이 있으므로, 헬퍼 메서드로 추출하는 것을 권장한다.♻️ 리팩토링 제안
+ private PageResult<MemberCoupon> toPageResult(Page<MemberCoupon> result) { + return new PageResult<>( + result.getContent(), + result.getTotalElements(), + result.getTotalPages(), + result.getNumber(), + result.getSize() + ); + } + `@Override` public PageResult<MemberCoupon> findAllByMemberId(Long memberId, int page, int size) { Page<MemberCoupon> result = memberCouponJpaRepository.findAllByMemberIdAndDeletedAtIsNull( memberId, PageRequest.of(page, size, Sort.by(Sort.Direction.DESC, "createdAt")) ); - return new PageResult<>( - result.getContent(), - result.getTotalElements(), - result.getTotalPages(), - result.getNumber(), - result.getSize() - ); + return toPageResult(result); } `@Override` public PageResult<MemberCoupon> findAllByCouponId(Long couponId, int page, int size) { Page<MemberCoupon> result = memberCouponJpaRepository.findAllByCouponIdAndDeletedAtIsNull( couponId, PageRequest.of(page, size, Sort.by(Sort.Direction.DESC, "createdAt")) ); - return new PageResult<>( - result.getContent(), - result.getTotalElements(), - result.getTotalPages(), - result.getNumber(), - result.getSize() - ); + return toPageResult(result); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/MemberCouponReaderImpl.java` around lines 30 - 56, Both findAllByMemberId and findAllByCouponId duplicate the logic that converts a Page<MemberCoupon> into a PageResult<MemberCoupon>; extract that conversion into a private helper method (e.g., mapPageToPageResult(Page<MemberCoupon> page)) inside MemberCouponReaderImpl and call it from both findAllByMemberId and findAllByCouponId so they simply retrieve the Page from memberCouponJpaRepository and return mapPageToPageResult(result); ensure the helper returns a new PageResult constructed with result.getContent(), getTotalElements(), getTotalPages(), getNumber(), and getSize().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8a070f04-d8f0-4680-aacd-3e62493d9325
📒 Files selected for processing (68)
apps/commerce-api/src/main/java/com/loopers/application/address/AddressFacade.javaapps/commerce-api/src/main/java/com/loopers/application/brand/BrandFacade.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/CouponInfo.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/MemberCouponInfo.javaapps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.javaapps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.javaapps/commerce-api/src/main/java/com/loopers/application/order/OrderInfo.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaapps/commerce-api/src/main/java/com/loopers/domain/address/Address.javaapps/commerce-api/src/main/java/com/loopers/domain/address/AddressService.javaapps/commerce-api/src/main/java/com/loopers/domain/brand/Brand.javaapps/commerce-api/src/main/java/com/loopers/domain/brand/BrandRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/Coupon.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponReader.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponStatus.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponType.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/MemberCoupon.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/MemberCouponReader.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/MemberCouponRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/order/Order.javaapps/commerce-api/src/main/java/com/loopers/domain/order/OrderReader.javaapps/commerce-api/src/main/java/com/loopers/domain/order/OrderService.javaapps/commerce-api/src/main/java/com/loopers/domain/product/Product.javaapps/commerce-api/src/main/java/com/loopers/domain/product/ProductReader.javaapps/commerce-api/src/main/java/com/loopers/domain/product/ProductRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/brand/BrandJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/brand/BrandRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponReaderImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/MemberCouponJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/MemberCouponReaderImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/MemberCouponRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/order/OrderJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/order/OrderReaderImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductReaderImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/address/AddressV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/brand/BrandAdminV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponAdminV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponV1Dto.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/order/OrderV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/order/OrderV1Dto.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductAdminV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/security/MemberAuthFilter.javaapps/commerce-api/src/test/java/com/loopers/domain/address/AddressServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/address/AddressTest.javaapps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/brand/BrandTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/MemberCouponTest.javaapps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.javaapps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/product/ProductTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/ConcurrencyE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderV1ApiE2ETest.java
apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.java
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.java
Show resolved
Hide resolved
Comment on lines
+16
to
+26
| public static MemberCouponInfo of(MemberCoupon memberCoupon, CouponInfo couponInfo) { | ||
| return new MemberCouponInfo( | ||
| memberCoupon.getId(), | ||
| memberCoupon.getMemberId(), | ||
| memberCoupon.getCouponId(), | ||
| memberCoupon.getStatus().name(), | ||
| memberCoupon.getUsedAt(), | ||
| memberCoupon.getExpiredAt(), | ||
| couponInfo | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MemberCouponInfo.java" -o -name "*.java" -path "*/coupon/*" | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1811
🏁 Script executed:
rg -n -B3 "MemberCouponInfo\.of\(" --type=java -A3Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3012
🏁 Script executed:
# Check the actual MemberCouponInfo.java file
cat -n apps/commerce-api/src/main/java/com/loopers/application/coupon/MemberCouponInfo.java | head -50Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 985
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4676
🏁 Script executed:
# Check CouponInfo.of() for null handling
cat -n apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponInfo.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1050
🏁 Script executed:
# Check MemberCoupon and Coupon domain classes
cat -n apps/commerce-api/src/main/java/com/loopers/domain/coupon/MemberCoupon.java | head -60Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2358
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/domain/coupon/Coupon.java | head -60Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2240
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.java | head -100Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4822
🏁 Script executed:
# Check CouponReader implementation
cat -n apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponReader.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 439
🏁 Script executed:
# Check if there's error handling in CouponService
rg -n "getCoupon|issueCoupon" apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.java -A 5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1607
🏁 Script executed:
# Check for potential null handling in CouponInfo.of and related code
rg -n "couponMap\.get|orElseThrow|Optional" apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.java -B 2 -A 2Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 398
🏁 Script executed:
# Verify the getMyCoupons flow - check if all coupons in couponMap are guaranteed
cat -n apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.java | sed -n '73,86p'Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1005
🏁 Script executed:
# Check what happens when coupon is null
rg -n "getType|getId" apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponInfo.java -B 1 -A 1Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 256
MemberCouponInfo.of() 및 CouponInfo.of()에서 null 입력에 대한 방어가 없다.
실제 null 위험은 CouponFacade.getMyCoupons() 라인 83에서 발생한다. couponMap.get(mc.getCouponId())은 참조된 쿠폰이 삭제된 경우 null을 반환할 수 있으며, 이를 CouponInfo.of()에 전달하면 NPE가 발생한다. 데이터 일관성 문제(고아 MemberCoupon)가 생기면 API 응답 실패로 이어진다.
다음을 고려해야 한다:
CouponInfo.of()및MemberCouponInfo.of()에서 null 입력 검증 추가- 또는
getMyCoupons()에서couponMap.get()결과 null 체크 및 필터링 - 데이터 일관성 검증: 삭제된 쿠폰과 연결된 MemberCoupon을 주기적으로 정리하는 정책 수립
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/application/coupon/MemberCouponInfo.java`
around lines 16 - 26, MemberCouponInfo.of and CouponInfo.of are not defensive
against null inputs causing NPEs when a coupon is missing; add null guards:
update CouponInfo.of(Coupon coupon) to return null immediately if coupon is null
(or throw a clear IllegalArgumentException if you prefer explicit failure), and
update MemberCouponInfo.of(MemberCoupon memberCoupon, CouponInfo couponInfo) to
handle a null couponInfo (e.g., allow creating MemberCouponInfo with a null
couponInfo field instead of dereferencing it); additionally, in
CouponFacade.getMyCoupons where you call couponMap.get(mc.getCouponId()), filter
out or handle entries where couponMap.get(...) returns null (skip those
MemberCoupon entries or map them to MemberCouponInfo with null couponInfo) to
avoid passing null into CouponInfo.of and prevent NPEs.
Comment on lines
+56
to
+63
| if (liked) { | ||
| brandService.increaseLikeCount(brandId); | ||
| } else { | ||
| brandService.decreaseLikeCount(brandId); | ||
| } | ||
|
|
||
| return new LikeToggleInfo(liked, likeCount); | ||
| Brand brand = brandService.getBrand(brandId); | ||
| return new LikeToggleInfo(liked, brand.getLikeCount()); |
There was a problem hiding this comment.
Brand 토글에도 동일한 stale read 문제 존재
toggleProductLike와 동일하게 @Modifying 쿼리 후 brandService.getBrand(brandId) 호출 시 영속성 컨텍스트 캐시로 인한 stale 데이터 반환 가능성이 있다. 위에서 제안한 동일한 수정안을 적용해야 한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java`
around lines 56 - 63, The call to brandService.getBrand(brandId) can return
stale data after the `@Modifying` increaseLikeCount/decreaseLikeCount update;
after invoking increaseLikeCount or decreaseLikeCount in LikeFacade, ensure the
persistence context is synchronized/cleared before reading back the Brand (e.g.,
call EntityManager.flush() and EntityManager.clear(), or use
EntityManager.refresh(brand) or a brandService method that flushes/clears) so
getBrand(brandId) reads the updated likeCount; update the LikeFacade flow around
the increaseLikeCount/decreaseLikeCount and getBrand calls to perform this
flush/clear/refresh.
apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.java
Outdated
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/infrastructure/order/OrderJpaRepository.java
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.java
Show resolved
Hide resolved
apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderConcurrencyE2ETest.java
Show resolved
Hide resolved
Comment on lines
+76
to
+103
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | ||
| CountDownLatch latch = new CountDownLatch(threadCount); | ||
| AtomicInteger successCount = new AtomicInteger(0); | ||
|
|
||
| // Act — 동시에 10개 주문 | ||
| for (int i = 0; i < threadCount; i++) { | ||
| final int idx = i; | ||
| executor.submit(() -> { | ||
| try { | ||
| var items = List.of(new OrderV1Dto.CreateOrderRequest.OrderItemRequest(productId, 1)); | ||
| var request = new OrderV1Dto.CreateOrderRequest(addressIds[idx], null, items); | ||
|
|
||
| ResponseEntity<ApiResponse<OrderV1Dto.OrderResponse>> response = testRestTemplate.exchange( | ||
| "/api/v1/orders", HttpMethod.POST, | ||
| new HttpEntity<>(request, authHeaders(loginIds[idx], "Test1234!")), | ||
| new ParameterizedTypeReference<>() {} | ||
| ); | ||
|
|
||
| if (response.getStatusCode() == HttpStatus.OK) { | ||
| successCount.incrementAndGet(); | ||
| } | ||
| } finally { | ||
| latch.countDown(); | ||
| } | ||
| }); | ||
| } | ||
| latch.await(); | ||
| executor.shutdown(); |
There was a problem hiding this comment.
동시성 테스트는 시작 시점을 맞추고 대기 시간을 제한해야 한다.
Line 76-103, Line 132-162, Line 193-223, Line 249-273은 요청을 제출하는 즉시 실행하고 완료만 기다린다. 운영 관점에서는 요청이 한꺼번에 출발하지 않아 실제 락 경합 창을 충분히 만들지 못해 거짓 음성으로 통과할 수 있고, 반대로 데드락이나 무한 대기가 생기면 await()가 끝나지 않아 CI가 그대로 묶인다. ready/start/done 래치로 동시에 출발시키고, 완료 대기는 제한 시간을 둔 공통 helper로 바꾸는 편이 안전하다. 추가 테스트로는 같은 시나리오를 여러 번 반복 실행하고, 한 작업을 의도적으로 지연시켜 타임아웃 경로가 즉시 실패하는지도 확인해야 한다. As per coding guidelines, "**/*Test*.java: 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."
Also applies to: 132-162, 193-223, 249-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/ConcurrencyE2ETest.java`
around lines 76 - 103, The current concurrency test submits tasks immediately
(see ExecutorService executor, CountDownLatch latch, successCount and the lambda
that calls testRestTemplate.exchange) which can produce false negatives or hang;
change it to use a coordinated ready/start/done latch pattern (e.g., a ready
latch incremented by each task, a start latch each task waits on before sending
the request, and a done latch for completion) and replace the unconditional
latch.await() with a timed wait (with assert/fail on timeout) in the common
helper used by all similar tests; additionally run the scenario multiple
iterations and add one intentional per-task delay in a variant to trigger
timeout paths so you exercise lock contention and timeouts reliably.
Comment on lines
+145
to
+155
| ResponseEntity<ApiResponse<OrderV1Dto.OrderResponse>> response = testRestTemplate.exchange( | ||
| "/api/v1/orders", HttpMethod.POST, | ||
| new HttpEntity<>(request, authHeaders(loginIds[idx], "Test1234!")), | ||
| new ParameterizedTypeReference<>() {} | ||
| ); | ||
|
|
||
| if (response.getStatusCode() == HttpStatus.OK) { | ||
| successCount.incrementAndGet(); | ||
| } else { | ||
| failCount.incrementAndGet(); | ||
| } |
There was a problem hiding this comment.
실패 응답을 non-OK로 묶으면 서버 오류도 정상 경쟁 결과로 통과한다.
Line 145-155와 Line 205-215는 OK가 아니면 모두 같은 실패로 집계한다. 운영 관점에서는 재고 부족/쿠폰 충돌과 500·401·직렬화 오류는 대응이 전혀 다른데, 현재 테스트는 후자도 기대된 경쟁 실패처럼 통과시킬 수 있다. 각 요청의 HTTP 상태와 도메인 에러 코드를 수집해 재고 테스트는 정의된 비즈니스 실패 5건, 쿠폰 테스트는 PR 설명대로 OK 1건 + 409 1건처럼 정확한 분포를 검증해야 한다. 추가 테스트로는 쿠폰 충돌 시 409와 기대 에러 코드가 내려오는지, 재고 부족 시 서버 오류가 아니라 도메인 오류가 내려오는지를 각각 명시적으로 검증해야 한다. As per coding guidelines, "**/*Test*.java: 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다. 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."
Also applies to: 205-215
juoklee
changed the title
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java (1)
10-10:⚠️ Potential issue | 🟡 Minor중복 import가 존재한다.
com.loopers.domain.PageResult가 line 3과 line 10에서 중복 import되어 있다. 컴파일 에러는 아니지만 코드 정리가 필요하다.수정 예시
import com.loopers.domain.PageResult; import com.loopers.support.error.CoreException; import com.loopers.support.error.ErrorType; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import com.loopers.domain.PageResult;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java` at line 10, The file ProductServiceTest contains a duplicate import of com.loopers.domain.PageResult (appearing twice); remove the redundant import so only a single import of PageResult remains in the ProductServiceTest class to clean up the imports and avoid clutter.
♻️ Duplicate comments (2)
apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderConcurrencyE2ETest.java (2)
75-103:⚠️ Potential issue | 🟠 Major동시성 하네스가 느슨해서 경합을 못 만들거나 CI를 무기한 붙잡을 수 있다.
현재 일부 케이스는 작업을 제출하자마자 실행하고, 다른 케이스는
ready/start를 쓰더라도await()를 제한 없이 기다린다. 운영 관점에서는 실제 락 경합 창이 충분히 열리지 않아 잘못된 구현도 통과할 수 있고, 반대로 데드락이나 블로킹이 생기면 CI가 그대로 멈춘다.ready/start/done패턴과 timed await를 공통 helper로 통일하고, 실패 시 각 요청의 상태를 함께 출력하도록 바꾸는 편이 안전하다. 추가 테스트로는 같은 시나리오를 반복 실행하고, 한 작업을 의도적으로 지연시켜 타임아웃 경로가 확실히 실패하는지도 검증해야 한다.As per coding guidelines, "
**/*Test*.java: 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다. 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."Also applies to: 131-161, 197-251, 294-330, 378-404, 457-500, 613-654
144-154:⚠️ Potential issue | 🟠 Major비정상 응답을 한 버킷으로 묶으면 500도 정상 경쟁 결과로 통과한다.
현재는
OK가 아니면 전부 같은 실패로 집계한다. 운영 관점에서는 재고 부족, 잠금 충돌, 인증 실패, 500이 구분되지 않아 서버 오류가 기대한 경쟁 실패처럼 녹색 통과할 수 있다. 각 요청의 HTTP status와 API error code를 수집해서 예상한 비즈니스 실패만 허용하고, 5xx나 인증 오류는 즉시 실패시키도록 바꾸는 편이 안전하다. 추가 테스트로는 재고 부족 응답의 상태/코드를 고정하고, 주문 취소 경합의 패배 요청도 문서화된 실패 경로만 내려오는지 검증해야 한다.As per coding guidelines, "
**/*Test*.java: 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다. 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."Also applies to: 387-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderConcurrencyE2ETest.java` around lines 144 - 154, The test currently lumps all non-OK responses into failCount using the variables response, successCount, and failCount in OrderConcurrencyE2ETest; change the logic to inspect response.getStatusCode() and the ApiResponse body to extract the API error code so that only expected business-failure codes (e.g., out-of-stock or documented concurrency loss codes) are counted as a valid “competition loss”, while 5xx server errors and auth errors (e.g., HttpStatus.UNAUTHORIZED / is5xxServerError()) immediately fail the test (increment a distinct serverErrorCount or trigger assertion), and any other unexpected status/codes are treated as test failures; update the branch that processes response (and the duplicate block at the other location) to parse response.getBody() for the API error code and categorize outcomes accordingly, and add separate test cases asserting fixed status/code for out-of-stock and documented cancel-competition failure paths.
🧹 Nitpick comments (15)
apps/commerce-api/src/test/java/com/loopers/interfaces/api/MemberV1ApiE2ETest.java (1)
146-170: 에러 응답 바디 검증이 누락되어 있다.현재 테스트는 HTTP 상태 코드(400)만 검증한다. 다른 BAD_REQUEST 원인(이메일 형식 오류, 중복 loginId 등)과 구분하기 위해 에러 응답 바디의 메시지나 에러 코드를 검증하는 것이 바람직하다.
♻️ 에러 응답 검증 추가 제안
// assert -assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST); +assertAll( + () -> assertThat(response.getStatusCode()).isEqualTo(HttpStatus.BAD_REQUEST), + () -> assertThat(response.getBody()).isNotNull(), + () -> assertThat(response.getBody().meta().result()).isEqualTo(ApiResponse.Metadata.Result.FAIL) +);또한 경계값 테스트로 null gender 케이스 추가를 권장한다. MemberFacade에서 null 입력 시
NullPointerException이 발생하여 500 에러가 반환되는지 확인하면, 프로덕션 코드의 null 처리 누락을 검증할 수 있다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/MemberV1ApiE2ETest.java` around lines 146 - 170, Update the returnsBadRequest_whenInvalidGender test in MemberV1ApiE2ETest to assert the error response body as well as status: after calling testRestTemplate.exchange(ENDPOINT_REGISTER, HttpMethod.POST, new HttpEntity<>(request), new ParameterizedTypeReference<>() {}), deserialize the ApiResponse body and assert the error message or error code matches the expected invalid-gender indicator (use the ApiResponse error/message fields used by your controllers) to distinguish this from other 400 causes; additionally add a new test (e.g., returnsBadRequest_whenNullGender or returnsInternalError_whenNullGender) that submits a MemberV1Dto.RegisterRequest with gender=null to verify whether the service returns a 400 with validation message or a 500 due to NullPointerException, and assert the expected status and error body accordingly.apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java (7)
32-32:FUTURE정적 필드의 잠재적 플래키니스 문제
FUTURE가 클래스 로드 시점에 고정되므로, 테스트 실행이 여러 날에 걸쳐 캐싱되거나 CI 환경에서 시간대 차이가 발생하면 예기치 않은 결과가 나올 수 있다. 테스트 메서드 내에서 동적으로 생성하거나 헬퍼 메서드로 분리하는 것이 더 안정적이다.♻️ 동적 생성 헬퍼 메서드 제안
- private static final LocalDateTime FUTURE = LocalDateTime.now().plusDays(30); + private static LocalDateTime futureDate() { + return LocalDateTime.now().plusDays(30); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` at line 32, FUTURE is a static LocalDateTime causing flaky tests across days/timezones; remove the static field in CouponAdminV1ApiE2ETest and replace usages with a dynamic helper (e.g., a getFuture() method) or compute LocalDateTime.now().plusDays(30) inside each test so the future timestamp is generated at test runtime; update all references to FUTURE to call the new helper or local computation.
274-297: 존재하지 않는 쿠폰 삭제 테스트 누락존재하는 쿠폰 삭제만 테스트하고 있다. 존재하지 않는 쿠폰(
/999)을 삭제 시도했을 때의 응답(404 또는 200)을 검증하는 테스트가 필요하다. 이는 API의 멱등성(idempotency) 설계 의도를 명시하고, 클라이언트 측 에러 핸들링 구현에 명확한 계약을 제공한다.`@DisplayName`("존재하지 않는 쿠폰을 삭제하면, 404 Not Found 응답을 받는다.") `@Test` void returnsNotFound_whenCouponNotExists() { ResponseEntity<ApiResponse<Void>> response = testRestTemplate.exchange( COUPON_ADMIN + "/999", HttpMethod.DELETE, adminEntity(null), new ParameterizedTypeReference<>() {} ); assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 274 - 297, The DeleteCoupon nested test only covers deleting an existing coupon (returnsOk_whenCouponExists); add a new test method (e.g., returnsNotFound_whenCouponNotExists) inside the CouponAdminV1ApiE2ETest.DeleteCoupon class that issues a DELETE to COUPON_ADMIN + "/999" via testRestTemplate.exchange using adminEntity(null) and a ParameterizedTypeReference for ApiResponse<Void>, and assert the response status is HttpStatus.NOT_FOUND; annotate it with `@Test` and `@DisplayName` describing the 404 expectation to make the contract explicit.
174-197: 빈 목록 조회 테스트 누락쿠폰이 존재하는 경우만 테스트하고 있다. 쿠폰이 없을 때 빈 목록(
[])을 정상적으로 반환하는지 검증하는 테스트가 필요하다. 운영 환경에서 데이터가 없는 초기 상태나 전체 삭제 후 상태에서 API가 오류 없이 동작하는지 확인해야 한다.`@DisplayName`("쿠폰이 없으면, 빈 목록을 반환한다.") `@Test` void returnsEmptyList_whenNoCouponsExist() { ResponseEntity<ApiResponse<CouponV1Dto.CouponListResponse>> response = testRestTemplate.exchange( COUPON_ADMIN, HttpMethod.GET, new HttpEntity<>(adminHeaders()), new ParameterizedTypeReference<>() {} ); assertAll( () -> assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK), () -> assertThat(response.getBody().data().coupons()).isEmpty() ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 174 - 197, Add a new test in the GetCoupons nested class to verify the empty-list case: create a test method (e.g., returnsEmptyList_whenNoCouponsExist) that performs the same GET call used in returnsCoupons_whenCouponsExist (using testRestTemplate.exchange with COUPON_ADMIN, HttpMethod.GET, new HttpEntity<>(adminHeaders()), new ParameterizedTypeReference<>() {}) and assert the response status is HttpStatus.OK and that response.getBody().data().coupons() is empty; ensure this test does not create any coupons beforehand so it validates the API returns an empty list.
341-348: 헬퍼 메서드 응답 검증 누락
createCoupon(),registerMember(),issueCoupon()헬퍼 메서드가 응답 상태를 검증하지 않는다. 테스트 데이터 준비 단계에서 실패하면 이후 테스트가 예상치 못한 이유로 실패하여 디버깅이 어려워진다. 특히 CI 환경에서 간헐적 실패 원인 파악이 지연될 수 있다.♻️ 응답 검증 추가 제안
private Long createCoupon(String name, String type, Long value) { var request = new CouponV1Dto.CreateCouponRequest(name, type, value, null, FUTURE, null, null); ResponseEntity<ApiResponse<CouponV1Dto.CouponResponse>> response = testRestTemplate.exchange( COUPON_ADMIN, HttpMethod.POST, adminEntity(request), new ParameterizedTypeReference<>() {} ); + assertThat(response.getStatusCode()).isEqualTo(HttpStatus.CREATED); return response.getBody().data().coupon().id(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 341 - 348, The helper methods createCoupon, registerMember, and issueCoupon do not validate HTTP responses, so add explicit assertions in each helper to check the ResponseEntity status is 2xx (or HttpStatus.CREATED/OK as appropriate), that response.getBody() is non-null, and that expected nested fields (e.g., data().coupon().id() for createCoupon) are present before returning values; update createCoupon to assert response.getStatusCode().is2xxSuccessful() and that data().coupon().id() is non-null, and apply analogous checks in registerMember and issueCoupon to fail fast and provide clear test failures when setup requests fail.
299-326: 발급 내역 조회 엣지 케이스 테스트 누락발급 내역이 있는 경우만 테스트하고 있다. 다음 시나리오 추가가 필요하다:
- 발급 내역이 없을 때 빈 목록 반환 검증
- 존재하지 않는 쿠폰 ID로 조회 시 404 응답 검증
운영 환경에서 잘못된 쿠폰 ID 접근이나 발급 전 상태 조회 시 예상치 못한 오류가 발생하지 않도록 사전 검증해야 한다.
`@DisplayName`("발급 내역이 없으면, 빈 목록을 반환한다.") `@Test` void returnsEmptyList_whenNoIssuesExist() { Long couponId = createCoupon("쿠폰", "FIXED", 5000L); ResponseEntity<ApiResponse<CouponV1Dto.MemberCouponListResponse>> response = testRestTemplate.exchange( COUPON_ADMIN + "/" + couponId + "/issues", HttpMethod.GET, new HttpEntity<>(adminHeaders()), new ParameterizedTypeReference<>() {} ); assertAll( () -> assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK), () -> assertThat(response.getBody().data().memberCoupons()).isEmpty() ); } `@DisplayName`("존재하지 않는 쿠폰의 발급 내역을 조회하면, 404 Not Found 응답을 받는다.") `@Test` void returnsNotFound_whenCouponNotExists() { ResponseEntity<ApiResponse<CouponV1Dto.MemberCouponListResponse>> response = testRestTemplate.exchange( COUPON_ADMIN + "/999/issues", HttpMethod.GET, new HttpEntity<>(adminHeaders()), new ParameterizedTypeReference<>() {} ); assertThat(response.getStatusCode()).isEqualTo(HttpStatus.NOT_FOUND); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 299 - 326, Add two edge-case tests to CouponAdminV1ApiE2ETest: one that verifies GET /api-admin/v1/coupons/{couponId}/issues returns HTTP 200 with an empty memberCoupons list when no issues exist (create a coupon via createCoupon(...) and call the same testRestTemplate.exchange used in returnsIssuedCoupons_whenIssuesExist), and another that verifies querying a non-existent coupon ID returns HTTP 404 (call COUPON_ADMIN + "/999/issues" with adminHeaders and assert status is NOT_FOUND); place these as new `@Test` methods (e.g., returnsEmptyList_whenNoIssuesExist and returnsNotFound_whenCouponNotExists) alongside the existing nested GetIssuedCoupons tests so they reuse the same request pattern and assertions.
350-359:registerMember()헬퍼에 Content-Type 헤더 누락
registerMember()가 헤더 없이HttpEntity를 생성하고 있다. Spring의TestRestTemplate이 자동으로 Content-Type을 설정할 수 있으나, 명시적으로 설정하지 않으면 서버 측 설정에 따라 415 Unsupported Media Type 오류가 발생할 수 있다. 일관성을 위해 다른 헬퍼와 동일하게 헤더를 설정하는 것이 좋다.♻️ 헤더 추가 제안
private void registerMember(String loginId, String password) { + HttpHeaders headers = new HttpHeaders(); + headers.set("Content-Type", "application/json"); var request = new MemberV1Dto.RegisterRequest( loginId, password, "홍길동", LocalDate.of(1990, 1, 15), "MALE", loginId + "@example.com", null ); testRestTemplate.exchange( - "/api/v1/members", HttpMethod.POST, new HttpEntity<>(request), + "/api/v1/members", HttpMethod.POST, new HttpEntity<>(request, headers), new ParameterizedTypeReference<ApiResponse<MemberV1Dto.MemberResponse>>() {} ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 350 - 359, The helper registerMember is constructing an HttpEntity without Content-Type which can cause 415 errors; update registerMember to create HttpHeaders, set Content-Type to MediaType.APPLICATION_JSON, wrap the request in new HttpEntity<>(request, headers) and pass that to testRestTemplate.exchange when calling "/api/v1/members" (same pattern used by other helpers), ensuring the exchange call (testRestTemplate.exchange) uses the headers-equipped HttpEntity.
160-171: 정률 쿠폰 경계값 테스트 부족
rate > 100검증 테스트는 존재하나,rate = 0,rate = 100, 음수 값(rate = -1) 등의 경계값 테스트가 누락되었다. 할인율 0%가 허용되는지, 100%가 유효한지에 대한 비즈니스 규칙을 테스트로 명시해야 향후 로직 변경 시 회귀 버그를 조기에 감지할 수 있다.추가 테스트 케이스 예시:
`@DisplayName`("정률 쿠폰의 할인율이 0이면, 400 Bad Request 응답을 받는다.") `@Test` void returnsBadRequest_whenRateIsZero() { var request = new CouponV1Dto.CreateCouponRequest("쿠폰", "RATE", 0L, null, FUTURE, null, null); // ... } `@DisplayName`("정률 쿠폰의 할인율이 100이면, 201 Created 응답을 받는다.") `@Test` void returnsCreated_whenRateIs100() { var request = new CouponV1Dto.CreateCouponRequest("쿠폰", "RATE", 100L, null, FUTURE, null, null); // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java` around lines 160 - 171, Add boundary tests in CouponAdminV1ApiE2ETest to cover rate = 0, rate = 100 and a negative rate (e.g., -1) using the same pattern as returnsBadRequest_whenRateExceeds100: create requests with CouponV1Dto.CreateCouponRequest("쿠폰", "RATE", <value>L, null, FUTURE, null, null), call testRestTemplate.exchange(COUPON_ADMIN, HttpMethod.POST, adminEntity(request), new ParameterizedTypeReference<>() {}), and assert the expected HTTP status for each case (e.g., BAD_REQUEST for invalid values and CREATED for valid 100 if that is the business rule); add `@DisplayName` and `@Test` methods for each scenario so boundary behavior is explicitly verified.apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductJpaRepositoryTest.java (2)
118-131:likeCount가 0일 때 감소하지 않는 경계 케이스 테스트가 누락되었다.
decreaseLikeCount쿼리에likeCount > 0조건이 있으나, 이를 검증하는 테스트가 없다. 운영에서 좋아요가 0인 상품에 대해 감소 요청이 들어왔을 때 정상적으로 무시되는지 확인하는 테스트가 필요하다.추가 테스트 예시
`@DisplayName`("likeCount가 0이면 감소하지 않는다.") `@Test` `@Transactional` void doesNotDecrease_whenLikeCountIsZero() { // arrange Product product = createAndSaveProduct(); // likeCount is 0 by default // act int updatedRows = productJpaRepository.decreaseLikeCount(product.getId()); // assert assertThat(updatedRows).isZero(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductJpaRepositoryTest.java` around lines 118 - 131, Add a boundary test verifying decreaseLikeCount does nothing when likeCount is 0: in ProductJpaRepositoryTest create a test method (e.g., doesNotDecrease_whenLikeCountIsZero) that uses createAndSaveProduct() to persist a Product with default likeCount (0), calls productJpaRepository.decreaseLikeCount(product.getId()), and asserts the returned updatedRows is 0; place the test alongside decreases_whenProductExistsAndLikeCountIsPositive to cover the likeCount > 0 guard in decreaseLikeCount.
24-27:@Transactional과truncateAllTables()조합 시 테스트 격리 확인이 필요하다.개별 테스트에
@Transactional이 적용되어 있어 롤백이 예상되지만,@AfterEach에서truncateAllTables()를 호출하고 있다.@Transactional테스트는 기본적으로 롤백되므로truncateAllTables()가 불필요하거나, 롤백되지 않는 상황에서만 필요할 수 있다. 테스트 격리 전략을 명확히 하는 것이 좋다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductJpaRepositoryTest.java` around lines 24 - 27, The test class uses `@Transactional` on tests while also calling databaseCleanUp.truncateAllTables() in the tearDown() method (tearDown / truncateAllTables), causing redundant or inconsistent isolation; either remove the truncateAllTables() call and rely on `@Transactional` rollback for isolation, or if tests perform non-transactional work that requires explicit cleanup, remove `@Transactional` and keep truncateAllTables() (or document why both are needed). Update ProductJpaRepositoryTest accordingly so the test isolation strategy is single and explicit.apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java (1)
313-322:FakeProductRepository의 like count 메서드가 항상 1을 반환하여 실패 케이스 테스트가 불가능하다.
increaseLikeCount와decreaseLikeCount가 항상 1을 반환하므로ProductService의 NOT_FOUND 예외 경로를 테스트할 수 없다. 운영에서 존재하지 않는 상품에 대한 좋아요 요청이 올바르게 처리되는지 검증하는 단위 테스트가 누락되어 있다.수정 예시 - Fake 확장 및 테스트 추가
static class FakeProductRepository implements ProductRepository { private final Map<Long, Product> products = new HashMap<>(); private final FakeProductReader fakeProductReader; private long idSequence = 1L; private int likeCountReturnValue = 1; // 설정 가능하도록 변경 // ... 기존 코드 ... void setLikeCountReturnValue(int value) { this.likeCountReturnValue = value; } `@Override` public int increaseLikeCount(Long id) { return likeCountReturnValue; } `@Override` public int decreaseLikeCount(Long id) { return likeCountReturnValue; } }테스트 추가:
`@DisplayName`("좋아요 수 증가 시, ") `@Nested` class IncreaseLikeCount { `@DisplayName`("존재하지 않는 상품이면, NOT_FOUND 예외가 발생한다.") `@Test` void throwsNotFound_whenProductNotExists() { // Arrange fakeProductRepository.setLikeCountReturnValue(0); // Act & Assert CoreException exception = assertThrows(CoreException.class, () -> productService.increaseLikeCount(999L) ); assertThat(exception.getErrorType()).isEqualTo(ErrorType.NOT_FOUND); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java` around lines 313 - 322, FakeProductRepository의 increaseLikeCount/decreaseLikeCount가 항상 1을 반환해 NOT_FOUND 경로를 테스트할 수 없으니 FakeProductRepository에 likeCountReturnValue 같은 설정 필드를 추가하고 setLikeCountReturnValue(int) 메서드를 만들어 반환값을 조절 가능하게 변경하고, 기존 increaseLikeCount(Long id)와 decreaseLikeCount(Long id)는 이 필드를 반환하도록 바꿔라; 그 다음 ProductServiceTest에 존재하지 않는 상품 요청 시 NOT_FOUND를 던지는 테스트(예: fakeProductRepository.setLikeCountReturnValue(0) 후 productService.increaseLikeCount(999L) 호출하고 CoreException의 ErrorType이 NOT_FOUND인지 assert)를 추가해 실패 케이스를 검증하라.apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java (1)
61-72: 응답 본문 역참조 전에 성공 응답 여부를 먼저 고정해야 한다.현재 여러 테스트 본문과 셋업 헬퍼가
response.getBody().data()를 바로 읽거나 아예 응답을 검증하지 않는다. 이 상태에서는 쿠폰 생성·회원 등록·발급 셋업이 4xx/5xx로 실패해도 실제 원인 대신 NPE나 연쇄 실패로 보이기 쉬워서, CI에서 장애 원인 추적이 늦어진다. 각 요청 직후에 기대 상태코드와body/data존재를 먼저 검증하고, 그 다음에couponId, 목록 크기, 상태값 같은 세부 단정을 수행하는 공용 헬퍼로 정리하는 편이 안전하다. 추가로 관리자 인증 오류나 회원 등록 실패처럼 셋업이 깨지는 케이스에서 테스트가 즉시 의미 있는 메시지로 실패하는 회귀 테스트를 두는 것이 좋다.예시 수정안
+ private static <T> ApiResponse<T> requireSuccess( + ResponseEntity<ApiResponse<T>> response, + HttpStatus expectedStatus + ) { + assertThat(response.getStatusCode()).isEqualTo(expectedStatus); + assertThat(response.getBody()).isNotNull(); + assertThat(response.getBody().data()).isNotNull(); + return response.getBody(); + } + private Long createCoupon(String name, String type, Long value, LocalDateTime expiredAt) { var request = new CouponV1Dto.CreateCouponRequest(name, type, value, null, expiredAt, null, null); ResponseEntity<ApiResponse<CouponV1Dto.CouponResponse>> response = testRestTemplate.exchange( COUPON_ADMIN, HttpMethod.POST, adminEntity(request), new ParameterizedTypeReference<>() {} ); - return response.getBody().data().coupon().id(); + return requireSuccess(response, HttpStatus.CREATED).data().coupon().id(); }Also applies to: 134-146, 188-198, 206-215, 229-240, 252-260, 278-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java` around lines 61 - 72, The test dereferences response.getBody().data() before verifying the response succeeded, risking NPEs that hide the real 4xx/5xx cause; update CouponV1ApiE2ETest to first assert the HTTP status and that response.getBody() and response.getBody().data() are non-null immediately after each testRestTemplate.exchange (e.g., replace the current combined assertAll with an initial trio of assertions: status equals expected and body/data not null), then perform the detailed assertions on couponId and status; factor the initial checks into a small reusable helper (e.g., assertResponseSuccessWithData(response, HttpStatus.CREATED)) and apply the same pattern to the other test ranges mentioned (134-146, 188-198, 206-215, 229-240, 252-260, 278-321).apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java (1)
118-127:updateShippingAddress에@Transactional누락Facade의
updateShippingAddress메서드에@Transactional이 없다.orderService.updateShippingAddress는@Transactional이 있지만, 이후getOrderItems호출 시 트랜잭션 컨텍스트가 끊어질 수 있다. 현재는getOrderItems가readOnly = true를 사용하므로 기능상 문제는 없으나, Facade 레벨에서 일관된 트랜잭션 경계를 유지하는 것이 바람직하다.운영 관점: 향후 추가 로직이 들어갈 경우 트랜잭션 분리로 인한 데이터 불일치 가능성이 있다.
♻️ 수정 제안
+ `@Transactional` public OrderInfo updateShippingAddress(String loginId, Long orderId, String recipientName, String recipientPhone, String zipCode, String address1, String address2) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java` around lines 118 - 127, The OrderFacade.updateShippingAddress method lacks a transactional boundary which can break transaction context between orderService.updateShippingAddress and orderService.getOrderItems; add a class- or method-level `@Transactional` (preferably on OrderFacade.updateShippingAddress) so the update and subsequent reads execute in the same transaction, keeping transactional consistency for future expanded logic.apps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceTest.java (1)
322-325: 이 fake는 새forUpdate경로 사용 여부를 구분하지 못한다.
findByIdAndMemberIdForUpdate()가 일반 조회를 그대로 위임해서,cancelOrder()가 잠금 조회 대신 일반 조회를 호출해도 이 테스트는 계속 통과한다. 운영 관점에서는 락 회귀가 단위 테스트를 통과한 뒤 느린 E2E나 운영에서만 드러난다. 호출 플래그를 추가하거나 취소 테스트에서 일반 조회가 호출되면 실패하도록 분리해서forUpdate경로 사용을 직접 검증하는 편이 안전하다. 추가 테스트로는 취소 성공 케이스에서findByIdAndMemberIdForUpdate()가 1회 호출되고 일반 조회는 호출되지 않는지 확인해야 한다.변경 예시
static class FakeOrderReader implements OrderReader { private final List<Order> orders = new ArrayList<>(); + private boolean forUpdateCalled; + private boolean plainLookupCalled; void addOrder(Order order) { orders.add(order); } `@Override` public Optional<Order> findById(Long id) { @@ `@Override` public Optional<Order> findByIdAndMemberId(Long id, Long memberId) { + plainLookupCalled = true; return orders.stream() .filter(o -> o.getId().equals(id) && o.getMemberId().equals(memberId)) .findFirst(); } `@Override` public Optional<Order> findByIdAndMemberIdForUpdate(Long id, Long memberId) { - return findByIdAndMemberId(id, memberId); + forUpdateCalled = true; + return orders.stream() + .filter(o -> o.getId().equals(id) && o.getMemberId().equals(memberId)) + .findFirst(); } + + boolean wasForUpdateCalled() { return forUpdateCalled; } + boolean wasPlainLookupCalled() { return plainLookupCalled; } }OrderService.CancelOrderResult result = orderService.cancelOrder(order.getId(), 1L); assertAll( () -> assertThat(result.order().getStatus()).isEqualTo(OrderStatus.CANCELLED), - () -> assertThat(result.items()).hasSize(1) + () -> assertThat(result.items()).hasSize(1), + () -> assertThat(fakeOrderReader.wasForUpdateCalled()).isTrue(), + () -> assertThat(fakeOrderReader.wasPlainLookupCalled()).isFalse() );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceTest.java` around lines 322 - 325, 현재 테스트용 fake가 findByIdAndMemberIdForUpdate()를 일반 조회로 위임해 for-update 경로 사용 여부를 검증하지 못합니다; 수정 방법은 테스트용 fake(사용된 가짜 리포지토리 클래스)에 호출 플래그나 카운터를 추가하여 findByIdAndMemberIdForUpdate()와 findByIdAndMemberId()를 별도로 기록하도록 변경하고, OrderServiceTest의 취소 성공 케이스에서 cancelOrder() 실행 후 findByIdAndMemberIdForUpdate()가 정확히 1회 호출되었고 findByIdAndMemberId()는 호출되지 않았음을 assert하도록 테스트를 분리하세요; 관련 식별자: findByIdAndMemberIdForUpdate, findByIdAndMemberId, cancelOrder, OrderServiceTest.apps/commerce-api/src/test/java/com/loopers/interfaces/api/LikeConcurrencyE2ETest.java (1)
218-245: 픽스처 생성 실패를 즉시 단언하지 않아 원인 파악이 늦어진다.운영 관점에서
registerMember는 등록 실패를 그대로 흘리고,registerBrand와registerProduct는 상태와 본문 확인 없이 바로 역참조하므로 세팅 API가 깨지면 동시성 문제가 아니라 후속401/404나NullPointerException처럼 보이게 된다. 각 helper에서2xx와body/data존재를 즉시 단언하고, 실패 메시지에 endpoint를 포함해야 한다. 추가로 잘못된 관리자 헤더나 validation 오류가 들어와도 세팅 단계에서 바로 실패하는지 확인하는 스모크 케이스를 하나 두는 편이 좋다. As per coding guidelines, '통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다.'🧪 예시 수정안
private void registerMember(String loginId, String password) { var request = new MemberV1Dto.RegisterRequest( loginId, password, "홍길동", LocalDate.of(1990, 1, 15), "MALE", loginId + "@example.com", null ); - testRestTemplate.exchange( + ResponseEntity<ApiResponse<MemberV1Dto.MemberResponse>> response = testRestTemplate.exchange( "/api/v1/members", HttpMethod.POST, new HttpEntity<>(request), new ParameterizedTypeReference<ApiResponse<MemberV1Dto.MemberResponse>>() {} ); + assertThat(response.getStatusCode().is2xxSuccessful()) + .as("member fixture creation failed: /api/v1/members") + .isTrue(); + assertThat(response.getBody()).isNotNull(); + assertThat(response.getBody().data()).isNotNull(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/LikeConcurrencyE2ETest.java` around lines 218 - 245, Update the helper methods registerMember, registerBrand, and registerProduct to immediately assert the HTTP response status is 2xx and that response.getBody() and the nested data (data/member, data/brand, data/product) are non-null before returning values; when an assertion fails, include the request endpoint and a short diagnostic (status code and body) in the message so failures in setup surface clearly (also validate adminEntity headers in registerBrand/registerProduct). Additionally add a lightweight smoke test that calls these helpers and fails fast if any setup step returns non-2xx or missing body/data to catch authorization/validation issues early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/member/MemberFacade.java`:
- Around line 27-32: The code calls Gender.valueOf(gender) without handling
null, causing a NullPointerException to escape; update the MemberFacade logic to
explicitly check if the input gender is null (or empty) before calling
Gender.valueOf and throw the same CoreException(ErrorType.BAD_REQUEST, "유효하지 않은
성별입니다: " + gender) for null/invalid values, or alternatively catch both
IllegalArgumentException and NullPointerException around the Gender.valueOf call
to convert them into that CoreException; also add an E2E/unit test that passes
null as gender to assert a BAD_REQUEST response.
In `@apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.java`:
- Around line 57-61: The current decreaseLikeCount in BrandService treats
repository update result 0 as NOT_FOUND, which misclassifies existing brands
with likeCount == 0; update decreaseLikeCount to first check existence (e.g.,
brandRepository.existsById(id) or findById(id)) and throw CoreException
NOT_FOUND only when the brand truly does not exist, then call
brandRepository.decreaseLikeCount(id) and treat a 0 update as a no-op (or clamp
within repository to return >=1 on success) so you don't return 404 for
already-zero likes; add unit tests covering "decrease when likeCount==0",
"decrease soft-deleted brand", and "decrease non-existent id" to enforce the
contract.
In `@apps/commerce-api/src/main/java/com/loopers/domain/order/Order.java`:
- Around line 83-89: The code in Order.java compares Long objects with the
reference operator != (totalAmount != originalAmount - discountAmount), which
can fail for values outside the Long cache; change the comparison to a
value-based check using primitives or equals. Replace that line with a value
comparison such as totalAmount.longValue() != originalAmount.longValue() -
discountAmount.longValue() or
!totalAmount.equals(Long.valueOf(originalAmount.longValue() -
discountAmount.longValue())), keeping the surrounding null checks for
memberCouponId/originalAmount/discountAmount intact.
In
`@apps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.java`:
- Around line 71-77: The decreaseLikeCount method currently throws NOT_FOUND
when productRepository.decreaseLikeCount(id) returns 0, which conflates "product
missing" with "likeCount already zero"; update decreaseLikeCount to first verify
product existence and current likeCount (e.g., use
productRepository.existsById(id) or load Product via findById and check
getLikeCount()) and then: if the product does not exist throw NOT_FOUND, else if
likeCount == 0 either return/ignore the request or throw a BAD_REQUEST (choose
per business rule). Ensure you reference and update the decreaseLikeCount method
and any repository calls (productRepository.decreaseLikeCount,
productRepository.existsById, productRepository.findById) to implement this
logic.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.java`:
- Around line 113-128: ApiControllerAdvice currently maps
OptimisticLockingFailureException, PessimisticLockingFailureException and
DataIntegrityViolationException all to ErrorType.CONFLICT which prevents clients
from distinguishing retryable lock conflicts from permanent integrity
violations; update ApiControllerAdvice to: (1) map
OptimisticLockingFailureException and PessimisticLockingFailureException to a
distinct retryable error (e.g., ErrorType.RETRYABLE_CONFLICT or include a
retryable flag in the ApiResponse) and a retry-friendly message, (2) change
DataIntegrityViolationException handling to inspect the root cause/constraint
name and return either a specific duplicate-key ErrorType (or translate known
duplicate constraints to CoreException in the service layer such as
MemberService where duplicate signups occur) or a non-retryable integrity
ErrorType with an appropriate message, and (3) add unit/web tests that throw
each of OptimisticLockingFailureException, PessimisticLockingFailureException,
duplicate-key DataIntegrityViolationException and non-duplicate
DataIntegrityViolationException to assert distinct status/code/message behavior;
reference ApiControllerAdvice, handleOptimisticLock, handlePessimisticLock,
handleDataIntegrity, and MemberService for where to perform translations.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.java`:
- Around line 176-223: The FakeBrandRepository used in BrandServiceTest
incorrectly always returns 1 and doesn't mutate likeCount, so tests
(IncreaseLikeCount/DecreaseLikeCount and methods brandService.increaseLikeCount
/ brandService.decreaseLikeCount) miss real storage behavior and boundary
failures; update FakeBrandRepository to store Brand entities and modify their
likeCount on increase/decrease (enforcing a 0 lower bound on decrease and
returning the actual updated count or 0 when no rows changed), then extend tests
to assert post-operation likeCount for cases: increase succeeds, decrease 1->0,
decrease at 0 stays 0, and existing vs non-existing ID throws NOT_FOUND to match
JPA contract and prevent regressions.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java`:
- Around line 140-146: The test in CouponV1ApiE2ETest currently only checks
expiredAt is before FUTURE and after now+6 days, which allows incorrect
validDays; capture timestamps immediately before issuing the coupon (e.g.,
beforeIssue = LocalDateTime.now()) and immediately after receiving the response
(afterIssue = LocalDateTime.now()), then assert
response.getBody().data().memberCoupon().expiredAt() is within a narrow window
around beforeIssue.plusDays(validDays) and afterIssue.plusDays(validDays) (e.g.,
>= beforeIssue.plusDays(validDays) and <= afterIssue.plusDays(validDays)), add
an extra regression test for validDays=1 to cover the boundary, and keep or
remove the global FUTURE assertion so the personal expiredAt assertion is not
masked.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/LikeConcurrencyE2ETest.java`:
- Around line 122-123: 현재 토글 동시성 테스트가 모든 비-200응답을 실패로 집계해 5xx가 정상 시나리오로 통과될 수
있으니, 테스트에서 허용 가능한 동시성 실패를 명확히 정하고 그 외 상태코드는 별도 집계해 0임을 단언하도록 수정하세요: 테스트에서
AtomicInteger successCount/failCount 대신 각각 okCount/conflictCount/otherCount를 두고,
응답 상태가 HttpStatus.OK이면 okCount++, HttpStatus.CONFLICT(409)이면 응답 바디의 도메인 에러
코드(LikeService/LikeFacade에서 사용하는 도메인 에러 코드)를 검증한 후 conflictCount++로 처리하며, 그
외(500/401/422 등)는 otherCount++로 처리하고 테스트 마지막에 otherCount.get()==0을 assert 하여
5xx/서드파티 에러가 통과하지 않도록 만드세요.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderConcurrencyE2ETest.java`:
- Around line 546-576: The concurrent cancellation/update test currently
swallows exceptions and allows either outcome; change the two executor.submit
tasks (the cancel lambda and the update lambda that builds
OrderV1Dto.UpdateShippingAddressRequest and calls testRestTemplate.exchange) to
capture and store each ResponseEntity and any thrown Exception instead of
ignoring them, assert the exact expected HTTP status codes (e.g., HttpStatus.OK
vs 4xx/5xx) for each scenario, ensure executor.awaitTermination(...) result is
checked before asserting, and after threads finish fetch the final order via
testRestTemplate.exchange and assert the shipping address on the
OrderV1Dto.OrderResponse equals the original address when cancel should win;
additionally split into two explicit tests (cancel-first and update-first) that
enforce ordering and validate both per-request responses and the final persisted
shipping address, using the existing cancelSuccess/updateSuccess counters only
for metrics not as the sole assertion.
---
Outside diff comments:
In
`@apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java`:
- Line 10: The file ProductServiceTest contains a duplicate import of
com.loopers.domain.PageResult (appearing twice); remove the redundant import so
only a single import of PageResult remains in the ProductServiceTest class to
clean up the imports and avoid clutter.
---
Duplicate comments:
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderConcurrencyE2ETest.java`:
- Around line 144-154: The test currently lumps all non-OK responses into
failCount using the variables response, successCount, and failCount in
OrderConcurrencyE2ETest; change the logic to inspect response.getStatusCode()
and the ApiResponse body to extract the API error code so that only expected
business-failure codes (e.g., out-of-stock or documented concurrency loss codes)
are counted as a valid “competition loss”, while 5xx server errors and auth
errors (e.g., HttpStatus.UNAUTHORIZED / is5xxServerError()) immediately fail the
test (increment a distinct serverErrorCount or trigger assertion), and any other
unexpected status/codes are treated as test failures; update the branch that
processes response (and the duplicate block at the other location) to parse
response.getBody() for the API error code and categorize outcomes accordingly,
and add separate test cases asserting fixed status/code for out-of-stock and
documented cancel-competition failure paths.
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java`:
- Around line 118-127: The OrderFacade.updateShippingAddress method lacks a
transactional boundary which can break transaction context between
orderService.updateShippingAddress and orderService.getOrderItems; add a class-
or method-level `@Transactional` (preferably on OrderFacade.updateShippingAddress)
so the update and subsequent reads execute in the same transaction, keeping
transactional consistency for future expanded logic.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceTest.java`:
- Around line 322-325: 현재 테스트용 fake가 findByIdAndMemberIdForUpdate()를 일반 조회로 위임해
for-update 경로 사용 여부를 검증하지 못합니다; 수정 방법은 테스트용 fake(사용된 가짜 리포지토리 클래스)에 호출 플래그나 카운터를
추가하여 findByIdAndMemberIdForUpdate()와 findByIdAndMemberId()를 별도로 기록하도록 변경하고,
OrderServiceTest의 취소 성공 케이스에서 cancelOrder() 실행 후 findByIdAndMemberIdForUpdate()가
정확히 1회 호출되었고 findByIdAndMemberId()는 호출되지 않았음을 assert하도록 테스트를 분리하세요; 관련 식별자:
findByIdAndMemberIdForUpdate, findByIdAndMemberId, cancelOrder,
OrderServiceTest.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java`:
- Around line 313-322: FakeProductRepository의
increaseLikeCount/decreaseLikeCount가 항상 1을 반환해 NOT_FOUND 경로를 테스트할 수 없으니
FakeProductRepository에 likeCountReturnValue 같은 설정 필드를 추가하고
setLikeCountReturnValue(int) 메서드를 만들어 반환값을 조절 가능하게 변경하고, 기존
increaseLikeCount(Long id)와 decreaseLikeCount(Long id)는 이 필드를 반환하도록 바꿔라; 그 다음
ProductServiceTest에 존재하지 않는 상품 요청 시 NOT_FOUND를 던지는 테스트(예:
fakeProductRepository.setLikeCountReturnValue(0) 후
productService.increaseLikeCount(999L) 호출하고 CoreException의 ErrorType이
NOT_FOUND인지 assert)를 추가해 실패 케이스를 검증하라.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductJpaRepositoryTest.java`:
- Around line 118-131: Add a boundary test verifying decreaseLikeCount does
nothing when likeCount is 0: in ProductJpaRepositoryTest create a test method
(e.g., doesNotDecrease_whenLikeCountIsZero) that uses createAndSaveProduct() to
persist a Product with default likeCount (0), calls
productJpaRepository.decreaseLikeCount(product.getId()), and asserts the
returned updatedRows is 0; place the test alongside
decreases_whenProductExistsAndLikeCountIsPositive to cover the likeCount > 0
guard in decreaseLikeCount.
- Around line 24-27: The test class uses `@Transactional` on tests while also
calling databaseCleanUp.truncateAllTables() in the tearDown() method (tearDown /
truncateAllTables), causing redundant or inconsistent isolation; either remove
the truncateAllTables() call and rely on `@Transactional` rollback for isolation,
or if tests perform non-transactional work that requires explicit cleanup,
remove `@Transactional` and keep truncateAllTables() (or document why both are
needed). Update ProductJpaRepositoryTest accordingly so the test isolation
strategy is single and explicit.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.java`:
- Line 32: FUTURE is a static LocalDateTime causing flaky tests across
days/timezones; remove the static field in CouponAdminV1ApiE2ETest and replace
usages with a dynamic helper (e.g., a getFuture() method) or compute
LocalDateTime.now().plusDays(30) inside each test so the future timestamp is
generated at test runtime; update all references to FUTURE to call the new
helper or local computation.
- Around line 274-297: The DeleteCoupon nested test only covers deleting an
existing coupon (returnsOk_whenCouponExists); add a new test method (e.g.,
returnsNotFound_whenCouponNotExists) inside the
CouponAdminV1ApiE2ETest.DeleteCoupon class that issues a DELETE to COUPON_ADMIN
+ "/999" via testRestTemplate.exchange using adminEntity(null) and a
ParameterizedTypeReference for ApiResponse<Void>, and assert the response status
is HttpStatus.NOT_FOUND; annotate it with `@Test` and `@DisplayName` describing the
404 expectation to make the contract explicit.
- Around line 174-197: Add a new test in the GetCoupons nested class to verify
the empty-list case: create a test method (e.g.,
returnsEmptyList_whenNoCouponsExist) that performs the same GET call used in
returnsCoupons_whenCouponsExist (using testRestTemplate.exchange with
COUPON_ADMIN, HttpMethod.GET, new HttpEntity<>(adminHeaders()), new
ParameterizedTypeReference<>() {}) and assert the response status is
HttpStatus.OK and that response.getBody().data().coupons() is empty; ensure this
test does not create any coupons beforehand so it validates the API returns an
empty list.
- Around line 341-348: The helper methods createCoupon, registerMember, and
issueCoupon do not validate HTTP responses, so add explicit assertions in each
helper to check the ResponseEntity status is 2xx (or HttpStatus.CREATED/OK as
appropriate), that response.getBody() is non-null, and that expected nested
fields (e.g., data().coupon().id() for createCoupon) are present before
returning values; update createCoupon to assert
response.getStatusCode().is2xxSuccessful() and that data().coupon().id() is
non-null, and apply analogous checks in registerMember and issueCoupon to fail
fast and provide clear test failures when setup requests fail.
- Around line 299-326: Add two edge-case tests to CouponAdminV1ApiE2ETest: one
that verifies GET /api-admin/v1/coupons/{couponId}/issues returns HTTP 200 with
an empty memberCoupons list when no issues exist (create a coupon via
createCoupon(...) and call the same testRestTemplate.exchange used in
returnsIssuedCoupons_whenIssuesExist), and another that verifies querying a
non-existent coupon ID returns HTTP 404 (call COUPON_ADMIN + "/999/issues" with
adminHeaders and assert status is NOT_FOUND); place these as new `@Test` methods
(e.g., returnsEmptyList_whenNoIssuesExist and
returnsNotFound_whenCouponNotExists) alongside the existing nested
GetIssuedCoupons tests so they reuse the same request pattern and assertions.
- Around line 350-359: The helper registerMember is constructing an HttpEntity
without Content-Type which can cause 415 errors; update registerMember to create
HttpHeaders, set Content-Type to MediaType.APPLICATION_JSON, wrap the request in
new HttpEntity<>(request, headers) and pass that to testRestTemplate.exchange
when calling "/api/v1/members" (same pattern used by other helpers), ensuring
the exchange call (testRestTemplate.exchange) uses the headers-equipped
HttpEntity.
- Around line 160-171: Add boundary tests in CouponAdminV1ApiE2ETest to cover
rate = 0, rate = 100 and a negative rate (e.g., -1) using the same pattern as
returnsBadRequest_whenRateExceeds100: create requests with
CouponV1Dto.CreateCouponRequest("쿠폰", "RATE", <value>L, null, FUTURE, null,
null), call testRestTemplate.exchange(COUPON_ADMIN, HttpMethod.POST,
adminEntity(request), new ParameterizedTypeReference<>() {}), and assert the
expected HTTP status for each case (e.g., BAD_REQUEST for invalid values and
CREATED for valid 100 if that is the business rule); add `@DisplayName` and `@Test`
methods for each scenario so boundary behavior is explicitly verified.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java`:
- Around line 61-72: The test dereferences response.getBody().data() before
verifying the response succeeded, risking NPEs that hide the real 4xx/5xx cause;
update CouponV1ApiE2ETest to first assert the HTTP status and that
response.getBody() and response.getBody().data() are non-null immediately after
each testRestTemplate.exchange (e.g., replace the current combined assertAll
with an initial trio of assertions: status equals expected and body/data not
null), then perform the detailed assertions on couponId and status; factor the
initial checks into a small reusable helper (e.g.,
assertResponseSuccessWithData(response, HttpStatus.CREATED)) and apply the same
pattern to the other test ranges mentioned (134-146, 188-198, 206-215, 229-240,
252-260, 278-321).
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/LikeConcurrencyE2ETest.java`:
- Around line 218-245: Update the helper methods registerMember, registerBrand,
and registerProduct to immediately assert the HTTP response status is 2xx and
that response.getBody() and the nested data (data/member, data/brand,
data/product) are non-null before returning values; when an assertion fails,
include the request endpoint and a short diagnostic (status code and body) in
the message so failures in setup surface clearly (also validate adminEntity
headers in registerBrand/registerProduct). Additionally add a lightweight smoke
test that calls these helpers and fails fast if any setup step returns non-2xx
or missing body/data to catch authorization/validation issues early.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/MemberV1ApiE2ETest.java`:
- Around line 146-170: Update the returnsBadRequest_whenInvalidGender test in
MemberV1ApiE2ETest to assert the error response body as well as status: after
calling testRestTemplate.exchange(ENDPOINT_REGISTER, HttpMethod.POST, new
HttpEntity<>(request), new ParameterizedTypeReference<>() {}), deserialize the
ApiResponse body and assert the error message or error code matches the expected
invalid-gender indicator (use the ApiResponse error/message fields used by your
controllers) to distinguish this from other 400 causes; additionally add a new
test (e.g., returnsBadRequest_whenNullGender or
returnsInternalError_whenNullGender) that submits a MemberV1Dto.RegisterRequest
with gender=null to verify whether the service returns a 400 with validation
message or a 500 due to NullPointerException, and assert the expected status and
error body accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5de1d1b-cfa8-4927-8206-d70228e1ca2a
📒 Files selected for processing (31)
apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.javaapps/commerce-api/src/main/java/com/loopers/application/member/MemberFacade.javaapps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaapps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponReader.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/MemberCoupon.javaapps/commerce-api/src/main/java/com/loopers/domain/order/Order.javaapps/commerce-api/src/main/java/com/loopers/domain/order/OrderService.javaapps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/brand/BrandJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponReaderImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/order/OrderJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.javaapps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/MemberCouponTest.javaapps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.javaapps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductJpaRepositoryTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponAdminV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponConcurrencyE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/LikeConcurrencyE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/MemberV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderConcurrencyE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/ProductV1ApiE2ETest.java
✅ Files skipped from review due to trivial changes (1)
- apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/commerce-api/src/test/java/com/loopers/domain/coupon/MemberCouponTest.java
- apps/commerce-api/src/main/java/com/loopers/domain/coupon/MemberCoupon.java
- apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.java
- apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponJpaRepository.java
- apps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.java
Comment on lines
+27
to
+32
| Gender genderEnum; | ||
| try { | ||
| genderEnum = Gender.valueOf(gender); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new CoreException(ErrorType.BAD_REQUEST, "유효하지 않은 성별입니다: " + gender); | ||
| } |
There was a problem hiding this comment.
gender가 null인 경우 NullPointerException이 발생하여 500 에러로 처리된다.
Gender.valueOf(null)은 IllegalArgumentException이 아닌 NullPointerException을 던진다. 현재 코드는 IllegalArgumentException만 catch하므로, null 입력 시 예상치 못한 500 에러가 반환된다.
🛡️ null 체크 추가 제안
Gender genderEnum;
+if (gender == null) {
+ throw new CoreException(ErrorType.BAD_REQUEST, "성별은 필수 입력값입니다.");
+}
try {
genderEnum = Gender.valueOf(gender);
} catch (IllegalArgumentException e) {
throw new CoreException(ErrorType.BAD_REQUEST, "유효하지 않은 성별입니다: " + gender);
}운영 환경에서 null이 입력되는 경우, 로그에 NPE 스택트레이스가 남고 클라이언트는 명확하지 않은 500 에러를 받게 된다. null 케이스에 대한 E2E 테스트도 추가를 권장한다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Gender genderEnum; | |
| try { | |
| genderEnum = Gender.valueOf(gender); | |
| } catch (IllegalArgumentException e) { | |
| throw new CoreException(ErrorType.BAD_REQUEST, "유효하지 않은 성별입니다: " + gender); | |
| } | |
| Gender genderEnum; | |
| if (gender == null) { | |
| throw new CoreException(ErrorType.BAD_REQUEST, "성별은 필수 입력값입니다."); | |
| } | |
| try { | |
| genderEnum = Gender.valueOf(gender); | |
| } catch (IllegalArgumentException e) { | |
| throw new CoreException(ErrorType.BAD_REQUEST, "유효하지 않은 성별입니다: " + gender); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/application/member/MemberFacade.java`
around lines 27 - 32, The code calls Gender.valueOf(gender) without handling
null, causing a NullPointerException to escape; update the MemberFacade logic to
explicitly check if the input gender is null (or empty) before calling
Gender.valueOf and throw the same CoreException(ErrorType.BAD_REQUEST, "유효하지 않은
성별입니다: " + gender) for null/invalid values, or alternatively catch both
IllegalArgumentException and NullPointerException around the Gender.valueOf call
to convert them into that CoreException; also add an E2E/unit test that passes
null as gender to assert a BAD_REQUEST response.
Comment on lines
+57
to
+61
| public void decreaseLikeCount(Long id) { | ||
| int updatedCount = brandRepository.decreaseLikeCount(id); | ||
| if (updatedCount == 0) { | ||
| throw new CoreException(ErrorType.NOT_FOUND, "브랜드를 찾을 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
0개 좋아요 브랜드를 NOT_FOUND로 오인할 수 있다
Line 58의 brandRepository.decreaseLikeCount(id)는 현재 저장소 쿼리 조건상 likeCount == 0이어도 0을 반환할 수 있는데, Line 59-60은 이를 전부 NOT_FOUND로 매핑한다. 그 결과 실제로 존재하는 브랜드에서도 좋아요 수가 이미 0이면 404가 내려가고, 운영에서는 실패율·재시도 판단·에러 집계가 모두 왜곡된다.
0건 업데이트 = 미존재라는 가정을 서비스/저장소 중 한 곳에서만 유지하도록 계약을 맞춰야 한다. 예를 들어 저장소가 기존 행이면 항상 1건을 반환하도록 0 하한으로 clamp 하거나, 여기서 먼저 존재 여부를 확인한 뒤 감소를 수행하는 방식으로 분리하는 편이 안전하다. 추가로 좋아요 0인 기존 브랜드 감소, soft-delete 브랜드 감소, 없는 ID 감소 테스트를 넣어야 한다.
As per coding guidelines "멱등성과 중복 처리 방지 전략을 점검한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.java`
around lines 57 - 61, The current decreaseLikeCount in BrandService treats
repository update result 0 as NOT_FOUND, which misclassifies existing brands
with likeCount == 0; update decreaseLikeCount to first check existence (e.g.,
brandRepository.existsById(id) or findById(id)) and throw CoreException
NOT_FOUND only when the brand truly does not exist, then call
brandRepository.decreaseLikeCount(id) and treat a 0 update as a no-op (or clamp
within repository to return >=1 on success) so you don't return 404 for
already-zero likes; add unit tests covering "decrease when likeCount==0",
"decrease soft-deleted brand", and "decrease non-existent id" to enforce the
contract.
Comment on lines
+83
to
+89
| if (memberCouponId != null) { | ||
| validateNotNull(originalAmount, "쿠폰 적용 시 원래 금액은 필수입니다."); | ||
| validateNotNull(discountAmount, "쿠폰 적용 시 할인 금액은 필수입니다."); | ||
| if (totalAmount != originalAmount - discountAmount) { | ||
| throw new CoreException(ErrorType.BAD_REQUEST, "주문 총액이 원래 금액에서 할인 금액을 뺀 값과 일치하지 않습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
Long 타입 비교 연산자 오류: !=는 참조 비교를 수행한다.
Line 86에서 totalAmount != originalAmount - discountAmount는 Long 객체를 !=로 비교하고 있다. Long 캐싱 범위(-128~127)를 벗어나는 금액에서는 동일한 값이라도 서로 다른 객체로 인식되어 검증이 실패할 수 있다. 예를 들어 totalAmount=95000L, originalAmount=100000L, discountAmount=5000L인 경우 정상 주문임에도 예외가 발생할 수 있다.
운영 영향: 쿠폰 적용 주문이 간헐적으로 실패하며, 특정 금액대에서만 발생하여 재현이 어렵다.
🐛 수정 제안
if (memberCouponId != null) {
validateNotNull(originalAmount, "쿠폰 적용 시 원래 금액은 필수입니다.");
validateNotNull(discountAmount, "쿠폰 적용 시 할인 금액은 필수입니다.");
- if (totalAmount != originalAmount - discountAmount) {
+ if (!totalAmount.equals(originalAmount - discountAmount)) {
throw new CoreException(ErrorType.BAD_REQUEST, "주문 총액이 원래 금액에서 할인 금액을 뺀 값과 일치하지 않습니다.");
}
}추가 테스트: Long 캐싱 범위를 벗어나는 금액(예: 150000원)으로 쿠폰 적용 주문 생성 테스트 케이스를 추가해야 한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/commerce-api/src/main/java/com/loopers/domain/order/Order.java` around
lines 83 - 89, The code in Order.java compares Long objects with the reference
operator != (totalAmount != originalAmount - discountAmount), which can fail for
values outside the Long cache; change the comparison to a value-based check
using primitives or equals. Replace that line with a value comparison such as
totalAmount.longValue() != originalAmount.longValue() -
discountAmount.longValue() or
!totalAmount.equals(Long.valueOf(originalAmount.longValue() -
discountAmount.longValue())), keeping the surrounding null checks for
memberCouponId/originalAmount/discountAmount intact.
Comment on lines
71
to
77
| @Transactional | ||
| public int decreaseLikeCount(Long id) { | ||
| Product product = getProduct(id); | ||
| product.decreaseLikeCount(); | ||
| return product.getLikeCount(); | ||
| public void decreaseLikeCount(Long id) { | ||
| int updatedCount = productRepository.decreaseLikeCount(id); | ||
| if (updatedCount == 0) { | ||
| throw new CoreException(ErrorType.NOT_FOUND, "상품을 찾을 수 없습니다. id: " + id); | ||
| } | ||
| } |
There was a problem hiding this comment.
decreaseLikeCount에서 likeCount가 0일 때도 NOT_FOUND가 발생한다.
decreaseLikeCount 쿼리는 likeCount > 0 조건이 있어, 상품이 존재하지만 좋아요 수가 이미 0인 경우에도 updatedCount == 0이 반환된다. 이 경우 실제로는 상품이 존재하므로 NOT_FOUND가 아닌 다른 처리(무시 또는 별도 예외)가 적절할 수 있다. 운영 관점에서 클라이언트가 혼란스러운 에러를 받게 되며, 로그 분석 시 실제 NOT_FOUND와 구분이 어려워진다.
수정 예시 - 상품 존재 여부를 먼저 확인
`@Transactional`
public void decreaseLikeCount(Long id) {
+ // 상품 존재 여부 먼저 확인
+ if (!productReader.findById(id).isPresent()) {
+ throw new CoreException(ErrorType.NOT_FOUND, "상품을 찾을 수 없습니다. id: " + id);
+ }
int updatedCount = productRepository.decreaseLikeCount(id);
- if (updatedCount == 0) {
- throw new CoreException(ErrorType.NOT_FOUND, "상품을 찾을 수 없습니다. id: " + id);
- }
+ // likeCount가 0인 경우는 무시 (정상 케이스)
}또는 비즈니스 요구사항에 따라 likeCount == 0일 때 별도 예외(예: BAD_REQUEST)를 던지는 방안도 고려할 수 있다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.java`
around lines 71 - 77, The decreaseLikeCount method currently throws NOT_FOUND
when productRepository.decreaseLikeCount(id) returns 0, which conflates "product
missing" with "likeCount already zero"; update decreaseLikeCount to first verify
product existence and current likeCount (e.g., use
productRepository.existsById(id) or load Product via findById and check
getLikeCount()) and then: if the product does not exist throw NOT_FOUND, else if
likeCount == 0 either return/ignore the request or throw a BAD_REQUEST (choose
per business rule). Ensure you reference and update the decreaseLikeCount method
and any repository calls (productRepository.decreaseLikeCount,
productRepository.existsById, productRepository.findById) to implement this
logic.
Comment on lines
+113
to
+128
| @ExceptionHandler | ||
| public ResponseEntity<ApiResponse<?>> handleOptimisticLock(OptimisticLockingFailureException e) { | ||
| log.warn("OptimisticLockingFailureException : {}", e.getMessage(), e); | ||
| return failureResponse(ErrorType.CONFLICT, "다른 요청과 충돌이 발생했습니다. 다시 시도해주세요."); | ||
| } | ||
|
|
||
| @ExceptionHandler | ||
| public ResponseEntity<ApiResponse<?>> handlePessimisticLock(PessimisticLockingFailureException e) { | ||
| log.warn("PessimisticLockingFailureException : {}", e.getMessage(), e); | ||
| return failureResponse(ErrorType.CONFLICT, "다른 요청과 충돌이 발생했습니다. 다시 시도해주세요."); | ||
| } | ||
|
|
||
| @ExceptionHandler | ||
| public ResponseEntity<ApiResponse<?>> handleDataIntegrity(DataIntegrityViolationException e) { | ||
| log.warn("DataIntegrityViolationException : {}", e.getMessage(), e); | ||
| return failureResponse(ErrorType.CONFLICT, "이미 처리된 요청입니다. 다시 시도해주세요."); |
There was a problem hiding this comment.
재시도 가능한 락 충돌과 영구적인 무결성 충돌을 같은 CONFLICT로 묶으면 클라이언트가 잘못 대응할 수 있다.
운영 관점에서 현재 매핑은 세 예외 모두 동일한 ErrorType.CONFLICT만 내려보내므로, 클라이언트와 모니터링 시스템이 재시도 가능 여부를 기계적으로 구분할 수 없게 만든다. 여기에 DataIntegrityViolationException까지 "이미 처리된 요청입니다. 다시 시도해주세요."로 고정하면, apps/commerce-api/src/main/java/com/loopers/domain/member/MemberService.java Line 20-29 같은 중복 회원가입 레이스나 FK/NOT NULL 위반도 같은 메시지로 뭉개져 원인별 대응이 어려워진다.
수정안은 락 충돌과 무결성 충돌을 분리하는 것이다. OptimisticLockingFailureException/PessimisticLockingFailureException에는 별도 ErrorType 또는 재시도 가능 여부를 나타내는 코드 값을 주고, DataIntegrityViolationException은 전역에서 일괄 "이미 처리된 요청"으로 처리하지 말고 알려진 중복 제약만 서비스 계층에서 CoreException으로 번역하거나, 최소한 제약 조건명/원인 예외를 보고 duplicate와 그 외 무결성 위반을 분기하는 편이 안전하다.
추가 테스트로는 ApiControllerAdvice 단위 또는 웹 계층 테스트에서 OptimisticLockingFailureException, PessimisticLockingFailureException, duplicate key 기반 DataIntegrityViolationException, non-duplicate 제약 위반을 각각 흘려 보내 상태 코드, 에러 코드, 메시지가 의도대로 구분되는지 검증해야 한다.
Based on learnings, 'enforce unified error handling by routing errors through CoreException to ApiControllerAdvice to ensure a consistent response format' 원칙을 적용했다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.java`
around lines 113 - 128, ApiControllerAdvice currently maps
OptimisticLockingFailureException, PessimisticLockingFailureException and
DataIntegrityViolationException all to ErrorType.CONFLICT which prevents clients
from distinguishing retryable lock conflicts from permanent integrity
violations; update ApiControllerAdvice to: (1) map
OptimisticLockingFailureException and PessimisticLockingFailureException to a
distinct retryable error (e.g., ErrorType.RETRYABLE_CONFLICT or include a
retryable flag in the ApiResponse) and a retry-friendly message, (2) change
DataIntegrityViolationException handling to inspect the root cause/constraint
name and return either a specific duplicate-key ErrorType (or translate known
duplicate constraints to CoreException in the service layer such as
MemberService where duplicate signups occur) or a non-retryable integrity
ErrorType with an appropriate message, and (3) add unit/web tests that throw
each of OptimisticLockingFailureException, PessimisticLockingFailureException,
duplicate-key DataIntegrityViolationException and non-duplicate
DataIntegrityViolationException to assert distinct status/code/message behavior;
reference ApiControllerAdvice, handleOptimisticLock, handlePessimisticLock,
handleDataIntegrity, and MemberService for where to perform translations.
Comment on lines
+176
to
+223
| @DisplayName("브랜드 좋아요 수를 증가할 때, ") | ||
| @Nested | ||
| class IncreaseLikeCount { | ||
|
|
||
| @DisplayName("존재하는 브랜드의 좋아요를 증가하면, 정상 처리된다.") | ||
| @Test | ||
| void succeeds_whenBrandExists() { | ||
| // Arrange | ||
| fakeBrandRepository.save(Brand.create("Nike", "Just Do It")); | ||
|
|
||
| // Act & Assert (예외 없이 정상 수행) | ||
| brandService.increaseLikeCount(1L); | ||
| } | ||
|
|
||
| @DisplayName("존재하지 않는 브랜드의 좋아요를 증가하면, NOT_FOUND 예외가 발생한다.") | ||
| @Test | ||
| void throwsNotFound_whenBrandNotExists() { | ||
| // Act & Assert | ||
| CoreException exception = assertThrows(CoreException.class, () -> { | ||
| brandService.increaseLikeCount(999L); | ||
| }); | ||
| assertThat(exception.getErrorType()).isEqualTo(ErrorType.NOT_FOUND); | ||
| } | ||
| } | ||
|
|
||
| @DisplayName("브랜드 좋아요 수를 감소할 때, ") | ||
| @Nested | ||
| class DecreaseLikeCount { | ||
|
|
||
| @DisplayName("존재하는 브랜드의 좋아요를 감소하면, 정상 처리된다.") | ||
| @Test | ||
| void succeeds_whenBrandExists() { | ||
| // Arrange | ||
| fakeBrandRepository.save(Brand.create("Nike", "Just Do It")); | ||
|
|
||
| // Act & Assert (예외 없이 정상 수행) | ||
| brandService.decreaseLikeCount(1L); | ||
| } | ||
|
|
||
| @DisplayName("존재하지 않는 브랜드의 좋아요를 감소하면, NOT_FOUND 예외가 발생한다.") | ||
| @Test | ||
| void throwsNotFound_whenBrandNotExists() { | ||
| // Act & Assert | ||
| CoreException exception = assertThrows(CoreException.class, () -> { | ||
| brandService.decreaseLikeCount(999L); | ||
| }); | ||
| assertThat(exception.getErrorType()).isEqualTo(ErrorType.NOT_FOUND); | ||
| } |
There was a problem hiding this comment.
좋아요 테스트가 저장소 계약을 잘못 모델링해 회귀를 놓친다
Line 285-291의 FakeBrandRepository는 브랜드가 존재하기만 하면 항상 1을 반환하고 실제 likeCount도 바꾸지 않는다. 그래서 Line 180-213 테스트는 예외 유무만 통과하면 성공 처리되어, 실제 JPA 구현처럼 decreaseLikeCount가 likeCount == 0에서 0건을 반환하는 경우를 전혀 잡지 못한다. 운영에서는 실저장소 경로만 404로 실패해도 단위 테스트는 계속 녹색으로 남는다.
가짜 저장소가 실제 계약을 반영하도록 증가/감소 시 카운트를 변경하고, 감소 시 0 하한 처리까지 동일하게 맞추는 편이 안전하다. 추가로 증가 후 값 검증, 1 -> 0 감소, 0에서 감소의 기대 동작, 없는 ID 케이스를 함께 넣어 회귀를 막아야 한다.
As per coding guidelines "단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다."
Also applies to: 284-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.java`
around lines 176 - 223, The FakeBrandRepository used in BrandServiceTest
incorrectly always returns 1 and doesn't mutate likeCount, so tests
(IncreaseLikeCount/DecreaseLikeCount and methods brandService.increaseLikeCount
/ brandService.decreaseLikeCount) miss real storage behavior and boundary
failures; update FakeBrandRepository to store Brand entities and modify their
likeCount on increase/decrease (enforcing a 0 lower bound on decrease and
returning the actual updated count or 0 when no rows changed), then extend tests
to assert post-operation likeCount for cases: increase succeeds, decrease 1->0,
decrease at 0 stays 0, and existing vs non-existing ID throws NOT_FOUND to match
JPA contract and prevent regressions.
Comment on lines
+140
to
+146
| // Assert — 개인 만료일이 쿠폰 만료일(30일 후)보다 이전(7일 후 근처) | ||
| assertAll( | ||
| () -> assertThat(response.getStatusCode()).isEqualTo(HttpStatus.CREATED), | ||
| () -> assertThat(response.getBody().data().memberCoupon().expiredAt()).isBefore(FUTURE), | ||
| () -> assertThat(response.getBody().data().memberCoupon().expiredAt()) | ||
| .isAfter(LocalDateTime.now().plusDays(6)) | ||
| ); |
There was a problem hiding this comment.
validDays 만료일 검증이 7일 규칙을 실제로 보장하지 못한다.
지금 검증은 FUTURE보다 이르고 현재 시각 + 6일보다 늦기만 하면 통과하므로, 개인 만료일이 8일·20일·29일로 잘못 계산되어도 테스트가 녹색으로 남는다. 이 경우 잘못된 쿠폰 유효기간이 운영에 반영되어도 CI가 막지 못한다. 발급 직전/직후 시각을 캡처한 뒤 expiredAt이 정확히 validDays(=7) 근처의 좁은 구간에 들어오는지 검증하도록 바꾸는 것이 좋다. 추가로 validDays=1 같은 경계값과, 개인 만료일이 전역 expiredAt 검증에 가려지지 않는 회귀 테스트를 함께 두는 편이 안전하다.
예시 수정안
registerMember("user1", "Test1234!");
// Act
+ LocalDateTime issuedBefore = LocalDateTime.now();
ResponseEntity<ApiResponse<CouponV1Dto.MemberCouponResponse>> response = testRestTemplate.exchange(
"/api/v1/coupons/" + couponId + "/issue", HttpMethod.POST,
new HttpEntity<>(authHeaders("user1", "Test1234!")),
new ParameterizedTypeReference<>() {}
);
+ LocalDateTime issuedAfter = LocalDateTime.now();
- // Assert — 개인 만료일이 쿠폰 만료일(30일 후)보다 이전(7일 후 근처)
+ // Assert — 개인 만료일이 발급 시점 기준 7일 후 범위에 있어야 한다.
assertAll(
() -> assertThat(response.getStatusCode()).isEqualTo(HttpStatus.CREATED),
- () -> assertThat(response.getBody().data().memberCoupon().expiredAt()).isBefore(FUTURE),
() -> assertThat(response.getBody().data().memberCoupon().expiredAt())
- .isAfter(LocalDateTime.now().plusDays(6))
+ .isAfterOrEqualTo(issuedBefore.plusDays(7)),
+ () -> assertThat(response.getBody().data().memberCoupon().expiredAt())
+ .isBeforeOrEqualTo(issuedAfter.plusDays(7))
);As per coding guidelines **/*Test*.java: 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/CouponV1ApiE2ETest.java`
around lines 140 - 146, The test in CouponV1ApiE2ETest currently only checks
expiredAt is before FUTURE and after now+6 days, which allows incorrect
validDays; capture timestamps immediately before issuing the coupon (e.g.,
beforeIssue = LocalDateTime.now()) and immediately after receiving the response
(afterIssue = LocalDateTime.now()), then assert
response.getBody().data().memberCoupon().expiredAt() is within a narrow window
around beforeIssue.plusDays(validDays) and afterIssue.plusDays(validDays) (e.g.,
>= beforeIssue.plusDays(validDays) and <= afterIssue.plusDays(validDays)), add
an extra regression test for validDays=1 to cover the boundary, and keep or
remove the global FUTURE assertion so the personal expiredAt assertion is not
masked.
Comment on lines
+69
to
+93
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | ||
| CountDownLatch latch = new CountDownLatch(threadCount); | ||
| AtomicInteger successCount = new AtomicInteger(0); | ||
|
|
||
| // Act — 10명이 동시에 좋아요 | ||
| for (int i = 0; i < threadCount; i++) { | ||
| final int idx = i; | ||
| executor.submit(() -> { | ||
| try { | ||
| ResponseEntity<ApiResponse<Object>> response = testRestTemplate.exchange( | ||
| "/api/v1/products/" + productId + "/likes", HttpMethod.POST, | ||
| new HttpEntity<>(authHeaders(loginIds[idx], "Test1234!")), | ||
| new ParameterizedTypeReference<>() {} | ||
| ); | ||
|
|
||
| if (response.getStatusCode() == HttpStatus.OK) { | ||
| successCount.incrementAndGet(); | ||
| } | ||
| } finally { | ||
| latch.countDown(); | ||
| } | ||
| }); | ||
| } | ||
| latch.await(); | ||
| executor.shutdown(); |
There was a problem hiding this comment.
동시성 하네스가 실제 경합을 충분히 만들지 못하고 CI를 멈출 수 있다.
운영 관점에서 현재 구조는 작업 제출 순서대로 요청이 흘러 실제 경합이 약해질 수 있고, 한 요청이 락이나 네트워크 이슈로 반환되지 않으면 latch.await()가 무기한 대기해 파이프라인이 멈춘다. startGate 또는 CyclicBarrier로 시작 시점을 맞추고, await(timeout)과 shutdownNow()를 finally에서 보장하는 형태로 바꿔야 한다. 추가로 thread 수를 늘린 반복 실행 케이스를 넣어 실제 경합이 걸릴 때도 재현성이 유지되는지 확인해야 한다. As per coding guidelines, '통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다.'
🧪 예시 수정안
ExecutorService executor = Executors.newFixedThreadPool(threadCount);
-CountDownLatch latch = new CountDownLatch(threadCount);
+CountDownLatch startGate = new CountDownLatch(1);
+CountDownLatch doneGate = new CountDownLatch(threadCount);
for (int i = 0; i < threadCount; i++) {
executor.submit(() -> {
try {
+ startGate.await();
ResponseEntity<?> response = testRestTemplate.exchange(
...
);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
} finally {
- latch.countDown();
+ doneGate.countDown();
}
});
}
-latch.await();
-executor.shutdown();
+startGate.countDown();
+assertThat(doneGate.await(10, java.util.concurrent.TimeUnit.SECONDS)).isTrue();
+executor.shutdownNow();Also applies to: 120-146, 179-203
Comment on lines
+122
to
+123
| AtomicInteger successCount = new AtomicInteger(0); | ||
| AtomicInteger failCount = new AtomicInteger(0); |
There was a problem hiding this comment.
토글 동시성 테스트가 5xx 회귀를 정상 시나리오로 통과시킬 수 있다.
운영 관점에서 현재 로직은 HttpStatus.OK가 아닌 모든 응답을 동일한 실패로 집계하므로, 두 요청이 500을 반환해도 최종 likeCount가 0이면 테스트가 통과한다. apps/commerce-api/src/main/java/com/loopers/domain/like/LikeService.java Line 17-29 와 apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java Line 32-47 기준으로 허용 가능한 경쟁 실패를 409 CONFLICT로 볼 것인지, 아니면 둘 다 200 OK로 직렬화할 것인지 계약을 먼저 고정하고, 그 외 상태코드는 별도 카운트 후 0을 단언해야 한다. 추가로 409를 허용한다면 응답 바디의 도메인 에러 코드까지 검증하는 케이스를 넣어 500/401/422가 절대 통과하지 못하게 해야 한다.
🧪 예시 수정안
AtomicInteger successCount = new AtomicInteger(0);
-AtomicInteger failCount = new AtomicInteger(0);
+AtomicInteger conflictCount = new AtomicInteger(0);
+AtomicInteger unexpectedStatusCount = new AtomicInteger(0);
...
if (response.getStatusCode() == HttpStatus.OK) {
successCount.incrementAndGet();
+} else if (response.getStatusCode() == HttpStatus.CONFLICT) {
+ conflictCount.incrementAndGet();
} else {
- failCount.incrementAndGet();
+ unexpectedStatusCount.incrementAndGet();
}
...
-assertThat(successCount.get() + failCount.get()).isEqualTo(threadCount);
+assertThat(unexpectedStatusCount.get()).isZero();
+assertThat(successCount.get() + conflictCount.get()).isEqualTo(threadCount);Also applies to: 135-139, 154-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/LikeConcurrencyE2ETest.java`
around lines 122 - 123, 현재 토글 동시성 테스트가 모든 비-200응답을 실패로 집계해 5xx가 정상 시나리오로 통과될 수
있으니, 테스트에서 허용 가능한 동시성 실패를 명확히 정하고 그 외 상태코드는 별도 집계해 0임을 단언하도록 수정하세요: 테스트에서
AtomicInteger successCount/failCount 대신 각각 okCount/conflictCount/otherCount를 두고,
응답 상태가 HttpStatus.OK이면 okCount++, HttpStatus.CONFLICT(409)이면 응답 바디의 도메인 에러
코드(LikeService/LikeFacade에서 사용하는 도메인 에러 코드)를 검증한 후 conflictCount++로 처리하며, 그
외(500/401/422 등)는 otherCount++로 처리하고 테스트 마지막에 otherCount.get()==0을 assert 하여
5xx/서드파티 에러가 통과하지 않도록 만드세요.
Comment on lines
+546
to
+576
| executor.submit(() -> { | ||
| ready.countDown(); | ||
| try { start.await(); } catch (InterruptedException ignored) {} | ||
| try { | ||
| ResponseEntity<ApiResponse<Object>> response = testRestTemplate.exchange( | ||
| "/api/v1/orders/" + orderId + "/cancel", HttpMethod.POST, | ||
| new HttpEntity<>(headers), | ||
| new ParameterizedTypeReference<>() {} | ||
| ); | ||
| if (response.getStatusCode() == HttpStatus.OK) { | ||
| cancelSuccess.incrementAndGet(); | ||
| } | ||
| } catch (Exception ignored) {} | ||
| }); | ||
|
|
||
| executor.submit(() -> { | ||
| ready.countDown(); | ||
| try { start.await(); } catch (InterruptedException ignored) {} | ||
| try { | ||
| var updateRequest = new OrderV1Dto.UpdateShippingAddressRequest( | ||
| "김철수", "010-9999-9999", "54321", "서울시 서초구", "202호" | ||
| ); | ||
| ResponseEntity<ApiResponse<OrderV1Dto.OrderResponse>> response = testRestTemplate.exchange( | ||
| "/api/v1/orders/" + orderId + "/shipping-address", HttpMethod.PUT, | ||
| new HttpEntity<>(updateRequest, headers), | ||
| new ParameterizedTypeReference<>() {} | ||
| ); | ||
| if (response.getStatusCode() == HttpStatus.OK) { | ||
| updateSuccess.incrementAndGet(); | ||
| } | ||
| } catch (Exception ignored) {} |
There was a problem hiding this comment.
이 테스트는 취소-배송지 수정 경합의 핵심 규칙을 실제로 검증하지 못한다.
두 작업에서 Exception을 무시하고 updateSuccess를 0 또는 1 모두 허용해서, 배송지 수정이 500으로 터지거나 취소 후 배송지가 변경돼도 테스트가 통과한다. 그리고 awaitTermination() 결과도 검사하지 않아 작업이 끝나기 전에 assert로 넘어갈 수 있다. 운영 관점에서는 가장 위험한 취소-수정 경합 회귀를 놓친다. 응답과 예외를 수집해 예상된 상태만 허용하고, 최종 주문의 배송지 값을 원본과 비교해 취소 이후 수정이 반영되지 않았음을 명시적으로 검증하는 편이 안전하다. 추가 테스트로는 cancel-first를 보장한 케이스와 update-first 케이스를 분리해 각각의 응답 코드와 최종 배송지 값을 고정해야 한다.
As per coding guidelines, "**/*Test*.java: 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다. 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."
Also applies to: 579-595
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/OrderConcurrencyE2ETest.java`
around lines 546 - 576, The concurrent cancellation/update test currently
swallows exceptions and allows either outcome; change the two executor.submit
tasks (the cancel lambda and the update lambda that builds
OrderV1Dto.UpdateShippingAddressRequest and calls testRestTemplate.exchange) to
capture and store each ResponseEntity and any thrown Exception instead of
ignoring them, assert the exact expected HTTP status codes (e.g., HttpStatus.OK
vs 4xx/5xx) for each scenario, ensure executor.awaitTermination(...) result is
checked before asserting, and after threads finish fetch the final order via
testRestTemplate.exchange and assert the shipping address on the
OrderV1Dto.OrderResponse equals the original address when cancel should win;
additionally split into two explicit tests (cancel-first and update-first) that
enforce ordering and validate both per-request responses and the final persisted
shipping address, using the existing cancelSuccess/updateSuccess counters only
for metrics not as the sole assertion.
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.
📌 Summary
🧭 Context & Decision
문제 정의
선택지와 결정
1. 쿠폰 발급 수량 동시성 제어
2. 쿠폰 사용 동시성 제어
3. 재고 동시성 제어 + 데드락 방지
4. 좋아요 카운트 동시성 제어
@ModifyingJPQL로 DB에서 직접 증감5. 주문-쿠폰 통합 트랜잭션 경계
6. 기간제 쿠폰 (validDays) 만료일 관리
validDays=7인 쿠폰을 발급하면 발급 시점 기준 7일 후 개인 만료일이 설정된다.🏗️ Design Overview
변경 범위
apps/commerce-api(Coupon, MemberCoupon — 신규, Order — 확장)@Transactional제거,coupon.isExpired()→memberCoupon.isExpired()사용 시점 만료 체크 변경주요 컴포넌트 책임
Domain Layer
Coupon: 쿠폰 템플릿 (정액 FIXED / 정률 RATE), 할인 계산(calculateDiscount), 최소주문금액 검증, 발급 수량 관리(issueOne), 만료 체크, validDays 기반 기간제 쿠폰 지원MemberCoupon: 사용자별 발급 쿠폰, 상태 전이(AVAILABLE → USED, USED → AVAILABLE), 개인별 만료일 관리, 소유권 검증,@Version낙관적 락CouponService: 쿠폰 CRUD, 발급(비관적 락 + 수량 차감 + 개인 만료일 계산), 사용(소유권/만료/최소금액 검증 + 할인 계산), 복원Application Layer
CouponFacade: 쿠폰 발급/조회 유스케이스 오케스트레이션, Member ↔ Coupon 도메인 연결OrderFacade: 주문 생성 시 쿠폰 적용(재고 차감 + 쿠폰 사용 + 주문 생성 원자성), 주문 취소 시 재고 복원 + 쿠폰 복원Infrastructure Layer
CouponJpaRepository:findByIdForUpdate—@Lock(PESSIMISTIC_WRITE)비관적 락ProductJpaRepository:findByIdForUpdate— 재고 차감 시 비관적 락OrderJpaRepository:findByIdForUpdate— 주문 취소 시 비관적 락LikeJpaRepository:incrementLikeCount/decrementLikeCount—@ModifyingAtomic UpdateAPI 엔드포인트
/api-admin/v1/coupons/api-admin/v1/coupons/api-admin/v1/coupons/{id}/api-admin/v1/coupons/{id}/api-admin/v1/coupons/{id}/api-admin/v1/coupons/{id}/issues/api/v1/coupons/{id}/issue/api/v1/users/me/coupons동시성 제어 전략
테스트 현황
🔁 Flow Diagram
쿠폰 적용 주문 생성
sequenceDiagram autonumber participant Client participant Controller participant OrderFacade participant ProductService participant CouponService participant OrderService participant DB Client->>Controller: POST /api/v1/orders (memberCouponId 포함) Controller->>OrderFacade: createOrder(loginId, addressId, items, memberCouponId) rect rgb(230, 245, 255) Note over OrderFacade: @Transactional (All-or-Nothing) loop 상품별 (productId 정렬 순) OrderFacade->>ProductService: getProductForUpdate(productId) Note over DB: SELECT FOR UPDATE (비관적 락) Note over OrderFacade: product.decreaseStock(qty) end OrderFacade->>CouponService: useCoupon(memberCouponId, memberId, orderAmount) Note over CouponService: 소유권 검증 + 만료 체크 + 최소금액 검증 Note over CouponService: 할인 계산 (FIXED: 정액, RATE: 정률) Note over CouponService: mc.use() — @Version 낙관적 락 CouponService-->>OrderFacade: CouponApplyResult(discount) OrderFacade->>OrderService: createOrder(memberId, 배송지, originalAmount, discount, memberCouponId) OrderService->>DB: save Order + OrderItems end OrderFacade-->>Controller: OrderInfo Controller-->>Client: 201 Created선착순 쿠폰 발급
sequenceDiagram autonumber participant Client participant Controller participant CouponFacade participant CouponService participant DB Client->>Controller: POST /api/v1/coupons/{couponId}/issue Controller->>CouponFacade: issueCoupon(loginId, couponId) CouponFacade->>CouponService: issueCoupon(couponId, memberId) CouponService->>DB: SELECT FOR UPDATE (비관적 락) DB-->>CouponService: Coupon (locked) Note over CouponService: 만료 체크 (Coupon.isExpired) Note over CouponService: 중복 발급 체크 (MemberCoupon 조회) Note over CouponService: coupon.issueOne() — 수량 차감 alt validDays != null Note over CouponService: expiredAt = now + validDays else validDays == null Note over CouponService: expiredAt = coupon.expiredAt end CouponService->>DB: save MemberCoupon(memberId, couponId, expiredAt) CouponService-->>CouponFacade: MemberCoupon CouponFacade-->>Controller: MemberCouponInfo Controller-->>Client: 201 Created주문 취소 (재고 + 쿠폰 복원)
sequenceDiagram autonumber participant Client participant Controller participant OrderFacade participant OrderService participant ProductService participant CouponService participant DB Client->>Controller: POST /api/v1/orders/{orderId}/cancel Controller->>OrderFacade: cancelOrder(loginId, orderId) rect rgb(255, 240, 240) Note over OrderFacade: @Transactional (원자성 보장) OrderFacade->>OrderService: cancelOrder(orderId, memberId) Note over DB: SELECT FOR UPDATE (비관적 락) Note over OrderService: 소유권 검증 + order.cancel() OrderService-->>OrderFacade: CancelOrderResult(order, items) loop 각 아이템별 OrderFacade->>ProductService: getProduct(productId) Note over OrderFacade: product.increaseStock(qty) end opt memberCouponId != null OrderFacade->>CouponService: restoreCoupon(memberCouponId) Note over CouponService: mc.restore() — USED → AVAILABLE end end OrderFacade-->>Controller: void Controller-->>Client: 200 OK]
변경 목적
새로운 Coupon 도메인을 추가하고 쿠폰 발급/사용과 재고, 좋아요 등 전체 도메인에 동시성 제어 전략을 통합하여 멀티스레드 환경에서의 데이터 일관성을 보장합니다.
핵심 변경점
Coupon 도메인 신규 추가 (18개 파일, 630라인): Coupon/MemberCoupon 엔티티, CouponService, CouponFacade, 컨트롤러(어드민/사용자), 리포지토리/리더 구현 및 관련 DTO 추가. 동시성 제어 적용: Product/Order/Coupon의 쿠폰 발급에 PESSIMISTIC_WRITE 락(3초 타임아웃), MemberCoupon 사용에 @Version 낙관적 락, 좋아요 카운트에 @Modifying 원자 업데이트 적용. Order 구조 확장: memberCouponId, originalAmount, discountAmount 필드 추가 및 할인 검증 로직 통합. API 개선: Address/Brand/Product의 update() → updateInfo() 메서드 이름 변경(의도 불명확), 브랜드 좋아요 카운트 업데이트 메서드 추가.
리스크 및 주의사항
테스트 및 검증 방법
14개 E2E 테스트(기존) + 3개 동시성 E2E 테스트(신규) 추가, 도메인별 단위 테스트(Coupon, MemberCoupon 포함) 추가로 검증. 동시성 시나리오: 10개 스레드의 동시 재고 감소, 제한 수량 쿠폰 동시 발급, 동일 쿠폰 동시 사용, 멀티 상품 교착 상태 방지, 주문 동시 취소 등 검증. OptimisticLockingFailureException/PessimisticLockingFailureException 예외 처리를 ApiControllerAdvice에서 CONFLICT 응답으로 변환.