[volume 4] - 4주차 과제 김남진#152
[volume 4] - 4주차 과제 김남진#152Namjin-kimm wants to merge 12 commits intoLoopers-dev-lab:Namjin-kimmfrom
Conversation
쿠폰 재시도 전략을 원자적 UPDATE 방식으로 변경함에 따라 불필요해진 spring-retry 의존성 제거
📝 WalkthroughWalkthrough쿠폰 시스템을 신규 구현하고 주문 시스템에 쿠폰 적용 기능을 통합하며, 트랜잭션 경계와 동시성 처리를 강화하고 도메인 엔티티의 책임을 재조정하는 변경사항이다. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OrderFacade
participant ProductService
participant CouponService
participant OrderService
participant DB
User->>OrderFacade: create(userId, items, couponId)
OrderFacade->>ProductService: findAllAndVerifyStock(비관적락)
ProductService->>DB: SELECT FOR UPDATE
DB-->>ProductService: products
ProductService-->>OrderFacade: verified products
OrderFacade->>OrderFacade: originalAmount 계산
alt 쿠폰 적용
OrderFacade->>CouponService: applyCoupon(couponId, userId, originalAmount)
CouponService->>DB: useIfAvailable (원자적 update)
DB-->>CouponService: discount amount
CouponService-->>OrderFacade: discount
else 쿠폰 미적용
OrderFacade->>OrderFacade: discount = 0
end
OrderFacade->>OrderFacade: finalAmount = originalAmount - discount
Note over OrderFacade: dirty-checking으로 재고 차감<br/>(SELECT FOR UPDATE 보유)
OrderFacade->>OrderService: create(userId, items, couponId, originalAmount, discount)
OrderService->>DB: INSERT Order with amounts
DB-->>OrderService: Order
OrderService-->>OrderFacade: Order
OrderFacade-->>User: OrderInfo
sequenceDiagram
participant User
participant CouponFacade
participant CouponService
participant UserCouponRepo
participant DB
User->>CouponFacade: issueCoupon(userId, templateId)
CouponFacade->>CouponService: issue(userId, templateId)
CouponService->>UserCouponRepo: existsByUserIdAndCouponTemplateId(중복확인)
UserCouponRepo->>DB: SELECT ... WHERE deletedAt IS NULL
DB-->>UserCouponRepo: exists?
UserCouponRepo-->>CouponService: boolean
alt 중복 발급 시도
CouponService-->>CouponFacade: CONFLICT 예외
else 새로운 발급
CouponService->>CouponService: template 존재 확인 & expiredAt 스냅샷
CouponService->>UserCouponRepo: save(new UserCoupon(...))
UserCouponRepo->>DB: INSERT user_coupon
DB-->>UserCouponRepo: UserCoupon
UserCouponRepo-->>CouponService: UserCoupon
CouponService-->>CouponFacade: UserCoupon
CouponFacade->>CouponFacade: UserCouponInfo로 변환<br/>(status = resolveStatus)
CouponFacade-->>User: UserCouponInfo
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 사유: 쿠폰 시스템이라는 신규 도메인이 추가되면서 도메인 엔티티(CouponTemplate, UserCoupon), 서비스, 저장소, API 계층에 걸쳐 일관된 설계가 필요하다. 동시에 기존 주문 시스템에 쿠폰 적용 로직이 깊숙이 통합되어 재고 관리 방식이 변경되었고, 트랜잭션 경계와 동시성 처리(원자적 useIfAvailable, saveAndFlush + 예외 변환)가 여러 곳에 산재해 있다. 또한 브랜드/상품 도메인의 책임 재조정(aggregate root 패턴 적용)과 메서드 시그니처 변경이 추가적으로 고려되어야 한다. 테스트 커버리지는 광범위하지만, 주문-쿠폰-재고 상호작용 시나리오와 동시성 상황이 복잡하게 얽혀 있어 검토에 상당한 주의가 필요하다. Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@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: 15
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 (5)
apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceIntegrationTest.java (1)
268-296:⚠️ Potential issue | 🟠 Major
비관적 락계약을 실제로 검증하는 테스트가 필요하다.현재 검증 범위는 "재고 미차감"뿐이라서, 구현에서 락이 빠지거나 lock timeout 설정이 사라져도 이 테스트는 그대로 통과한다. 운영에서는 동일 상품 동시 주문 시 과다판매나 요청 스레드 장시간 점유로 이어질 수 있다. 별도 스레드와 독립 트랜잭션으로 동일 상품을 선점한 뒤, 두 번째
findAllAndVerifyStock호출이 설정된 시간 안에 대기 후 실패하거나 첫 번째 트랜잭션이 끝나기 전까지 진행되지 않음을 단언하는 케이스를 추가하는 편이 안전하다. 추가로 timeout 이후 예외 타입과 재고 불변까지 함께 검증해 달라. Based on learnings,findOptionByIdWithLock()usesentityManager.find(ProductOption.class, optionId, LockModeType.PESSIMISTIC_WRITE)without a lock timeout hint. The call flow is:OrderService.prepareOrderItems()→findProductOnly()(loads only Product, not ProductOption) →deductStock()→findOptionByIdWithLock(). Since ProductOption is never loaded into the 1st-level cache before this point, there is no stale read risk. However, the lock timeout hint (jakarta.persistence.lock.timeout) is missing, risking infinite wait.🤖 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/ProductServiceIntegrationTest.java` around lines 268 - 296, Add an integration test that actually verifies the pessimistic lock behavior: spawn a separate thread/transaction that acquires a PESSIMISTIC_WRITE lock on the same ProductOption (use entityManager.find(ProductOption.class, optionId, LockModeType.PESSIMISTIC_WRITE, Map.of("jakarta.persistence.lock.timeout", 5000)) or equivalent) and holds it briefly, then in the main test call productService.findAllAndVerifyStock(...) and assert that the second call either blocks and then fails with the configured lock timeout exception (and that product stock remains unchanged); additionally, modify findOptionByIdWithLock() to pass a lock timeout hint (jakarta.persistence.lock.timeout) when calling entityManager.find(..., LockModeType.PESSIMISTIC_WRITE) so the DB/ORM enforces a bounded wait rather than infinite blocking.apps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.java (1)
54-74:⚠️ Potential issue | 🟡 Minor
originalAmount,discountAmountnull 검증 테스트가 누락되어 있다.생성자 파라미터가 확장되었으나
originalAmount,discountAmount가 null일 때의 검증 테스트가 없다. 도메인 엔티티의 필수 필드 검증 누락은 런타임 NPE로 이어질 수 있으며, 장애 원인 추적이 어려워진다.추가해야 할 테스트:
`@DisplayName`("originalAmount가 null이면 BAD_REQUEST 에러가 발생한다.") `@Test` void throwsBadRequest_whenOriginalAmountIsNull() { CoreException result = assertThrows(CoreException.class, () -> new Order(USER_ID, List.of(validOrderItem()), null, null, ZERO_DISCOUNT)); assertThat(result.getErrorType()).isEqualTo(ErrorType.BAD_REQUEST); } `@DisplayName`("discountAmount가 null이면 BAD_REQUEST 에러가 발생한다.") `@Test` void throwsBadRequest_whenDiscountAmountIsNull() { CoreException result = assertThrows(CoreException.class, () -> new Order(USER_ID, List.of(validOrderItem()), null, VALID_PRICE, null)); assertThat(result.getErrorType()).isEqualTo(ErrorType.BAD_REQUEST); }🤖 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 54 - 74, Add missing null-validation unit tests for the expanded Order constructor: add two tests in OrderTest that assert a CoreException with ErrorType.BAD_REQUEST is thrown when constructing new Order(...) with originalAmount set to null and when discountAmount is null; use the same pattern as existing tests, calling new Order(USER_ID, List.of(validOrderItem()), null, null, ZERO_DISCOUNT) for the originalAmount case and new Order(USER_ID, List.of(validOrderItem()), null, VALID_PRICE, null) for the discountAmount case, and assert result.getErrorType() equals ErrorType.BAD_REQUEST.apps/commerce-api/src/main/java/com/loopers/application/order/OrderCreateCommand.java (1)
5-13:⚠️ Potential issue | 🟠 Major커맨드 생성 시 방어적 복사와 기본 불변식 검증을 추가해야 한다.
record도List<Item>은 가변 참조이므로 호출자가 원본 리스트를 수정하면 주문 대상이 뒤늦게 변경될 수 있다. 주문 생성은 금액 계산, 쿠폰 검증, 재고 차감으로 이어지므로 이런 입력 변이는 운영에서 재현하기 어려운 불일치를 야기한다. compact constructor에서items = List.copyOf(items)로 방어적 복사를 하고, 빈 목록과userCouponId <= 0같은 기본 불변식도 프로젝트 표준 예외로 즉시 차단해야 한다. 추가로 원본 리스트 수정이 커맨드에 반영되지 않는 테스트와 빈 목록 및 음수 쿠폰 ID가BAD_REQUEST로 거절되는 테스트를 보강해야 한다.🔧 제안 수정
public record OrderCreateCommand( List<Item> items, Long userCouponId // nullable. 쿠폰 미적용 시 null (BR-O09) ) { + public OrderCreateCommand { + if (items == null || items.isEmpty()) { + throw new CoreException(ErrorType.BAD_REQUEST, "주문 상품은 1개 이상이어야 합니다."); + } + items = List.copyOf(items); + + if (userCouponId != null && userCouponId <= 0L) { + throw new CoreException(ErrorType.BAD_REQUEST, "유효하지 않은 쿠폰 식별자입니다."); + } + } public record Item( Long productId, int quantity ) {} }
CoreException과ErrorTypeimport를 추가해야 한다.🤖 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/OrderCreateCommand.java` around lines 5 - 13, OrderCreateCommand's items list must be defensively copied and basic invariants validated in the record's compact constructor: make items = List.copyOf(items) to prevent external mutation, validate items is not empty and that if userCouponId is non-null it is > 0, and throw the project-standard CoreException with the appropriate ErrorType (BAD_REQUEST) on violation; add required imports for CoreException and ErrorType and update tests to assert original list mutation does not affect OrderCreateCommand and that empty items or non-positive coupon IDs produce BAD_REQUEST.apps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceIntegrationTest.java (1)
82-99:⚠️ Potential issue | 🟡 Minor재조회 기준 검증이 없어 JPA 금액 매핑 회귀를 놓친다.
지금 검증은
create()가 반환한 엔티티 값에만 걸려 있어서, 새로 추가된 금액 스냅샷 컬럼 매핑이 깨져도 테스트가 통과한다. 운영에서는 주문 저장 직후 조회 응답과 정산 데이터가 어긋날 수 있다.saved에 대해서도originalAmount,discountAmount,finalAmount를 모두 검증하라. 추가로 이 재조회 검증을 유지해 컬럼명/매핑 변경 회귀를 막아야 한다.변경 예시
Order saved = orderJpaRepository.findById(result.getId()).orElseThrow(); assertThat(saved.getOrderItems()).hasSize(1); assertThat(saved.getOrderItems().get(0).getProductName()).isEqualTo("나이키 에어맥스"); + assertThat(saved.getOriginalAmount().getAmount()).isEqualTo(originalAmount.getAmount()); + assertThat(saved.getDiscountAmount().getAmount()).isZero(); + assertThat(saved.getFinalAmount().getAmount()).isEqualTo(originalAmount.getAmount());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/test/java/com/loopers/domain/order/OrderServiceIntegrationTest.java` around lines 82 - 99, The test currently only asserts amounts on the returned Order from orderService.create(...) so JPA mapping regressions can slip by; update OrderServiceIntegrationTest to also re-query and assert monetary fields on the persisted entity by using orderJpaRepository.findById(result.getId()).orElseThrow() (the saved variable) and add assertions that saved.getOriginalAmount().getAmount(), saved.getDiscountAmount().getAmount(), and saved.getFinalAmount().getAmount() equal the expected values (matching the assertions on result) to prevent column/mapping regressions after persist.apps/commerce-api/src/main/java/com/loopers/interfaces/api/order/OrderV1Dto.java (1)
26-46:⚠️ Potential issue | 🟠 Major금액 필드
int타입 사용으로 인한 오버플로우 위험 개선 필요도메인 레이어에서 금액을 관리하는
Money클래스는 음수 검증만 수행하며, 상한값 검증이 없다.OrderFacade에서 주문 항목들의 금액을int타입으로 합산할 때 오버플로우 가능성이 있다. 또한Order생성자의finalAmount = new Money(originalAmount.getAmount() - discountAmount.getAmount())계산도 int 범위를 초과할 경우 오버플로우가 발생한다.
long타입으로 변경하거나,Money클래스에 최대 금액 상한값(예: 1억 원) 검증을 추가한다.- 경계값 테스트를 추가하여
Integer.MAX_VALUE근처에서의 동작을 검증한다.🤖 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 26 - 46, The OrderResponse record and its from(OrderInfo info) method use int for monetary fields (originalAmount, discountAmount, finalAmount) which risks overflow; change these fields to long in OrderResponse and update the from(OrderInfo info) mapping accordingly so it accepts long values from OrderInfo (or casts safely), and either update the Money domain to enforce a max cap or switch all monetary arithmetic in OrderFacade/Order constructor to use long (or a bounded Money validation); also add boundary unit tests that construct orders near Integer.MAX_VALUE to verify sums and finalAmount calculations do not overflow.
🟡 Minor comments (8)
apps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.java-49-51 (1)
49-51:⚠️ Potential issue | 🟡 Minor할인 적용 시
finalAmount계산 로직 검증 테스트가 누락되어 있다.현재
ZERO_DISCOUNT케이스만 검증하므로finalAmount = originalAmount - discountAmount계산 로직이 실제로 동작하는지 확인할 수 없다. 운영 중 할인 계산 오류 발생 시 고객 클레임과 정산 불일치가 발생할 수 있다.추가 테스트 케이스:
- 할인 적용 시
finalAmount가 정확히 계산되는지 검증discountAmount > originalAmount시 도메인 규칙에 따른 처리 검증`@DisplayName`("할인이 적용된 Order 생성 시 finalAmount가 정확히 계산된다.") `@Test` void createsOrder_whenDiscountApplied() { // arrange Money discountAmount = new Money(5000); Money originalAmount = new Money(20000); // act Order order = new Order(USER_ID, List.of(validOrderItem()), 1L, originalAmount, discountAmount); // assert assertThat(order.getFinalAmount().getAmount()).isEqualTo(15000); }🤖 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 49 - 51, Add tests to verify the finalAmount calculation logic when discounts are applied: create a test that constructs an Order (using the Order constructor) with a non-zero Money discount (e.g., discountAmount = new Money(5000) and originalAmount = new Money(20000)) and assert order.getFinalAmount().getAmount() equals originalAmount - discountAmount; also add a test for the domain rule when discountAmount > originalAmount (using Order and Money) to assert the expected handling (e.g., finalAmount floor to zero or exception per domain rule) so getFinalAmount and discount handling are covered beyond the ZERO_DISCOUNT case.apps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.java-23-23 (1)
23-23:⚠️ Potential issue | 🟡 Minor
VALID_PRICE.getAmount() * VALID_QUANTITY.getValue()연산에서 정수 오버플로우 위험이 있다.현재 테스트 값(10000 * 2 = 20000)은 안전하지만,
int범위(약 21억) 내에서 향후 테스트 데이터 변경 시 오버플로우가 발생할 수 있다.Money클래스에multiply(int quantity)메서드를 추가하거나, 테스트에서 오버플로우 경계 케이스를 명시적으로 검증하는 것이 바람직하다. 장시간 운영되는 e-커머스 시스템에서 대량 주문이나 고가 상품의 경우 이 문제가 발생할 수 있으므로, 안전한 연산 메서드를 도메인 계층에 구현하도록 제안한다.🤖 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` at line 23, Tests risk integer overflow when computing VALID_PRICE.getAmount() * VALID_QUANTITY.getValue(); add a safe multiplication operation on the domain Money class (e.g., Money.multiply(int quantity) or Money.times(int)) that performs the multiplication using a wider type (long/BigDecimal) and returns a Money, and update OrderTest to use Money.multiply(VALID_QUANTITY.getValue()) (or add a specific overflow boundary test) instead of doing the raw int multiply; reference Money, Money.multiply (new), VALID_PRICE and VALID_QUANTITY to locate and replace the vulnerable expression.apps/commerce-api/src/main/java/com/loopers/domain/like/LikeService.java-30-35 (1)
30-35:⚠️ Potential issue | 🟡 Minor예외 cause 보존 및 로깅 누락
DataIntegrityViolationException을CoreException으로 변환할 때 원본 예외가 보존되지 않아 운영 환경에서 장애 원인 추적이 어려워진다. 또한DataIntegrityViolationException은 UNIQUE 제약 위반 외에도 FK 제약, NOT NULL 등 다양한 경우에 발생하므로, 모든 케이스를 CONFLICT로 처리하면 실제 문제를 마스킹할 수 있다.🛠️ 수정 제안
try { return likeRepository.saveAndFlush(new Like(userId, productId)); } catch (DataIntegrityViolationException e) { - // exists 체크를 동시에 통과한 경우 UNIQUE 제약 위반 → CONFLICT 에러로 변환하여 반환 - throw new CoreException(ErrorType.CONFLICT, "이미 좋아요한 상품입니다."); + // exists 체크를 동시에 통과한 경우 UNIQUE 제약 위반 → CONFLICT 에러로 변환 + // cause 보존으로 운영 시 원인 추적 가능 + throw new CoreException(ErrorType.CONFLICT, "이미 좋아요한 상품입니다.", e); }추가 테스트 권장: UNIQUE 제약 위반이 아닌 다른
DataIntegrityViolationException발생 시 의도대로 동작하는지 확인하는 테스트 케이스 추가를 권장한다.As per coding guidelines: "예외 처리 시 cause를 보존하고, 사용자 메시지와 로그 메시지를 분리하도록 제안한다."
🤖 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/like/LikeService.java` around lines 30 - 35, The current catch in LikeService around likeRepository.saveAndFlush swallows the original DataIntegrityViolationException and unconditionally maps it to ErrorType.CONFLICT; preserve the cause and add logging, and only convert to CONFLICT for unique-constraint violations: catch DataIntegrityViolationException e, log the original exception (e.g., logger.error("Failed to save Like for userId={}, productId={}", userId, productId, e)), inspect e.getCause() (e.g., instanceof org.hibernate.exception.ConstraintViolationException or inspect SQLState/errorCode on SQLException) to determine if it is a UNIQUE constraint—if so rethrow new CoreException(ErrorType.CONFLICT, "이미 좋아요한 상품입니다.", e) else rethrow a more appropriate CoreException (e.g., ErrorType.BAD_REQUEST or ErrorType.SERVER_ERROR) preserving e as the cause; update unit tests to cover UNIQUE vs non-unique DataIntegrityViolationException scenarios.apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponTemplateTest.java-70-112 (1)
70-112:⚠️ Potential issue | 🟡 Minor할인·만료 경계값이 비어 있어 회귀를 놓친다.
현재는 RATE 할인에서 나누어떨어지지 않는 금액과
expiredAt == now경계가 빠져 있다. 이 두 케이스는 구현이 조금만 바뀌어도 실제 결제 금액이나 쿠폰 사용 가능 여부가 달라져 운영 이슈로 번진다.9_999같은 비정수 할인 결과가 나오는 금액으로 소수점 처리 정책을 고정하고,expiredAt과 비교 시각이 정확히 같은 경우의 기대값도 명시하라. 두 경계값 케이스를 별도 테스트로 추가하라.As per coding guidelines "단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다."
Also applies to: 152-176
🤖 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/CouponTemplateTest.java` around lines 70 - 112, Add two boundary tests in CouponTemplateTest: one in the CalculateDiscount nested class that uses validRateTemplate (e.g., value 10) on a non-divisible order amount such as 9_999 and asserts the discount follows the project's policy (explicitly assert truncation/floor to 999) so the fractional-case is covered; and one that constructs a CouponTemplate whose expiredAt equals Instant.now() (use the builder/helper you already use to set expiry) and asserts calculateDiscount(now) returns the expected result for the expiry-boundary (explicitly assert coupon is treated as expired and returns 0), referencing CouponTemplate.calculateDiscount and your validRateTemplate/expiry builder helpers so the tests pin the intended behavior.apps/commerce-api/src/test/java/com/loopers/domain/coupon/UserCouponTest.java-23-30 (1)
23-30:⚠️ Potential issue | 🟡 Minor발급 만료일 스냅샷이 검증되지 않는다.
이 PR의 핵심 규칙 중 하나가 발급 시
expiredAt을 고정 저장하는 것인데, 현재 테스트는usedAt만 확인해서 그 회귀를 잡지 못한다. 운영에서는 템플릿 만료일이 변경될 때 이미 발급된 쿠폰의 유효성 판단이 흔들릴 수 있다.getExpiredAt()도 함께 검증하라. 추가로 템플릿 만료일 변경과 무관하게UserCoupon.expiredAt이 유지되는 흐름은 통합 테스트로 한 번 더 고정하라.변경 예시
assertThat(userCoupon.getCouponTemplateId()).isEqualTo(COUPON_TEMPLATE_ID); assertThat(userCoupon.getUserId()).isEqualTo(USER_ID); + assertThat(userCoupon.getExpiredAt()).isEqualTo(FUTURE_EXPIRED_AT); assertThat(userCoupon.getUsedAt()).isNull();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/test/java/com/loopers/domain/coupon/UserCouponTest.java` around lines 23 - 30, The test UserCouponTest.createsCoupon_whenValidParameters() currently only asserts usedAt; add an assertion to verify userCoupon.getExpiredAt() equals FUTURE_EXPIRED_AT to ensure expiredAt is captured at issuance; additionally, add/integrate an integration test that issues a UserCoupon from a CouponTemplate, mutates the template's expiry, reloads the issued UserCoupon, and asserts the persisted UserCoupon.expiredAt still equals the original FUTURE_EXPIRED_AT to guard against template-driven regressions.apps/commerce-api/src/test/java/com/loopers/interfaces/api/order/OrderAdminV1ApiE2ETest.java-83-89 (1)
83-89:⚠️ Potential issue | 🟡 Minor이 헬퍼는 무쿠폰 주문만 만들어 새 응답 필드를 검증하지 못한다.
지금 구성으로는
userCouponId = null,discountAmount = 0인 주문만 저장된다. 운영에서는 관리자 조회 응답의userCouponId,originalAmount,discountAmount,finalAmount직렬화가 깨져도 이 테스트는 계속 통과한다. 쿠폰 적용 주문 fixture를 하나 더 저장하고 목록/상세 응답에서 네 필드를 모두 assert하는 케이스를 추가하는 편이 안전하다. 추가 테스트로discountAmount > 0이고finalAmount = originalAmount - discountAmount인 주문을 검증해야 한다. 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/order/OrderAdminV1ApiE2ETest.java` around lines 83 - 89, 현재 createSavedOrder 헬퍼는 userCouponId=null, discountAmount=0인 무쿠폰 주문만 생성하므로 쿠폰 적용 시리얼라이제이션/계산 로직을 검증하지 못합니다; fix: 추가로 쿠폰이 적용된 주문을 만드는 헬퍼(createSavedOrderWithCoupon 또는 확장된 createSavedOrder)를 구현하여 Order 생성 시 userCouponId(예: someCouponId), discountAmount>0을 설정하고 finalAmount를 originalAmount - discountAmount로 계산해 Order 인스턴스에 넣어 orderJpaRepository.save로 저장한 뒤, 기존 E2E 목록 및 상세 테스트에서 해당 쿠폰 주문의 userCouponId, originalAmount, discountAmount, finalAmount를 모두 assert하도록 추가하세요; 관련 식별자: createSavedOrder, Order, OrderItem, orderJpaRepository, USER_ID.apps/commerce-api/src/main/java/com/loopers/domain/coupon/UserCouponRepository.java-26-28 (1)
26-28:⚠️ Potential issue | 🟡 Minor주석을 업데이트하여 실제 원자적 보호 조건을 명확히 해야 한다.
현재 주석(26-27줄)은 "WHERE used_at IS NULL 조건 사용"만 언급하지만, 실제 SQL(UserCouponJpaRepository 라인 31)에는
deletedAt IS NULL조건도 포함되어 있다. 또한applyCoupon()서비스에서validateNotExpired()로 만료 검증을 선행한다. 주석을 다음과 같이 개선하면 코드 의도가 명확해진다:"원자적 쿠폰 사용 처리. 만료 및 soft delete 쿠폰은 서비스에서 선행 검증하고, UPDATE는
used_at IS NULL AND deleted_at IS NULL조건으로 동시 요청 중 하나만 성공하도록 보장한다."만료 직후와 soft delete 쿠폰이 모두 0건 업데이트로 끝나는지 검증하는 통합 테스트를 추가하면, 선행 검증과 원자적 UPDATE가 함께 작동함을 확인할 수 있다.
🤖 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/UserCouponRepository.java` around lines 26 - 28, Update the comment on the useIfAvailable method to explicitly state that atomic protection uses the UPDATE condition "used_at IS NULL AND deleted_at IS NULL" (the soft-delete check is in UserCouponJpaRepository) and that expiration is validated earlier in applyCoupon via validateNotExpired; also add an integration test that ensures attempting to apply an expired coupon or a soft-deleted coupon results in 0 rows updated to verify the pre-validation plus atomic UPDATE behavior works together.apps/commerce-api/src/main/java/com/loopers/application/coupon/UserCouponInfo.java-33-41 (1)
33-41:⚠️ Potential issue | 🟡 Minor쿠폰 상태 결정 로직의 경계 조건에 대한 테스트 추가 필요
now.isAfter(expiredAt)조건으로 만료 여부를 판단하면, 만료 시각과 정확히 같은 시점에서AVAILABLE상태를 반환한다. 운영 중에는 쿠폰 조회 시점과 실제 결제 처리 시점 사이에 시간 차이가 발생하여, 조회 시AVAILABLE이었던 쿠폰이 결제 직전에 만료될 수 있다. 경계 시점의 동작이 비즈니스 요구사항과 일치하는지 명확히 검증해야 한다.
resolveStatus메서드에 대한 단위 테스트를 추가하여 다음 케이스들을 검증한다:
usedAt이 존재하면USED반환- 현재 시각이 만료 시각보다 뒤면
EXPIRED반환- 현재 시각이 만료 시각과 같으면
AVAILABLE반환 (경계 조건)- 현재 시각이 만료 시각보다 앞으면
AVAILABLE반환🤖 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/UserCouponInfo.java` around lines 33 - 41, The resolveStatus logic in UserCouponInfo currently uses now.isAfter(expiredAt) which treats now == expiredAt as AVAILABLE; add unit tests for the resolveStatus(UserCoupon userCoupon, LocalDateTime expiredAt, LocalDateTime now) method to explicitly verify behavior: (1) when userCoupon.getUsedAt() != null return CouponStatus.USED, (2) when now.isAfter(expiredAt) return CouponStatus.EXPIRED, (3) when now.isEqual(expiredAt) return CouponStatus.AVAILABLE (boundary), and (4) when now.isBefore(expiredAt) return CouponStatus.AVAILABLE; locate tests around UserCouponInfo.resolveStatus and assert against CouponStatus enum values to lock in the intended boundary semantics.
🧹 Nitpick comments (14)
apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceIntegrationTest.java (1)
314-329: 실패 경로에서도 재고 불변을 함께 단언하는 편이 안전하다.현재
throwsBadRequest_whenStockIsInsufficient()는 예외 타입만 확인하므로, 검증 도중 재고가 잘못 변경되어도 놓친다. 운영에서는 주문이 실패했는데 재고만 줄어드는 형태로 나타나 장애 분석이 어려워진다. 예외 발생 후 상품을 다시 조회해 수량이 기존 값1그대로인지 확인하고, 가능하면 다른 정상 상품이 함께 있을 때도 어떤 상품도 변경되지 않는 케이스를 추가해 달라. 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/domain/product/ProductServiceIntegrationTest.java` around lines 314 - 329, Update the test throwsBadRequest_whenStockIsInsufficient to also assert the product's Stock is unchanged after the exception: after calling productService.findAllAndVerifyStock and catching CoreException, reload the Product via productJpaRepository.findById(product.getId()) and assert its Stock quantity equals the original value (1); additionally, add a second saved Product and include it in a combined quantityMap to assert no product's stock changed on failure (reload both via productJpaRepository and assert their Stock quantities remain unchanged). This uses the existing test method, Product, Stock, productJpaRepository, and productService.findAllAndVerifyStock to locate where to add the post-exception verification.apps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.java (1)
31-34: 할인 시나리오 테스트를 위한 헬퍼 메서드 오버로드가 필요하다.현재
validOrder(Long userId)는 항상ZERO_DISCOUNT를 사용하므로 할인 적용 시나리오를 테스트할 수 없다. 운영 환경에서 쿠폰 할인 계산 오류는 매출 손실로 직결되므로, 다양한 할인 금액에 대한 테스트가 필수다.제안: 할인 금액을 파라미터로 받는 오버로드 추가
private Order validOrder(Long userId) { Money originalAmount = new Money(VALID_PRICE.getAmount() * VALID_QUANTITY.getValue()); return new Order(userId, List.of(validOrderItem()), null, originalAmount, ZERO_DISCOUNT); } + +private Order validOrderWithDiscount(Long userId, Money discountAmount) { + Money originalAmount = new Money(VALID_PRICE.getAmount() * VALID_QUANTITY.getValue()); + return new Order(userId, List.of(validOrderItem()), 1L, originalAmount, discountAmount); +}🤖 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 31 - 34, The helper validOrder(Long userId) always uses ZERO_DISCOUNT so discount scenarios can't be tested; add an overloaded helper validOrder(Long userId, Money discount) that constructs and returns a new Order(userId, List.of(validOrderItem()), null, originalAmount, discount) (compute originalAmount same way as existing validOrder), and update tests to call the new overload when different discounts need to be exercised; keep the existing validOrder(Long userId) as a convenience delegating to the new overload with ZERO_DISCOUNT to preserve current tests.apps/commerce-api/src/test/java/com/loopers/domain/like/LikeConcurrencyTest.java (3)
66-67: 테스트 데이터 생성 시 likeCount 초기값 확인 필요
DifferentUsersConcurrentLike테스트에서 likeCount가 THREAD_COUNT와 일치하는지 검증하는데, Product 생성 시 likeCount 초기값이 0임을 명시적으로 확인하면 테스트 의도가 명확해진다.// arrange 단계에서 초기값 검증 추가 assertThat(product.getLikeCount()).isEqualTo(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/like/LikeConcurrencyTest.java` around lines 66 - 67, In the DifferentUsersConcurrentLike test, make the initial likeCount explicit: after creating/saving the Product via productJpaRepository.save (the local variable product in LikeConcurrencyTest), add an assertion that product.getLikeCount() is 0 to document the starting state before the concurrent like operations; this clarifies the test intent and ensures the subsequent assertion comparing to THREAD_COUNT is valid.
123-127: DifferentUsersConcurrentLike 테스트 실패 감지 누락서로 다른 사용자의 동시 좋아요는 모두 성공해야 하므로, 예외 발생 시 명시적으로 실패를 기록해야 한다. 현재 구현은 예외가 발생해도 테스트가 통과할 수 있어 실제 문제를 놓칠 수 있다.
🧪 수정 제안
+ private final AtomicInteger failureCount = new AtomicInteger(0); + private final List<Throwable> failures = Collections.synchronizedList(new ArrayList<>()); executor.submit(() -> { try { startLatch.await(); likeFacade.create(userId, product.getId()); successCount.incrementAndGet(); } catch (Exception e) { - // 예상치 못한 실패 + failureCount.incrementAndGet(); + failures.add(e); } finally { doneLatch.countDown(); } });assert 부분에 추가:
assertThat(failureCount.get()) .withFailMessage("Unexpected failures: %s", failures) .isEqualTo(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/like/LikeConcurrencyTest.java` around lines 123 - 127, The catch block inside the DifferentUsersConcurrentLike test currently swallows exceptions so failures are not detected; update the catch to record errors by incrementing the AtomicInteger failureCount and appending the exception or message to the failures collection (used in the test), and keep the doneLatch.countDown() in finally; after waiting for threads (after await on doneLatch) add an assertion like assertThat(failureCount.get()).withFailMessage("Unexpected failures: %s", failures).isEqualTo(0) to explicitly fail the test if any thread threw an exception; refer to failureCount, failures, doneLatch and the catch/finally in LikeConcurrencyTest.
81-85: 예외 무시로 인한 테스트 신뢰성 저하 가능성catch 블록에서 모든 예외를 무시하면 DB 연결 실패, 트랜잭션 타임아웃 등 예상치 못한 예외도 정상 케이스로 처리되어 테스트 결과가 왜곡될 수 있다. CONFLICT 예외만 예상된 실패로 처리하고, 나머지는 실패로 기록하여 테스트 신뢰성을 높이는 것을 권장한다.
🧪 수정 제안
+ private AtomicInteger conflictCount = new AtomicInteger(0); + private AtomicInteger unexpectedFailureCount = new AtomicInteger(0); // act for (int i = 0; i < THREAD_COUNT; i++) { executor.submit(() -> { try { startLatch.await(); likeService.create(USER_ID, product.getId()); successCount.incrementAndGet(); - } catch (Exception e) { - // 예상된 실패 (TOCTOU 경쟁 또는 exists 체크에서 CONFLICT) + } catch (CoreException e) { + if (e.getErrorType() == ErrorType.CONFLICT) { + conflictCount.incrementAndGet(); + } else { + unexpectedFailureCount.incrementAndGet(); + } + } catch (Exception e) { + unexpectedFailureCount.incrementAndGet(); } finally { doneLatch.countDown(); } }); }assert 부분에 추가:
assertThat(unexpectedFailureCount.get()).isEqualTo(0); assertThat(successCount.get() + conflictCount.get()).isEqualTo(THREAD_COUNT);🤖 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/like/LikeConcurrencyTest.java` around lines 81 - 85, The current catch block in the concurrent test swallows all exceptions; update the catch in the block surrounding doneLatch.countDown() to only treat conflict/TOCTOU (e.g., the specific exception type or message that indicates CONFLICT) as an expected failure by incrementing conflictCount, and for any other exception increment an unexpectedFailureCount (or rethrow/wrap) so unexpected DB/timeout errors are surfaced; then add the suggested assertions after the threads complete: assertThat(unexpectedFailureCount.get()).isEqualTo(0) and assertThat(successCount.get() + conflictCount.get()).isEqualTo(THREAD_COUNT) to enforce test reliability.apps/commerce-api/src/main/java/com/loopers/infrastructure/like/LikeJpaRepository.java (1)
20-22: @Modifying 쿼리 후 영속성 컨텍스트 정리 설정 필요
@ModifyingJPQL 쿼리는 데이터베이스에서 직접 DELETE를 실행하지만, 영속성 컨텍스트의 엔티티를 자동으로 제거하지 않는다. 테스트 코드(LikeServiceIntegrationTest 라인 187-188)에서deleteAllByProductId()직후existsByUserIdAndProductId()로 조회하는 패턴이 있으며,clearAutomatically속성이 없으면 같은 트랜잭션 내에서 stale 엔티티가 반환될 수 있다.현재 ProductAdminFacade, BrandAdminFacade의 운영 흐름상 삭제 후 재조회가 없으므로 즉시 문제는 아니나, 향후 유지보수 시 의도하지 않은 데이터 불일치를 초래할 위험이 있다.
clearAutomatically 속성 추가 제안
- `@Modifying` + `@Modifying`(clearAutomatically = true) `@Query`("DELETE FROM Like l WHERE l.productId = :productId") void deleteAllByProductId(`@Param`("productId") Long productId); - `@Modifying` + `@Modifying`(clearAutomatically = true) `@Query`("DELETE FROM Like l WHERE l.productId IN :productIds") void deleteAllByProductIdIn(`@Param`("productIds") Collection<Long> productIds);두 메서드 모두에
clearAutomatically = true를 설정하여 영속성 컨텍스트 정합성을 보장하기 바란다.🤖 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/like/LikeJpaRepository.java` around lines 20 - 22, The `@Modifying` JPQL delete in LikeJpaRepository (method deleteAllByProductId) must include clearAutomatically=true to avoid returning stale entities from the persistence context after the bulk delete; update the `@Modifying` annotation on deleteAllByProductId (and any other `@Modifying` delete methods in LikeJpaRepository) to set clearAutomatically = true so the persistence context is cleared automatically after the DELETE and subsequent existsByUserIdAndProductId() or other reads see the correct state.apps/commerce-api/src/main/java/com/loopers/application/product/ProductAdminFacade.java (1)
44-53: brandNameMap에서 null 반환 가능성 검토 필요.운영 관점에서 상품이 참조하는 브랜드가 soft-delete 되었거나 데이터 정합성 문제로 brandService.findNamesByIds()에서 누락될 경우, brandNameMap.get(product.getBrandId())가 null을 반환한다. ProductInfo.from()에 null brandName이 전달되면 NPE 또는 사용자에게 불완전한 데이터가 노출될 수 있다.
수정안: brandNameMap.getOrDefault(product.getBrandId(), "알 수 없음") 또는 null 체크 후 명시적 처리를 권장한다.
추가 테스트: 삭제된 브랜드를 참조하는 상품 조회 시나리오에 대한 테스트 추가를 권장한다.
♻️ null 방어 코드 제안
// 기존의 상품 List 각각의 product에 해당하는 브랜드명 매핑 - return products.map(product -> ProductInfo.from(product, brandNameMap.get(product.getBrandId()))); + return products.map(product -> ProductInfo.from(product, brandNameMap.getOrDefault(product.getBrandId(), "알 수 없음")));🤖 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/product/ProductAdminFacade.java` around lines 44 - 53, In ProductAdminFacade.findAll the brandNameMap returned by brandService.findNamesByIds(…) can miss entries (soft-deleted or inconsistent data) causing brandNameMap.get(product.getBrandId()) to be null; update the mapping to supply a safe default (e.g., use brandNameMap.getOrDefault(product.getBrandId(), "알 수 없음") or explicitly map null to a placeholder before calling ProductInfo.from) and add a unit/integration test for the case where a product references a deleted/missing brand to assert the placeholder is used and no NPE occurs.apps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.java (1)
20-22: 시간 기준을 주입해 만료 판정 기준을 고정하는 편이 안전하다.이 클래스는
LocalDateTime.now()를 직접 읽고 있어서 배포 환경의 timezone이나 clock skew에 따라 같은 쿠폰의 상태가 달라질 수 있다. 운영에서는 만료 경계 시점에 노드별 응답이 갈리면 CS 이슈로 바로 이어진다.Clock또는 공용 TimeProvider를 주입해 발급 응답과 목록 조회가 같은 기준 시각을 사용하게 하고, 추가 테스트로 고정 시계에서expiredAt경계값 상태가 일관적인지 검증하는 편이 안전하다.Also applies to: 31-35
🤖 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/CouponFacade.java` around lines 20 - 22, The issue: CouponFacade.issue(...) calls LocalDateTime.now() directly which makes expiration checks non-deterministic; change CouponFacade to accept an injected Clock or TimeProvider (e.g., via constructor or `@Autowired`) and use that to obtain the current time in issue and any other methods (also update the similar code in the 31-35 region). Replace LocalDateTime.now() with LocalDateTime.now(clock) or timeProvider.now() inside UserCouponInfo.from(...) call sites, update the CouponFacade constructor/signature to store the clock/timeProvider, and add unit tests using a fixed Clock to assert consistent expiredAt boundary behavior.apps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponV1Dto.java (2)
37-44: import 구문 정리
java.util.List가 inline으로 사용되었다. 상단에 import 구문을 추가하여 일관성을 유지하는 것이 좋다.♻️ 수정 제안
import com.loopers.application.coupon.UserCouponInfo; import com.loopers.domain.coupon.CouponStatus; import java.time.LocalDateTime; import java.time.ZonedDateTime; +import java.util.List;public record MyCouponListResponse( - java.util.List<UserCouponResponse> coupons + List<UserCouponResponse> coupons ) { - public static MyCouponListResponse from(java.util.List<UserCouponInfo> infos) { + public static MyCouponListResponse from(List<UserCouponInfo> infos) {🤖 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 37 - 44, The inline usage of java.util.List in the MyCouponListResponse record should be replaced with a top-level import for List to keep imports consistent; add "import java.util.List;" to the class imports and update the record declaration and the static factory method signatures/usages (MyCouponListResponse, UserCouponResponse::from, and parameter type UserCouponInfo list in from) to refer to List instead of java.util.List so all list types are consistent with the rest of the file.
14-31: 시간 타입 불일치 확인 필요
issuedAt은ZonedDateTime이고,expiredAt/usedAt은LocalDateTime이다. 운영 관점에서 타임존 처리가 일관되지 않으면 만료 시점 판단 시 오류가 발생할 수 있다. 특히 서버 타임존과 클라이언트 타임존이 다른 경우 쿠폰 만료 여부가 잘못 계산될 수 있다.도메인 레이어에서 시간 타입을 통일하거나, API 응답 시 명시적으로 타임존을 포함하여 반환하는 것을 권장한다. 타임존 관련 엣지 케이스(예: DST 전환, 다국적 사용자)에 대한 테스트를 추가하는 것이 운영 안정성을 높인다.
🤖 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 14 - 31, UserCouponResponse has inconsistent time types (issuedAt: ZonedDateTime vs expiredAt/usedAt: LocalDateTime) which can cause TZ bugs; update the DTO and mapping in UserCouponResponse.from(UserCouponInfo) to use a single consistent temporal type (prefer ZonedDateTime or Instant with zone normalization) for all three fields, or convert expiredAt/usedAt from LocalDateTime to ZonedDateTime using a specified ZoneId when constructing the record; ensure the chosen zone is applied consistently and adjust the record field types and mapping logic in from(UserCouponInfo) accordingly.apps/commerce-api/src/test/java/com/loopers/interfaces/api/coupon/CouponAdminV1ApiE2ETest.java (1)
46-46: 테스트 시간값의 플레이키 가능성
FUTURE_EXPIRED_AT가 static 필드로LocalDateTime.now().plusDays(30)을 사용한다. 테스트 클래스 로드 시점에 값이 고정되므로, 장시간 실행되는 테스트 스위트나 CI 환경에서 예상치 못한 시간 관련 실패가 발생할 수 있다.테스트 메서드 내부에서 시간값을 생성하거나, 테스트용 고정 시간을 사용하는 것을 권장한다.
♻️ 수정 제안
- private static final LocalDateTime FUTURE_EXPIRED_AT = LocalDateTime.now().plusDays(30); + // 테스트 메서드 내에서 LocalDateTime.now().plusDays(30) 호출 권장 + // 또는 고정된 미래 시간 사용 + private static final LocalDateTime FUTURE_EXPIRED_AT = LocalDateTime.of(2099, 12, 31, 23, 59, 59);🤖 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/coupon/CouponAdminV1ApiE2ETest.java` at line 46, FUTURE_EXPIRED_AT is a static field computed once with LocalDateTime.now().plusDays(30) which can cause flaky time-dependent tests; change the code so the timestamp is created inside each test (or use a test fixed clock) instead of the static constant in CouponAdminV1ApiE2ETest—replace references to FUTURE_EXPIRED_AT with a locally computed LocalDateTime.now().plusDays(30) inside test methods or inject a deterministic Clock for generating the expiry time.apps/commerce-api/src/test/java/com/loopers/domain/order/OrderFacadeIntegrationTest.java (1)
41-42: 테스트 시간값 플레이키 가능성
FUTURE_EXPIRED_AT와PAST_EXPIRED_AT가LocalDateTime.now()기반으로 static 필드에 정의되어 있다. 앞서CouponAdminV1ApiE2ETest와 동일한 문제로, 장시간 실행되는 CI 환경에서 테스트 실패 가능성이 있다.특히
PAST_EXPIRED_AT = LocalDateTime.now().minusDays(1)는 테스트 실행 시점에 따라 만료 여부 판단이 달라질 수 있다.♻️ 수정 제안
- private static final LocalDateTime FUTURE_EXPIRED_AT = LocalDateTime.now().plusDays(30); - private static final LocalDateTime PAST_EXPIRED_AT = LocalDateTime.now().minusDays(1); + private static final LocalDateTime FUTURE_EXPIRED_AT = LocalDateTime.of(2099, 12, 31, 23, 59, 59); + private static final LocalDateTime PAST_EXPIRED_AT = LocalDateTime.of(2000, 1, 1, 0, 0, 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/order/OrderFacadeIntegrationTest.java` around lines 41 - 42, The static test timestamps FUTURE_EXPIRED_AT and PAST_EXPIRED_AT in OrderFacadeIntegrationTest are computed from LocalDateTime.now() making tests flaky in long-running CI; change them to deterministic values by either (a) replacing the static fields with fixed constants (e.g. a specific ISO LocalDateTime string parsed to LocalDateTime) or (b) inject/override a Clock used by the code under test and compute FUTURE_EXPIRED_AT/PAST_EXPIRED_AT from a fixed Clock in a `@BeforeEach/`@BeforeAll setup; update references to FUTURE_EXPIRED_AT and PAST_EXPIRED_AT to use the new deterministic sources so expiration checks are stable across runs.apps/commerce-api/src/test/java/com/loopers/interfaces/api/coupon/CouponV1ApiE2ETest.java (1)
87-92: 내 쿠폰 조회 E2E가 사용자 격리를 검증하지 못한다.현재는 자격증명 helper가 단일 사용자에 고정되어 있고, positive case도 쿠폰 1건만 만들어서 구현이 인증 사용자 필터를 빠뜨려도 테스트가 통과한다. 운영에서는 다른 회원 쿠폰이 섞여 내려오는 권한/개인정보 회귀를 놓칠 수 있으므로, 두 번째 사용자의 쿠폰도 함께 만든 뒤 응답이 현재 사용자 쿠폰만 포함하는지까지 단언하도록 바꾸는 편이 안전하다. 헤더 helper도 사용자별 자격증명을 받을 수 있게 바꾸고, 각 사용자가 서로 다른 목록을 받는 케이스를 추가해야 한다. As per coding guidelines,
**/*Test*.java: 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다.Also applies to: 204-226
🤖 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/coupon/CouponV1ApiE2ETest.java` around lines 87 - 92, The createUserHeaders helper currently returns a fixed single user's credentials causing the E2E "my coupons" test to miss user-isolation bugs; modify the helper (createUserHeaders) to accept loginId and password parameters and return headers for those credentials, then update the E2E test(s) that create/list coupons (the test block around the previous single-coupon positive case and the tests referenced in the 204-226 region) to create coupons for two distinct users, invoke the list-my-coupons endpoint for each user using createUserHeaders with their respective credentials, and add assertions that each response contains only that user's coupons (and does not include the other user's coupon ids) to ensure proper authentication-based isolation.apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponFacadeIntegrationTest.java (1)
216-233: soft delete 테스트가 실제 soft delete 여부를 보장하지 못한다.지금은 active count가 0인지밖에 보지 않아 구현이 hard delete로 바뀌어도 테스트가 통과한다. 운영에서는 발급 이력과 감사 추적 손실을 놓칠 수 있으므로, 삭제 전
UserCouponid를 저장해 두고 soft-deleted row를 조회할 수 있는 경로로deletedAt이 채워졌는지까지 검증하는 편이 안전하다. 여러 발급 건이 모두 soft delete 되는 케이스도 함께 추가해야 한다. 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/domain/coupon/CouponFacadeIntegrationTest.java` around lines 216 - 233, Update the test softDeletesTemplateAndRelatedIssues_whenDeleted to assert soft-delete semantics: before calling couponAdminFacade.delete(template.getId()) save and capture the id(s) of the created UserCoupon entities (use userCouponJpaRepository.save(...) and keep their getId()), create multiple UserCoupon rows to cover multi-issue case, then after delete verify the CouponTemplate deletedAt is set and for each captured UserCoupon id load the row via userCouponJpaRepository.findById(id).orElseThrow() and assert its getDeletedAt() is not null (instead of only checking active count); keep references to CouponTemplate, UserCoupon, couponAdminFacade.delete, userCouponJpaRepository and couponTemplateJpaRepository so the changes are easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfe8b3d3-d6f7-4130-8399-bc4cb77a4be3
⛔ Files ignored due to path filters (5)
.claude/skills/anaylize-query/SKILL.mdis excluded by!**/*.mdand included by**.docs/design/01-requirements.mdis excluded by!**/*.mdand included by**.docs/design/02-sequence-diagrams.mdis excluded by!**/*.mdand included by**.docs/design/03-class-diagrams.mdis excluded by!**/*.mdand included by**.docs/design/04-erd.mdis excluded by!**/*.mdand included by**
📒 Files selected for processing (56)
apps/commerce-api/build.gradle.ktsapps/commerce-api/src/main/java/com/loopers/application/brand/BrandAdminFacade.javaapps/commerce-api/src/main/java/com/loopers/application/brand/BrandFacade.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/CouponAdminFacade.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/CouponTemplateInfo.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/CouponTemplateRegisterCommand.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/CouponTemplateUpdateCommand.javaapps/commerce-api/src/main/java/com/loopers/application/coupon/UserCouponInfo.javaapps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.javaapps/commerce-api/src/main/java/com/loopers/application/order/OrderCreateCommand.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/ProductAdminFacade.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/CouponService.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponStatus.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponTemplate.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponTemplateRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponType.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/UserCoupon.javaapps/commerce-api/src/main/java/com/loopers/domain/coupon/UserCouponRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/like/LikeRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/like/LikeService.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/coupon/CouponTemplateJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponTemplateRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/UserCouponJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/UserCouponRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/like/LikeJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/like/LikeRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponAdminV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponAdminV1Dto.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/test/java/com/loopers/domain/brand/BrandServiceIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponConcurrencyTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponFacadeIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponTemplateTest.javaapps/commerce-api/src/test/java/com/loopers/domain/coupon/UserCouponTest.javaapps/commerce-api/src/test/java/com/loopers/domain/like/LikeConcurrencyTest.javaapps/commerce-api/src/test/java/com/loopers/domain/order/OrderConcurrencyTest.javaapps/commerce-api/src/test/java/com/loopers/domain/order/OrderFacadeIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/domain/order/OrderServiceIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/domain/order/OrderTest.javaapps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/coupon/CouponAdminV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/coupon/CouponV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/order/OrderAdminV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/order/OrderV1ApiE2ETest.java
💤 Files with no reviewable changes (2)
- apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.java
- apps/commerce-api/src/test/java/com/loopers/domain/brand/BrandServiceIntegrationTest.java
| Map<Long, Quantity> quantityByProductId = command.items().stream() | ||
| .collect(Collectors.toMap( | ||
| OrderCreateCommand.Item::productId, | ||
| item -> new Quantity(item.quantity()) | ||
| )); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 파일 위치 확인 및 코드 내용 조회
find . -name "OrderFacade.java" -type fRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 165
🏁 Script executed:
# 파일 내용 확인 (54-70 라인)
cat -n apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java | head -80 | tail -30Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1741
🏁 Script executed:
# CoreException 정의 확인
find . -name "CoreException.java" -type fRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 163
🏁 Script executed:
# ErrorType 정의 확인
find . -name "ErrorType.java" -type fRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 159
🏁 Script executed:
# CoreException 클래스 정의 확인
cat -n ./apps/commerce-api/src/main/java/com/loopers/support/error/CoreException.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 741
🏁 Script executed:
# ErrorType 정의 확인
cat -n ./apps/commerce-api/src/main/java/com/loopers/support/error/ErrorType.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1057
🏁 Script executed:
# OrderCreateCommand 구조 확인 (items 메서드, Item 클래스)
find . -name "OrderCreateCommand.java" -type fRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 172
🏁 Script executed:
# OrderCreateCommand 내용 확인
cat -n ./apps/commerce-api/src/main/java/com/loopers/application/order/OrderCreateCommand.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 462
🏁 Script executed:
# ApiControllerAdvice 확인하여 CoreException 처리 확인
find . -name "ApiControllerAdvice.java" -o -name "*ControllerAdvice.java" | head -5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 170
🏁 Script executed:
# ApiControllerAdvice 내용 확인
cat -n ./apps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 6820
중복 상품 ID 요청이 500으로 응답된다.
Collectors.toMap은 동일 productId가 들어오면 IllegalStateException을 던진다. 현재 ApiControllerAdvice의 일반 예외 처리기(111-114행)에서 이를 INTERNAL_ERROR(500)으로 반환한다. 운영에서는 잘못된 주문 입력이 일관된 400이 아니라 간헐적 500으로 보이며, 프로젝트의 CoreException 통합 에러 처리 패턴을 활용하지 못한다. merge function으로 CoreException을 명시적으로 던지면 ApiControllerAdvice의 CoreException 전용 처리기에서 400 BAD_REQUEST로 정확하게 응답한다.
수정안
Map<Long, Quantity> quantityByProductId = command.items().stream()
.collect(Collectors.toMap(
OrderCreateCommand.Item::productId,
- item -> new Quantity(item.quantity())
+ item -> new Quantity(item.quantity()),
+ (existing, duplicate) -> {
+ throw new CoreException(ErrorType.BAD_REQUEST, "동일 상품은 한 번만 주문할 수 있습니다.");
+ }
));동일 productId가 두 번 들어온 주문 요청이 400 응답을 반환하는지 테스트로 검증해야 한다.
📝 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.
| Map<Long, Quantity> quantityByProductId = command.items().stream() | |
| .collect(Collectors.toMap( | |
| OrderCreateCommand.Item::productId, | |
| item -> new Quantity(item.quantity()) | |
| )); | |
| Map<Long, Quantity> quantityByProductId = command.items().stream() | |
| .collect(Collectors.toMap( | |
| OrderCreateCommand.Item::productId, | |
| item -> new Quantity(item.quantity()), | |
| (existing, duplicate) -> { | |
| throw new CoreException(ErrorType.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/application/order/OrderFacade.java`
around lines 54 - 58, The current use of Collectors.toMap when building
quantityByProductId will throw IllegalStateException on duplicate
OrderCreateCommand.Item.productId, causing a 500; change the collector to supply
a merge function that throws your domain CoreException (e.g., new
CoreException(BAD_REQUEST, "duplicate productId")) so the ApiControllerAdvice
CoreException handler returns 400, and update the construction of
quantityByProductId (the stream -> collect call) to use that merge behavior;
also add a unit/integration test that sends an OrderCreateCommand with the same
productId twice and asserts the API returns 400 BAD_REQUEST.
| // ④ 재고 차감 (dirty checking, 비관적 락 범위 내) | ||
| // products List는 IN 절 쿼리를 사용했으므로 Mysql이 PK를 오름차순으로 정렬해줌 -> 데드락 방지 | ||
| for (Product product : products) { | ||
| Quantity quantity = quantityByProductId.get(product.getId()); | ||
| product.decreaseStock(quantity); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "OrderFacade.java" -type fRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 165
🏁 Script executed:
git ls-files | grep -E "(OrderFacade|Product|Quantity)" | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1814
🏁 Script executed:
wc -l ./apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 169
🏁 Script executed:
sed -n '60,100p' ./apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2006
🏁 Script executed:
cat -n ./apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java | head -120Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 5456
🏁 Script executed:
grep -n "SELECT.*FOR UPDATE" ./apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
grep -r "SELECT.*FOR UPDATE" --include="*.java" apps/commerce-api/Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 223
🏁 Script executed:
cat -n ./apps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 5092
🏁 Script executed:
cat -n ./apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3061
🏁 Script executed:
cat -n ./apps/commerce-api/src/test/java/com/loopers/domain/order/OrderFacadeIntegrationTest.java | head -100Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4463
🏁 Script executed:
cat -n ./apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2702
🏁 Script executed:
grep -n "findAllByIdsForUpdate\|deadlock\|데드락" ./apps/commerce-api/src/test/java/com/loopers/domain/order/OrderFacadeIntegrationTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
find ./apps/commerce-api -name "*.java" -path "*/test/*" -exec grep -l "deadlock\|데드락\|동시\|concurrent" {} \;Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 432
🏁 Script executed:
cat -n ./apps/commerce-api/src/test/java/com/loopers/domain/order/OrderConcurrencyTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 6655
SELECT ... FOR UPDATE 쿼리에 명시적 ORDER BY id를 추가하고 역순 상품 조합의 동시 주문 회귀 테스트를 작성하여 데드락을 방지한다.
Line 84의 코멘트처럼 IN 절이 항상 PK 오름차순으로 정렬된다고 가정할 수 없다. ProductJpaRepository.java의 findAllByIdsForUpdate() 쿼리에는 명시적인 ORDER BY 절이 없으며, 쿼리 실행 계획이나 데이터 분포에 따라 락 획득 순서가 바뀔 수 있다. 예를 들어 Thread1이 상품 [1, 2] 순으로, Thread2가 [2, 1] 순으로 동시에 주문할 경우 데드락이 발생한다. OrderConcurrencyTest.java의 기존 테스트는 단일 상품 주문만 검증하므로 이 시나리오를 포착하지 못한다. 다음 두 가지를 수정한다:
- ProductJpaRepository의 쿼리에
ORDER BY p.id추가로 락 획득 순서 고정 - 역순 상품 조합 두 건을 동시에 주문하는 회귀 테스트 추가
🤖 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 83 - 88, Add a deterministic lock order to the FOR UPDATE query and
a regression test that triggers the reverse-order deadlock scenario: update
ProductJpaRepository.findAllByIdsForUpdate() to include an explicit "ORDER BY
p.id" so rows are locked in a consistent order, and add a concurrency test in
OrderConcurrencyTest that submits two orders with identical product IDs in
opposite orders (e.g., [1,2] and [2,1]) concurrently to verify no deadlock
occurs.
There was a problem hiding this comment.
리뷰포인트로 남긴 부분...멀티 로우 락의 순서 보장을 mysql InnoDB에 의존하냐 마냐의 문제
There was a problem hiding this comment.
@Namjin-kimm, 의도적으로 남긴 트레이드오프 포인트임을 확인하였다. 기술적 관점에서 다음을 보완한다.
현재 접근법의 근거 (InnoDB 의존)
IN 절로 PK를 조회할 경우, InnoDB의 B+Tree 클러스터드 인덱스 탐색 특성상 PK 오름차순으로 행을 스캔하여 락을 획득한다. 따라서 동일한 상품 집합을 포함한 두 트랜잭션이 같은 순서로 락을 획득하게 되어 데드락이 방지된다.
리스크 포인트
SELECT FOR UPDATE는 MySQL이 동일한 실행 방식으로 처리할 경우에만 데드락이 발생하지 않으며, 실행 방식이 달라질 수 있다는 점이 명시되어 있다.
구체적으로 다음 시나리오에서 가정이 깨질 수 있다:
| 상황 | 영향 |
|---|---|
| 쿼리 옵티마이저가 풀 테이블 스캔 선택 | 락 순서가 물리적 레코드 배치에 의존 |
| 인덱스 통계 부패 또는 재분석 후 실행 계획 변경 | PK 순서 보장 소멸 |
| MySQL 버전 업그레이드 후 옵티마이저 동작 변경 | 기존 가정 무효화 |
실제 위험도
InnoDB는 데드락을 자동으로 감지하여 충돌 트랜잭션 중 하나를 롤백한다. 따라서 데드락 발생 시 서버가 무한 대기에 빠지지는 않으나, 주문 트랜잭션이 예기치 않게 실패하여 사용자 경험에 영향을 준다.
결론
InnoDB 의존 방식은 현행 단일 DB 환경에서 실용적으로 동작하지만, 명시적 ORDER BY id를 쿼리에 추가하면 실행 계획과 MySQL 버전에 무관하게 락 순서를 보장할 수 있다. 설계 문서에 이 의존성을 명시적으로 기록하는 것도 운영 관점에서 권장한다.
✏️ Learnings added
Learnt from: Namjin-kimm
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 152
File: apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java:86-91
Timestamp: 2026-03-06T09:00:13.956Z
Learning: In Loopers-dev-lab/loop-pack-be-l2-vol3-java PR `#152`, OrderFacade.java's deadlock prevention for multi-product orders relies on MySQL InnoDB's implicit behavior of scanning IN-clause PK queries in ascending order (B+Tree clustered index traversal). This is a deliberate design decision acknowledged by the author as a trade-off: InnoDB generally locks rows in PK ascending order for IN queries, but this is not a formal SQL standard guarantee. If the query optimizer changes execution plans (e.g., full table scan), lock order may change and deadlocks could occur. The safe alternative is adding explicit ORDER BY id to the SELECT FOR UPDATE query in ProductJpaRepository.findAllByIdsForUpdate(). InnoDB does auto-detect and roll back deadlocked transactions, so hangs are avoided but transaction failures still occur.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: jsj1215
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 142
File: apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.java:80-94
Timestamp: 2026-03-06T06:41:37.434Z
Learning: In `ProductRepositoryImpl.java` (Loopers-dev-lab/loop-pack-be-l2-vol3-java), `findOptionByIdWithLock()` uses `entityManager.find(ProductOption.class, optionId, LockModeType.PESSIMISTIC_WRITE)` without a lock timeout hint. The call flow is: `OrderService.prepareOrderItems()` → `findProductOnly()` (loads only Product, not ProductOption) → `deductStock()` → `findOptionByIdWithLock()`. Since ProductOption is never loaded into the 1st-level cache before this point, there is no stale read risk. However, the lock timeout hint (`jakarta.persistence.lock.timeout`) is missing, risking infinite wait. Fix: use the 4-parameter `entityManager.find()` overload with `Map.of("jakarta.persistence.lock.timeout", 3000L)`.
Learnt from: madirony
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 0
File: :0-0
Timestamp: 2026-03-06T04:36:52.920Z
Learning: In PR `#148` of Loopers-dev-lab/loop-pack-be-l2-vol3-java, ALL WithLock repository methods — including `OrderJpaRepository.findByIdWithLock` — have `QueryHints(QueryHint(name = "jakarta.persistence.lock.timeout", value = "3000"))` applied consistently. No WithLock method is missing the timeout hint. This was confirmed by reading the full file content of OrderJpaRepository.java.
Learnt from: yoon-yoo-tak
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 6
File: apps/commerce-api/src/main/java/com/loopers/interfaces/api/user/UserV1Dto.java:9-10
Timestamp: 2026-02-04T05:52:09.963Z
Learning: 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. Do not introduce MethodArgumentNotValidException handlers or Bean Validation handling, as that would create inconsistent error handling patterns. This guidance applies to all Java files under the commerce-api module (source code under apps/commerce-api/src/main/java/).
| return new OrderInfo( | ||
| order.getId(), | ||
| order.getUserId(), | ||
| order.getUserCouponId(), | ||
| order.getOriginalAmount().getAmount(), | ||
| order.getDiscountAmount().getAmount(), | ||
| order.getFinalAmount().getAmount(), | ||
| order.getCreatedAt(), | ||
| items | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/commerce-api && find . -type f -name "*.java" | grep -E "(OrderInfo|Order\.java)" | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 197
🏁 Script executed:
find . -type f -name "*.sql" -o -name "*.java" | grep -i migration | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
find apps/commerce-api -type f -name "*.java" | xargs rg "class Order\b" -lRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 152
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/application/order/OrderInfo.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2051
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/domain/order/Order.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4038
🏁 Script executed:
find apps/commerce-api -type f \( -name "*.sql" -o -name "V*.java" -o -name "*.yml" \) | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 137
🏁 Script executed:
cat apps/commerce-api/src/main/resources/application.ymlRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1044
🏁 Script executed:
find apps/commerce-api -type d -name "db" -o -name "migration* " -o -name "flyway" -o -name "liquibase"Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
find . -type f -name "V*.sql" -o -name "U*.sql" 2>/dev/null | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/domain/product/Money.java 2>/dev/null || echo "Money class not found at expected path"Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 799
금액 스냅샷 필드의 null 안전성을 확보해야 한다.
Order 엔티티의 originalAmount, discountAmount, finalAmount 필드는 데이터베이스 제약으로만 NOT NULL이 설정되어 있고, Java 수준의 @NonNull 또는 @NotNull 어노테이션이 없다. OrderInfo.of() 메서드에서 이들 필드를 직접 체이닝으로 역참조할 때, 스키마 변경 이전의 기존 주문 데이터나 불완전한 마이그레이션으로 인해 null 값이 남아 있으면 NullPointerException이 발생하여 조회 API가 500 오류로 실패한다.
운영 영향: 과거 주문 목록/상세 조회가 일괄 장애로 번질 수 있다.
수정안:
- 데이터베이스 마이그레이션 파일과 기존 데이터 backfill 스크립트를 이 PR에 포함하여 모든 주문 레코드의 세 금액 필드를 채운 후 NOT NULL 제약을 적용한다.
- 또는 Order 엔티티에 Java 수준의
@NonNull어노테이션을 추가하고, null 가능성을 처리할 수 있는 방어적 로직(예: 기본값 반환)을 OrderInfo.of()에 추가한다.
추가 테스트: 금액 스냅샷이 비어 있는 레거시 주문 데이터를 읽는 시나리오를 테스트하여 기대 동작을 검증한다.
🤖 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/OrderInfo.java`
around lines 24 - 33, OrderInfo.of(...) currently dereferences
order.getOriginalAmount(), getDiscountAmount(), and getFinalAmount() directly
which can NPE for legacy rows; update by adding Java-level non-null annotations
to the Order entity fields (e.g., annotate originalAmount, discountAmount,
finalAmount with `@NonNull/`@NotNull) and modify OrderInfo.of (and the OrderInfo
constructor call site) to defensively handle nulls — check each of
order.getOriginalAmount(), order.getDiscountAmount(), order.getFinalAmount() and
substitute a safe default Money/amount (e.g., zero) or use Optional-like getters
before calling getAmount() so API reads never throw NullPointerException; also
add tests that construct Orders with null amount fields to verify behavior.
There was a problem hiding this comment.
기존 DB의 null 값이 들어가 있던 상황이라면 에러 발생 가능, 하지만 현재 상황에는 그럴 일이 없으므로 수정하지 않음
There was a problem hiding this comment.
@Namjin-kimm, 이해했다. 신규 스키마에서 기존 레거시 데이터가 없는 상황이라면 현실적인 위험이 없다는 판단은 합리적이다. 다만 향후 스키마 변경이나 데이터 마이그레이션이 발생할 경우 해당 지점을 다시 검토하기를 권장한다.
🧠 Learnings used
Learnt from: yoon-yoo-tak
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 6
File: apps/commerce-api/src/main/java/com/loopers/interfaces/api/user/UserV1Dto.java:9-10
Timestamp: 2026-02-04T05:52:09.963Z
Learning: 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. Do not introduce MethodArgumentNotValidException handlers or Bean Validation handling, as that would create inconsistent error handling patterns. This guidance applies to all Java files under the commerce-api module (source code under apps/commerce-api/src/main/java/).
| public UserCoupon issue(Long userId, Long couponTemplateId) { | ||
| if (userCouponRepository.existsByUserIdAndCouponTemplateId(userId, couponTemplateId)) { | ||
| throw new CoreException(ErrorType.CONFLICT, "이미 발급받은 쿠폰입니다."); | ||
| } | ||
| // 쿠폰 템플릿 존재 여부 검증 | ||
| CouponTemplate template = findTemplateById(couponTemplateId); | ||
| return userCouponRepository.save(new UserCoupon(couponTemplateId, userId, template.getExpiredAt())); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 파일 위치 확인
fd -t f 'CouponService|UserCoupon|Coupon.*Test' --extension java | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1114
🏁 Script executed:
# 스키마 파일 탐색
fd -t f -e sql -e yml | grep -i 'schema\|migration\|ddl' | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
# 디렉토리 구조 파악
find apps/commerce-api/src -type d | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1127
🏁 Script executed:
# 파일 크기 확인
wc -l apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.java
wc -l apps/commerce-api/src/main/java/com/loopers/domain/coupon/UserCoupon.java
wc -l apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponConcurrencyTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 330
🏁 Script executed:
# CouponService.java 전체 내용
cat -n apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 6003
🏁 Script executed:
# UserCoupon.java 내용
cat -n apps/commerce-api/src/main/java/com/loopers/domain/coupon/UserCoupon.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2575
🏁 Script executed:
# CouponConcurrencyTest 파일 읽기
cat -n apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponConcurrencyTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4737
🏁 Script executed:
# UserCouponRepository 관련 파일 읽기
wc -l apps/commerce-api/src/main/java/com/loopers/domain/coupon/UserCouponRepository.java
cat -n apps/commerce-api/src/main/java/com/loopers/domain/coupon/UserCouponRepository.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1438
🏁 Script executed:
# UserCouponRepositoryImpl 확인
wc -l apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/UserCouponRepositoryImpl.java
cat -n apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/UserCouponRepositoryImpl.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2486
🏁 Script executed:
# UserCouponJpaRepository 확인 (useIfAvailable 구현)
fd -t f 'UserCouponJpaRepository.java' --extension java
wc -l apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/UserCouponJpaRepository.java
cat -n apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/UserCouponJpaRepository.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2416
🏁 Script executed:
# issue() 메서드의 동시성 테스트 확인
rg -n "issue|발급" apps/commerce-api/src/test/java/com/loopers/domain/coupon/ -A 5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 10141
issue() 메서드의 중복 발급이 동시성 하에서 취약하다.
현재 구현은 existsByUserIdAndCouponTemplateId 조회 후 save하는 select-then-act 패턴으로, 동일 사용자/템플릿에 대한 동시 요청이 들어오면 두 트랜잭션이 모두 중복 검사를 통과해 중복 발급될 수 있다. UserCoupon.java의 주석(라인 20-21)에서 "UNIQUE 제약은 soft delete 테이블이므로 DB 미적용, 애플리케이션 레벨 검증"이라고 명시했으나, 애플리케이션 레벨의 단순 SELECT 조회는 트랜잭션 격리 수준 하에서도 경합 조건을 보장하지 못한다.
운영에서는 BR-C03(중복 발급 방지)이 깨져 할인 부채와 CS 이슈로 이어진다. 다음 중 하나로 개선해야 한다:
- 원자적 UPDATE 방식: applyCoupon()의
useIfAvailable()처럼 데이터베이스 UPDATE 쿼리에 WHERE 조건을 포함해 원자성 보장 - 비관적 락:
SELECT ... FOR UPDATE로 발급 시점의 행을 잠금 - 낙관적 락:
@Version필드 추가로 version 기반 동시성 제어 - 멱등성 보장: 이미 발급된 쿠폰이면 중복 발급 대신 기존 쿠폰을 반환하는 idempotent 구현
또한 CouponConcurrencyTest는 현재 applyCoupon()만 검증하며, issue() 메서드의 동시 요청 시나리오(같은 userId/couponTemplateId로 10개 이상의 스레드가 동시 발급 요청)가 누락되어 있다. 성공 요청이 정확히 1건만 남고 DB 행 수도 1건인지 검증하는 테스트를 추가해야 한다.
🤖 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 29 - 35, issue() uses a select-then-act pattern
(existsByUserIdAndCouponTemplateId -> save) and can produce duplicate UserCoupon
rows under concurrency; change issue() to perform an atomic
insert-with-fallback: wrap creation in a `@Transactional` method, attempt to
save(new UserCoupon(...)) and catch DataIntegrityViolationException (or
DuplicateKeyException) indicating a concurrent insert, then load and return the
existing coupon via userCouponRepository.findByUserIdAndCouponTemplateId(userId,
couponTemplateId); remove the initial existsByUserIdAndCouponTemplateId
pre-check or keep it only for fast-fail but rely on the exception for
correctness; finally add a concurrent test in CouponConcurrencyTest that spawns
many threads calling CouponService.issue(userId, couponTemplateId) and asserts
exactly one persisted UserCoupon and that all threads receive the same coupon
(or only one successful issuance).
There was a problem hiding this comment.
쿠폰 중복 발급 여부는 현재 단계에서 방지하지 않음
There was a problem hiding this comment.
@Namjin-kimm, 결정을 확인하였다. 현재 단계에서 issue() 메서드의 동시 중복 발급 방지는 구현 범위 밖임을 인지하였다.
향후 단계에서 해당 기능이 필요해질 경우, useIfAvailable()과 유사한 원자적 UPDATE 방식 또는 DB UNIQUE 제약 기반의 예외 처리 방식을 검토하면 될 것이다.
✏️ Learnings added
Learnt from: Namjin-kimm
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 152
File: apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.java:29-35
Timestamp: 2026-03-06T09:19:43.842Z
Learning: In the loop-pack-be-l2-vol3-java project, the `issue()` method in `CouponService.java` (apps/commerce-api/src/main/java/com/loopers/domain/coupon/CouponService.java) uses a select-then-act pattern (existsByUserIdAndCouponTemplateId -> save) and does NOT implement concurrent duplicate issuance prevention. This is an intentional design decision for the current stage; duplicate issuance prevention (BR-C03 concurrency hardening) is deferred to a later phase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yoon-yoo-tak
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 6
File: apps/commerce-api/src/main/java/com/loopers/interfaces/api/user/UserV1Dto.java:9-10
Timestamp: 2026-02-04T05:52:09.963Z
Learning: 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. Do not introduce MethodArgumentNotValidException handlers or Bean Validation handling, as that would create inconsistent error handling patterns. This guidance applies to all Java files under the commerce-api module (source code under apps/commerce-api/src/main/java/).
| public CouponTemplate(String name, CouponType type, int value, Integer minOrderAmount, LocalDateTime expiredAt) { | ||
| validateName(name); | ||
| validateType(type); | ||
| validateValue(value); | ||
| validateExpiredAt(expiredAt); |
There was a problem hiding this comment.
템플릿 검증에서 핵심 불변식을 빠뜨렸다.
현재는 minOrderAmount < 0 같은 설정도 저장되고, 정률 쿠폰에 value > 100도 허용된다. 운영에서는 관리자 오입력만으로 최소 주문 조건이 사실상 무력화되거나 할인액이 주문 금액을 넘어가 결제 금액이 음수가 되는 사고가 날 수 있으므로, minOrderAmount 검증을 추가하고 RATE는 1..100 범위만 허용하도록 타입별 검증으로 바꾸는 편이 안전하다. 등록/수정 모두 음수 최소 주문 금액과 101% 할인을 거절하는 테스트, 그리고 100% 할인 경계 테스트를 추가해야 한다. As per coding guidelines, **/domain/**/*.java: 엔티티/값 객체/DTO 경계를 명확히 하고, 불변성과 캡슐화를 점검한다.
Also applies to: 55-59, 103-109, 123-133
🤖 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/CouponTemplate.java`
around lines 41 - 45, The constructor CouponTemplate(...) currently calls
validateName, validateType, validateValue, validateExpiredAt but misses
validation for minOrderAmount and type-specific rate bounds; update the class to
add and call validateMinOrderAmount(minOrderAmount) to reject negative values
and change validateValue(int value) into type-aware checks (e.g., for
CouponType.RATE require 1..100 inclusive, for CouponType.AMOUNT require value >=
0) and ensure these validations are invoked both on construction and in any
update methods (e.g., methods that modify template fields). Also add unit tests
covering creation and update: reject negative minOrderAmount, reject RATE > 100
(e.g., 101), allow RATE == 100 (boundary), and reject negative fixed amount
where applicable to enforce these invariants.
| // 원자적 쿠폰 사용: WHERE used_at IS NULL 조건으로 동시 요청 중 하나만 성공 | ||
| // flushAutomatically: UPDATE 전 pending 변경사항 flush 보장 | ||
| // clearAutomatically 미사용: 같은 트랜잭션 내 Product 등 다른 관리 엔티티의 dirty checking을 유지하기 위함 | ||
| @Modifying(flushAutomatically = true) | ||
| @Query("UPDATE UserCoupon uc SET uc.usedAt = :now WHERE uc.id = :id AND uc.userId = :userId AND uc.usedAt IS NULL AND uc.deletedAt IS NULL") | ||
| int useIfAvailable(@Param("id") Long id, @Param("userId") Long userId, @Param("now") LocalDateTime now); |
There was a problem hiding this comment.
만료 검증이 TOCTOU에 취약하다.
지금 구조에서는 만료 검증과 사용 처리가 분리되어 있어, 만료 시각 경계에서 이미 만료된 쿠폰이 DB에서는 사용 처리될 수 있다. 운영에서는 만료 쿠폰 적용으로 주문 금액이 잘못 계산되므로, useIfAvailable의 WHERE 절에 만료 조건까지 포함해 사용 가능성 전체를 DB 한 번에 판정해야 한다. updated == 0도 곧바로 “이미 사용됨”으로 단정하지 말고 만료/삭제를 재판별하거나 메시지를 일반화하는 편이 안전하다. expiredAt 직전/직후 시점에서 정확히 한쪽만 성공하는 경계 테스트도 추가해야 한다. As per coding guidelines, **/*Repository*.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/infrastructure/coupon/UserCouponJpaRepository.java`
around lines 27 - 32, The current useIfAvailable update query can let TOCTOU
expiry races pass; modify the UserCoupon repository method useIfAvailable so its
WHERE clause also includes expiry-check (e.g., AND uc.expiredAt > :now or
appropriate not-expired condition) alongside uc.usedAt IS NULL and uc.deletedAt
IS NULL, pass the same now parameter and update the method signature to accept
it; change any caller logic that treats a returned 0 as strictly “already used”
to instead re-query or return a generalized failure reason
(used/expired/deleted) so callers can distinguish and handle expiry, and add
boundary tests around expiredAt (just before/after) to ensure only one
concurrent request wins.
| public ApiResponse<CouponAdminV1Dto.CouponTemplateListResponse> getTemplates( | ||
| @RequestParam(defaultValue = "0") int page, | ||
| @RequestParam(defaultValue = "20") int size) | ||
| { | ||
| return ApiResponse.success( | ||
| CouponAdminV1Dto.CouponTemplateListResponse.from( | ||
| couponAdminFacade.findAllTemplates(PageRequest.of(page, size)))); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'ApiControllerAdvice\.java$|ControllerAdvice.*\.java$' apps/commerce-api/src/main/java -x cat -n {}Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 6820
🏁 Script executed:
rg -n "PageRequest\\.of\(|@ExceptionHandler|IllegalArgumentException|CoreException" apps/commerce-api/src/main/java apps/commerce-api/src/test/java | head -100Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 15015
🏁 Script executed:
cat -n apps/commerce-api/src/main/java/com/loopers/interfaces/api/coupon/CouponAdminV1Controller.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4804
페이지 및 크기 파라미터를 유효성 검증 없이 PageRequest에 전달하면 공통 에러 포맷이 깨진다.
PageRequest.of(page, size)는 page < 0 또는 size <= 0일 때 IllegalArgumentException을 던지는데, ApiControllerAdvice에 해당 예외 핸들러가 없어 catch-all Throwable 핸들러로 빠져 INTERNAL_ERROR(500)로 응답한다. 클라이언트 입력 오류이므로 BAD_REQUEST(400)로 응답해야 한다. 또한 과도한 size 값이 들어올 경우 데이터베이스 부하가 증가하므로 상한선 검증도 필수다.
컨트롤러에서 다음과 같이 수정하면 된다:
page >= 0검증size > 0및size <= MAX_PAGE_SIZE검증- 위반 시
CoreException(BAD_REQUEST, "page and size parameters must be valid")로 throw
테스트로는 page=-1, size=0, size=초과값 요청이 모두 ApiResponse.FAIL 응답(code: 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/interfaces/api/coupon/CouponAdminV1Controller.java`
around lines 28 - 35, Validate request parameters in
CouponAdminV1Controller.getTemplates before calling PageRequest.of: add checks
that page >= 0 and size > 0 && size <= MAX_PAGE_SIZE (introduce a private static
final int MAX_PAGE_SIZE constant), and if any check fails throw new
CoreException(BAD_REQUEST, "page and size parameters must be valid") so client
input errors return BAD_REQUEST instead of bubbling as IllegalArgumentException;
keep using couponAdminFacade.findAllTemplates(...) after validation and add
tests for page=-1, size=0, and size>MAX_PAGE_SIZE to assert ApiResponse.FAIL
with code BAD_REQUEST.
| ExecutorService executor = Executors.newFixedThreadPool(THREAD_COUNT); | ||
| CountDownLatch startLatch = new CountDownLatch(1); | ||
| CountDownLatch doneLatch = new CountDownLatch(THREAD_COUNT); | ||
|
|
||
| for (int i = 0; i < THREAD_COUNT; i++) { | ||
| executor.submit(() -> { | ||
| try { | ||
| startLatch.await(); // 모든 스레드를 대기기 시키고 동시에 applyCoupon 작업을 시작할 수 있도록 | ||
| couponService.applyCoupon(coupon.getId(), user.getId(), ORDER_AMOUNT); | ||
| successCount.incrementAndGet(); | ||
| } catch (Exception e) { | ||
| // 예상된 실패 (이미 사용된 쿠폰) | ||
| } finally { | ||
| doneLatch.countDown(); | ||
| } | ||
| }); | ||
| } | ||
| // startLatch의 카운트 다운은 메인스레드에 존재하기 때문에, 반복문이 다 실행되고 아래 코드가 실행이 됨. | ||
| // 그때 비로소 10개의 대기중인 백그라운드 코드가 동시에 실행이 됨. | ||
| startLatch.countDown(); // 모든 스레드 동시 출발 | ||
| doneLatch.await(); | ||
| executor.shutdown(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -path "*commerce-api/src/test/java/com/loopers/domain/coupon/CouponConcurrencyTest.java" | head -5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 171
🏁 Script executed:
wc -l ./apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponConcurrencyTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 174
🏁 Script executed:
sed -n '60,97p' ./apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponConcurrencyTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2074
🏁 Script executed:
sed -n '1,70p' ./apps/commerce-api/src/test/java/com/loopers/domain/coupon/CouponConcurrencyTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2927
🏁 Script executed:
find . -type f -name "*.java" | xargs grep -l "class CoreException" | head -3Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 246
🏁 Script executed:
find . -type f -name "CouponService.java" -path "*/main/*" | xargs grep -A 10 "applyCoupon" | head -30Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 643
동시성 테스트가 무한 대기(hang) 또는 중요한 버그를 은폐할 수 있다.
doneLatch.await()에 timeout이 없어서 실제 데드락 또는 리소스 고갈이 발생하면 CI 파이프라인이 실패하지 않고 무한 대기로 멈춘다. 여기에 catch (Exception)이 모든 예외를 삼키므로, 데이터베이스 연결 오류, 트랜잭션 타임아웃, 동시성 버그까지 "예상된 비즈니스 실패"로 위장된다. 결과적으로 운영 환경에서 가장 위험한 동시성 회귀를 테스트가 놓치게 된다.
다음 조치가 필요하다:
doneLatch.await(timeout, TimeUnit.SECONDS)로 5초 내 완료 강제executor.shutdownNow()를finally블록에서 항상 실행하여 리소스 누수 방지catch (CoreException e)로 범위를 좁혀서 비즈니스 예외만 수집,failures큐에 기록- 단일 성공(successCount=1) + 나머지는 모두 기대한 CoreException 타입만 허용하도록 검증
추가로 예외 타입과 메시지가 일정하고, 실패 시나리오에서도 정해진 시간 내 테스트가 종료되는지 별도 케이스로 검증해야 한다.
수정 예시
+import com.loopers.support.error.CoreException;
import java.time.LocalDateTime;
import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ConcurrentLinkedQueue;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
@@
AtomicInteger successCount = new AtomicInteger(0);
+ ConcurrentLinkedQueue<Throwable> failures = new ConcurrentLinkedQueue<>();
ExecutorService executor = Executors.newFixedThreadPool(THREAD_COUNT);
CountDownLatch startLatch = new CountDownLatch(1);
CountDownLatch doneLatch = new CountDownLatch(THREAD_COUNT);
- for (int i = 0; i < THREAD_COUNT; i++) {
- executor.submit(() -> {
- try {
- startLatch.await(); // 모든 스레드를 대기기 시키고 동시에 applyCoupon 작업을 시작할 수 있도록
- couponService.applyCoupon(coupon.getId(), user.getId(), ORDER_AMOUNT);
- successCount.incrementAndGet();
- } catch (Exception e) {
- // 예상된 실패 (이미 사용된 쿠폰)
- } finally {
- doneLatch.countDown();
- }
- });
- }
- // startLatch의 카운트 다운은 메인스레드에 존재하기 때문에, 반복문이 다 실행되고 아래 코드가 실행이 됨.
- // 그때 비로소 10개의 대기중인 백그라운드 코드가 동시에 실행이 됨.
- startLatch.countDown(); // 모든 스레드 동시 출발
- doneLatch.await();
- executor.shutdown();
+ try {
+ for (int i = 0; i < THREAD_COUNT; i++) {
+ executor.submit(() -> {
+ try {
+ startLatch.await();
+ couponService.applyCoupon(coupon.getId(), user.getId(), ORDER_AMOUNT);
+ successCount.incrementAndGet();
+ } catch (CoreException e) {
+ failures.add(e);
+ } finally {
+ doneLatch.countDown();
+ }
+ });
+ }
+ startLatch.countDown();
+ assertThat(doneLatch.await(5, TimeUnit.SECONDS)).isTrue();
+ } finally {
+ executor.shutdownNow();
+ }
@@
UserCoupon used = userCouponJpaRepository.findById(coupon.getId()).orElseThrow();
assertThat(successCount.get()).isEqualTo(1); // 정확히 1번만 성공
assertThat(used.getUsedAt()).isNotNull(); // DB에 used_at 기록 확인
+ assertThat(failures).hasSize(THREAD_COUNT - 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/CouponConcurrencyTest.java`
around lines 68 - 89, The test can hang and hide real errors; update the
concurrency block so doneLatch.await(...) uses a timeout (e.g.,
doneLatch.await(5, TimeUnit.SECONDS)) and always shuts down the executor in a
finally block (executor.shutdownNow()); narrow the catch to business failures
(catch CoreException e) and record them into a failures collection, keep
incrementing successCount only on successful couponService.applyCoupon(...)
calls, and assert afterwards that successCount.get() == 1 and all other failures
are CoreException with expected messages to fail fast on DB/transaction issues.
| CountDownLatch startLatch = new CountDownLatch(1); | ||
| CountDownLatch doneLatch = new CountDownLatch(OVER_THREAD_COUNT); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "OrderConcurrencyTest.java" | head -5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 169
🏁 Script executed:
wc -l ./apps/commerce-api/src/test/java/com/loopers/domain/order/OrderConcurrencyTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 173
🏁 Script executed:
cat -n ./apps/commerce-api/src/test/java/com/loopers/domain/order/OrderConcurrencyTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 6655
동시성 테스트가 무기한 대기로 빌드를 멈출 수 있다.
doneLatch.await()에 제한 시간이 없어서 락 회귀나 스레드 누수가 발생하면 테스트가 실패하지 않고 빌드 전체를 붙잡는다. await(timeout, TimeUnit.SECONDS)로 상한을 두고, ExecutorService는 shutdownNow()와 awaitTermination()을 호출해 자원을 정확히 정리해야 한다. 두 테스트 메서드(라인 89-90, 132-133)에서 동일한 패턴을 수정하고, 의도적으로 락을 오래 점유하는 시나리오에서 테스트가 제한 시간 내 실패하는지 확인하면 된다.
제안 수정
+import java.util.concurrent.TimeUnit;
@@
- doneLatch.await();
- executor.shutdown();
+ assertThat(doneLatch.await(10, TimeUnit.SECONDS)).isTrue();
+ executor.shutdownNow();
+ assertThat(executor.awaitTermination(5, TimeUnit.SECONDS)).isTrue();두 번째 테스트(라인 132-133)에도 동일한 패턴을 적용하면 된다.
🤖 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/OrderConcurrencyTest.java`
around lines 71 - 72, 동시성 테스트가 무기한 대기할 수 있으니 CountDownLatch의 무제한 await를 제한시간이 있는
await(timeout, TimeUnit.SECONDS)로 변경하고, 테스트에서 사용하는 ExecutorService를 종료할 때
shutdownNow()를 호출한 다음 awaitTermination(timeout, TimeUnit.SECONDS)로 자원 정리를 보장하도록
수정하세요; 구체적으로 OrderConcurrencyTest의 CountDownLatch 변수들(startLatch, doneLatch,
OVER_THREAD_COUNT)을 사용하는 두 테스트 메서드(각 테스트의 doneLatch.await() 호출)를 찾아서 타임아웃 버전으로
바꾸고, 테스트 종료 블록에서 ExecutorService.shutdownNow()와
ExecutorService.awaitTermination(...)를 호출해 강제 종료와 대기 로직을 추가해 주세요.
| executor.submit(() -> { | ||
| try { | ||
| startLatch.await(); // 모든 스레드를 대기시키고 동시에 출발 | ||
| orderFacade.create(USER_ID, command); | ||
| successCount.incrementAndGet(); | ||
| } catch (Exception e) { | ||
| // 예상된 실패 (재고 부족 - BAD_REQUEST) | ||
| } finally { | ||
| doneLatch.countDown(); | ||
| } |
There was a problem hiding this comment.
예상 실패와 비정상 실패를 분리해서 검증해야 한다.
지금은 Exception을 모두 삼켜서 첫 번째 테스트가 재고 부족 대신 데드락, 락 타임아웃, NPE로 실패해도 성공 건수만 맞으면 통과할 수 있다. 운영 관점에서는 "정상적인 품절 차단"과 "동시성 장애"를 구분하지 못해 회귀를 놓치게 된다. 실패 예외를 별도로 수집해 기대한 비즈니스 예외만 허용하고, 그 외 예외는 즉시 테스트를 실패시키도록 바꿔 달라. 추가로 초과 주문 시 정확히 OVER_THREAD_COUNT - STOCK_COUNT건만 재고 부족으로 실패하고, 락 타임아웃/데드락 계열 예외는 0건임을 검증하는 어설션을 넣어 달라.
As per coding guidelines "**/*Test*.java: 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다. 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."
Also applies to: 119-128
1 participant
📌 Summary
구현중 고민한 것들
[쿠폰 만료일 관리 방식]
CouponTemplate.expiredAt참조 → 관리자가 만료일 수정 시 발급된 쿠폰의 유효 기간이 소급 변경됨UserCoupon.expiredAt에 스냅샷 저장 → 발급 이후 템플릿 변경에 독립적minOrderAmount는 현행 정책 기준 적용이 자연스러우므로 스냅샷 제외.UserCoupon.expiredAt만으로 상태 계산 가능 → N+1 없음. 단, 만료일 일괄 수정이 기발급 쿠폰에 반영되지 않음.[여러 상품 주문 시 데드락 방지]
[재고 동시성 제어]
[상품 좋아요 컬럼 업데이트 방식]
🏗️Design Overview
변경 범위
CouponTemplate,UserCoupon,CouponService,CouponFacade,CouponAdminFacade,CouponTemplateRepository,UserCouponRepository,CouponV1Controller,CouponAdminV1ControllerCouponFacadeIntegrationTest,CouponConcurrencyTest,OrderFacadeIntegrationTest,OrderConcurrencyTest,LikeConcurrencyTestspring-retry의존성 제거 (낙관적 락 재시도 미사용)주요 컴포넌트 책임
CouponTemplate: 쿠폰 정책(정액/정률 할인, 최소 주문 금액) 캡슐화.calculateDiscount(),validateMinOrderAmount()도메인 메서드 제공UserCoupon: 발급 시expiredAt스냅샷 보존.validateNotExpired(now)로 만료 검증 (Tell, don't ask)CouponService.applyCoupon(): 소유권 확인 → 만료 검증 → 최소 금액 검증 → 원자적 사용 처리 → 할인 금액 반환OrderFacade.create(): 재고 확인(비관적 락) → 쿠폰 적용 → 재고 차감(dirty checking) → 주문 저장의 단일 트랜잭션 오케스트레이션ProductService.findAllAndVerifyStock():SELECT FOR UPDATE로 재고 확인. 호출자의 트랜잭션에 참여하여 락 유지리뷰포인트
1. 재고 차감 - atomic update VS 비관적 락
재고 차감은 UPDATE product SET stock = stock - N WHERE id = ? AND stock >= N 형태로 확인과 차감을 단일 쿼리로 결합할 수 있어 atomic update만으로도 동시성을 보장할 수 있습니다. 그러나 이 방식은 UPDATE가 0 rows를 반환했을 때 어떤 상품의 재고가 얼마나 부족한지 애플리케이션 레벨에서 파악하기 어려워 구체적인 에러 메시지를 만들기 위해 추가 SELECT가 필요합니다. 사용자에게 "나이키 에어맥스 (현재 재고: 3개, 주문 수량: 5개)"처럼 구체적인 메시지를 추가 조회 없이 반환하기 위해 SELECT FOR UPDATE로 재고를 먼저 확인하는 비관적 락 방식을 선택하였습니다. atomic update를 사용할 수 있음에도 사용자 ux를 위해 비관적 락을 사용하였는데 이런 판단이 합리적인지 멘토님의 의견이 궁금합니다.
2. 데드락 방지
MySQL InnoDB는 PK 인덱스 오름차순으로 락을 잡는 사실을 알게 되었습니다. 그래서 여러 고객들이 동일한 상품 조합을 서로 다른 순서로 동시에 주문하면 데드락이 발생할 수 있는데, 현재는 이 부분을 mysql의 기능에 의존함으로써 데드락을 방지하는 상황입니다. 다만 이 동작은 SQL 표준이 아닌 MySQL 구현에 의존하는 것이므로 별도로 코드 레벨에서 상품의 재고 차감 순서를 항시 오름차순으로 보장하는 구현이 필요한지 궁금합니다.
3. 좋아요 유니크 제약 처리 위치
한 고객의 좋아요 요청이 동시에 요청될 경우 나머지 하나의 요청은 좋아요 테이블의 유니크 키(user_id, product_id) 제약에 의해 DataIntegrityViolationException 에러를 발생하게 됩니다.
현재는 위와 같이 jpa의 saveAndFlush 사용하여 바로 DataIntegrityViolationException 에러를 발생하게 하고 catch 하여서 에러 메세지를 커스터마이징 하는 방법을 선택하였습니다. 이 방법 외에 ApiControllerAdvice에서 DataIntegrityViolationException을 전역으로 처리하여 "이미 처리되었습니다" 수준의 메시지를 반환하는 방법도 있는데, 어떤 방식이 더 적합한지 궁금합니다.
4. 실무에서의 동시성 제어 처리 방법
멘토링에서 말씀해주신 atomic > 낙관적 락 > 비관적 락 , 이 우선순위 순으로 최대한 적용하는게 좋다 라는 말씀을 듣고 모든 동시성 제어가 필요한 부분에 각각의 방법을 적용했을때 발생하는 장단점에 대해서 고민을 많이 했습니다. 재고 차감 부분에서 '사용자에게 구체적인 에러 메세지 반환'의 의도만 없었다면 락 없이 atomic update만 사용했을것 같습니다.
그치만 추후 주문 단계에 외부 API 호출(예: 결제)이나 여러 도메인을 조율하는 로직이 추가된다면 락 범위나 방식을 재검토해야 할 것 같은데, 실무에서는 이런 확장 가능성을 미리 감안하여 동시성 제어 방식을 선택하는지, 아니면 현재 요구사항에 맞게 최소한으로 적용하고 필요 시 리팩터링하는지 궁금합니다.
5. 도메인 주도 설계에서 조금은 벗어나는 느낌
동시성 제어가 필요한 부분에 최대한 atomic update를 사용할 수 있는 부분은 해당 방법을 적용하려고 노력했습니다. 하지만 구현하고 나서 코드를 보니 도메인 주도 설계와 조금은 멀어졌다는 느낌을 받았습니다. 재고 차감, 좋아요 등 바로 쿼리를 통해 상태 변경을 함으로써 도메인 모델에서의 명시적인 행위가 사라졌다는 느낌을 받았습니다. 어떻게 보면 인프라 레이어에 해당 책임을 위임한것 아닌가..?라는 생각이 들기도 했는데요, 그럼에도 내린 결론은 현재 저희 프로젝트는 DIP를 통해 도메인 주도 설계를 하고 있고, atomic update로 인해 도메인 모델의 행위가 사라졌다고 해도 repository interface는 domain layer에서 정의한 것이기 때문에, 해당 interface를 호출해서 진행하는 것 또한 도메인 비즈니스 로직이다라는 결론을 내렸습니다. 이에 대한 멘토님의 의견이 궁금합니다.