[volume-5] 인덱스 추가 및 캐시 도입 - 김선민#211
Open
seonminKim1122 wants to merge 7 commits intoLoopers-dev-lab:seonminKim1122from
Open
[volume-5] 인덱스 추가 및 캐시 도입 - 김선민#211seonminKim1122 wants to merge 7 commits intoLoopers-dev-lab:seonminKim1122from
seonminKim1122 wants to merge 7 commits intoLoopers-dev-lab:seonminKim1122from
Conversation
- ProductAssembler.toInfos → toInfoMap 으로 변경 (반환 타입 List → Map<Long, ProductInfo>) - ProductQueryService.getList 를 resolveFromCache / resolveFromDatabase 로 분리 - partial cache HIT 처리 시 index 기반 for loop 제거, Map 기반 putAll/forEach 로 대체 - ProductFacade 도 toInfoMap 사용으로 통일 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
캐시 유효 기간은 application layer의 정책이므로 RedisProductCacheStore 에서 정의하던 TTL 상수를 ProductCacheStore 인터페이스로 이동. 키 포맷은 Redis 구현 세부사항으로 infrastructure에 유지. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
seonminKim1122
changed the title
📝 WalkthroughWalkthrough제품 조회 성능을 위해 캐시 우선 전략을 도입한다. ProductQueryService를 새로 추가하여 Redis 기반 캐시와 데이터베이스를 조율하고, ProductAssembler의 반환 타입을 List에서 Map으로 변경하며, 제품 목록 및 상세 조회 시 캐시 검증 및 무효화 기능을 통합한다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ProductFacade
participant ProductQueryService
participant ProductCacheStore
participant ProductRepository
participant BrandRepository
rect rgba(100, 200, 100, 0.5)
Note over ProductFacade,BrandRepository: 제품 목록 조회 - 캐시 히트 경로
Client->>ProductFacade: getList(pageable, brandId, sort)
ProductFacade->>ProductQueryService: getList(pageable, brandId, sort)
ProductQueryService->>ProductCacheStore: getList(brandId, sort, page, size)
ProductCacheStore-->>ProductQueryService: Optional<ProductListCacheEntry>
ProductQueryService->>ProductCacheStore: multiGet(cachedProductIds)
ProductCacheStore-->>ProductQueryService: Map<Long, ProductInfo>
ProductQueryService-->>ProductFacade: PageResponse<ProductInfo>
ProductFacade-->>Client: PageResponse<ProductInfo>
end
rect rgba(100, 150, 200, 0.5)
Note over ProductFacade,BrandRepository: 제품 목록 조회 - 캐시 미스 경로
Client->>ProductFacade: getList(pageable, brandId, sort)
ProductFacade->>ProductQueryService: getList(pageable, brandId, sort)
ProductQueryService->>ProductCacheStore: getList(brandId, sort, page, size)
ProductCacheStore-->>ProductQueryService: Optional.empty()
ProductQueryService->>ProductRepository: findByBrandIdWithSort(brandId, sort, pageable)
ProductRepository-->>ProductQueryService: List<Product>
ProductQueryService->>BrandRepository: findAllById(brandIds)
BrandRepository-->>ProductQueryService: List<Brand>
ProductQueryService->>ProductCacheStore: putList(brandId, sort, page, size, productIds, totalPages)
ProductQueryService->>ProductCacheStore: put(productId, productInfo) [개별 아이템]
ProductCacheStore-->>ProductQueryService: (void)
ProductQueryService-->>ProductFacade: PageResponse<ProductInfo>
ProductFacade-->>Client: PageResponse<ProductInfo>
end
rect rgba(200, 100, 100, 0.5)
Note over ProductFacade,BrandRepository: 제품 상세 조회 - 캐시 무효화
Client->>ProductFacade: update(productId, ...)
ProductFacade->>ProductRepository: save(product)
ProductRepository-->>ProductFacade: (void)
ProductFacade->>ProductQueryService: evict(productId)
ProductQueryService->>ProductCacheStore: evict(productId)
ProductCacheStore-->>ProductQueryService: (void)
ProductQueryService-->>ProductFacade: (void)
ProductFacade-->>Client: (response)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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/main/java/com/loopers/application/product/ProductFacade.java (1)
45-54:⚠️ Potential issue | 🟠 Major캐시 eviction은 커밋 이후로 미뤄야 한다.
현재는 DB 커밋 전에 cache key를 먼저 지우므로, 같은 상품을 동시에 읽는 요청이 들어오면 아직 커밋되지 않은 이전 DB 값을 다시 읽어 상세 캐시를 재생성할 수 있다. 운영에서는 update/delete 직후 stale 데이터가 TTL 동안 되살아나는 전형적인 cache-aside race가 되므로,
TransactionSynchronization이나 트랜잭션 이벤트를 사용해 성공 커밋 후에만 eviction을 실행해야 한다. 롤백 시 cache가 건드려지지 않으며 동시 조회가 와도 이전 값이 재캐시되지 않는 통합 테스트를 추가해야 한다.패치 예시
- productQueryService.evict(productId); + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + `@Override` + public void afterCommit() { + productQueryService.evict(productId); + } + });Also applies to: 80-85
🤖 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 45 - 54, Move cache eviction to run only after a successful DB commit: in ProductFacade.update (and the analogous delete/update methods around lines 80-85) register a post-commit callback instead of calling productQueryService.evict(productId) directly so eviction runs in TransactionSynchronization.afterCommit (or use `@TransactionalEventListener` for AFTER_COMMIT). Replace the direct productQueryService.evict(productId) call with a TransactionSynchronizationManager.registerSynchronization(...) or an AfterCommit event publisher that invokes productQueryService.evict(productId), ensuring no eviction on rollback; add an integration test that simulates a rollback and concurrent reads to verify no stale recache occurs.
🧹 Nitpick comments (2)
apps/commerce-api/src/test/java/com/loopers/interfaces/api/product/ProductApiE2ETest.java (2)
195-199: 관리자 수정 호출의 성공 여부를 즉시 단언해 실패 원인을 분리해야 한다.Line [195]-Line [199]에서 PUT 결과를 바로 검증하지 않아, 인증/인가 실패가 발생해도 후속 GET 실패로만 나타나 원인 파악이 늦어진다. 운영 장애 분석 관점에서 진단 시간이 늘어나는 패턴이다.
수정안은 PUT 응답을 변수로 받고HttpStatus.OK또는HttpStatus.NO_CONTENT를 먼저 단언하는 것이다.
추가 테스트로X-Loopers-Ldap누락/오류 시 401 또는 403 응답을 검증하는 실패 케이스를 넣는 것이 좋다.예시 수정안
- testRestTemplate.exchange(updateUrl, HttpMethod.PUT, new HttpEntity<>(updateRequest, adminHeaders), Void.class); + ResponseEntity<Void> updateResponse = + testRestTemplate.exchange(updateUrl, HttpMethod.PUT, new HttpEntity<>(updateRequest, adminHeaders), Void.class); + assertThat(updateResponse.getStatusCode()).isIn(HttpStatus.OK, HttpStatus.NO_CONTENT); ResponseEntity<ApiResponse<ProductDto.DetailResponse>> response = testRestTemplate.exchange(detailUrl, HttpMethod.GET, HttpEntity.EMPTY, responseType);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/product/ProductApiE2ETest.java` around lines 195 - 199, Capture and assert the PUT response from testRestTemplate.exchange(updateUrl, HttpMethod.PUT, new HttpEntity<>(updateRequest, adminHeaders), Void.class) into a variable and immediately assert its status is HttpStatus.OK or HttpStatus.NO_CONTENT to separate update failures from subsequent GET failures; keep the existing GET using detailUrl/responseType but only after the successful PUT assertion. Also add explicit negative tests for the update endpoint that call testRestTemplate.exchange with missing/invalid adminHeaders (X-Loopers-Ldap) and assert expected 401 or 403 responses to cover authentication/authorization failure paths.
95-118: 캐시 HIT 검증이 DB 우회 여부를 충분히 증명하지 못한다.Line [95]-Line [118], Line [150]-Line [173]은 캐시 키 존재만 확인하므로 내부가 매 요청마다 DB를 조회해도 통과할 수 있다. 운영 관점에서 트래픽 급증 시 DB 부하 회귀를 사전에 탐지하지 못하는 테스트 구조다.
수정안은 1차 조회로 캐시를 워밍한 뒤,ProductJpaRepository로 원본 데이터를 직접 삭제/변경하고 2차 조회가 1차 응답과 동일함을 단언해 캐시 경로를 증명하는 방식이다.
추가 테스트로 목록/상세 각각에 “워밍 후 DB 데이터 삭제(또는 직접 변경) 후 2차 조회” 케이스를 넣어야 한다.예시 수정안 (상세 HIT 테스트 강화)
- testRestTemplate.exchange(url, HttpMethod.GET, HttpEntity.EMPTY, responseType); + ResponseEntity<ApiResponse<ProductDto.DetailResponse>> first = + testRestTemplate.exchange(url, HttpMethod.GET, HttpEntity.EMPTY, responseType); + + productJpaRepository.deleteById(product.getId()); + productJpaRepository.flush(); // act (캐시 HIT) ResponseEntity<ApiResponse<ProductDto.DetailResponse>> response = testRestTemplate.exchange(url, HttpMethod.GET, HttpEntity.EMPTY, responseType); // assert String cacheKey = "product:detail:" + product.getId(); assertAll( () -> assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK), - () -> assertThat(response.getBody().data().name()).isEqualTo("나이키 에어맥스"), + () -> assertThat(response.getBody().data().name()).isEqualTo(first.getBody().data().name()), () -> assertThat(redisTemplate.hasKey(cacheKey)).isTrue() );As per coding guidelines
**/*Test*.java: 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다.Also applies to: 150-173
🤖 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/product/ProductApiE2ETest.java` around lines 95 - 118, The current returnsSameProductList_whenCacheHit test only asserts the Redis key exists but doesn't prove the DB was bypassed; after warming the cache with the first GET in ProductApiE2ETest.returnsSameProductList_whenCacheHit, delete or mutate the saved Product via productJpaRepository (e.g., productJpaRepository.deleteById(product.getId()) or update fields) before making the second GET, then assert the second response body equals the first (same size and content) and redisTemplate.hasKey(listCacheKey) is true to prove the hit path; apply the same pattern to the detail-test counterpart (the test covering single-product detail) to create a “warm cache -> delete DB entity -> second GET unchanged” assertion.
🤖 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/product/ProductCacheStore.java`:
- Around line 16-24: Product list caches aren't evicted today (only detail evict
exists), so add a contract on ProductCacheStore to invalidate list-level entries
and update callers/tests to use it: extend the ProductCacheStore interface with
a method such as evictList(Long brandId, String sort) (or
evictListsForBrand(Long brandId) if you need brand-wide), ensure implementations
of getList/putList/ProductListCacheEntry respect a namespace/version or
implement the new evictList to remove/increment namespace for that brand+sort,
and call this new method from the product deletion/price-change/like-change
flows where evict(Long productId) is currently used; add a unit/integration test
that performs delete/price/like and then verifies subsequent getList does not
return stale productIds.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductQueryService.java`:
- Around line 33-40: In getList, normalize the sort once with ProductSortType
sortType = ProductSortType.from(sort) and use sortType.name() (or another
canonical value) for all cache interactions (productCacheStore.getList and
productCacheStore.putList) instead of the raw sort string so requests like
null/latest/LATEST share the same cache key; apply the same change in the other
block (resolveFromDatabase/resolveFromCache paths, lines ~72-89) and add a
unit/integration test that calls the service with null, lowercase and uppercase
sort inputs and asserts they hit the same cache entry.
In
`@apps/commerce-api/src/main/java/com/loopers/domain/product/ProductSortType.java`:
- Around line 9-10: The LIKE_COUNT sort (constant LIKE_COUNT in ProductSortType)
is unstable because it only sorts by likeCount; update it to append a
deterministic secondary key (e.g., id DESC or createdAt DESC) so ties are
resolved consistently (use Sort.by(...).and(...) to combine likeCount DESC with
id DESC or createdAt DESC), and add a unit/integration test that seeds multiple
products with identical likeCount and asserts paginated results (page 1 then
page 2) remain stable across repeated requests.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/product/RedisProductCacheStore.java`:
- Around line 47-49: RedisProductCacheStore currently swallows Redis failures
only for get/put/getList/putList but lets evict and multiGet throw; wrap both
evict(Long) and multiGet(Collection<Long>) calls to redisTemplate.delete(...)
and redisTemplate.multiGet(...) in try-catch blocks that log the exception via
the existing logger, increment a failure metric, and perform safe fallbacks
(evict: no-op on failure; multiGet: return an empty map/empty collection so
callers fall back to DB). Update method bodies in RedisProductCacheStore to
catch RuntimeException (or Redis-specific exceptions), emit the same log/metric
style used by get/put methods, and ensure behavior parity with write APIs
(tests: add cases asserting that when redisTemplate.delete/multiGet throw, write
operations still succeed and read operations fall back to DB/return empty map).
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/product/ProductApiE2ETest.java`:
- Around line 208-214: Replace the hardcoded ID 999999 in the test method
returnsNotFound_whenProductNotExist with a value that won't collide with real
data (e.g., Long.MAX_VALUE) and, before calling testRestTemplate.exchange,
assert non-existence by calling the product repository's existsById (e.g.,
productRepository.existsById(targetId)) to guarantee the ID is absent; update
the test to use that non-existent ID and keep the ResponseEntity exchange logic
unchanged.
---
Outside diff comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`:
- Around line 45-54: Move cache eviction to run only after a successful DB
commit: in ProductFacade.update (and the analogous delete/update methods around
lines 80-85) register a post-commit callback instead of calling
productQueryService.evict(productId) directly so eviction runs in
TransactionSynchronization.afterCommit (or use `@TransactionalEventListener` for
AFTER_COMMIT). Replace the direct productQueryService.evict(productId) call with
a TransactionSynchronizationManager.registerSynchronization(...) or an
AfterCommit event publisher that invokes productQueryService.evict(productId),
ensuring no eviction on rollback; add an integration test that simulates a
rollback and concurrent reads to verify no stale recache occurs.
---
Nitpick comments:
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/product/ProductApiE2ETest.java`:
- Around line 195-199: Capture and assert the PUT response from
testRestTemplate.exchange(updateUrl, HttpMethod.PUT, new
HttpEntity<>(updateRequest, adminHeaders), Void.class) into a variable and
immediately assert its status is HttpStatus.OK or HttpStatus.NO_CONTENT to
separate update failures from subsequent GET failures; keep the existing GET
using detailUrl/responseType but only after the successful PUT assertion. Also
add explicit negative tests for the update endpoint that call
testRestTemplate.exchange with missing/invalid adminHeaders (X-Loopers-Ldap) and
assert expected 401 or 403 responses to cover authentication/authorization
failure paths.
- Around line 95-118: The current returnsSameProductList_whenCacheHit test only
asserts the Redis key exists but doesn't prove the DB was bypassed; after
warming the cache with the first GET in
ProductApiE2ETest.returnsSameProductList_whenCacheHit, delete or mutate the
saved Product via productJpaRepository (e.g.,
productJpaRepository.deleteById(product.getId()) or update fields) before making
the second GET, then assert the second response body equals the first (same size
and content) and redisTemplate.hasKey(listCacheKey) is true to prove the hit
path; apply the same pattern to the detail-test counterpart (the test covering
single-product detail) to create a “warm cache -> delete DB entity -> second GET
unchanged” assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ad35cf7-43ba-4127-bfa4-92fe558bc7a9
📒 Files selected for processing (9)
apps/commerce-api/src/main/java/com/loopers/application/product/ProductAssembler.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheStore.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductQueryService.javaapps/commerce-api/src/main/java/com/loopers/domain/product/ProductSortType.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/RedisProductCacheStore.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductQueryServiceTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/product/ProductApiE2ETest.java
Comment on lines
+16
to
+24
| void evict(Long productId); | ||
|
|
||
| record ProductListCacheEntry(List<Long> productIds, int totalPages) {} | ||
|
|
||
| Optional<ProductListCacheEntry> getList(Long brandId, String sort, int page, int size); | ||
|
|
||
| void putList(Long brandId, String sort, int page, int size, List<Long> productIds, int totalPages); | ||
|
|
||
| Map<Long, ProductInfo> multiGet(List<Long> productIds); |
There was a problem hiding this comment.
목록 캐시를 무효화할 계약이 필요하다.
현재 인터페이스는 상세 캐시만 evict할 수 있어서, 삭제/가격 변경 뒤에도 목록의 productIds 캐시는 TTL 동안 그대로 남는다. 운영에서는 삭제된 상품이 목록에 남거나 PRICE_ASC·LIKE_COUNT 결과가 실제 DB 순서와 달라질 수 있으므로, 브랜드/정렬 단위의 목록 캐시 무효화 API를 추가하거나 key scan 대신 namespace version 기반 일괄 폐기 전략으로 바꿔야 한다. 삭제·가격 변경·좋아요 수 변경 직후 다음 목록 조회가 이전 page 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/product/ProductCacheStore.java`
around lines 16 - 24, Product list caches aren't evicted today (only detail
evict exists), so add a contract on ProductCacheStore to invalidate list-level
entries and update callers/tests to use it: extend the ProductCacheStore
interface with a method such as evictList(Long brandId, String sort) (or
evictListsForBrand(Long brandId) if you need brand-wide), ensure implementations
of getList/putList/ProductListCacheEntry respect a namespace/version or
implement the new evictList to remove/increment namespace for that brand+sort,
and call this new method from the product deletion/price-change/like-change
flows where evict(Long productId) is currently used; add a unit/integration test
that performs delete/price/like and then verifies subsequent getList does not
return stale productIds.
Comment on lines
+33
to
+40
| public PageResponse<ProductInfo> getList(Pageable pageable, Long brandId, String sort) { | ||
| int page = pageable.getPageNumber(); | ||
| int size = pageable.getPageSize(); | ||
| PageRequest pageRequest = PageRequest.of(page, size, ProductSortType.from(sort).getSort()); | ||
|
|
||
| return productCacheStore.getList(brandId, sort, page, size) | ||
| .map(entry -> resolveFromCache(entry, page, size)) | ||
| .orElseGet(() -> resolveFromDatabase(brandId, pageRequest, sort, page, size)); |
There was a problem hiding this comment.
캐시 키에는 정규화된 sort 값을 써야 한다.
DB 조회는 ProductSortType.from(sort)로 정규화하지만, 캐시 조회/저장은 원본 sort 문자열을 그대로 사용한다. 그래서 null, latest, LATEST처럼 같은 의미의 요청이 서로 다른 list key로 분산되어 hit rate가 떨어지고 DB 부하 저감 효과가 약해지므로, ProductSortType을 한 번만 해석한 뒤 sortType.name() 같은 정규화된 값을 getList/putList 전체에 사용해야 한다. null·소문자·대문자 입력이 같은 캐시 엔트리를 재사용하는 테스트를 추가해야 한다.
패치 예시
`@Transactional`(readOnly = true)
public PageResponse<ProductInfo> getList(Pageable pageable, Long brandId, String sort) {
int page = pageable.getPageNumber();
int size = pageable.getPageSize();
- PageRequest pageRequest = PageRequest.of(page, size, ProductSortType.from(sort).getSort());
+ ProductSortType sortType = ProductSortType.from(sort);
+ String normalizedSort = sortType.name();
+ PageRequest pageRequest = PageRequest.of(page, size, sortType.getSort());
- return productCacheStore.getList(brandId, sort, page, size)
+ return productCacheStore.getList(brandId, normalizedSort, page, size)
.map(entry -> resolveFromCache(entry, page, size))
- .orElseGet(() -> resolveFromDatabase(brandId, pageRequest, sort, page, size));
+ .orElseGet(() -> resolveFromDatabase(brandId, pageRequest, normalizedSort, page, size));
}Also applies to: 72-89
🤖 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/ProductQueryService.java`
around lines 33 - 40, In getList, normalize the sort once with ProductSortType
sortType = ProductSortType.from(sort) and use sortType.name() (or another
canonical value) for all cache interactions (productCacheStore.getList and
productCacheStore.putList) instead of the raw sort string so requests like
null/latest/LATEST share the same cache key; apply the same change in the other
block (resolveFromDatabase/resolveFromCache paths, lines ~72-89) and add a
unit/integration test that calls the service with null, lowercase and uppercase
sort inputs and asserts they hit the same cache entry.
Comment on lines
+9
to
+10
| PRICE_ASC(Sort.by(Sort.Direction.ASC, "price")), | ||
| LIKE_COUNT(Sort.by(Sort.Direction.DESC, "likeCount")); |
There was a problem hiding this comment.
LIKE_COUNT 정렬에 보조 키를 추가해야 한다.
likeCount만으로 정렬하면 동률 구간에서 DB가 임의 순서를 반환하므로, 운영 중 페이지 경계에서 상품 중복/누락이 발생할 수 있다. 현재 PR처럼 페이지별 productIds를 캐시하면 이 비결정성이 그대로 캐시되어 같은 요청도 다른 ID 집합을 반환할 수 있으므로, id DESC 또는 createdAt DESC를 보조 정렬로 붙여 순서를 고정해야 한다. 같은 likeCount를 가진 상품이 여러 개 있을 때 1페이지/2페이지 결과가 반복 호출에도 안정적으로 유지되는 테스트를 추가해야 한다.
패치 예시
- LIKE_COUNT(Sort.by(Sort.Direction.DESC, "likeCount"));
+ LIKE_COUNT(
+ Sort.by(Sort.Direction.DESC, "likeCount")
+ .and(Sort.by(Sort.Direction.DESC, "id"))
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PRICE_ASC(Sort.by(Sort.Direction.ASC, "price")), | |
| LIKE_COUNT(Sort.by(Sort.Direction.DESC, "likeCount")); | |
| PRICE_ASC(Sort.by(Sort.Direction.ASC, "price")), | |
| LIKE_COUNT( | |
| Sort.by(Sort.Direction.DESC, "likeCount") | |
| .and(Sort.by(Sort.Direction.DESC, "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/domain/product/ProductSortType.java`
around lines 9 - 10, The LIKE_COUNT sort (constant LIKE_COUNT in
ProductSortType) is unstable because it only sorts by likeCount; update it to
append a deterministic secondary key (e.g., id DESC or createdAt DESC) so ties
are resolved consistently (use Sort.by(...).and(...) to combine likeCount DESC
with id DESC or createdAt DESC), and add a unit/integration test that seeds
multiple products with identical likeCount and asserts paginated results (page 1
then page 2) remain stable across repeated requests.
Comment on lines
+47
to
+49
| @Override | ||
| public void evict(Long productId) { | ||
| redisTemplate.delete(DETAIL_KEY_PREFIX + productId); |
There was a problem hiding this comment.
evict와 multiGet도 Redis 장애를 비치명적으로 처리해야 한다.
이 클래스는 get/put/getList/putList만 실패를 흡수하고, evict와 multiGet은 Redis 예외를 그대로 전파한다. 운영에서는 Redis 일시 장애만으로 상품 수정/삭제 트랜잭션이 롤백되거나 목록 조회가 DB fallback 없이 500으로 끝날 수 있으므로, 두 메서드도 no-op/빈 맵으로 폴백하고 실패 로그·메트릭을 남기도록 맞춰야 한다. redisTemplate.delete(...)와 multiGet(...)이 예외를 던질 때 쓰기 API는 성공하고 읽기 API는 DB로 폴백하는 테스트를 추가해야 한다.
패치 예시
public void evict(Long productId) {
- redisTemplate.delete(DETAIL_KEY_PREFIX + productId);
+ try {
+ redisTemplate.delete(DETAIL_KEY_PREFIX + productId);
+ } catch (Exception e) {
+ log.warn("Failed to evict product cache. productId={}", productId, e);
+ }
}
`@Override`
public Map<Long, ProductInfo> multiGet(List<Long> productIds) {
- List<String> keys = productIds.stream()
- .map(id -> DETAIL_KEY_PREFIX + id)
- .toList();
- List<String> values = redisTemplate.opsForValue().multiGet(keys);
Map<Long, ProductInfo> result = new HashMap<>();
- if (values == null) return result;
- for (int i = 0; i < productIds.size(); i++) {
- String value = values.get(i);
- if (value != null) {
- try {
- result.put(productIds.get(i), objectMapper.readValue(value, ProductInfo.class));
- } catch (Exception ignored) {
+ try {
+ List<String> keys = productIds.stream()
+ .map(id -> DETAIL_KEY_PREFIX + id)
+ .toList();
+ List<String> values = redisTemplate.opsForValue().multiGet(keys);
+ if (values == null) return result;
+ for (int i = 0; i < productIds.size(); i++) {
+ String value = values.get(i);
+ if (value != null) {
+ try {
+ result.put(productIds.get(i), objectMapper.readValue(value, ProductInfo.class));
+ } catch (Exception e) {
+ log.warn("Failed to deserialize product cache. productId={}", productIds.get(i), e);
+ }
}
}
+ } catch (Exception e) {
+ log.warn("Failed to read product caches. productIds={}", productIds, e);
}
return result;
}Also applies to: 74-90
🤖 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/product/RedisProductCacheStore.java`
around lines 47 - 49, RedisProductCacheStore currently swallows Redis failures
only for get/put/getList/putList but lets evict and multiGet throw; wrap both
evict(Long) and multiGet(Collection<Long>) calls to redisTemplate.delete(...)
and redisTemplate.multiGet(...) in try-catch blocks that log the exception via
the existing logger, increment a failure metric, and perform safe fallbacks
(evict: no-op on failure; multiGet: return an empty map/empty collection so
callers fall back to DB). Update method bodies in RedisProductCacheStore to
catch RuntimeException (or Redis-specific exceptions), emit the same log/metric
style used by get/put methods, and ensure behavior parity with write APIs
(tests: add cases asserting that when redisTemplate.delete/multiGet throw, write
operations still succeed and read operations fall back to DB/return empty map).
Comment on lines
+208
to
+214
| @DisplayName("존재하지 않는 상품 ID 로 조회 시, 404 응답을 반환한다.") | ||
| @Test | ||
| void returnsNotFound_whenProductNotExist() { | ||
| // act | ||
| ParameterizedTypeReference<ApiResponse<ProductDto.DetailResponse>> responseType = new ParameterizedTypeReference<>() {}; | ||
| ResponseEntity<ApiResponse<ProductDto.DetailResponse>> response = | ||
| testRestTemplate.exchange(ENDPOINT + "/999999", HttpMethod.GET, HttpEntity.EMPTY, responseType); |
There was a problem hiding this comment.
존재하지 않는 ID로 고정값 999999 사용은 장기적으로 플래키할 수 있다.
Line [214]의 고정값은 시퀀스 증가/테스트 데이터 누적으로 실제 ID와 충돌할 가능성이 있다. 운영 관점에서 CI가 간헐적으로 깨지는 원인이 된다.
수정안은 Long.MAX_VALUE 같은 충돌 가능성이 낮은 값을 사용하고, 호출 전 existsById로 미존재를 한 번 보장하는 방식이다.
추가 테스트로 경계값 ID(0, 음수, 숫자 외 path) 요청의 400/404 정책을 명시적으로 검증하는 케이스를 권장한다.
예시 수정안
- ResponseEntity<ApiResponse<ProductDto.DetailResponse>> response =
- testRestTemplate.exchange(ENDPOINT + "/999999", HttpMethod.GET, HttpEntity.EMPTY, responseType);
+ long nonExistentId = Long.MAX_VALUE;
+ assertThat(productJpaRepository.existsById(nonExistentId)).isFalse();
+ ResponseEntity<ApiResponse<ProductDto.DetailResponse>> response =
+ testRestTemplate.exchange(ENDPOINT + "/" + nonExistentId, HttpMethod.GET, HttpEntity.EMPTY, responseType);As per coding guidelines **/*Test*.java: 통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @DisplayName("존재하지 않는 상품 ID 로 조회 시, 404 응답을 반환한다.") | |
| @Test | |
| void returnsNotFound_whenProductNotExist() { | |
| // act | |
| ParameterizedTypeReference<ApiResponse<ProductDto.DetailResponse>> responseType = new ParameterizedTypeReference<>() {}; | |
| ResponseEntity<ApiResponse<ProductDto.DetailResponse>> response = | |
| testRestTemplate.exchange(ENDPOINT + "/999999", HttpMethod.GET, HttpEntity.EMPTY, responseType); | |
| `@DisplayName`("존재하지 않는 상품 ID 로 조회 시, 404 응답을 반환한다.") | |
| `@Test` | |
| void returnsNotFound_whenProductNotExist() { | |
| // act | |
| ParameterizedTypeReference<ApiResponse<ProductDto.DetailResponse>> responseType = new ParameterizedTypeReference<>() {}; | |
| long nonExistentId = Long.MAX_VALUE; | |
| assertThat(productJpaRepository.existsById(nonExistentId)).isFalse(); | |
| ResponseEntity<ApiResponse<ProductDto.DetailResponse>> response = | |
| testRestTemplate.exchange(ENDPOINT + "/" + nonExistentId, HttpMethod.GET, HttpEntity.EMPTY, responseType); |
🤖 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/product/ProductApiE2ETest.java`
around lines 208 - 214, Replace the hardcoded ID 999999 in the test method
returnsNotFound_whenProductNotExist with a value that won't collide with real
data (e.g., Long.MAX_VALUE) and, before calling testRestTemplate.exchange,
assert non-existence by calling the product repository's existsById (e.g.,
productRepository.existsById(targetId)) to guarantee the ID is absent; update
the test to use that non-existent ID and keep the ResponseEntity exchange logic
unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Summary
🧭 Context & Decision
문제 정의
브랜드 필터,최신순,가격 오름차순,인기순(좋아요 내림차순)등의 기능을 제공 중인데 여러 조합들에 대해서 모두 table full scan 이 발생선택지와 결정
sort_buffer_size가 256KB, 브랜드 별 상품 수가 많아 size 초과 시 정렬 성능에 급격한 저하 우려문제 정의
선택지와 결정
🏗️ Design Overview
변경 범위
주요 컴포넌트 책임
변경 목적
상품 목록 조회의 성능 저하 및 DB 부하 증가에 대응하기 위해 Redis 기반 캐시 계층과 조회 로직을 추가하여 조회 성능 개선 및 DB 부하 경감.
핵심 변경점
리스크/주의사항
테스트/검증 방법