[volume-5] 상품 조회 읽기 성능 최적화 - 정인철#195
Conversation
- ProductModel에 likeCount 필드 추가 (비정규화) - JPA @Modifying 쿼리로 SQL 레벨 Atomic Update (like_count +1/-1) - 좋아요 등록/취소 시 likeCount 동기화 (LikeFacade → ProductService) - 기존 COUNT 쿼리 의존 제거로 N+1 문제 해소 기반 마련
- Comparator 기반 정렬 → DB ORDER BY 전환 (ProductSortType.toSort()) - 브랜드 N+1 조회 → getByIds 배치 조회로 개선 - productLikeService.countByProductId() → product.getLikeCount() 전환 - buildAndSort 헬퍼 제거, 각 메서드에서 직접 조립
- 복합 인덱스 4개 추가 (brandId+likeCount, brandId+price, likeCount, price) - Redis Read-Through 캐시 구현 (상품 상세 TTL 10분, 목록 TTL 5분) - 좋아요 등록/취소 시 캐시 무효화 (detail + list) - KEYS 명령어 대신 ProductSortType.values() 기반 키 구성
📝 WalkthroughWalkthroughRedis 기반 제품 캐싱 레이어(ProductCacheService)를 도입하고, 제품 조회 시 캐시 적중/미스 처리를 추가하며, 좋아요 기능에서 캐시 무효화를 구현한다. 또한 제품 모델에 likeCount 필드와 원자적 증감 연산을 추가하고, 정렬 기능을 Spring Data Sort 기반으로 리팩토링한다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProductFacade
participant ProductCacheService
participant Redis
participant ProductRepository
participant Database
Client->>ProductFacade: getById(productId)
ProductFacade->>ProductCacheService: getProductDetail(productId)
ProductCacheService->>Redis: get(detail:{productId})
alt Cache Hit
Redis-->>ProductCacheService: ProductDetailInfo
ProductCacheService-->>ProductFacade: Optional<ProductDetailInfo>
ProductFacade-->>Client: ProductDetailInfo
else Cache Miss
Redis-->>ProductCacheService: null
ProductCacheService-->>ProductFacade: Optional.empty()
ProductFacade->>ProductRepository: findById(productId)
ProductRepository->>Database: SELECT * FROM product
Database-->>ProductRepository: ProductModel
ProductFacade->>ProductFacade: build ProductDetailInfo
ProductFacade->>ProductCacheService: setProductDetail(productId, info)
ProductCacheService->>Redis: set(detail:{productId}, info, TTL=10min)
Redis-->>ProductCacheService: OK
ProductFacade-->>Client: ProductDetailInfo
end
sequenceDiagram
participant Client
participant LikeFacade
participant ProductService
participant ProductRepository
participant Database
participant ProductCacheService
participant Redis
Client->>LikeFacade: addLike(userId, productId)
LikeFacade->>ProductService: increaseLikeCount(productId)
ProductService->>ProductRepository: increaseLikeCount(productId)
ProductRepository->>Database: UPDATE product SET likeCount = likeCount + 1
Database-->>ProductRepository: success
ProductRepository-->>ProductService: void
ProductService-->>LikeFacade: void
LikeFacade->>ProductCacheService: evictProductDetail(productId)
ProductCacheService->>Redis: del(detail:{productId})
LikeFacade->>ProductCacheService: evictProductList()
ProductCacheService->>Redis: del(list:*) for all sort types
Redis-->>ProductCacheService: OK
ProductCacheService-->>LikeFacade: void
LikeFacade-->>Client: Like added & caches invalidated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 다수의 계층(캐싱, 도메인, 인프라, 애플리케이션)에서 밀접한 상호작용을 수행하는 주요 기능 변경이다. ProductCacheService의 Redis 기반 구현 검증, 캐시 무효화 시점의 정확성 검증, 동시성 시나리오에서의 캐시 일관성 보장, like count 원자성 검증 등이 필요하다. Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/commerce-api/src/test/java/com/loopers/domain/like/LikeConcurrencyTest.java (1)
61-81:⚠️ Potential issue | 🟡 MinorExecutorService가 종료되지 않아 테스트 리소스 누수가 발생한다.
Executors.newFixedThreadPool(threadCount)로 생성한 ExecutorService가 테스트 종료 후 shutdown되지 않는다. 테스트가 반복 실행되면 스레드 풀이 누적된다.🔧 ExecutorService 종료 추가
latch.await(); + executor.shutdown(); // then: 좋아요 수 == 성공 수, 전원 성공🤖 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 61 - 81, The test creates an ExecutorService via Executors.newFixedThreadPool(threadCount) in LikeConcurrencyTest but never shuts it down, leaking threads across test runs; fix by shutting down the executor after work completes: after latch.await(...) call executor.shutdown() and awaitTermination (with a timeout) and if termination fails call executor.shutdownNow() (or place shutdown in a finally block) to ensure the ExecutorService created in the test is always terminated.apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeUnitTest.java (1)
79-93:⚠️ Potential issue | 🟡 Minor캐시 스텁 누락으로 테스트가 NPE로 실패할 수 있다.
failWithNotFoundProduct테스트에서productCacheService.getProductDetail(999L)스텁이 없다. Mockito는 스텁되지 않은Optional반환 타입에 대해null을 반환하므로,ProductFacade.getById()에서cached.isPresent()호출 시 NPE가 발생한다.🔧 캐시 미스 스텁 추가
`@Test` void failWithNotFoundProduct() { // given + when(productCacheService.getProductDetail(999L)).thenReturn(Optional.empty()); when(productService.getById(999L)) .thenThrow(new CoreException(ErrorType.NOT_FOUND, "존재하지 않는 상품입니다."));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeUnitTest.java` around lines 79 - 93, The test failWithNotFoundProduct is missing a stub for productCacheService.getProductDetail(999L) causing an NPE when ProductFacade.getById() calls cached.isPresent(); add a Mockito stub in the test (for the ProductFacadeUnitTest.failWithNotFoundProduct method) to return Optional.empty() for productCacheService.getProductDetail(999L) before invoking productFacade.getById(999L) so the cache miss is simulated and the service-layer CoreException behaviour is asserted.
🧹 Nitpick comments (8)
apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceUnitTest.java (1)
95-130: 정렬 해피패스가 중복되고, 새 likeCount 서비스 경로 검증이 비어 있다.현재 추가된 두 테스트는 모두
getByBrandId(brandId, sort)의 정상 반환만 반복 확인하고 있어 유지보수 비용만 늘고 회귀 탐지 폭은 넓어지지 않는다. 반면 이번 PR에서 새로 공개된increaseLikeCount()와decreaseLikeCount()는 서비스에서 바로 리포지토리 위임만 하므로, 메서드 연결이 바뀌거나 누락돼도 이 파일에서는 잡히지 않는다. 정렬 검증은 하나의 대표 케이스로 정리하고, 대신 두 신규 메서드가 각각 올바른 리포지토리 메서드를 정확히 1회 호출하는 테스트를 추가하는 편이 낫다. 추가로 서비스 계약을0건 갱신 시 예외로 가져갈 계획이라면 그 실패 케이스까지 함께 넣어 두는 것이 좋다. 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/ProductServiceUnitTest.java` around lines 95 - 130, Tests duplicate the same "happy path" for getByBrandId and miss verifying the new like-count methods; refactor by keeping one representative sort test for productService.getByBrandId(brandId, sort) that verifies productRepository.findAllByBrandId(...) is called with the Sort, then add two new unit tests for productService.increaseLikeCount(productId) and productService.decreaseLikeCount(productId) that each verify the correct repository method is invoked exactly once (e.g., verify(productRepository).increaseLikeCount(productId) / verify(productRepository).decreaseLikeCount(productId)); optionally add a failure-case test for when the repository update affects 0 rows to assert the service throws the expected exception.apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java (1)
7-7: 애플리케이션 레이어에서 인프라스트럭처 레이어 직접 의존은 아키텍처 위반이다.
ProductCacheService는infrastructure패키지에 위치하나,LikeFacade는application레이어이다. 클린 아키텍처 원칙상 애플리케이션 레이어는 도메인 레이어의 인터페이스에만 의존해야 한다.수정안: 도메인 레이어에
ProductCachePort인터페이스를 정의하고,ProductCacheService가 이를 구현하도록 변경한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java` at line 7, LikeFacade currently imports and depends on ProductCacheService from the infrastructure layer, violating clean architecture; create a new domain-layer interface ProductCachePort (define required methods used by LikeFacade), change LikeFacade to depend on ProductCachePort instead of ProductCacheService, update the infrastructure class ProductCacheService to implement ProductCachePort, and adjust DI/bean configuration to inject ProductCacheService as the ProductCachePort implementation so the application layer only depends on the domain port.apps/commerce-api/src/test/java/com/loopers/domain/like/LikeConcurrencyTest.java (1)
71-73: 테스트가 프로덕션 코드(LikeFacade) 흐름과 다르다.프로덕션에서는
LikeFacade.addLike()가productLikeService.addLike()와productService.increaseLikeCount()를 하나의 트랜잭션에서 호출한다. 그러나 이 테스트는 각 스레드에서 두 메서드를 별도로 호출하여 트랜잭션 경계가 다르다.운영 관점: 실제 동시성 이슈는 트랜잭션 경계와 락 획득 순서에서 발생할 수 있으므로,
LikeFacade를 직접 호출하는 통합 테스트가 더 유의미하다.🧪 LikeFacade를 사용하는 통합 테스트 예시
`@Autowired` private LikeFacade likeFacade; `@Test` void concurrentAddLikeViaFacade() throws InterruptedException { // given: 회원 10명 생성 // ... // when: LikeFacade.addLike() 동시 호출 for (int i = 0; i < threadCount; i++) { String loginId = "user" + i; executor.submit(() -> { try { likeFacade.addLike(loginId, password, productId); successCount.incrementAndGet(); } catch (Exception e) { failCount.incrementAndGet(); } finally { latch.countDown(); } }); } // ... }🤖 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 71 - 73, The test in LikeConcurrencyTest calls productLikeService.addLike(...) and productService.increaseLikeCount(...) separately per thread, which differs from production where LikeFacade.addLike(...) invokes both within a single transactional boundary; update the test to call LikeFacade.addLike(...) (e.g., inject LikeFacade into the test and invoke likeFacade.addLike(loginId, password, productId) concurrently) so the transaction and lock ordering match production behavior, and remove the direct calls to productLikeService.addLike and productService.increaseLikeCount in the threaded execution.apps/commerce-api/src/test/java/com/loopers/application/like/LikeFacadeUnitTest.java (2)
38-43: @mock 선언이@InjectMocks이후에 위치하여 가독성이 저하된다.Mockito는 선언 순서와 무관하게 모든
@Mock필드를 주입하나, 관례상@Mock선언을@InjectMocks이전에 배치하는 것이 가독성 측면에서 좋다.♻️ Mock 선언 순서 정리
`@Mock` private ProductService productService; + `@Mock` + private ProductCacheService productCacheService; + `@InjectMocks` private LikeFacade likeFacade; - - `@Mock` - private ProductCacheService productCacheService;🤖 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/application/like/LikeFacadeUnitTest.java` around lines 38 - 43, The `@Mock` fields should be declared before the `@InjectMocks` field to improve readability and follow common Mockito conventions; move the declaration of productCacheService (and any other `@Mock` annotated fields) so they appear above the LikeFacade likeFacade field annotated with `@InjectMocks`, ensuring the test class's mock declarations precede the injection target (LikeFacade) for clearer structure.
151-169: removeLike 실패 시 캐시 무효화가 호출되지 않음을 검증하는 테스트가 누락되었다.
addLike실패 시나리오(Line 100-104)에서는 캐시 무효화가 호출되지 않음을 검증하나,removeLike실패 시나리오(failWithNotLiked)에서는 이 검증이 없다.🧪 캐시 무효화 비호출 검증 추가
// then assertThat(result.getErrorType()).isEqualTo(ErrorType.NOT_FOUND); + + // 실패 시 캐시 무효화도 호출되면 안 됨 + verify(productCacheService, never()).evictProductDetail(10L); + verify(productCacheService, never()).evictProductList(); }🤖 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/application/like/LikeFacadeUnitTest.java` around lines 151 - 169, The test failWithNotLiked currently asserts the CoreException but misses verifying that the cache invalidation is not called on failure; update the test to assert that the cache invalidation mock is never invoked when likeFacade.removeLike("testuser", "password1!@", 10L) throws—use Mockito.verify(<your cache mock>, never()).invalidateLikeCache(10L) (or the actual cache invalidation method name used in the codebase) after asserting the exception; keep existing mocks (memberService.getMyInfo, productService.getById, productLikeService.removeLike) unchanged.apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeUnitTest.java (1)
119-140: 목록 조회 테스트에서 캐시 관련 스텁이 누락되었다.
sortByPriceAsc테스트에서productCacheService.getProductList(...)스텁이 없다. 캐시 미스 시나리오를 명시적으로 스텁해야 테스트 의도가 명확해진다.🔧 캐시 미스 스텁 추가
void sortByPriceAsc() { // given ProductModel expensive = new ProductModel(1L, "에어맥스", "러닝화", 200000, 100, null); ReflectionTestUtils.setField(expensive, "id", 10L); ProductModel cheap = new ProductModel(1L, "에어포스", "캐주얼화", 100000, 50, null); ReflectionTestUtils.setField(cheap, "id", 11L); BrandModel brand = new BrandModel("나이키", "스포츠 브랜드", "https://example.com/nike.png"); ReflectionTestUtils.setField(brand, "id", 1L); + when(productCacheService.getProductList(eq("PRICE_ASC"), eq(null))).thenReturn(Optional.empty()); when(productService.getAll(any(Sort.class))).thenReturn(List.of(cheap, expensive)); when(brandService.getByIds(Set.of(1L))).thenReturn(Map.of(1L, brand));🤖 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/application/product/ProductFacadeUnitTest.java` around lines 119 - 140, The test sortByPriceAsc is missing a cache-miss stub for productCacheService.getProductList(...) so the intent (cache miss leading to productService.getAll call) isn't explicit; add a stub like when(productCacheService.getProductList(any())).thenReturn(Optional.empty()) (or the appropriate empty value your cache method returns) before calling productFacade.getProducts(ProductSortType.PRICE_ASC) so the test explicitly simulates a cache miss and exercises productService.getAll and brandService.getByIds as intended.apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java (2)
54-83: 목록 캐싱 시 10만건 전체 반환은 캐시 효과가 미미할 수 있다.PR 설명에도 명시된 대로 "목록 전체 반환(10만건)으로 캐시 효과 미미"하다. 향후 페이지네이션 적용 시 캐시 키 전략 재설계가 필요하다.
운영 관점: 10만건 JSON 직렬화/역직렬화 비용, Redis 메모리 사용량, 네트워크 전송 비용이 DB 조회 비용보다 클 수 있다. 캐시 효과를 측정하고 비용 대비 이점이 없다면 목록 캐싱을 제거하는 것도 검토가 필요하다.
🤖 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/ProductFacade.java` around lines 54 - 83, The current getProducts method caches and returns the entire product list (~100k items), which is inefficient; update the caching strategy in ProductFacade.getProducts to avoid caching full lists by either (a) removing list-level caching and rely on DB for full-list requests (remove productCacheService.getProductList / setProductList usage for full fetchs), or (b) switch to paginated caching by adding pagination parameters to getProducts and using those params in productCacheService.getProductList/setProductList keys, or (c) add a size threshold guard (e.g., if products.size() > N then do not cache) so you still use productService.getAll for DB fetches but only cache safe-sized results; adjust related symbols productCacheService.getProductList, productCacheService.setProductList, productService.getAll and the ProductFacade.getProducts signature/logic accordingly.
8-8: 애플리케이션 레이어에서 인프라스트럭처 레이어 직접 의존은 아키텍처 위반이다.
LikeFacade에서 이미 지적한 것과 동일한 이슈이다.ProductCacheService는infrastructure패키지에 위치하나,ProductFacade는application레이어이다.수정안: 도메인 레이어에 캐시 포트 인터페이스를 정의하고, 인프라스트럭처 레이어에서 구현하도록 변경한다.
🤖 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/ProductFacade.java` at line 8, ProductFacade currently depends directly on the infrastructure class ProductCacheService which violates layer boundaries; define a cache port interface (e.g., ProductCachePort) in the domain layer, change ProductFacade to depend on and use that interface (inject via constructor) and remove the import of com.loopers.infrastructure.product.ProductCacheService, then implement ProductCachePort inside the infrastructure package (class implements ProductCachePort) and wire that implementation into the application context/config so ProductFacade receives the infra implementation at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java`:
- Around line 21-31: The cache eviction inside the `@Transactional` method addLike
risks DB/cache inconsistency on rollback; refactor by publishing a
ProductLikeChangedEvent (e.g., record ProductLikeChangedEvent(Long productId))
from LikeFacade.addLike via applicationEventPublisher.publishEvent(...) after
performing productLikeService.addLike and productService.increaseLikeCount, and
move the cache invalidation into an event listener method onLikeChanged
annotated with `@TransactionalEventListener`(phase =
TransactionPhase.AFTER_COMMIT) which calls
productCacheService.evictProductDetail(productId) and
productCacheService.evictProductList().
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`:
- Around line 71-77: The stream mapping in ProductFacade uses
brandMap.get(product.getBrandId()) which can be null and leads to NPE inside
ProductDetailInfo.of(product, brand, ...); update the mapping to handle missing
brands: either filter out products with null brand before mapping (e.g., skip
when brand == null) or detect null and replace with a safe/default BrandModel or
throw a clear explicit exception with product id/context; modify the lambda
where brand is resolved (brandMap.get(...)) and ensure ProductDetailInfo.of is
only called with a non-null BrandModel or after handling the null case.
In `@apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.java`:
- Around line 31-35: getByIds currently returns only existing brands (via
findAllByIds) which can cause NPEs when callers (e.g., ProductFacade.getProducts
using brandMap.get(product.getBrandId()) and ProductDetailInfo.of) expect every
requested id; update getByIds to detect missing IDs by comparing the incoming
ids Set with the returned map keys and throw a clear exception (e.g.,
BrandNotFoundException) listing the missing IDs, or alternatively, if you prefer
the other approach, keep getByIds as-is and modify ProductFacade.getProducts to
null-check/filter products whose brandId is missing before calling
ProductDetailInfo.of (choose and implement one of these fixes and ensure the
unique symbols involved are getByIds, findAllByIds, ProductFacade.getProducts,
ProductDetailInfo.of, and brandMap.get).
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/brand/BrandRepositoryImpl.java`:
- Around line 28-31: ProductFacade.getProducts currently uses
BrandService.getByIds results returned via BrandRepositoryImpl.findAllByIds and
then calls ProductDetailInfo.of with brandMap.get(id), which can be null if some
requested brand IDs are missing; update ProductFacade.getProducts to either
validate that brandMap.size() == requestedBrandIds.size() and throw a clear
exception, or filter out/skip products whose brand lookup returns null before
calling ProductDetailInfo.of (ensuring you reference ProductFacade.getProducts,
BrandService.getByIds, BrandRepositoryImpl.findAllByIds, ProductDetailInfo.of
and brandMap.get), and add an integration test that creates a product
referencing a non-existent brandId to assert the chosen defensive behavior.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheService.java`:
- Around line 87-97: evictProductList() only deletes global list keys
(brandId=null) and misses brand-specific keys
(product:list:{sortType}:brand:{brandId}), causing stale brand lists; update
evictProductList() in ProductCacheService to also remove brand-specific keys by
scanning and deleting matching patterns for each ProductSortType (e.g.,
"product:list:{sortType}:brand:*") using a non-blocking SCAN via redisTemplate
(or RedisCallback/scan command) rather than KEYS, or alternatively enumerate
known brandIds and call buildListKey(sortType.name(), brandId) and delete those
keys; also add a test that caches a brand-specific list, updates likes, calls
evictProductList(), and asserts the brand-specific cache was removed
(references: evictProductList(), buildListKey(),
ProductFacade.getProductsByBrandId(), redisTemplate.delete).
- Around line 28-59: The code is missing Redis command timeout configuration
which can cause threads to hang during slow/failed Redis operations; update the
Lettuce client configuration (where LettuceClientConfiguration.builder() is
created) to add a commandTimeout (e.g., Duration.ofMillis(1000)) or add
spring.redis.timeout in redis.yml, and then verify ProductCacheService methods
(getProductDetail, setProductDetail, evictProductDetail) rely on that client so
blocking calls to redisTemplate.opsForValue().get/set/delete will honor the
timeout; optionally plan for Resilience4j circuit-breaker integration later.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.java`:
- Around line 22-28: The JPQL bulk updates leave the persistence context stale
and provide no feedback on affected rows; change the repository methods
increaseLikeCount and decreaseLikeCount to use `@Modifying`(clearAutomatically =
true, flushAutomatically = true) and return int instead of void so callers can
see how many rows were updated, and update any callers (e.g., removeLike/addLike
flows) or tests to handle the int result; also add/adjust an integration test
that loads a ProductModel via the service, calls the repository update, then
re-reads the entity in the same transaction to assert the updated likeCount is
visible.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/like/LikeConcurrencyTest.java`:
- Line 43: The test currently registers a ProductModel with a hard-coded brandId
(productService.register(1L,...)) which can pass because ProductModel.brandId
lacks a FK constraint; instead inject and use BrandService in the test setUp():
call brandService.register(...) to create a real BrandModel, then pass
brand.getId() into productService.register(...) so product creation uses an
existing brand; update the test class to autowire BrandService, create the brand
in setUp(), and replace the hard-coded 1L with brand.getId() when setting
productId.
---
Outside diff comments:
In
`@apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeUnitTest.java`:
- Around line 79-93: The test failWithNotFoundProduct is missing a stub for
productCacheService.getProductDetail(999L) causing an NPE when
ProductFacade.getById() calls cached.isPresent(); add a Mockito stub in the test
(for the ProductFacadeUnitTest.failWithNotFoundProduct method) to return
Optional.empty() for productCacheService.getProductDetail(999L) before invoking
productFacade.getById(999L) so the cache miss is simulated and the service-layer
CoreException behaviour is asserted.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/like/LikeConcurrencyTest.java`:
- Around line 61-81: The test creates an ExecutorService via
Executors.newFixedThreadPool(threadCount) in LikeConcurrencyTest but never shuts
it down, leaking threads across test runs; fix by shutting down the executor
after work completes: after latch.await(...) call executor.shutdown() and
awaitTermination (with a timeout) and if termination fails call
executor.shutdownNow() (or place shutdown in a finally block) to ensure the
ExecutorService created in the test is always terminated.
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.java`:
- Line 7: LikeFacade currently imports and depends on ProductCacheService from
the infrastructure layer, violating clean architecture; create a new
domain-layer interface ProductCachePort (define required methods used by
LikeFacade), change LikeFacade to depend on ProductCachePort instead of
ProductCacheService, update the infrastructure class ProductCacheService to
implement ProductCachePort, and adjust DI/bean configuration to inject
ProductCacheService as the ProductCachePort implementation so the application
layer only depends on the domain port.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`:
- Around line 54-83: The current getProducts method caches and returns the
entire product list (~100k items), which is inefficient; update the caching
strategy in ProductFacade.getProducts to avoid caching full lists by either (a)
removing list-level caching and rely on DB for full-list requests (remove
productCacheService.getProductList / setProductList usage for full fetchs), or
(b) switch to paginated caching by adding pagination parameters to getProducts
and using those params in productCacheService.getProductList/setProductList
keys, or (c) add a size threshold guard (e.g., if products.size() > N then do
not cache) so you still use productService.getAll for DB fetches but only cache
safe-sized results; adjust related symbols productCacheService.getProductList,
productCacheService.setProductList, productService.getAll and the
ProductFacade.getProducts signature/logic accordingly.
- Line 8: ProductFacade currently depends directly on the infrastructure class
ProductCacheService which violates layer boundaries; define a cache port
interface (e.g., ProductCachePort) in the domain layer, change ProductFacade to
depend on and use that interface (inject via constructor) and remove the import
of com.loopers.infrastructure.product.ProductCacheService, then implement
ProductCachePort inside the infrastructure package (class implements
ProductCachePort) and wire that implementation into the application
context/config so ProductFacade receives the infra implementation at runtime.
In
`@apps/commerce-api/src/test/java/com/loopers/application/like/LikeFacadeUnitTest.java`:
- Around line 38-43: The `@Mock` fields should be declared before the `@InjectMocks`
field to improve readability and follow common Mockito conventions; move the
declaration of productCacheService (and any other `@Mock` annotated fields) so
they appear above the LikeFacade likeFacade field annotated with `@InjectMocks`,
ensuring the test class's mock declarations precede the injection target
(LikeFacade) for clearer structure.
- Around line 151-169: The test failWithNotLiked currently asserts the
CoreException but misses verifying that the cache invalidation is not called on
failure; update the test to assert that the cache invalidation mock is never
invoked when likeFacade.removeLike("testuser", "password1!@", 10L) throws—use
Mockito.verify(<your cache mock>, never()).invalidateLikeCache(10L) (or the
actual cache invalidation method name used in the codebase) after asserting the
exception; keep existing mocks (memberService.getMyInfo, productService.getById,
productLikeService.removeLike) unchanged.
In
`@apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeUnitTest.java`:
- Around line 119-140: The test sortByPriceAsc is missing a cache-miss stub for
productCacheService.getProductList(...) so the intent (cache miss leading to
productService.getAll call) isn't explicit; add a stub like
when(productCacheService.getProductList(any())).thenReturn(Optional.empty()) (or
the appropriate empty value your cache method returns) before calling
productFacade.getProducts(ProductSortType.PRICE_ASC) so the test explicitly
simulates a cache miss and exercises productService.getAll and
brandService.getByIds as intended.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/like/LikeConcurrencyTest.java`:
- Around line 71-73: The test in LikeConcurrencyTest calls
productLikeService.addLike(...) and productService.increaseLikeCount(...)
separately per thread, which differs from production where
LikeFacade.addLike(...) invokes both within a single transactional boundary;
update the test to call LikeFacade.addLike(...) (e.g., inject LikeFacade into
the test and invoke likeFacade.addLike(loginId, password, productId)
concurrently) so the transaction and lock ordering match production behavior,
and remove the direct calls to productLikeService.addLike and
productService.increaseLikeCount in the threaded execution.
In
`@apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceUnitTest.java`:
- Around line 95-130: Tests duplicate the same "happy path" for getByBrandId and
miss verifying the new like-count methods; refactor by keeping one
representative sort test for productService.getByBrandId(brandId, sort) that
verifies productRepository.findAllByBrandId(...) is called with the Sort, then
add two new unit tests for productService.increaseLikeCount(productId) and
productService.decreaseLikeCount(productId) that each verify the correct
repository method is invoked exactly once (e.g.,
verify(productRepository).increaseLikeCount(productId) /
verify(productRepository).decreaseLikeCount(productId)); optionally add a
failure-case test for when the repository update affects 0 rows to assert the
service throws the expected exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54affb50-c631-4b02-9f0d-f537bb6ba088
📒 Files selected for processing (17)
apps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductSortType.javaapps/commerce-api/src/main/java/com/loopers/domain/brand/BrandRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.javaapps/commerce-api/src/main/java/com/loopers/domain/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/brand/BrandRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheService.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/LikeFacadeUnitTest.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeUnitTest.javaapps/commerce-api/src/test/java/com/loopers/domain/like/LikeConcurrencyTest.javaapps/commerce-api/src/test/java/com/loopers/domain/product/ProductModelTest.javaapps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceUnitTest.java
| @Transactional(readOnly = true) | ||
| public Map<Long, BrandModel> getByIds(Set<Long> ids) { | ||
| return brandRepository.findAllByIds(ids).stream() | ||
| .collect(Collectors.toMap(BrandModel::getId, Function.identity())); | ||
| } |
There was a problem hiding this comment.
존재하지 않는 브랜드 ID 요청 시 NPE 발생 가능성이 있다.
findAllByIds는 존재하지 않는 ID를 무시하고 반환한다. ProductFacade.getProducts() (Context snippet 2, Line 74)에서 brandMap.get(product.getBrandId())가 null을 반환하면, ProductDetailInfo.of(product, null, count) 호출 시 brand.getName() 접근에서 NPE가 발생한다.
운영 관점: 브랜드가 삭제되었으나 상품이 남아있는 데이터 불일치 상황에서 전체 목록 API가 500 에러를 반환하게 된다.
수정안: 두 가지 방안 중 선택이 필요하다.
- 요청한 ID와 반환된 결과의 ID를 비교하여 누락된 경우 예외를 던진다.
- 호출부에서 null 체크 후 필터링하거나 기본값을 사용한다.
🛡️ 방안 1: 누락된 브랜드에 대해 예외 발생
`@Transactional`(readOnly = true)
public Map<Long, BrandModel> getByIds(Set<Long> ids) {
- return brandRepository.findAllByIds(ids).stream()
- .collect(Collectors.toMap(BrandModel::getId, Function.identity()));
+ List<BrandModel> brands = brandRepository.findAllByIds(ids);
+ Map<Long, BrandModel> brandMap = brands.stream()
+ .collect(Collectors.toMap(BrandModel::getId, Function.identity()));
+
+ if (brandMap.size() != ids.size()) {
+ Set<Long> missing = ids.stream()
+ .filter(id -> !brandMap.containsKey(id))
+ .collect(Collectors.toSet());
+ throw new CoreException(ErrorType.NOT_FOUND,
+ "존재하지 않는 브랜드가 포함되어 있습니다: " + missing);
+ }
+ return brandMap;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/commerce-api/src/main/java/com/loopers/domain/brand/BrandService.java`
around lines 31 - 35, getByIds currently returns only existing brands (via
findAllByIds) which can cause NPEs when callers (e.g., ProductFacade.getProducts
using brandMap.get(product.getBrandId()) and ProductDetailInfo.of) expect every
requested id; update getByIds to detect missing IDs by comparing the incoming
ids Set with the returned map keys and throw a clear exception (e.g.,
BrandNotFoundException) listing the missing IDs, or alternatively, if you prefer
the other approach, keep getByIds as-is and modify ProductFacade.getProducts to
null-check/filter products whose brandId is missing before calling
ProductDetailInfo.of (choose and implement one of these fixes and ensure the
unique symbols involved are getByIds, findAllByIds, ProductFacade.getProducts,
ProductDetailInfo.of, and brandMap.get).
| @Override | ||
| public List<BrandModel> findAllByIds(Set<Long> ids) { | ||
| return brandJpaRepository.findAllById(ids); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: ProductFacade에서 brandMap.get() 결과가 null일 때의 처리 확인
# Expected: null 처리 또는 방어 로직 존재 확인
rg -n -A 5 'brandMap\.get' --type=javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 831
🏁 Script executed:
# ProductFacade 파일 전체 내용 확인 (라인 수 먼저 확인)
wc -l apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 171
🏁 Script executed:
# ProductFacade 라인 50-85 확인 (brandMap 생성 및 사용 맥락)
sed -n '50,85p' apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java | cat -nRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1526
🏁 Script executed:
# ProductDetailInfo.of() 메서드 서명 및 구현 확인
rg -n -A 8 'static.*ProductDetailInfo.*of\(' --type=javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1291
🏁 Script executed:
# brandMap 변수 생성 및 초기화 확인
rg -n -B 3 'brandMap\s*=' --type=java | head -30Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 642
🏁 Script executed:
# ProductDetailInfo.of() 메서드 전체 구현 확인
rg -n -A 15 'public static ProductDetailInfo of' --type=javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1696
🏁 Script executed:
# BrandService.getByIds() 메서드 구현 확인
rg -n -A 10 'getByIds' --type=java | grep -A 10 'BrandService\|public.*getByIds'Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1919
🏁 Script executed:
# Product 엔티티의 brandId 필드 및 FK 제약 확인
rg -n -B 2 -A 2 'brandId|@Column.*brand|@JoinColumn.*brand' apps/commerce-api/src/main/java/com/loopers/domain/product/ProductModel.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1007
ProductFacade.getProducts()에서 브랜드 조회 실패 시 NullPointerException 발생 위험이 있다.
BrandService.getByIds()는 존재하지 않는 브랜드 ID를 무시하고 찾은 것만 반환하므로, 데이터 불일치 발생 시 brandMap.get()이 null을 반환한다. ProductFacade 라인 25-26에서 null 브랜드를 ProductDetailInfo.of()에 전달하면, of() 메서드 내 brand.getName() 호출(ProductDetailInfo.java:24) 시 NullPointerException이 발생한다.
수정 방안:
- ProductFacade에서 brandMap과 실제 조회된 브랜드 수가 일치하는지 검증하거나, null 브랜드 필터링
- 또는 요청한 모든 브랜드 ID가 존재하도록 보장하는 데이터 무결성 관리
테스트 추가:
productId가 존재하지 않는 brandId를 참조하는 경우에 대한 통합 테스트를 작성하여 방어 로직 동작 확인
🤖 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/brand/BrandRepositoryImpl.java`
around lines 28 - 31, ProductFacade.getProducts currently uses
BrandService.getByIds results returned via BrandRepositoryImpl.findAllByIds and
then calls ProductDetailInfo.of with brandMap.get(id), which can be null if some
requested brand IDs are missing; update ProductFacade.getProducts to either
validate that brandMap.size() == requestedBrandIds.size() and throw a clear
exception, or filter out/skip products whose brand lookup returns null before
calling ProductDetailInfo.of (ensuring you reference ProductFacade.getProducts,
BrandService.getByIds, BrandRepositoryImpl.findAllByIds, ProductDetailInfo.of
and brandMap.get), and add an integration test that creates a product
referencing a non-existent brandId to assert the chosen defensive behavior.
📌 Summary
filesort전 케이스 제거. Redis Read-Through 캐시 적용, 상품 상세 API 응답시간 1.7배 개선. 프로덕션 12개 / 테스트 5개 파일 변경.🧭 Context & Decision
문제 정의
COUNT쿼리로 조회(N+1), 정렬은 JavaComparator, 인덱스 없음, 캐시 없음선택 1. 비정규화 선택 과정
COUNT쿼리)like_count비정규화 (채택)왜 C를 선택했는가: 이커머스 상품 목록은 조회가 압도적으로 많고 좋아요 변경은 상대적으로 적습니다. 비정규화로 읽기 성능을 확보하고, 쓰기 시 정합성은 Atomic Update(
SET like_count = like_count + 1)로 보장했습니다. 다만 이 결정 때문에 Atomic Update 동기화, 캐시 무효화 복잡도, 잠재적 불일치 보정 같은 후속 과제가 따라왔습니다.선택 2. 캐시 무효화 전략 — 전체 evict vs 선택적 evict vs TTL 의존
왜 B를 선택했는가: 현재 서비스 규모에서는 정합성을 우선시했습니다. 좋아요 변경 시 정렬 순서가 바뀔 수 있으므로 목록 캐시를 전체 무효화하는 것이 안전하다고 판단했고, 트래픽이 늘면 TTL 의존 or 선택적 evict로 전환 가능하도록 설계했습니다.
트레이드오프
product:list:{sort}:brand:{brandId})는 키 조합이 많아 전체 evict가 어려워 TTL(5분) 만료에 의존하고 있습니다. 좋아요 변경 후 최대 5분간 stale 가능.@TransactionalEventListener로 커밋 후 선택적 evict 전환이 필요할 것 같습니다.선택 3. Redis 캐시 구현 —
RedisTemplate직접 사용 vs@Cacheable@Cacheable어노테이션RedisTemplate직접 사용 (채택)왜 B를 선택했는가: 처음 Redis 캐시를 적용하는 입장에서, Read-Through 패턴의 흐름(조회 → 미스 → DB → 저장)을 코드로 직접 보면서 이해하는 것이 학습에 더 효과적이라고 판단했습니다. 또한 상세 캐시(TTL 10분)와 목록 캐시(TTL 5분)의 TTL을 키 레벨에서 다르게 설정해야 했습니다.
🏗️ Design Overview
변경 범위
ProductModel.java— likeCount 비정규화 + 인덱스 4개,ProductRepository.java,ProductService.java,BrandRepository.java—findAllByIds추가,BrandService.java—getByIds배치 조회,BrandRepositoryImpl.javaProductJpaRepository.java— Atomic Update,ProductRepositoryImpl.java,ProductCacheService.java(신규)ProductFacade.java— Read-Through 캐시 + N+1 해결,ProductSortType.java— Sort 전환,LikeFacade.java— 캐시 무효화ProductModelTest.java,ProductServiceUnitTest.java,ProductFacadeUnitTest.java,LikeFacadeUnitTest.java,LikeConcurrencyTest.java1. Atomic Update — 동시성 안전한 likeCount 증감
like_count > 0조건으로 음수 방지. Atomic Update이므로 별도 락 없이도 동시성 안전.2. DB 정렬 전환 —
ProductSortType.toSort()기존 Java
Comparator정렬을 Spring DataSort로 전환하여 DBORDER BY가 실행되도록 변경. 복합 인덱스와 결합하면filesort없이 정렬된 결과를 반환하도록 설계했습니다.3. 복합 인덱스 4개 선언
복합 인덱스의 컬럼 순서는 왼쪽부터 사용 규칙에 따라 필터(
brand_id) → 정렬(like_count/price) 순서로 배치했습니다.4. Read-Through 캐시 — 상품 상세 조회
캐시 히트 시 DB를 거치지 않고 바로 반환하고, 캐시 미스 시 DB 조회 후 결과를 캐시에 저장하는 Read-Through 패턴입니다.
Redis 장애 시에도
ProductCacheService가Optional.empty()를 반환하므로 DB 폴백으로 정상 동작하도록 설계했습니다.5. 캐시 키 설계 + 목록 캐시 무효화
프로덕션에서
KEYS명령은 O(N) 블로킹으로 Redis 전체를 멈출 수 있습니다.ProductSortType.values()를 순회하여 삭제 대상 키를 직접 구성하는 방식으로 회피했고, 브랜드별 캐시(product:list:{sort}:brand:{brandId})는 TTL(5분) 만료에 의존하고 있습니다.6. 브랜드 배치 조회 — N+1 해결
상품 N개 조회 시 브랜드를 개별 조회(N+1)하던 구조를
findAllById배치 조회 1회로 전환했습니다.7. 좋아요 등록 시 캐시 무효화
좋아요 등록/취소 시 Atomic Update로
like_count를 변경한 뒤, 해당 상품 상세 캐시와 목록 캐시를 즉시 무효화하도록 했습니다. 다음 조회 요청에서 캐시 미스가 발생하면 DB에서 최신 데이터를 읽어 캐시를 갱신하도록 설계했습니다.📊 EXPLAIN Before / After
케이스 1: 브랜드 필터 + 좋아요순 정렬
idx_product_brand_like_countbrandId로 필터하여 99,422 → 2,021건으로 축소, likeCount DESC 인덱스 순서로 읽어 filesort 제거. rows 49배 감소.
케이스 2: 브랜드 필터 + 가격순 정렬
idx_product_brand_price동일하게 brandId 필터로 2,021건 축소, price ASC 인덱스 정렬로 filesort 제거. rows 49배 감소.
케이스 3: 전체 좋아요순 정렬
idx_product_like_count인덱스 순서 자체가 likeCount DESC이므로 앞에서 20건만 스캔. rows 99,422 → 20, 4,971배 감소.
케이스 4: 전체 가격순 정렬
idx_product_priceprice ASC 인덱스 앞에서 20건만 스캔. 케이스 3과 동일하게 rows 4,971배 감소, filesort 제거.
전체 요약
카디널리티 분석
ididx_product_brand_like_countbrand_ididx_product_brand_like_countlike_countidx_product_brand_pricepricebrand_id(카디널리티 51)를 선행 컬럼으로, 카디널리티가 높은like_count/price를 후행 컬럼으로 배치하여 필터 → 정렬 순서로 인덱스가 동작하도록 설계했습니다.API 응답시간 측정
목록 API 캐시 효과가 미미한 이유: 현재 목록 API는 페이지네이션 없이 전체를 반환하고 있습니다. 10만건 JSON 직렬화 + HTTP 전송 비용이 지배적이며, 이 비용은 캐시 히트든 미스든 동일합니다. 페이지네이션(
LIMIT 20) 도입 시 캐시 효과가 뚜렷해질 것으로 예상됩니다.🧪 테스트
ProductModelTestProductServiceUnitTestProductFacadeUnitTestLikeFacadeUnitTestLikeConcurrencyTest🔁 Flow Diagram
상품 조회 (캐시 히트 / 미스)
sequenceDiagram participant Client participant Facade as ProductFacade participant Cache as ProductCacheService participant Service as ProductService participant DB Client->>Facade: GET /products/{id} Facade->>Cache: getProductDetail(id) alt 캐시 히트 Cache-->>Facade: Optional.of(info) Facade-->>Client: 200 OK (from cache) else 캐시 미스 Cache-->>Facade: Optional.empty() Facade->>Service: getById(id) Service->>DB: SELECT * FROM product WHERE id = ? DB-->>Service: ProductModel Facade->>Cache: setProductDetail(id, info) Facade-->>Client: 200 OK (from DB) end좋아요 등록 (Atomic Update + 캐시 무효화)
sequenceDiagram participant Client participant Facade as LikeFacade participant LikeService as ProductLikeService participant ProductService participant Cache as ProductCacheService participant DB Client->>Facade: POST /likes (productId) Facade->>LikeService: addLike(memberId, productId) LikeService->>DB: INSERT INTO product_like Facade->>ProductService: increaseLikeCount(productId) ProductService->>DB: UPDATE product SET like_count = like_count + 1 rect rgb(255, 230, 230) Note over Facade, Cache: 캐시 무효화 구간 Facade->>Cache: evictProductDetail(productId) Facade->>Cache: evictProductList() end Facade-->>Client: 200 OK✅ 체크리스트
like_count컬럼 비정규화 + Atomic Update 동기화ORDER BY로 동작try-catch+Optional.empty())Pageable도입 (LIMIT + OFFSET)@TransactionalEventListener)