[5주차] 인덱스 설계와 조회 최적화 - 윤유탁#189
Open
yoon-yoo-tak wants to merge 5 commits intoLoopers-dev-lab:yoon-yoo-takfrom
Open
[5주차] 인덱스 설계와 조회 최적화 - 윤유탁#189yoon-yoo-tak wants to merge 5 commits intoLoopers-dev-lab:yoon-yoo-takfrom
yoon-yoo-tak wants to merge 5 commits intoLoopers-dev-lab:yoon-yoo-takfrom
Conversation
- ProductReadModel(불변 record) 기반 캐시로 mutable 엔티티 캐싱 문제 해결 - ProductReadCache 인터페이스로 캐시 구현 추상화 (Caffeine/Redis 전환 용이) - afterCommit 시점 무효화로 트랜잭션 커밋 전 stale 캐시 문제 해결 - ArchUnit 규칙에 Infrastructure/Config의 ApplicationService 직접 의존 금지 추가
- ProductPageReadCache 인터페이스로 목록 캐시 추상화 - 전체 조회(brandId=null)만 캐시, 브랜드 필터 조회는 인덱스로 커버 - 쓰기 시 상세 캐시 + 페이지 캐시 모두 afterCommit 무효화 - V1 컨트롤러 목록 조회를 ProductQueryService로 전환 - 브랜드 삭제 시 evictAll 경로 회귀 방지 테스트 추가
📝 WalkthroughWalkthrough상품 조회 성능 개선을 위해 Caffeine 기반의 단건 및 페이지 캐시 인터페이스·구현을 추가하고, 캐시 설정 빈과 트랜잭션 경계 이빅션 통합, DB 인덱스 추가 및 컨트롤러/DTO를 ProductReadModel 중심으로 리팩터링한 변경이다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as ProductV1Controller
participant QueryService as ProductQueryService
participant DetailCache as ProductReadCache
participant PageCache as ProductPageReadCache
participant Repo as ProductRepository
participant DB as Database
Note over Client,DB: 단건 조회 (캐시 히트)
Client->>Controller: GET /products/{id}
Controller->>QueryService: getById(id)
QueryService->>DetailCache: get(id, loader)
DetailCache-->>QueryService: ProductReadModel (cached)
QueryService-->>Controller: ProductReadModel
Controller-->>Client: ApiResponse
Note over Client,DB: 단건 조회 (캐시 미스)
Client->>Controller: GET /products/{id}
Controller->>QueryService: getById(id)
QueryService->>DetailCache: get(id, loader)
DetailCache->>Repo: loader.get()
Repo->>DB: SELECT ... WHERE id=?
DB-->>Repo: Product
Repo-->>DetailCache: ProductReadModel
DetailCache->>DetailCache: store
DetailCache-->>QueryService: ProductReadModel
QueryService-->>Controller: ProductReadModel
Controller-->>Client: ApiResponse
Note over Client,DB: 페이지 조회 (brandId 없음, 캐시 미스)
Client->>Controller: GET /products?sort=...&page=0&size=10
Controller->>QueryService: getAll(null, sort, page, size)
QueryService->>PageCache: get(sort,page,size,loader)
PageCache->>Repo: loader.get()
Repo->>DB: SELECT ... LIMIT/OFFSET
DB-->>Repo: Page<Product>
Repo-->>PageCache: PageResult<ProductReadModel>
PageCache->>PageCache: store
PageCache-->>QueryService: PageResult
QueryService-->>Controller: PageResult
Controller-->>Client: ApiResponse
Note over Client,DB: 페이지 조회 (brandId 필터, 캐시 스킵)
Client->>Controller: GET /products?brandId=1...
Controller->>QueryService: getAll(1, sort, page, size)
QueryService->>Repo: queryFromDb(...)
Repo->>DB: SELECT ... WHERE brand_id=1
DB-->>Repo: Page<Product>
Repo-->>QueryService: PageResult
QueryService-->>Controller: PageResult
Controller-->>Client: ApiResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 분 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[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/interfaces/api/product/ProductV1Controller.java (1)
31-39:⚠️ Potential issue | 🟠 Major잘못된
page/size를CoreException으로 막아야 한다.현재 음수
page나 0 이하size가 들어오면 하위PageRequest.of(...)에서IllegalArgumentException이 발생할 수 있다. 운영에서는 잘못된 요청이 공통 에러 포맷을 우회해 500 계열로 보이거나 로그 분류가 흐려진다. 컨트롤러 진입 시 범위를 검증하고 프로젝트에서 사용하는 4xxErrorType의CoreException으로 변환해 일관된 응답을 유지하는 편이 안전하다.추가 테스트로
page=-1,size=0요청이 모두 동일한 API 에러 응답 포맷으로 반환되는지 컨트롤러 테스트를 추가해야 한다. As per coding guidelines,**/*Controller*.java: Controller는 요청 검증과 응답 조립에 집중하고 비즈니스 로직은 Service로 이동한다. Based on learnings,apps/commerce-api/src/main/java/에서는 오류를CoreException으로 라우팅해ApiControllerAdvice의 일관된 응답 포맷을 유지해야 한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Controller.java` around lines 31 - 39, Validate the incoming page and size at the start of ProductV1Controller.getAll: if page is negative or size is <= 0, throw a CoreException using the project’s 4xx ErrorType (e.g., BAD_REQUEST) so ApiControllerAdvice emits the standard API error format; keep validation in the controller (not in ProductQueryService) and then call productQueryService.getAll(...) as before. Also add controller tests that call getAll with page=-1 and size=0 and assert both produce the same API error response shape handled by ApiControllerAdvice.
🧹 Nitpick comments (5)
apps/commerce-api/src/main/java/com/loopers/config/CacheConfig.java (1)
15-24: 운영 환경에서 캐시 모니터링을 위해recordStats()추가가 필요하다.캐시 히트/미스율, eviction 수 등 운영 지표 수집이 불가능하면 캐시 효율성 판단 및 장애 시 원인 분석이 어렵다. Caffeine은 Micrometer와 통합되어 Spring Actuator
/metrics엔드포인트로 노출 가능하다.♻️ 수정안
`@Bean` public Cache<Long, ProductReadModel> productDetailCache( `@Value`("${cache.product.maximum-size:10000}") long maximumSize, `@Value`("${cache.product.expire-after-write-minutes:5}") long expireAfterWriteMinutes ) { return Caffeine.newBuilder() .maximumSize(maximumSize) .expireAfterWrite(expireAfterWriteMinutes, TimeUnit.MINUTES) + .recordStats() .build(); }추가로
CaffeineCacheMetrics.monitor()또는MeterBinder빈 등록을 통해 Micrometer에 바인딩하면 Prometheus/Grafana 대시보드에서 확인 가능하다.🤖 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/config/CacheConfig.java` around lines 15 - 24, Update the productDetailCache bean to enable Caffeine statistics by calling recordStats() on the Caffeine.newBuilder() chain and expose those stats to Micrometer; specifically, inside the productDetailCache() method add .recordStats() to the Caffeine builder and then register the cache with Micrometer (e.g., via CaffeineCacheMetrics.monitor(...) or by exposing a MeterBinder bean that binds the returned Cache<Long, ProductReadModel>), so hit/miss/eviction metrics from productDetailCache are available to /metrics and Prometheus.apps/commerce-api/src/main/java/com/loopers/application/product/ProductPageReadCache.java (1)
8-11:brandId전제가 계약에 드러나지 않는다.현재 시그니처는 범용 페이지 캐시처럼 보이지만 키 차원은
sort/page/size뿐이다. 이후 브랜드 목록 조회가 이 인터페이스에 연결되면 서로 다른 브랜드의 동일 페이지가 같은 엔트리를 재사용해 잘못된 상품 목록이 응답될 수 있다.Long 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/application/product/ProductPageReadCache.java` around lines 8 - 11, The ProductPageReadCache contract hides a brandId precondition: update the interface (ProductPageReadCache) so the cache key includes the brand identifier (add a Long brandId parameter to the get(...) signature) or alternatively rename/limit the interface to explicitly indicate it is brand-agnostic; ensure implementations use brandId when composing cache keys and adjust evictAll() semantics if needed; and add a unit/integration test that requests the same sort/page/size for two different brandIds and verifies they do not return the same cached PageResult to prevent cross-brand bleed.apps/commerce-api/src/test/java/com/loopers/benchmark/ProductIndexBenchmarkTest.java (1)
26-28: 벤치마크 테스트 격리는 이미 구성되어 있으나, 명시적 실행 방안이 필요하다.
build.gradle.kts의tasks.test블록에excludeTags("benchmark")가 이미 적용되어 있어 기본 테스트 실행에서 벤치마크 테스트가 제외된다. 따라서 CI/로컬testtask 실행 시 ProductIndexBenchmarkTest가 10만 건 시딩을 수행하지 않는다.다만 개발자가 벤치마크를 명시적으로 실행하려면 현재 별도의 Gradle task가 없다.
.docs/디렉토리의 벤치마크 보고서 관례를 고려하면,./gradlew benchmark같은 별도 task를 정의하여 문서화된 실행 절차를 제공하는 것이 권장된다.🤖 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/benchmark/ProductIndexBenchmarkTest.java` around lines 26 - 28, The repository excludes benchmark tests via excludeTags("benchmark") in build.gradle.kts so ProductIndexBenchmarkTest never runs in normal test runs; add a dedicated Gradle Test task (e.g., named "benchmark") in build.gradle.kts that includes tests with tag "benchmark" (use includeTags("benchmark")), configures any needed JVM args or system properties for heavy seeding, and wires its reports/output to the documented .docs/ benchmark report location so developers can run ./gradlew benchmark to execute ProductIndexBenchmarkTest and generate the benchmark artifacts.apps/commerce-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductReadCacheTest.java (1)
32-84: 동일 키 동시 미스 시 loader 단일 호출을 테스트로 고정해야 한다.현재 테스트는 단일 스레드 경로만 검증하므로 구현이 나중에
getIfPresent후put형태로 바뀌어도 그대로 통과할 수 있다. 운영에서는 인기 상품 상세 조회가 동시에 몰릴 때 동일한 DB 조회가 중복 실행되어 캐시 도입 효과가 크게 약해진다.CountDownLatch나CyclicBarrier로 같은 키를 동시에 조회시키고loaderCallCount가 1인지 검증하는 케이스를 추가하는 편이 안전하다.추가 테스트로 loader가 예외를 던질 때 실패 결과가 캐시에 남지 않는지도 함께 확인해야 한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductReadCacheTest.java` around lines 32 - 84, Add a concurrency test in CaffeineProductReadCacheTest to ensure productReadCache.get(key, loader) invokes the loader only once under simultaneous cache misses: spawn multiple threads that wait on a CountDownLatch/CyclicBarrier, all call productReadCache.get(1L, () -> { loaderCallCount.incrementAndGet(); return expected; }), then assert loaderCallCount.get() == 1 and caffeineCache.getIfPresent(1L) == expected; also add a test where the loader throws an exception (loader that throws) invoked concurrently and assert that no value is left in caffeineCache (caffeineCache.getIfPresent(key) is null) and subsequent calls can still load normally.apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductCacheIntegrationTest.java (1)
195-210: 브랜드 필터 비캐시 요구사항을 직접 검증하지 못한다.현재 시나리오는 쓰기 후 결과만 확인하므로 브랜드 필터 결과가 잘못 캐시되더라도 쓰기 시
evictAll()이 함께 동작하면 그대로 통과할 수 있다. 운영에서는 브랜드별 페이지 캐시가 의도치 않게 쌓여 메모리 사용량과 무효화 비용이 증가하는데, 이 테스트가 그 회귀를 잡지 못한다. 필터 조회 직후 페이지 캐시 엔트리가 생성되지 않는지 직접 확인하거나,ProductQueryServiceImplTest에서brandId != null일 때ProductPageReadCache.get(...)가 호출되지 않는지 검증하는 편이 안전하다.추가 테스트로 브랜드 필터 조회를 두 번 연속 호출한 뒤 페이지 캐시 크기나 캐시 상호작용 횟수가 0회인지 확인하는 케이스를 넣어야 한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductCacheIntegrationTest.java` around lines 195 - 210, The test currently only verifies results after a write and can miss whether brand-filtered queries are being cached; update ProductCacheIntegrationTest to explicitly assert no page cache entry is created for brand-filtered queries by calling productQueryService.getAll(brandId, ProductSortType.LATEST, 0, 20) twice and then verifying the ProductPageReadCache was not interacted with (e.g., assert cache size remains 0 or mock/spy ProductPageReadCache.get(...) was never called), or add a complementary test in ProductQueryServiceImplTest that spies/mocks ProductPageReadCache and asserts ProductPageReadCache.get(...) is not invoked when brandId != null. Ensure you reference the productQueryService.getAll(...) call and ProductPageReadCache.get(...) in the assertions so the test fails if brand-filtered pages are cached.
🤖 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/infrastructure/product/ProductQueryServiceImpl.java`:
- Around line 66-71: The pagination sort in toSort(ProductSortType) uses single
keys causing non-deterministic ordering; update toSort to add a unique
tie-breaker (e.g., "id") to every Sort so each branch uses a deterministic
secondary sort (e.g., PRICE_ASC -> Sort.by(price ASC, id ASC), LIKES_DESC ->
Sort.by(likeCount DESC, id ASC), LATEST -> Sort.by(createdAt DESC, id ASC));
ensure the secondary id direction you choose matches your index strategy and
update any DB indexes to match the composite sort order, then add tests that
create multiple products with identical price/likeCount/createdAt and assert
page 0 and page 1 have no overlap and stable ordering.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Dto.java`:
- Around line 35-40: The ProductPageResponse.from method currently filters out
products missing from brandMap which makes page metadata inconsistent; change it
to require a 1:1 mapping by checking brandMap.containsKey(product.brandId()) for
each item and throw a CoreException when a brand is missing (do not silently
filter), then map using ProductResponse.from(product, brand) to build content;
add tests covering both the successful 1:1 mapping case (metadata equals content
size) and the missing-brand case (assert CoreException is propagated through
ApiControllerAdvice to produce the consistent API error response).
In
`@apps/commerce-api/src/test/java/com/loopers/benchmark/ProductIndexBenchmarkTest.java`:
- Around line 48-60: The benchmark() flow leaves prior data and auxiliary
indexes in place; before seeding and before each strategy run you must reset the
table and indexes to a known baseline: call a TRUNCATE TABLE products and drop
all non-PK indexes (ensure only PRIMARY remains via SHOW INDEX) prior to
seedData() and again immediately before runStrategy(IndexStrategy) (or instead
create strategy-specific temp tables/isolation), then seed data, run
analyzeTable(), and verify row count and index list match the baseline (add
assertions that SHOW INDEX returns only PRIMARY and row count equals the
baseline) so repeated runs start from identical state; update benchmark(),
seedData(), and runStrategy() call sites to perform these resets and
validations.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductPageReadCacheTest.java`:
- Around line 80-99: Add a new unit test that verifies ProductPageCacheKey
includes page and size by calling pageReadCache.get with different page and size
combinations (e.g., page=0 vs page=1 and size=20 vs size=50) while using
ProductSortType (reuse ProductSortType.LATEST or PRICE_ASC) and distinct
PageResult payloads from createPage; for each unique (sort,page,size)
combination assert that the returned items match the expected payload and that
the loaderCallCount increments only when a cache miss occurs (use AtomicInteger
loaderCallCount in the lambda passed to pageReadCache.get to detect whether the
loader ran), ensuring different page/size combos produce separate cache entries
and no unintended sharing as enforced by ProductPageCacheKey.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductQueryServiceImplTest.java`:
- Around line 130-150: Update the test in ProductQueryServiceImplTest so it
returns a non-empty Page<Product> from
productJpaRepository.findAllByBrandIdAndDeletedAtIsNull (create at least one
Product with known fields) instead of the emptyPage, call
productQueryService.getAll(1L, ...) and assert the returned
PageResult<ProductReadModel> items contain correctly mapped fields (verify
Product -> ProductReadModel mapping), assert totalElements/totalPages match the
Page<Product> metadata, and verify the PageRequest used in the repository call
preserved the requested page, size and Sort (check Sort direction/field for the
passed ProductSortType); also add equivalent tests for ProductSortType.LATEST,
PRICE_ASC and LIKES_DESC to cover sort mapping and ensure
productPageReadCache.get is never invoked for brandId paths.
---
Outside diff comments:
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Controller.java`:
- Around line 31-39: Validate the incoming page and size at the start of
ProductV1Controller.getAll: if page is negative or size is <= 0, throw a
CoreException using the project’s 4xx ErrorType (e.g., BAD_REQUEST) so
ApiControllerAdvice emits the standard API error format; keep validation in the
controller (not in ProductQueryService) and then call
productQueryService.getAll(...) as before. Also add controller tests that call
getAll with page=-1 and size=0 and assert both produce the same API error
response shape handled by ApiControllerAdvice.
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductPageReadCache.java`:
- Around line 8-11: The ProductPageReadCache contract hides a brandId
precondition: update the interface (ProductPageReadCache) so the cache key
includes the brand identifier (add a Long brandId parameter to the get(...)
signature) or alternatively rename/limit the interface to explicitly indicate it
is brand-agnostic; ensure implementations use brandId when composing cache keys
and adjust evictAll() semantics if needed; and add a unit/integration test that
requests the same sort/page/size for two different brandIds and verifies they do
not return the same cached PageResult to prevent cross-brand bleed.
In `@apps/commerce-api/src/main/java/com/loopers/config/CacheConfig.java`:
- Around line 15-24: Update the productDetailCache bean to enable Caffeine
statistics by calling recordStats() on the Caffeine.newBuilder() chain and
expose those stats to Micrometer; specifically, inside the productDetailCache()
method add .recordStats() to the Caffeine builder and then register the cache
with Micrometer (e.g., via CaffeineCacheMetrics.monitor(...) or by exposing a
MeterBinder bean that binds the returned Cache<Long, ProductReadModel>), so
hit/miss/eviction metrics from productDetailCache are available to /metrics and
Prometheus.
In
`@apps/commerce-api/src/test/java/com/loopers/benchmark/ProductIndexBenchmarkTest.java`:
- Around line 26-28: The repository excludes benchmark tests via
excludeTags("benchmark") in build.gradle.kts so ProductIndexBenchmarkTest never
runs in normal test runs; add a dedicated Gradle Test task (e.g., named
"benchmark") in build.gradle.kts that includes tests with tag "benchmark" (use
includeTags("benchmark")), configures any needed JVM args or system properties
for heavy seeding, and wires its reports/output to the documented .docs/
benchmark report location so developers can run ./gradlew benchmark to execute
ProductIndexBenchmarkTest and generate the benchmark artifacts.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductReadCacheTest.java`:
- Around line 32-84: Add a concurrency test in CaffeineProductReadCacheTest to
ensure productReadCache.get(key, loader) invokes the loader only once under
simultaneous cache misses: spawn multiple threads that wait on a
CountDownLatch/CyclicBarrier, all call productReadCache.get(1L, () -> {
loaderCallCount.incrementAndGet(); return expected; }), then assert
loaderCallCount.get() == 1 and caffeineCache.getIfPresent(1L) == expected; also
add a test where the loader throws an exception (loader that throws) invoked
concurrently and assert that no value is left in caffeineCache
(caffeineCache.getIfPresent(key) is null) and subsequent calls can still load
normally.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductCacheIntegrationTest.java`:
- Around line 195-210: The test currently only verifies results after a write
and can miss whether brand-filtered queries are being cached; update
ProductCacheIntegrationTest to explicitly assert no page cache entry is created
for brand-filtered queries by calling productQueryService.getAll(brandId,
ProductSortType.LATEST, 0, 20) twice and then verifying the ProductPageReadCache
was not interacted with (e.g., assert cache size remains 0 or mock/spy
ProductPageReadCache.get(...) was never called), or add a complementary test in
ProductQueryServiceImplTest that spies/mocks ProductPageReadCache and asserts
ProductPageReadCache.get(...) is not invoked when brandId != null. Ensure you
reference the productQueryService.getAll(...) call and
ProductPageReadCache.get(...) in the assertions so the test fails if
brand-filtered pages are cached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7b078642-e51a-453b-aaf6-e5739a5e2d92
⛔ Files ignored due to path filters (7)
.docs/index-benchmark-100k.mdis excluded by!**/*.mdand included by**.docs/index-benchmark-1m.mdis excluded by!**/*.mdand included by**.docs/index-benchmark-200k.mdis excluded by!**/*.mdand included by**.docs/index-benchmark-500k.mdis excluded by!**/*.mdand included by**.docs/index-optimization-report.mdis excluded by!**/*.mdand included by**.docs/practical_read_opt.mdis excluded by!**/*.mdand included by**.docs/r5quest.mdis excluded by!**/*.mdand included by**
📒 Files selected for processing (20)
apps/commerce-api/build.gradle.ktsapps/commerce-api/src/main/java/com/loopers/application/product/ProductPageReadCache.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductQueryService.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductReadCache.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductReadModel.javaapps/commerce-api/src/main/java/com/loopers/config/CacheConfig.javaapps/commerce-api/src/main/java/com/loopers/domain/product/Product.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/CaffeineProductPageReadCache.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/CaffeineProductReadCache.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductQueryServiceImpl.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Dto.javaapps/commerce-api/src/test/java/com/loopers/ArchitectureTest.javaapps/commerce-api/src/test/java/com/loopers/benchmark/ProductIndexBenchmarkTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductPageReadCacheTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductReadCacheTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductCacheIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductQueryServiceImplTest.javabuild.gradle.kts
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductQueryServiceImpl.java
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Dto.java
Show resolved
Hide resolved
apps/commerce-api/src/test/java/com/loopers/benchmark/ProductIndexBenchmarkTest.java
Show resolved
Hide resolved
...e-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductPageReadCacheTest.java
Show resolved
Hide resolved
...mmerce-api/src/test/java/com/loopers/infrastructure/product/ProductQueryServiceImplTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.java (1)
55-63: 현재 코드베이스의 Bean Validation 사용 패턴과 일치하나, 프로젝트의 도메인 계층 검증 원칙과는 불일치한다.HandlerMethodValidationException 핸들러는 기존 MethodArgumentNotValidException 핸들러와 동일한 방식을 따르며, ProductV1Controller의
@Min/@max 어노테이션이@RequestParam에적용되어 있으므로 Spring 6+ 환경에서 이 예외는 발생한다. 따라서 핸들러 추가는 현재 구현 패턴과 일치한다.다만 프로젝트 학습 정보에 따르면 이 프로젝트는 도메인 계층에서 검증을 수행하고 Bean Validation 핸들러 추가를 지양해야 한다고 기록되어 있다. 현재 코드베이스는 UserV1Controller, OrderV1Controller, CartV1Controller 등 다수의 컨트롤러에서
@Valid를활용 중이므로, 이는 의도된 아키텍처와 괴리가 있다.추가로 현재 구현은
.findFirst()로 첫 번째 검증 오류만 반환한다. 운영 관점에서 복수의 검증 오류 발생 시 일부가 숨겨져 디버깅이 어려울 수 있으므로, 로그에는 전체 오류 목록을 기록하는 것을 권장한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.java` around lines 55 - 63, Either remove the HandlerMethodValidationException handler to adhere to the project's domain-validation guidance (i.e., delete the handleBadRequest method) or, if you keep it for controllers like ProductV1Controller where `@Min/`@Max on `@RequestParam` can trigger this exception, change handleBadRequest to aggregate and expose all errors instead of returning only the first: collect all error.getDefaultMessage() values (from e.getAllValidationResults() -> getResolvableErrors()) into a single joined message (e.g., join with "; "), pass that combined message into failureResponse(ErrorType.BAD_REQUEST, ...), and log the full list of validation errors using the existing logger so no errors are silently hidden; keep references to HandlerMethodValidationException, handleBadRequest, failureResponse, ErrorType.BAD_REQUEST, ProductV1Controller and MethodArgumentNotValidException in your changes.
🤖 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/infrastructure/product/ProductQueryServiceImpl.java`:
- Around line 18-20: Add a read-only transaction boundary to prevent routing
reads to the primary and disable JPA dirty checking: annotate the
ProductQueryServiceImpl class (the public class ProductQueryServiceImpl
implementing ProductQueryService) with `@Transactional`(readOnly = true) (or
alternatively annotate each read method in ProductQueryServiceImpl) so all read
operations are executed with readOnly=true; ensure you import
org.springframework.transaction.annotation.Transactional and run replica-routing
tests in environments with read replicas to verify queries hit the replica.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Controller.java`:
- Around line 52-55: In ProductV1Controller.getById, guard against
product.brandId() being null before calling brandApplicationService.getById;
either perform an explicit null check on product.brandId() and throw a
CoreException with a clear message (e.g., "brandId is null for product
{productId}") or treat Brand as optional by passing null-safe handling and
allowing ProductV1Dto.ProductResponse.from to accept a null Brand; update the
method to validate ProductReadModel.brandId(), call
brandApplicationService.getById only when non-null, and surface a meaningful
exception via CoreException (or return a null-safe response) instead of letting
a NPE or lower-level entity-not-found error occur.
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.java`:
- Around line 55-63: Either remove the HandlerMethodValidationException handler
to adhere to the project's domain-validation guidance (i.e., delete the
handleBadRequest method) or, if you keep it for controllers like
ProductV1Controller where `@Min/`@Max on `@RequestParam` can trigger this exception,
change handleBadRequest to aggregate and expose all errors instead of returning
only the first: collect all error.getDefaultMessage() values (from
e.getAllValidationResults() -> getResolvableErrors()) into a single joined
message (e.g., join with "; "), pass that combined message into
failureResponse(ErrorType.BAD_REQUEST, ...), and log the full list of validation
errors using the existing logger so no errors are silently hidden; keep
references to HandlerMethodValidationException, handleBadRequest,
failureResponse, ErrorType.BAD_REQUEST, ProductV1Controller and
MethodArgumentNotValidException in your changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05844756-b95f-4ae7-9f78-bc20be1cdbec
📒 Files selected for processing (6)
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductQueryServiceImpl.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/ApiControllerAdvice.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Dto.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductPageReadCacheTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductQueryServiceImplTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/commerce-api/src/test/java/com/loopers/infrastructure/product/ProductQueryServiceImplTest.java
- apps/commerce-api/src/test/java/com/loopers/infrastructure/product/CaffeineProductPageReadCacheTest.java
- apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Dto.java
Comment on lines
+18
to
+20
| @RequiredArgsConstructor | ||
| @Component | ||
| public class ProductQueryServiceImpl implements ProductQueryService { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
읽기 전용 서비스에 @Transactional(readOnly = true) 누락
운영 환경에서 read replica를 운영할 경우, readOnly = true 설정이 없으면 읽기 쿼리도 primary DB로 라우팅되어 불필요한 부하가 발생한다. 또한 JPA의 dirty checking이 비활성화되지 않아 메모리 낭비가 생긴다.
🔧 수정안
+import org.springframework.transaction.annotation.Transactional;
+
`@RequiredArgsConstructor`
`@Component`
+@Transactional(readOnly = true)
public class ProductQueryServiceImpl implements ProductQueryService {추가 검증: read replica가 설정된 환경에서 쿼리 로그를 확인하여 replica로 라우팅되는지 테스트 필요.
🤖 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/ProductQueryServiceImpl.java`
around lines 18 - 20, Add a read-only transaction boundary to prevent routing
reads to the primary and disable JPA dirty checking: annotate the
ProductQueryServiceImpl class (the public class ProductQueryServiceImpl
implementing ProductQueryService) with `@Transactional`(readOnly = true) (or
alternatively annotate each read method in ProductQueryServiceImpl) so all read
operations are executed with readOnly=true; ensure you import
org.springframework.transaction.annotation.Transactional and run replica-routing
tests in environments with read replicas to verify queries hit the replica.
Comment on lines
52
to
+55
| public ApiResponse<ProductV1Dto.ProductResponse> getById(@PathVariable Long productId) { | ||
| ProductWithBrand result = productApplicationService.getProductWithBrand(productId); | ||
| return ApiResponse.success(ProductV1Dto.ProductResponse.from(result.product(), result.brand())); | ||
| ProductReadModel product = productQueryService.getById(productId); | ||
| Brand brand = brandApplicationService.getById(product.brandId()); | ||
| return ApiResponse.success(ProductV1Dto.ProductResponse.from(product, brand)); |
There was a problem hiding this comment.
getById에서도 동일한 null brandId 문제가 존재한다.
product.brandId()가 null인 경우 brandApplicationService.getById(null) 호출 시 NPE 또는 엔티티 조회 실패 예외가 발생할 수 있다. 장애 시 원인 추적을 위해 명시적인 null 체크와 의미 있는 예외 메시지가 필요하다.
수정안: brandId null 체크 후 적절한 CoreException을 던지거나, Brand가 선택적인 경우 null-safe 처리를 구현한다.
ProductReadModel product = productQueryService.getById(productId);
+if (product.brandId() == null) {
+ throw new CoreException(ErrorType.INTERNAL_SERVER_ERROR, "상품의 브랜드 정보가 누락되었습니다.");
+}
Brand brand = brandApplicationService.getById(product.brandId());📝 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
| public ApiResponse<ProductV1Dto.ProductResponse> getById(@PathVariable Long productId) { | |
| ProductWithBrand result = productApplicationService.getProductWithBrand(productId); | |
| return ApiResponse.success(ProductV1Dto.ProductResponse.from(result.product(), result.brand())); | |
| ProductReadModel product = productQueryService.getById(productId); | |
| Brand brand = brandApplicationService.getById(product.brandId()); | |
| return ApiResponse.success(ProductV1Dto.ProductResponse.from(product, brand)); | |
| public ApiResponse<ProductV1Dto.ProductResponse> getById(`@PathVariable` Long productId) { | |
| ProductReadModel product = productQueryService.getById(productId); | |
| if (product.brandId() == null) { | |
| throw new CoreException(ErrorType.INTERNAL_SERVER_ERROR, "상품의 브랜드 정보가 누락되었습니다."); | |
| } | |
| Brand brand = brandApplicationService.getById(product.brandId()); | |
| return ApiResponse.success(ProductV1Dto.ProductResponse.from(product, brand)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Controller.java`
around lines 52 - 55, In ProductV1Controller.getById, guard against
product.brandId() being null before calling brandApplicationService.getById;
either perform an explicit null check on product.brandId() and throw a
CoreException with a clear message (e.g., "brandId is null for product
{productId}") or treat Brand as optional by passing null-safe handling and
allowing ProductV1Dto.ProductResponse.from to accept a null Brand; update the
method to validate ProductReadModel.brandId(), call
brandApplicationService.getById only when non-null, and surface a meaningful
exception via CoreException (or return a null-safe response) instead of letting
a NPE or lower-level entity-not-found error 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.
📌 Summary
🧭 Context & Decision
문제 정의
선택지와 결정
인덱스 전략
4가지 전략을 10만~100만건 데이터에서 직접 측정해서 비교했습니다.
(brand_id)— 데이터가 늘어나면 오히려 Baseline보다 느려집니다 (인덱스 랜덤 I/O + 대량 filesort)(brand_id, deleted_at, like_count DESC)— 좋아요순만 커버하고, 가격순/최신순은 여전히 filesort가 발생합니다캐시 전략
@Cacheable— 프레임워크에 의존하게 되고, 무효화 타이밍을 제어하기 어렵습니다ProductReadCache/ProductPageReadCache인터페이스를 Application 계층에 두고, Caffeine 구현체를 Infrastructure에 배치했습니다.캐시 무효화
🏗️ Design Overview
변경 범위
주요 컴포넌트 책임
ProductReadModel: 불변(record) 읽기 전용 모델입니다. Product 엔티티를 캐시 가능한 형태로 변환합니다. 엔티티를 직접 캐시하면 영속성 컨텍스트와 충돌할 수 있어서 별도 record로 분리했습니다.ProductReadCache: 상품 상세 조회 캐시 추상화입니다.get(id, loader)패턴으로 cache-aside를 구현합니다.ProductPageReadCache: 전체 목록 조회 캐시 추상화입니다. 브랜드 필터 없는 전체 조회만 캐시합니다.CaffeineProductReadCache/CaffeineProductPageReadCache: Caffeine 기반 로컬 캐시 구현체입니다. 최대 크기와 TTL을 프로퍼티로 설정할 수 있습니다.ProductQueryServiceImpl: 읽기 전용 조회를 담당하는 서비스입니다. 캐시 조회 → miss 시 DB 조회 → ProductReadModel 변환 흐름을 처리합니다.ProductRepositoryImpl: 쓰기 작업(save, softDelete, like 증감) 시 afterCommit 콜백으로 관련 캐시를 무효화합니다.🔁 Flow Diagram
Main Flow — 상품 상세 조회 (캐시)
sequenceDiagram autonumber participant Client participant Controller participant QueryService participant ReadCache participant DB Client->>Controller: GET /products/{id} Controller->>QueryService: getById(id) QueryService->>ReadCache: get(id, loader) alt cache hit ReadCache-->>QueryService: ProductReadModel else cache miss ReadCache->>DB: findByIdAndDeletedAtIsNull(id) DB-->>ReadCache: Product ReadCache-->>QueryService: ProductReadModel end QueryService-->>Controller: ProductReadModel Controller-->>Client: ResponseMain Flow — 전체 목록 조회 (캐시)
sequenceDiagram autonumber participant Client participant Controller participant QueryService participant PageCache participant DB Client->>Controller: GET /products?sort=LIKES_DESC Controller->>QueryService: getAll(null, sort, page, size) QueryService->>PageCache: get(sort, page, size, loader) alt cache hit PageCache-->>QueryService: PageResult<ProductReadModel> else cache miss PageCache->>DB: findAllByDeletedAtIsNull(pageable) DB-->>PageCache: Page<Product> PageCache-->>QueryService: PageResult<ProductReadModel> end QueryService-->>Controller: PageResult<ProductReadModel> Controller-->>Client: Response캐시 무효화 흐름
📊 인덱스 벤치마크 결과
10만~100만건 데이터에서 4가지 인덱스 전략을 비교 측정했습니다. 상세 보고서는
.docs/index-optimization-report.md를 참고해주세요.핵심 결과 (100만건 기준)
(brand_id)단일 인덱스는 데이터가 늘어나면 오히려 Baseline보다 느려졌습니다. 필터만 되고 정렬을 커버하지 못하는 인덱스의 함정입니다.
선정한 인덱스 4개
설계 근거: 컬럼 순서를
(등치 조건) → (등치 조건) → (정렬 컬럼)으로 배치했습니다. MySQL 8.0에서IS NULL은 등치 조건(ref)으로 처리되기 때문에 인덱스 정렬까지 활용할 수 있습니다.🔮 현재 구조와 향후 계획
현재 유지하는 것들
ProductReadCache로 개별 상품 조회를 캐시합니다ProductPageReadCache로 brandId가 null인 전체 조회만 캐시합니다쓰기 이벤트별 무효화 현황
상황별 다음 단계
지금 당장 추가 개선은 필요 없고, 운영하면서 응답 시간·DB 부하·캐시 hit rate를 관찰한 뒤 이상 징후가 보일 때 다음을 검토할 생각입니다.
evictAll()을 정렬/페이지 단위 무효화로 세분화하는 걸 검토합니다변경 목적
상품 도메인 조회 성능을 개선하기 위해 10만~100만건 규모 벤치마크를 통해 인덱스 전략을 선정하고, 전체 목록/상세 조회 응답을 Caffeine 로컬 캐시로 보완합니다.
핵심 변경점
복합 인덱스 4개 추가: (brand_id, deleted_at, like_count DESC), (brand_id, deleted_at, price ASC), (brand_id, deleted_at, created_at DESC), (deleted_at, like_count DESC). Application 레이어에 ProductReadCache/ProductPageReadCache 추상화 추가 및 Infrastructure에 Caffeine 구현체(CaffeineProductReadCache, CaffeineProductPageReadCache) 도입; 상세 캐시 개별 evict, 페이지 캐시는 evictAll; 트랜잭션 커밋 후(afterCommit) 무효화 로직 적용. ProductQueryService 도입으로 CQRS식 조회 경로 분리.
리스크/주의사항
인덱스만으로는 "전체 목록 + 가격/최신순" 일부 쿼리가 캐시 없이는 여전히 비용이 클 수 있으며(벤치마크에서 일부 쿼수십~수백 ms), 캐시 미스 빈도가 높으면 성능 저하 우려가 있음. 인덱스 추가/변경은 운영 DB에 대한 DDL 영향(잠금/IO)을 수반합니다. 확인이 필요한 부분: 운영에서 전체 목록(brandId=null)·가격·최신 정렬 쿼리의 실제 트래픽 비중과 현재 캐시 적중률 모니터링 계획은 어떻게 되나요?
테스트/검증
벤치마크 리포트(.docs/index-optimization-report.md)에 쿼리별 성능/EXPLAIN 및 DDL 기록 포함. 단위 테스트로 Caffeine 캐시 동작(CaffeineProductReadCacheTest, CaffeineProductPageReadCacheTest) 검증, 통합 테스트(ProductCacheIntegrationTest)로 트랜잭션 이후 캐시 무효화(상품 수정/삭제, 브랜드 삭제, 페이지 캐시 갱신) 시나리오 검증. ArchUnit 규칙 추가로 인프라/설정이 ApplicationService에 직접 의존하지 않도록 검증.