[Volume-5] 상품 조회 성능 개선 및 Redis 캐시 적용#239
Open
SeungchanKKK wants to merge 4 commits into
Open
Conversation
Volume 4 transactional operation
LikeFacade 신설 (Application Layer 조율) Like 전용 Facade를 만들고 거기서 양쪽 트랜잭션을 하나로 묶는 방식
productModel @Index 2개 추가 LikeModel @Index 2개 추가
📝 Walkthrough변경 목적상품 목록 조회 시 발생하는 N+1 쿼리 문제(좋아요 수 조회)와 정렬 비효율성을 해결하기 위해, 좋아요 수를 ProductModel에 비정규화하고 Redis 캐싱을 도입하며, 데이터베이스 인덱스를 최적화합니다. 핵심 변경점좋아요 카운트 비정규화: ProductModel에 리스크/주의사항확인 필요 사항: (1) ProductModel 생성자에서 테스트/검증LikeFacadeTest(좋아요/취소 시 이미 상태인 경우 멱등성 검증), ProductCacheServiceTest(캐시 히트/미스/무효화 동작), ProductFacadeTest(캐시 적용 시나리오와 캐시 무효화), LikeServiceTest(boolean 반환값), ProductModelTest(incrementLikeCount/decrementLikeCount 메커니즘) 등의 단위 테스트로 각 컴포넌트 동작 검증. 통합 테스트를 통해 pessimistic lock 기반의 동시성 제어와 트랜잭션 원자성 확인 권장. Walkthrough
Changes상품 캐싱, 좋아요 카운트 관리, 정렬 위임
CLAUDE.md 문서 정리
Sequence Diagram(s)sequenceDiagram
participant Controller
participant LikeFacade
participant LikeService
participant ProductService
participant ProductCacheService
participant Redis
rect rgba(70, 130, 180, 0.5)
note over Controller, Redis: 좋아요 등록 흐름
Controller->>LikeFacade: like(memberId, productId)
LikeFacade->>LikeService: like(memberId, productId)
LikeService-->>LikeFacade: true (신규) / false (중복)
alt true
LikeFacade->>ProductService: incrementLikeCount(productId)
Note over ProductService: findWithLock → increment
LikeFacade->>ProductCacheService: evictProductDetail(productId)
ProductCacheService->>Redis: delete("product:detail:{id}")
LikeFacade->>ProductCacheService: evictAllProductLists()
ProductCacheService->>Redis: keys("product:list:*") → delete
end
end
rect rgba(180, 80, 80, 0.5)
note over Controller, Redis: 상품 목록 조회 캐시 흐름
Controller->>LikeFacade: getProductsWithDetail(brandId, sort)
LikeFacade->>ProductCacheService: getProductList(brandId, sort)
ProductCacheService->>Redis: get("product:list:{brandId}:sort:{sort}")
alt 캐시 히트
Redis-->>ProductCacheService: JSON
ProductCacheService-->>LikeFacade: Optional<List>
else 캐시 미스
Redis-->>ProductCacheService: null
LikeFacade->>ProductService: getActiveProducts(brandId, sort)
ProductService-->>LikeFacade: List<ProductModel>
LikeFacade->>ProductCacheService: cacheProductList(...)
ProductCacheService->>Redis: set(key, JSON, TTL 3m)
end
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
SeungchanKKK
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/like/LikeServiceTest.java (1)
57-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win재좋아요 회귀 케이스를 테스트에 포함해야 한다
현재 unlike 관련 검증은 “활성 없음” 분기까지는 다루지만, 운영에서 가장 위험한 “soft delete 이후 재좋아요” 경계가 비어 있어 유니크 충돌/500 회귀를 CI에서 놓칠 수 있다.
수정안은 unlike 이후 같은(memberId, productId)로 like를 다시 호출하는 테스트를 추가해 성공 반환과 상태 복구(또는 단일 row 재활성화 정책)를 명시적으로 검증하는 것이다.
추가 테스트로 동시 요청 상황에서 멱등 응답 계약(예외 대신 false 또는 정책된 결과)을 확인하는 통합 테스트도 함께 권장한다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/test/java/com/loopers/domain/like/LikeServiceTest.java` around lines 57 - 83, The Unlike test class is missing a regression test for the "re-like after unlike" scenario, which is a critical boundary case that could cause unique constraint violations or 500 errors in production. Add a new test method to the Unlike nested class that verifies the behavior when unlike is called followed immediately by another like call on the same (memberId, productId) pair. The test should confirm that the re-like call succeeds (returns true) and properly handles state recovery by either reactivating the soft-deleted row or creating a new one according to your like policy. Additionally, consider adding an integration test to verify idempotent behavior and proper responses under concurrent requests scenarios.
🧹 Nitpick comments (2)
apps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheService.java (1)
36-37: ⚡ Quick winLine [36] 예외 로그에서 stacktrace를 버려 장애 원인 추적성이 떨어진다.
운영 장애 관점에서 현재 로그는
e.getMessage()만 남겨 원인 체인과 실패 지점을 복구하기 어렵다.log.warn("...", e)형태로 cause를 보존하고, 메시지는 운영자용 로그 문맥과 분리해 남기도록 수정이 필요하다. 추가로 Redis 장애 테스트에서 예외 전파 억제뿐 아니라 로그에 throwable이 포함되는지 검증하는 테스트를 추가하는 것이 안전하다.
As per coding guidelines, "예외 처리 시 cause를 보존하고, 사용자 메시지와 로그 메시지를 분리하도록 제안한다." 규칙을 적용한 제안이다.Also applies to: 46-47, 59-60, 70-71, 78-79, 89-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheService.java` around lines 36 - 37, The exception logging in the catch block is losing stacktrace information by only including e.getMessage() in the log statement, which reduces debuggability in production. Modify all six catch blocks (the one at line 36-37 and those referenced at lines 46-47, 59-60, 70-71, 78-79, 89-90) to pass the exception object directly as the last parameter to log.warn instead of extracting just the message. Change the pattern from log.warn("Redis 캐시 조회 실패 (product:detail:{}): {}", productId, e.getMessage()) to log.warn("Redis 캐시 조회 실패 (product:detail:{})", productId, e) so the logger framework can capture and preserve the full exception cause chain and stacktrace for operational debugging.Source: Coding guidelines
apps/commerce-api/src/test/java/com/loopers/application/product/ProductCacheServiceTest.java (1)
54-139: ⚡ Quick win목록 캐시 경로 테스트가 빠져 있어 회귀를 놓칠 가능성이 크다.
운영 관점에서 실제 트래픽 핵심 경로인 목록 캐시(
getProductList,cacheProductList)의 직렬화/역직렬화/TTL 계약이 검증되지 않아, 배포 후 캐시 미스 급증이나 역직렬화 실패를 사전에 탐지하기 어렵다.ProductSortType별 키 생성, 정상 히트 반환, 캐시 미스 반환, Redis 예외 시 fail-open 동작, TTL 3분 저장 검증 테스트를 추가하는 수정이 필요하다. 특히cacheProductList는set(key, json, 3L, MINUTES)호출 인자 검증 테스트를 별도로 추가해야 한다.
As per coding guidelines, "**/*Test*.java: 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다." 규칙에 따른 보강 제안이다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/test/java/com/loopers/application/product/ProductCacheServiceTest.java` around lines 54 - 139, Add missing test coverage for the product list cache operations by creating two new nested test classes: GetProductList (similar to GetProductDetail, testing cache hits with proper JSON deserialization, cache misses returning empty Optional, and Redis failures returning empty) and CacheProductList (similar to CacheProductDetail, verifying that the set method is called with the correct key, JSON string, 3L timeout, and TimeUnit.MINUTES, plus testing that Redis failures do not throw exceptions). Ensure the GetProductList tests validate that keys are generated correctly based on ProductSortType parameters, and verify both successful deserialization of ProductListInfo and fail-open behavior on Redis exceptions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java`:
- Around line 19-20: Add explicit null-safety validation to both the like method
(at the beginning, checking memberId and productId parameters) and the unlike
method (lines 29-30, checking the same parameters) in the LikeFacade class. Use
Objects.requireNonNull() to validate that both memberId and productId are not
null immediately upon method entry, throwing a NullPointerException with
descriptive messages if either parameter is null. This ensures validation
happens at the facade entry point rather than in lower service layers, enabling
faster error identification in production. Additionally, add unit tests that
verify exceptions are thrown immediately when calling like(null, 1L), like(1L,
null), unlike(null, 1L), and unlike(1L, null).
- Around line 18-25: The cache invalidation in the like method is being executed
before the transaction commits, creating a race condition where concurrent
requests can read uncommitted data and re-cache stale values. Move the cache
eviction calls (evictProductDetail and evictAllProductLists) to execute after
transaction commit by either using
TransactionSynchronizationManager.registerSynchronization or refactoring to use
`@TransactionalEventListener` with phase set to AFTER_COMMIT. Additionally, add
integration tests to verify that the final cache contains the latest likeCount
even when queries occur before commit, and that cache invalidation does not
execute on transaction rollback.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheService.java`:
- Around line 83-88: The evictAllProductLists method uses synchronous
redisTemplate.keys() which blocks Redis and causes performance issues during
high traffic. Replace the blocking KEYS operation with a SCAN-based approach
that progressively deletes keys in batches without blocking Redis, or implement
a versioned namespace strategy (e.g., product:list:v{n}:*) to atomically
invalidate caches. Additionally, add tests in ProductCacheServiceTest to verify
that cache invalidation completes within acceptable time limits even with large
key volumes (10k or more keys).
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`:
- Around line 57-59: The getProductsWithDetail method in ProductFacade does not
validate null input for the sort parameter, which can cause NullPointerException
when sort.name() is called in cache key generation or during sorting logic. Add
null-safety handling by either normalizing a null sort parameter to a default
value like ProductSortType.LATEST at the method entry point, or by throwing an
explicit validation exception. Additionally, add test cases in ProductFacadeTest
to verify that null sort inputs are handled correctly and default to the
expected sorting behavior.
- Around line 58-61: The cache hit path in the getProductsWithDetail method
bypasses brand status validation and returns cached data directly, while the
cache miss path validates brand status through the toDetailInfo method. This
creates a data consistency issue where suspended or deleted brands can still
serve cached product lists for up to the TTL duration. To fix this, inject
ProductCacheService as a dependency into BrandService, then add
productCacheService.evictAllProductLists() calls at the end of the suspendBrand,
deleteBrand, and reinstateBrand methods to ensure the cache is invalidated
whenever brand status changes. Additionally, write an integration test that
verifies the entire flow: suspend a brand, confirm the cache is cleared, then
verify that re-querying the product list either returns empty results or
properly reflects the brand's suspended status.
In `@apps/commerce-api/src/main/java/com/loopers/domain/like/LikeService.java`:
- Around line 16-22: The like method in LikeService is checking only active
likes but doesn't account for soft-deleted records, causing unique constraint
violations in two scenarios: when re-liking after unlike, and during concurrent
like requests. This breaks the idempotent contract expected by LikeFacade.
Modify the like method to query all records (including soft-deleted) instead of
just active ones, then implement branching logic: if an active like exists
return false, if a soft-deleted like exists update it to active and return true,
if no record exists insert a new one and return true. Additionally, wrap the
insert operation in a try-catch to gracefully handle unique constraint violation
exceptions by returning false instead of propagating the exception, ensuring the
method maintains its boolean contract under concurrent requests.
In
`@apps/commerce-api/src/test/java/com/loopers/application/like/LikeFacadeTest.java`:
- Around line 49-58: The idempotent test case
does_not_change_anything_when_already_liked is missing a verification to ensure
that evictAllProductLists() on productCacheService is never called when a
product is already liked. Add verify(productCacheService,
never()).evictAllProductLists() to this test method, and apply the same
verification to the other idempotent test method mentioned in the comment.
Optionally, also add verifyNoMoreInteractions(productService,
productCacheService) at the end of both tests to strictly enforce that no other
unexpected interactions occur.
---
Outside diff comments:
In
`@apps/commerce-api/src/test/java/com/loopers/domain/like/LikeServiceTest.java`:
- Around line 57-83: The Unlike test class is missing a regression test for the
"re-like after unlike" scenario, which is a critical boundary case that could
cause unique constraint violations or 500 errors in production. Add a new test
method to the Unlike nested class that verifies the behavior when unlike is
called followed immediately by another like call on the same (memberId,
productId) pair. The test should confirm that the re-like call succeeds (returns
true) and properly handles state recovery by either reactivating the
soft-deleted row or creating a new one according to your like policy.
Additionally, consider adding an integration test to verify idempotent behavior
and proper responses under concurrent requests scenarios.
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheService.java`:
- Around line 36-37: The exception logging in the catch block is losing
stacktrace information by only including e.getMessage() in the log statement,
which reduces debuggability in production. Modify all six catch blocks (the one
at line 36-37 and those referenced at lines 46-47, 59-60, 70-71, 78-79, 89-90)
to pass the exception object directly as the last parameter to log.warn instead
of extracting just the message. Change the pattern from log.warn("Redis 캐시 조회 실패
(product:detail:{}): {}", productId, e.getMessage()) to log.warn("Redis 캐시 조회 실패
(product:detail:{})", productId, e) so the logger framework can capture and
preserve the full exception cause chain and stacktrace for operational
debugging.
In
`@apps/commerce-api/src/test/java/com/loopers/application/product/ProductCacheServiceTest.java`:
- Around line 54-139: Add missing test coverage for the product list cache
operations by creating two new nested test classes: GetProductList (similar to
GetProductDetail, testing cache hits with proper JSON deserialization, cache
misses returning empty Optional, and Redis failures returning empty) and
CacheProductList (similar to CacheProductDetail, verifying that the set method
is called with the correct key, JSON string, 3L timeout, and TimeUnit.MINUTES,
plus testing that Redis failures do not throw exceptions). Ensure the
GetProductList tests validate that keys are generated correctly based on
ProductSortType parameters, and verify both successful deserialization of
ProductListInfo and fail-open behavior on Redis exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6457bcf1-45d2-431e-a841-4e58d97b8a47
📒 Files selected for processing (16)
CLAUDE.mdapps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheService.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaapps/commerce-api/src/main/java/com/loopers/domain/like/LikeModel.javaapps/commerce-api/src/main/java/com/loopers/domain/like/LikeService.javaapps/commerce-api/src/main/java/com/loopers/domain/product/ProductModel.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/product/ProductJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.javaapps/commerce-api/src/test/java/com/loopers/application/like/LikeFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductCacheServiceTest.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/domain/like/LikeServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/product/ProductModelTest.java
💤 Files with no reviewable changes (1)
- CLAUDE.md
Comment on lines
+18
to
+25
| @Transactional | ||
| public void like(Long memberId, Long productId) { | ||
| boolean liked = likeService.like(memberId, productId); | ||
| if (liked) { | ||
| productService.incrementLikeCount(productId); | ||
| productCacheService.evictProductDetail(productId); | ||
| productCacheService.evictAllProductLists(); | ||
| } |
There was a problem hiding this comment.
트랜잭션 커밋 전에 캐시를 무효화하면 stale 데이터가 재캐싱될 수 있다
운영 관점에서 현재 구조는 DB 커밋 전에 캐시를 비우므로, 동시 요청이 커밋 전 값을 읽어 오래된 리스트/상세를 다시 캐싱할 수 있는 창이 생긴다. 캐시 무효화는 커밋 이후(after commit)로 지연해야 한다.
수정안으로 TransactionSynchronizationManager.registerSynchronization(...) 또는 @TransactionalEventListener(phase = AFTER_COMMIT)로 무효화를 옮기는 것을 권장한다.
추가 테스트로 “커밋 전 조회가 발생해도 최종 캐시에 최신 likeCount가 남는지”와 “롤백 시 캐시 무효화가 실행되지 않는지”를 통합 테스트로 검증해야 한다.
수정 예시(diff)
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.transaction.support.TransactionSynchronizationManager;
`@RequiredArgsConstructor`
`@Component`
public class LikeFacade {
@@
`@Transactional`
public void like(Long memberId, Long productId) {
boolean liked = likeService.like(memberId, productId);
if (liked) {
productService.incrementLikeCount(productId);
- productCacheService.evictProductDetail(productId);
- productCacheService.evictAllProductLists();
+ runAfterCommit(() -> {
+ productCacheService.evictProductDetail(productId);
+ productCacheService.evictAllProductLists();
+ });
}
}
@@
public void unlike(Long memberId, Long productId) {
boolean unliked = likeService.unlike(memberId, productId);
if (unliked) {
productService.decrementLikeCount(productId);
- productCacheService.evictProductDetail(productId);
- productCacheService.evictAllProductLists();
+ runAfterCommit(() -> {
+ productCacheService.evictProductDetail(productId);
+ productCacheService.evictAllProductLists();
+ });
}
}
+
+ private void runAfterCommit(Runnable task) {
+ if (!TransactionSynchronizationManager.isSynchronizationActive()) {
+ task.run();
+ return;
+ }
+ TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
+ `@Override`
+ public void afterCommit() {
+ task.run();
+ }
+ });
+ }
}Also applies to: 28-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java`
around lines 18 - 25, The cache invalidation in the like method is being
executed before the transaction commits, creating a race condition where
concurrent requests can read uncommitted data and re-cache stale values. Move
the cache eviction calls (evictProductDetail and evictAllProductLists) to
execute after transaction commit by either using
TransactionSynchronizationManager.registerSynchronization or refactoring to use
`@TransactionalEventListener` with phase set to AFTER_COMMIT. Additionally, add
integration tests to verify that the final cache contains the latest likeCount
even when queries occur before commit, and that cache invalidation does not
execute on transaction rollback.
Comment on lines
+19
to
+20
| public void like(Long memberId, Long productId) { | ||
| boolean liked = likeService.like(memberId, productId); |
There was a problem hiding this comment.
입력 파라미터 null 안전성을 명시적으로 보장해야 한다
운영 관점에서 memberId/productId가 null로 유입되면 하위 레이어에서 늦게 예외가 발생해 장애 분석 시간이 길어진다. 파사드 진입점에서 즉시 검증해 예외 흐름을 고정해야 한다.
수정안으로 Objects.requireNonNull(...) 또는 @Validated + @NotNull``을 적용하는 것을 권장한다.
추가 테스트로 like(null, 1L), `like(1L, null)`, `unlike(null, 1L)`, `unlike(1L, null)`에서 의도된 예외가 즉시 발생하는지 검증해야 한다.
As per coding guidelines, **/*.java: Ensure null-safety in all code; use Optional in Java for null handling.
Also applies to: 29-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java`
around lines 19 - 20, Add explicit null-safety validation to both the like
method (at the beginning, checking memberId and productId parameters) and the
unlike method (lines 29-30, checking the same parameters) in the LikeFacade
class. Use Objects.requireNonNull() to validate that both memberId and productId
are not null immediately upon method entry, throwing a NullPointerException with
descriptive messages if either parameter is null. This ensures validation
happens at the facade entry point rather than in lower service layers, enabling
faster error identification in production. Additionally, add unit tests that
verify exceptions are thrown immediately when calling like(null, 1L), like(1L,
null), unlike(null, 1L), and unlike(1L, null).
Source: Coding guidelines
Comment on lines
+83
to
+88
| public void evictAllProductLists() { | ||
| try { | ||
| Set<String> keys = redisTemplate.keys(LIST_KEY_PREFIX + "*"); | ||
| if (keys != null && !keys.isEmpty()) { | ||
| redisTemplate.delete(keys); | ||
| } |
There was a problem hiding this comment.
Line [85] RedisTemplate.keys 기반 전체 키 스캔은 트래픽 시 Redis 지연을 유발한다.
운영 관점에서 KEYS product:list:*는 키 개수에 비례해 Redis를 블로킹할 수 있어, 좋아요/수정/삭제 시 무효화가 자주 발생하면 응답 지연과 장애 전파 위험이 커진다. SCAN 기반 점진 삭제 또는 버전드 네임스페이스(예: product:list:v{n}:...)로 전환해 O(N) 블로킹을 피하는 수정이 필요하다. 추가로 대량 키(예: 10k 이상) 조건에서 무효화가 타임아웃 없이 완료되는 테스트를 ProductCacheServiceTest에 보강해야 한다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheService.java`
around lines 83 - 88, The evictAllProductLists method uses synchronous
redisTemplate.keys() which blocks Redis and causes performance issues during
high traffic. Replace the blocking KEYS operation with a SCAN-based approach
that progressively deletes keys in batches without blocking Redis, or implement
a versioned namespace strategy (e.g., product:list:v{n}:*) to atomically
invalidate caches. Additionally, add tests in ProductCacheServiceTest to verify
that cache invalidation completes within acceptable time limits even with large
key volumes (10k or more keys).
Comment on lines
57
to
+59
| public List<ProductDetailInfo> getProductsWithDetail(Long brandId, ProductSortType sort) { | ||
| return productService.getActiveProducts(brandId).stream() | ||
| Optional<List<ProductDetailInfo>> cached = productCacheService.getProductList(brandId, sort); | ||
| if (cached.isPresent()) { |
There was a problem hiding this comment.
Line [57] sort null 입력 시 조회 경로가 500으로 실패할 수 있다.
운영 관점에서 sort가 null로 유입되면 캐시 키 생성(sort.name()) 또는 정렬 변환 경로에서 NPE가 발생해 요청이 실패한다. 진입점에서 sort 기본값을 ProductSortType.LATEST로 정규화하거나, 명시적 검증 예외로 실패 원인을 고정하는 수정이 필요하다. 추가로 sort == null 요청이 기본 정렬로 정상 처리되는 테스트를 ProductFacadeTest에 추가해야 한다.
As per coding guidelines, "**/*.java: Ensure null-safety in all code" 규칙에 따라 입력 경계에서 null-safe 처리가 필요하다.
Also applies to: 63-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`
around lines 57 - 59, The getProductsWithDetail method in ProductFacade does not
validate null input for the sort parameter, which can cause NullPointerException
when sort.name() is called in cache key generation or during sorting logic. Add
null-safety handling by either normalizing a null sort parameter to a default
value like ProductSortType.LATEST at the method entry point, or by throwing an
explicit validation exception. Additionally, add test cases in ProductFacadeTest
to verify that null sort inputs are handled correctly and default to the
expected sorting behavior.
Source: Coding guidelines
Comment on lines
+58
to
+61
| Optional<List<ProductDetailInfo>> cached = productCacheService.getProductList(brandId, sort); | ||
| if (cached.isPresent()) { | ||
| return cached.get(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) 브랜드 상태 변경/삭제 유스케이스 후보 식별
rg -n -C2 --type=java '\b(suspend|unsuspend|delete|remove)\w*\s*\(' apps/commerce-api/src/main/java/com/loopers | rg -n 'Brand|brand'
# 2) 캐시 무효화 호출 위치 확인 (브랜드 유스케이스와 연결 여부 확인)
rg -n -C3 --type=java 'productCacheService\.evict(AllProductLists|ProductDetail)\(' apps/commerce-api/src/main/java/com/loopers
# 3) ProductFacade 캐시 히트 경로 재확인
sed -n '57,85p' apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaRepository: loopers-labs/loop-pack-be-l2-vol4-java
Length of output: 9512
🏁 Script executed:
# 1) BrandService의 deleteBrand/suspendBrand 전체 구현 확인
sed -n '46,60p' apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.java
# 2) productService.getActiveProducts() 구현 확인
rg -n -A10 'getActiveProducts\(' apps/commerce-api/src/main/java/com/loopers --type=java | head -30
# 3) 이벤트 기반 캐시 무효화 메커니즘 확인
rg -n -C3 'EventListener|`@Event`|publish' apps/commerce-api/src/main/java/com/loopers --type=java | rg -i 'brand|cache' | head -20
# 4) ProductCacheService 인터페이스 및 구현 확인
find apps/commerce-api -name 'ProductCacheService.java' -o -name '*CacheService*.java' | xargs catRepository: loopers-labs/loop-pack-be-l2-vol4-java
Length of output: 12129
🏁 Script executed:
# 1) 이벤트 리스너나 발행자 패턴 검색
rg -n '`@EventListener`|ApplicationEvent|publisher|event' apps/commerce-api/src/main/java --type=java | head -30
# 2) 브랜드 관련 통합 테스트 검색
fd -e java -path '*test*' | xargs rg -l 'Brand.*cache|cache.*Brand|suspension' --type=java
# 3) ProductFacade의 캐시 관련 통합 테스트 확인
fd -e java -path '*test*' -name '*ProductFacade*' -o -path '*test*' -name '*ProductIntegration*'
# 4) 브랜드 상태 변경 후 상품 조회를 검증하는 테스트
rg -n -B5 -A10 'suspendBrand|deleteBrand' apps/commerce-api/src --type=java | grep -i testRepository: loopers-labs/loop-pack-be-l2-vol4-java
Length of output: 420
🏁 Script executed:
# 1) 이벤트 리스너나 발행자 검색
rg -n '`@EventListener`|ApplicationEvent|Event' apps/commerce-api/src/main/java --type=java | head -20
# 2) 테스트 파일 찾기
find apps/commerce-api/src -name '*Test.java' -o -name '*IntegrationTest.java' | head -20
# 3) BrandService 테스트에서 cache 관련 테스트 확인
rg -n -B3 -A8 'deleteBrand|suspendBrand' apps/commerce-api/src/test --type=java | head -40
# 4) ProductFacade 테스트 파일 확인
find apps/commerce-api/src/test -name '*ProductFacade*'Repository: loopers-labs/loop-pack-be-l2-vol4-java
Length of output: 1836
🏁 Script executed:
# ProductFacadeTest 파일 검사
wc -l apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.java
# BrandService 테스트 파일 찾기
find apps/commerce-api/src/test -name '*Brand*Test*.java'
# ProductFacadeTest 내용 확인 (특히 캐시 관련)
grep -n 'cache\|brand\|suspend' apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.java -iRepository: loopers-labs/loop-pack-be-l2-vol4-java
Length of output: 3777
🏁 Script executed:
# ProductFacadeTest의 캐시 히트 시나리오 상세 확인
sed -n '67,98p' apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.java
# Brand 상태 변경 시 product 캐시 무효화를 테스트하는 부분이 있는지 확인
grep -n 'suspend\|delete' apps/commerce-api/src/test/java/com/loopers/application/brand --type=java -r
# BrandService 테스트 파일 확인
find apps/commerce-api/src/test -name '*Brand*' -type fRepository: loopers-labs/loop-pack-be-l2-vol4-java
Length of output: 1786
🏁 Script executed:
# BrandService 테스트 파일 찾기 (다시 시도)
find apps/commerce-api/src/test -type f -name '*Brand*.java'
# BrandService 테스트에서 브랜드 상태 변경 테스트 확인
rg -n 'suspendBrand|deleteBrand' apps/commerce-api/src/test -A5 -B2
# ProductFacade 통합 테스트 파일에서 브랜드 상태 + 캐시 테스트
sed -n '100,120p' apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.javaRepository: loopers-labs/loop-pack-be-l2-vol4-java
Length of output: 1218
🏁 Script executed:
# BrandService 테스트 파일 위치 재확인
find apps/commerce-api -path '*test*' -name '*Brand*.java' -type f
# BrandService 소스 파일 완전 확인 (메서드 전체)
sed -n '1,100p' apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.java
# 최종 확인: 브랜드 관련 모든 public 메서드에서 cache 호출 여부
rg -n 'productCacheService|evict' apps/commerce-api/src/main/java/com/loopers/domain/brand --type=javaRepository: loopers-labs/loop-pack-be-l2-vol4-java
Length of output: 1772
캐시 히트 경로에서 브랜드 상태 검증 누락으로 인한 데이터 일관성 문제
ProductFacade.getProductsWithDetail() 58-61줄의 캐시 히트 경로에서는 brandService.findBrand() 호출 없이 캐시된 데이터를 직접 반환한다. 반면 캐시 미스 시(63-66줄)는 toDetailInfo() 메서드에서 브랜드 상태를 검증한다.
운영 관점에서, 브랜드 중지/삭제 후 캐시 TTL(3분) 동안 정책상 비노출되어야 할 상품 목록이 반환되는 문제가 발생한다. BrandService.suspendBrand(), deleteBrand(), reinstateBrand() 메서드에서 productCacheService.evictAllProductLists() 호출이 완전히 누락되어 있다.
수정 방안:
BrandService에ProductCacheService의존성 추가suspendBrand(),deleteBrand(),reinstateBrand()메서드 마지막에productCacheService.evictAllProductLists()호출 추가- 브랜드 상태 변경 후 상품 목록 캐시가 무효화되는지 검증하는 통합 테스트 작성 (브랜드 중지 → 캐시 무효화 확인 → 상품 목록 재조회 시 빈 목록 또는 오류 반환 검증)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`
around lines 58 - 61, The cache hit path in the getProductsWithDetail method
bypasses brand status validation and returns cached data directly, while the
cache miss path validates brand status through the toDetailInfo method. This
creates a data consistency issue where suspended or deleted brands can still
serve cached product lists for up to the TTL duration. To fix this, inject
ProductCacheService as a dependency into BrandService, then add
productCacheService.evictAllProductLists() calls at the end of the suspendBrand,
deleteBrand, and reinstateBrand methods to ensure the cache is invalidated
whenever brand status changes. Additionally, write an integration test that
verifies the entire flow: suspend a brand, confirm the cache is cleared, then
verify that re-querying the product list either returns empty results or
properly reflects the brand's suspended status.
Comment on lines
+16
to
22
| public boolean like(Long memberId, Long productId) { | ||
| if (likeRepository.findActiveLike(memberId, productId).isPresent()) { | ||
| return false; | ||
| } | ||
| likeRepository.save(new LikeModel(memberId, productId)); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
재좋아요/동시요청에서 boolean 계약이 깨질 수 있다
Line 16-22는 active like만 확인하고 새 row를 insert하므로, soft delete 이력이 있는 동일 (member_id, product_id)에서 유니크 충돌이 발생해 운영 시 500으로 실패할 수 있다. 또한 동시 like 경쟁에서도 동일한 충돌로 false 대신 예외가 발생해 LikeFacade의 멱등 제어(카운트/캐시 갱신 조건)가 깨진다.
수정안은 “활성만 조회” 대신 동일 키 row를 포함 조회해 활성=true면 false 반환 / soft-deleted면 복구 후 true / 없으면 insert true로 처리하고, insert 경쟁 충돌은 graceful하게 false로 매핑하는 경로를 두는 것이다.
추가 테스트는 (1) unlike 후 like 재요청 시 성공/복구 검증, (2) 동시 like 2건에서 한 건만 상태변경되고 나머지는 예외 없이 멱등 응답되는 통합 테스트를 권장한다.
수정 방향 예시 diff
`@Transactional`
public boolean like(Long memberId, Long productId) {
- if (likeRepository.findActiveLike(memberId, productId).isPresent()) {
- return false;
- }
- likeRepository.save(new LikeModel(memberId, productId));
- return true;
+ return likeRepository.findByMemberIdAndProductId(memberId, productId)
+ .map(existing -> {
+ if (existing.isDeleted()) {
+ existing.restore();
+ return true;
+ }
+ return false;
+ })
+ .orElseGet(() -> {
+ likeRepository.save(new LikeModel(memberId, productId));
+ return true;
+ });
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/commerce-api/src/main/java/com/loopers/domain/like/LikeService.java`
around lines 16 - 22, The like method in LikeService is checking only active
likes but doesn't account for soft-deleted records, causing unique constraint
violations in two scenarios: when re-liking after unlike, and during concurrent
like requests. This breaks the idempotent contract expected by LikeFacade.
Modify the like method to query all records (including soft-deleted) instead of
just active ones, then implement branching logic: if an active like exists
return false, if a soft-deleted like exists update it to active and return true,
if no record exists insert a new one and return true. Additionally, wrap the
insert operation in a try-catch to gracefully handle unique constraint violation
exceptions by returning false instead of propagating the exception, ensuring the
method maintains its boolean contract under concurrent requests.
Comment on lines
+49
to
+58
| @DisplayName("이미 좋아요한 상품이면 likeCount 와 캐시를 변경하지 않는다. (멱등)") | ||
| @Test | ||
| void does_not_change_anything_when_already_liked() { | ||
| when(likeService.like(1L, 2L)).thenReturn(false); | ||
|
|
||
| likeFacade.like(1L, 2L); | ||
|
|
||
| verify(productService, never()).incrementLikeCount(2L); | ||
| verify(productCacheService, never()).evictProductDetail(2L); | ||
| } |
There was a problem hiding this comment.
멱등 시나리오에서 전체 리스트 캐시 무효화 미호출 검증이 빠져 있다
운영 관점에서 false 경로에서 evictAllProductLists()가 실수로 호출되면 캐시 churn으로 DB 부하가 급증하는데, 현재 테스트는 이를 잡지 못한다.
수정안으로 두 멱등 테스트에 verify(productCacheService, never()).evictAllProductLists();를 추가해야 한다.
추가 테스트는 현재 두 테스트를 확장하면 충분하며, 필요 시 verifyNoMoreInteractions(productService, productCacheService)로 부수효과를 더 엄격히 고정하는 것을 권장한다.
수정 예시(diff)
void does_not_change_anything_when_already_liked() {
when(likeService.like(1L, 2L)).thenReturn(false);
likeFacade.like(1L, 2L);
verify(productService, never()).incrementLikeCount(2L);
verify(productCacheService, never()).evictProductDetail(2L);
+ verify(productCacheService, never()).evictAllProductLists();
}
@@
void does_not_change_anything_when_no_active_like() {
when(likeService.unlike(1L, 2L)).thenReturn(false);
likeFacade.unlike(1L, 2L);
verify(productService, never()).decrementLikeCount(2L);
verify(productCacheService, never()).evictProductDetail(2L);
+ verify(productCacheService, never()).evictAllProductLists();
}Also applies to: 77-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/test/java/com/loopers/application/like/LikeFacadeTest.java`
around lines 49 - 58, The idempotent test case
does_not_change_anything_when_already_liked is missing a verification to ensure
that evictAllProductLists() on productCacheService is never called when a
product is already liked. Add verify(productCacheService,
never()).evictAllProductLists() to this test method, and apply the same
verification to the other idempotent test method mentioned in the comment.
Optionally, also add verifyNoMoreInteractions(productService,
productCacheService) at the end of both tests to strictly enforce that no other
unexpected interactions occur.
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.
🧭 Context & Decision
문제 정의
추가로, LIKES_DESC 정렬시 모든 상품의 좋아요 수를 메모리 올려서 Java Comparator 로 정렬하기 때문에 DB 인덱스 사용 불가능
선택지와 결정
고려한 대안:
A: LikeService 에서 ProductRepository 를 의존하여 @transactional 안에서 like_count 를 증감,
Like 도메인이 Product 도메인을 직접 참조하게 되어 도메인 간 결합이 발생
B: ProductFacade 에서 likeService.like() 호출 후 productService.incrementLikeCount()를 호출
C: LikeFacade 를 따로 만들어 LikeService 와 ProductService를 트랙젠션으로 묶어서 도메인 의존 없이 만들기
최종 결정: C
트레이드오프: 추가적으로 LikeFacade 를 추가해서 불필요한 클래스가 늘어나는 느낌이지만, 도메인 순수성과 트랜젝션 원자성을 확보할수있음.
추후 개선 여지(있다면): 좋아요 트래픽이 많아지면, LikeFacade 내부의 비관적락이 병목이 생길수있다. 이경우에는 비동기 이벤트 발행을 통해 쓰기 경합을 줄이는 방향으로 결정할예정
🤔 고민한 점 / 막혔던 부분
LikeService.like()의 반환 타입 변경이 필요했음, 기존에는 void 타입을 사용해서 LikeFacade가 "실제로 새 좋아요가 등록됐는지" 알 수 없었다. 이미 좋아요한 상태에서 다시 호출하면 멱등으로 무시되지만, like_count는 증가하면 안 된다. 이를 위해 boolean 반환으로 변경하여 실제 상태 변경 여부를 전달하도록 했다. unlike()도 동일한 이유로 boolean 반환으로 변경했다.
🙋 기타