[volume-5] 인덱스와 캐시를 활용한 상품 조회 읽기 성능 최적화#199
Open
madirony wants to merge 8 commits intoLoopers-dev-lab:madironyfrom
Open
[volume-5] 인덱스와 캐시를 활용한 상품 조회 읽기 성능 최적화#199madirony wants to merge 8 commits intoLoopers-dev-lab:madironyfrom
madirony wants to merge 8 commits intoLoopers-dev-lab:madironyfrom
Conversation
- subtract()에서 IllegalArgumentException 대신 CoreException(BAD_REQUEST) 사용 - @JsonCreator/@JsonProperty 추가로 Redis 캐시 직렬화/역직렬화 지원
- (brand_id, deleted, like_count DESC) 복합 인덱스 추가 — fullscan 제거 - likeCount 증감 도메인 메서드 제거 — DB 원자적 @Modifying 쿼리로 대체 완료 - findByBrandIdWithPaging 페이징 쿼리 추가 (likeCount DESC 정렬) - ddl-auto를 update로 변경하여 인덱스 자동 생성 지원
- ProductCacheManager 인터페이스를 application 계층에 정의 (DIP) - RedisProductCacheManager를 infrastructure 계층에 구현 - TTL Jitter 적용 (목록 60+1~10초, 상세 300+1~30초) — Hot Key Stampede 방어 - 예외 흡수 후 Optional.empty() 반환 — DB fallback 보장 - CachedBrandProductPage, CachedProductDetail 캐시 전용 DTO 추가 - RedisConfig에 Fail-Fast 타임아웃 설정 (COMMAND 500ms, CONNECT 300ms)
- ProductAppService에 Cache-Aside 조회 메서드 추가 (상세/목록) - page 0~2만 캐싱하여 Cache Pollution 방지 (CACHEABLE_PAGE_LIMIT = 2) - AdminProductAppService update/delete 시 evictProductCaches 호출 - ProductInfo에 @Jacksonized 추가로 캐시 역직렬화 지원 - 단위 테스트: 캐시 히트/미스/딥페이지 바이패스 3건 추가 - 통합 테스트: 브랜드별 페이징 조회 검증 4건 추가
- GET /api/v1/brands/{brandId}/products 엔드포인트 신규 추가
- ProductFacade 상세 조회를 CachedProductDetail 기반으로 전환
- ProductFacade 브랜드별 목록 조회 오케스트레이션 추가
- BrandProductResponse/BrandProductListResponse DTO 추가
- Deep Paging 방지: page 0~50 범위 제한 (MAX_PAGE = 50)
- Facade 단위 테스트: 캐시 기반 상세 조회 + 브랜드별 목록 조회 검증
- E2E 테스트: 정렬/페이징/브랜드 필터링/에러 케이스 5건 추가
📝 WalkthroughWalkthrough제품 목록·상세 조회에 Redis 기반 캐싱과 캐시 DTO/인터페이스 및 Redis 구현을 추가하고, ProductAppService/AdminProductAppService/ProductFacade/Controller에 캐시 사용과 무효화 로직을 통합했으며 페이징·인덱스·직렬화 및 테스트가 변경되었다다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant Facade
participant AppService
participant CacheManager
participant Redis
participant Repository
participant Database
rect rgba(100,150,200,0.5)
Note over Client,Database: 제품 상세 조회 (캐시 우선)
Client->>Controller: GET /api/v1/products/{id}
Controller->>Facade: getProductDetail(productId)
Facade->>AppService: getProductDetailCached(productId)
AppService->>CacheManager: getProductDetail(productId)
CacheManager->>Redis: GET product:detail:{id}
alt 캐시 Hit
Redis-->>CacheManager: CachedProductDetail
CacheManager-->>AppService: Optional[CachedProductDetail]
else 캐시 Miss
CacheManager-->>AppService: Optional.empty()
AppService->>Repository: findById(productId)
Repository->>Database: SELECT Product
Database-->>Repository: Product
AppService->>AppService: assemble options & likeCount
AppService->>CacheManager: putProductDetail(productId, detail)
CacheManager->>Redis: SET product:detail:{id} (TTL 300s+jitter)
end
AppService-->>Facade: CachedProductDetail
Facade->>Controller: ProductInfo
Controller-->>Client: ApiResponse<ProductDto>
end
rect rgba(200,100,150,0.5)
Note over Client,Database: 브랜드별 제품 목록 조회 (페이징 + 캐시)
Client->>Controller: GET /api/v1/brands/{brandId}/products?page=0&size=20
Controller->>Facade: getProductsByBrand(brandId,page,size)
Facade->>AppService: getProductsByBrandIdCached(brandId,page,size)
AppService->>CacheManager: getProductList(brandId,page,size)
CacheManager->>Redis: GET product:list:brand:{brandId}:page:{page}
alt 페이지 0-1 & 캐시 Hit
Redis-->>CacheManager: CachedBrandProductPage
CacheManager-->>AppService: Optional[CachedBrandProductPage]
else 캐시 Miss or 깊은 페이지
CacheManager-->>AppService: Optional.empty()
AppService->>Repository: findByBrandIdWithPaging(brandId, pageable)
Repository->>Database: SELECT Products ORDER BY like_count DESC
Database-->>Repository: Page<Product>
AppService->>AppService: map to summaries
alt 페이지 0-1
AppService->>CacheManager: putProductList(brandId,page,cachedPage)
CacheManager->>Redis: SET product:list:... (TTL 60s+jitter)
end
end
AppService-->>Facade: CachedBrandProductPage
Facade-->>Controller: BrandProductListResponse
Controller-->>Client: ApiResponse<BrandProductListResponse>
end
rect rgba(150,200,100,0.5)
Note over Client,Redis: 제품 수정/삭제 시 캐시 무효화
Client->>Controller: PUT/DELETE /api/v1/admin/products/{id}
Controller->>AppService: update/delete(productId)
AppService->>Repository: save/delete product
Repository->>Database: UPDATE/DELETE Product
AppService->>CacheManager: evictProductCaches(productId, brandId)
CacheManager->>Redis: DEL product:detail:{id}
CacheManager->>Redis: DEL product:list:brand:{brandId}:page:{n}
Redis-->>CacheManager: OK
CacheManager-->>AppService: void
AppService-->>Controller: success
Controller-->>Client: ApiResponse<Void>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 CodeRabbit can enforce grammar and style rules using `languagetool`.Configure 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
modules/jpa/src/main/resources/jpa.yml (1)
34-40:⚠️ Potential issue | 🟠 Major공용 JPA 설정에서
ddl-auto: update를 켜면 로컬 스키마 변경이 비결정적으로 진행된다.
modules/jpa/src/main/resources/jpa.yml40행의 변경으로local프로필의ddl-auto가update로 설정되었다. 이 설정은apps/commerce-api/src/main/resources/application.yml과apps/commerce-batch/src/main/resources/application.yml양쪽에서 함께 import되므로, 여러 애플리케이션이 같은 데이터베이스에 연결될 때 어느 앱을 먼저 기동했는지, 기존 스키마 상태는 어떤지에 따라 추가 DDL이 점진적으로 누적된다. 이로 인해 성능 측정, 장애 재현, 로컬 검증 결과가 환경마다 달라져 운영 재현성이 손상된다.수정안:
- 공용 설정(
modules/jpa/src/main/resources/jpa.yml)은ddl-auto: none으로 유지하고, 이번 인덱스 추가(idx_products_brand_deleted_likes)는 Flyway나 Liquibase 같은 버전드 마이그레이션으로 관리할 것.- 로컬 편의성이 필수라면
local-ddl같은 별도 프로필을 생성해 공용local프로필과 분리할 것.추가 테스트:
- 빈 데이터베이스 상태에서 기동 후
INFORMATION_SCHEMA.STATISTICS에서idx_products_brand_deleted_likes인덱스의 존재 및 컬럼 구성을 검증하는 통합 테스트 작성.commerce-api와commerce-batch를 같은 로컬 DB에 기동 순서만 바꿔서 실행해도 추가 DDL diff가 발생하지 않는지 확인하는 설정 회귀 테스트 작성.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/jpa/src/main/resources/jpa.yml` around lines 34 - 40, The shared JPA profile currently sets spring.jpa.hibernate.ddl-auto: update for the local profile — change spring.jpa.hibernate.ddl-auto to none in the shared JPA config and remove index DDL from runtime schema auto-update; instead create a versioned migration (Flyway/Liquibase) that adds idx_products_brand_deleted_likes and run it in CI/deploy pipelines; if local convenience is required, add a separate profile (e.g. local-ddl) that sets ddl-auto to update so it does not affect the shared local profile; add integration tests that (1) start from an empty DB and assert idx_products_brand_deleted_likes exists with the correct columns via INFORMATION_SCHEMA.STATISTICS and (2) start commerce-api and commerce-batch in both orders against the same DB to ensure no runtime DDL diffs occur.modules/redis/src/main/java/com/loopers/config/redis/RedisConfig.java (1)
43-46:⚠️ Potential issue | 🟠 MajorRedis 캐시 무효화 직후 stale read 위험이 존재한다.
운영 관점에서
evictProductCaches()가 마스터에 반영된 직후, 같은 replica-preferred 템플릿을 통한getProductDetail()이 수행되면, 레플리카 동기화 지연 시간 동안 오래된 값이 응답될 수 있다. 현재RedisProductCacheManager는@Qualifier없이@PrimaryRedisTemplate(ReadFrom.REPLICA_PREFERRED)을 주입받으므로, 읽기 경로가 레플리카를 우선한다.조치 사항:
RedisProductCacheManager에@Qualifier(REDIS_TEMPLATE_MASTER)주입하여 읽기 템플릿을 마스터 전용으로 변경한다.- 상품 삭제 후 즉시 캐시 조회 시 이전 데이터를 반환하지 않음을 검증하는 통합 테스트를 추가한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/redis/src/main/java/com/loopers/config/redis/RedisConfig.java` around lines 43 - 46, RedisTemplate bean in RedisConfig is configured with ReadFrom.REPLICA_PREFERRED, causing stale reads after evictProductCaches(); change injection in RedisProductCacheManager to use the master-only template by adding `@Qualifier`(REDIS_TEMPLATE_MASTER) (reference the REDIS_TEMPLATE_MASTER constant and the RedisProductCacheManager class) so reads after evictions go to the master, and add an integration test that calls evictProductCaches() then immediately calls getProductDetail() and asserts the deleted/updated value is not returned (verify master consistency).apps/commerce-api/src/main/java/com/loopers/application/admin/product/AdminProductAppService.java (1)
53-60:⚠️ Potential issue | 🟠 Major브랜드 삭제 시 캐시 무효화 누락
deleteByBrandId()메서드에서 상품들을 삭제하면서 캐시 무효화를 수행하지 않는다. 이로 인해 삭제된 상품이 캐시에 남아 사용자에게 노출될 수 있다.🐛 수정안
`@Transactional` public void deleteByBrandId(Long brandId) { List<Product> products = productRepository.findByBrandId(brandId); for (Product product : products) { product.delete(); deleteOptionsByProductId(product.getId()); + productCacheManager.evictProductCaches(product.getId(), 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/admin/product/AdminProductAppService.java` around lines 53 - 60, deleteByBrandId currently deletes Product entities via product.delete() and deleteOptionsByProductId(product.getId()) but does not evict related cache entries; add cache invalidation calls for each deleted product (e.g. invoke your product cache service such as productCacheService.evict(product.getId()) or cacheService.invalidateProductCache(product.getId())) immediately after deleteOptionsByProductId(product.getId()), and also invalidate any brand-level or collection caches (e.g. productListCache.evict(brandId) or similar) so removed products are not served from cache; update the method deleteByBrandId(Long brandId) to call these cache eviction methods for symbols product.delete(), deleteOptionsByProductId(...), and the appropriate cache service methods.
🧹 Nitpick comments (11)
apps/commerce-api/src/main/java/com/loopers/application/product/ProductInfo.java (1)
48-59:withLikedByUser()가 옵션 리스트 참조를 그대로 재사용한다.운영 관점에서는 역직렬화된
options가 mutable list이면, 한 응답 객체의 옵션 변경이 다른 복사본이나 캐시된 객체에 전파되어 요청 간 데이터 오염이 생길 수 있다.options는 생성 시점과 복사 시점 모두List.copyOf(...)로 방어적 복사를 적용하는 편이 안전하다. 추가로 mutable list로 생성한 뒤 원본 리스트를 변경해도ProductInfo와withLikedByUser()결과가 변하지 않는지 테스트를 보강하는 것이 좋다.As per coding guidelines,
**/*.java: 'null 처리, 방어적 복사, 불변성, equals/hashCode/toString 구현 안정성을 점검한다.'🤖 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/ProductInfo.java` around lines 48 - 59, withLikedByUser currently reuses the same options list reference, risking shared-mutable state; change ProductInfo.withLikedByUser to defensively copy the options (e.g., use List.copyOf(this.options) or List.copyOf(Optional.ofNullable(this.options).orElse(Collections.emptyList()))) when passing to the builder and ensure the ProductInfo builder/constructor also applies List.copyOf to its options field and handles null safely; add/strengthen a unit test that mutates an original mutable options list after creating ProductInfo and after calling withLikedByUser to assert neither ProductInfo instance is affected.apps/commerce-api/src/main/java/com/loopers/domain/product/ProductRepository.java (1)
3-4: 도메인 저장소 계약에 Spring Data 타입이 스며든다.운영 관점에서는
Page/Pageable가 도메인 계층까지 올라오면 테스트 대역 작성과 계층 분리가 어려워지고, 이후 페이징 정책 변경 시 도메인 계약까지 함께 흔들린다.ProductPageQuery/ProductPageResult같은 도메인 전용 타입으로 계약을 유지하고,ProductRepositoryImpl에서 Spring Data 타입으로 변환하는 편이 안전하다. 추가로 Spring 클래스 없이도 저장소 계약을 검증할 수 있는 단위 테스트를 보강하는 것이 좋다.As per coding guidelines,
**/domain/**/*.java: '도메인 규칙과 인프라 관심사가 섞이면 분리하도록 제안한다.'Also applies to: 16-16
🤖 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/ProductRepository.java` around lines 3 - 4, ProductRepository currently exposes Spring Data types (org.springframework.data.domain.Page/Pageable); change the repository contract to use domain-specific DTOs (e.g., ProductPageQuery and ProductPageResult) and move the Spring-specific conversions into ProductRepositoryImpl where you translate ProductPageQuery → Pageable and Page → ProductPageResult; update or add unit tests that validate ProductRepository behavior using these domain types (without importing Spring classes) and keep the Spring Data dependencies confined to ProductRepositoryImpl implementation.modules/redis/src/main/java/com/loopers/config/redis/RedisConfig.java (1)
28-29: Redis 타임아웃이 코드에 고정되어 있다.운영 관점에서는 네트워크 RTT와 장애 허용치가 환경마다 달라
300ms/500ms를 상수로 박아두면 장애 시 즉시 튜닝할 수 없고, 피크 시간대에 DB fallback이 급증할 수 있다.RedisProperties로 외부화해서 환경별 override가 가능하게 하고, 코드에는 기본값만 두는 편이 안전하다. 추가로 properties binding 테스트와 느린 Redis 응답 조건에서 fallback 임계값을 검증하는 통합 테스트를 보강하는 것이 좋다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/redis/src/main/java/com/loopers/config/redis/RedisConfig.java` around lines 28 - 29, The hardcoded Redis timeouts in RedisConfig (COMMAND_TIMEOUT and CONNECT_TIMEOUT) should be externalized into configurable RedisProperties with sensible defaults; update RedisConfig to inject RedisProperties and use properties.getCommandTimeout()/getConnectTimeout() (falling back to Duration.ofMillis(500) and Duration.ofMillis(300) only as defaults), add a properties binding unit test to verify values are read from configuration into RedisProperties, and add/instrument an integration test that simulates slow Redis responses to assert the fallback/threshold behavior uses the configured timeouts rather than the hardcoded constants.apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceIntegrationTest.java (1)
75-92: 전체 페이지 초과 요청 테스트 누락
totalPages가 2인 상황에서page=5와 같이 존재하지 않는 페이지 요청 시의 동작 테스트가 누락되어 있다. 빈 리스트 반환인지 예외 발생인지 명확히 검증해야 한다.추가 테스트:
`@Test` `@DisplayName`("존재하지 않는 페이지 요청 시 빈 결과를 반환한다") void getProductsByBrandId_pageOutOfRange() { // given productRepository.save(Product.create(testBrand.getId(), "상품", Money.of(BigDecimal.valueOf(1000)))); // when Page<Product> result = productAppService.getProductsByBrandId(testBrand.getId(), 100, 20); // then assertThat(result.getContent()).isEmpty(); assertThat(result.getTotalElements()).isEqualTo(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceIntegrationTest.java` around lines 75 - 92, Add an integration test to verify out-of-range paging behavior for ProductAppService#getProductsByBrandId: create at least one product via productRepository.save(...), call productAppService.getProductsByBrandId(testBrand.getId(), 100, 20) (or another clearly out-of-range page index), and assert that the returned Page has an empty content list and that totalElements still equals the actual total (e.g., 1); place this new test in ProductAppServiceIntegrationTest with a descriptive `@DisplayName` like "존재하지 않는 페이지 요청 시 빈 결과를 반환한다".apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java (2)
33-40: 브랜드 존재 여부 검증 순서
getProductsByBrand()에서 상품 조회 후 브랜드를 조회하는데, 존재하지 않는 브랜드 ID로 요청 시 상품 조회(캐시 저장 포함)가 먼저 수행된 후 브랜드 조회에서 예외가 발생한다. 불필요한 DB/캐시 연산이 발생하므로 브랜드 존재 여부를 먼저 검증하는 것이 효율적이다.♻️ 수정안
public Page<ProductInfo> getProductsByBrand(Long brandId, int page, int size) { + Brand brand = brandAppService.getById(brandId); CachedBrandProductPage cached = productAppService.getProductsByBrandIdCached(brandId, page, size); - Brand brand = brandAppService.getById(brandId); List<ProductInfo> content = cached.getContent().stream() .map(summary -> toProductInfo(summary, brand)) .toList(); return new PageImpl<>(content, PageRequest.of(page, size), cached.getTotalElements()); }🤖 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 33 - 40, getProductsByBrand currently fetches cached products before verifying the brand exists, causing wasted DB/cache work when brandId is invalid; change the call order in ProductFacade.getProductsByBrand to first validate the brand via brandAppService.getById(brandId) (or a brand existence check method) and only then call productAppService.getProductsByBrandIdCached(brandId, page, size), then map summaries with toProductInfo(summary, brand) as before to avoid unnecessary cache/storage operations for non-existent brands.
84-96: 브랜드 상품 목록에서 사용자 좋아요 상태 미반영
toProductInfo(ProductSummary, Brand)메서드에서likedByUser가 항상false로 설정된다. PR 목표상 캐시 성능 최적화를 위한 의도적 설계로 보이나, API 응답에서 해당 필드가 항상 false인 점을 API 문서에 명시하거나 응답 DTO에서 해당 필드를 제외하는 것이 클라이언트 혼란을 방지한다.🤖 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 84 - 96, toProductInfo currently hardcodes ProductInfo.likedByUser to false, causing API responses to always show not-liked; change the mapping in toProductInfo(CachedBrandProductPage.ProductSummary summary, Brand brand) to set likedByUser from the summary (e.g. summary.isLikedByUser() or summary.getLikedByUser()); if ProductSummary has no liked flag, add one to CachedBrandProductPage.ProductSummary (and populate it where the cache is built) so toProductInfo can read it, otherwise remove the likedByUser field from ProductInfo or explicitly document its always-false behavior in the API docs.apps/commerce-api/src/test/java/com/loopers/interfaces/api/BrandProductApiE2ETest.java (2)
60-95: 빈 브랜드 조회 테스트 누락브랜드에 상품이 없는 경우의 테스트가 누락되어 있다. 운영 환경에서 신규 브랜드 등록 직후 조회 시 빈 목록이 정상 반환되는지 검증이 필요하다.
추가 테스트 케이스:
`@DisplayName`("상품이 없는 브랜드 조회 시 빈 목록을 반환한다.") `@Test` void getBrandProducts_emptyBrand() { // act (상품 없이 조회) ResponseEntity<ApiResponse<ProductDto.BrandProductListResponse>> response = testRestTemplate.exchange( "/api/v1/brands/" + testBrand.getId() + "/products?page=0", HttpMethod.GET, null, new ParameterizedTypeReference<>() {} ); // assert assertAll( () -> assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK), () -> assertThat(response.getBody().data().products()).isEmpty(), () -> assertThat(response.getBody().data().totalElements()).isEqualTo(0) ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/interfaces/api/BrandProductApiE2ETest.java` around lines 60 - 95, Add a new test method getBrandProducts_emptyBrand in BrandProductApiE2ETest that calls testRestTemplate.exchange on "/api/v1/brands/" + testBrand.getId() + "/products?page=0" without creating any Product entities, and assert the response is HttpStatus.OK, response.getBody().data().products() is empty, and response.getBody().data().totalElements() == 0; place it alongside getBrandProducts_success and use the same ParameterizedTypeReference<> and ApiResponse<ProductDto.BrandProductListResponse> types so the test verifies an empty product list for a newly registered brand.
159-175: 경계값 테스트 누락
page=51에 대한 400 응답 테스트는 존재하나,page=50(허용 범위 최대값)에서 정상 동작하는지 확인하는 경계값 테스트가 누락되어 있다. 경계값에서의 동작 검증은 off-by-one 오류 방지에 필수적이다.추가 테스트 케이스:
`@DisplayName`("허용 범위 최대 페이지(50) 요청 시 정상 응답한다.") `@Test` void getBrandProducts_maxPage_success() { // act ResponseEntity<ApiResponse<ProductDto.BrandProductListResponse>> response = testRestTemplate.exchange( "/api/v1/brands/" + testBrand.getId() + "/products?page=50", HttpMethod.GET, null, new ParameterizedTypeReference<>() {} ); // assert assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); }🤖 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/BrandProductApiE2ETest.java` around lines 159 - 175, Add a boundary test to BrandProductApiE2ETest to verify the maximum allowed page (50) returns 200 OK: create a new test method getBrandProducts_maxPage_success that uses testRestTemplate.exchange with "/api/v1/brands/" + testBrand.getId() + "/products?page=50", the same HttpMethod.GET and ParameterizedTypeReference<ProductDto.BrandProductListResponse>, then assert response.getStatusCode() is HttpStatus.OK (and optionally validate response body metadata/result if desired) to ensure off-by-one errors are caught.apps/commerce-api/src/main/java/com/loopers/application/admin/product/AdminProductAppService.java (1)
37-43: 트랜잭션 롤백 시 캐시 불일치 가능성
evictProductCaches()가 트랜잭션 내부에서 호출되므로, 이후 로직에서 예외 발생 시 DB는 롤백되나 캐시는 이미 삭제된 상태가 된다. 이로 인해 캐시 미스 후 DB에서 변경 전 데이터를 다시 캐싱하게 되어 정합성에 문제가 없으나, 불필요한 캐시 무효화가 발생한다.권장 수정안:
@TransactionalEventListener(phase = AFTER_COMMIT)를 활용하여 커밋 후 캐시 무효화를 수행하거나, 현재 구조를 유지할 경우 이 트레이드오프를 문서화할 것.🤖 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/admin/product/AdminProductAppService.java` around lines 37 - 43, The update method in AdminProductAppService currently calls productCacheManager.evictProductCaches(id, product.getBrandId()) inside the `@Transactional` method, risking cache eviction before commit; refactor to perform eviction after successful commit by publishing a transactional event (e.g., create and publish a ProductUpdatedEvent in update) and handle it in a separate listener annotated with `@TransactionalEventListener`(phase = AFTER_COMMIT) which invokes productCacheManager.evictProductCaches using the event payload; alternatively, if you keep eviction inside update, add an explicit comment/documentation in AdminProductAppService.update explaining the rollback vs cache-tradeoff.apps/commerce-api/src/main/java/com/loopers/application/product/CachedBrandProductPage.java (1)
21-37: deleted 필드 캐싱의 불필요성
ProductSummary에서deleted필드를 캐싱하고 있으나, DB 쿼리에서 이미deleted=false조건으로 필터링하므로 캐시된 데이터는 항상deleted=false이다. 캐시 용량 낭비이며, 만약 삭제된 상품이 캐시에 남아있다면 캐시 무효화 로직에 버그가 있는 것이다.
ProductFacade.toProductInfo()에서도 이 필드를 사용하므로, 제거 시 연관 코드도 함께 수정 필요하다. 현재 구조 유지 시 방어적 코딩으로 간주할 수 있으나, 캐시 크기 최적화가 필요할 경우 제거 고려.🤖 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/CachedBrandProductPage.java` around lines 21 - 37, ProductSummary caches a redundant deleted flag (always false due to DB filtering); remove the deleted field and its getter/builder usage from CachedBrandProductPage.ProductSummary and its from(Product) mapper, then update any consumers (notably ProductFacade.toProductInfo) to stop reading ProductSummary.deleted — either derive deleted from the live Product/DB where needed or stop passing it altogether; ensure builders, constructors, and imports are cleaned up after removal.apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.java (1)
126-166: 브랜드 목록 테스트가 페이징 회귀를 충분히 막지 못한다.지금은 내용과
totalElements만 확인해서 페이지 번호, 페이지 크기, 총 페이지 수 계산이 틀려도 테스트가 통과한다. 운영에서는 이 메타데이터가 깨지면 클라이언트의 페이지 이동이 바로 망가진다. 성공 케이스 assertion에 페이징 메타데이터를 포함하고, 빈 페이지와 마지막 페이지 같은 경계 케이스를 별도 테스트로 추가하는 편이 안전하다. 추가 테스트로totalElements=21,size=20일 때 총 페이지 수가 2인지, 마지막 페이지 이후 요청에서 빈 결과가 일관되게 내려오는지 검증해야 한다. 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/application/product/ProductFacadeTest.java` around lines 126 - 166, Update ProductFacadeTest to assert paging metadata and add boundary-case tests: inside getProductsByBrand_success (and/or a new test), assert that the returned page's page number, page size, and totalPages match expectations based on the CachedBrandProductPage (use productFacade.getProductsByBrand result to check getNumber(), getSize(), getTotalPages(), getTotalElements()); add separate tests that mock CachedBrandProductPage with totalElements=21 and page size 20 to assert totalPages == 2 and that requesting page 2 returns the remaining item while requesting page 3 returns an empty content list; reuse symbols CachedBrandProductPage, productAppService.getProductsByBrandIdCached(...), brandAppService.getById(...), and productFacade.getProductsByBrand(...) to locate where to add/adjust assertions and new test methods.
🤖 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/CachedProductDetail.java`:
- Around line 24-34: CachedProductDetail.from currently assumes the options
parameter is non-null and will NPE when options is null; update the method
(CachedProductDetail.from) to defensively handle a null options by treating it
as an empty list before mapping (e.g., coalesce options to an empty list and
then call options.stream().map(ProductInfo.OptionInfo::from).toList()), ensuring
likeCount and other fields are unchanged and the builder receives an empty
options list when null is passed.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductAppService.java`:
- Around line 49-52: The service methods getProductsByBrandId and
getProductsByBrandIdCached call PageRequest.of(page, size) without validating
inputs; add defensive validation at the service entry (or a private helper like
createValidatedPageRequest) to ensure page >= 0, size > 0 and size <=
MAX_PAGE_SIZE, and convert invalid inputs to
CoreException(ErrorType.BAD_REQUEST) instead of letting IllegalArgumentException
propagate; update both methods to use the validated PageRequest helper so direct
service calls and controller calls produce consistent BAD_REQUEST behavior and
add tests for boundary cases (page=-1, size=0, size=Integer.MAX_VALUE) to assert
CoreException/ErrorType.BAD_REQUEST is thrown.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheManager.java`:
- Around line 7-15: evictProductCaches currently can't fully invalidate list
entries because getProductList/putProductList store entries by (brandId, page,
size) but evictProductCaches(Long productId, Long brandId) lacks page/size (or a
brand-level namespace) info; update the contract to either (a) include page and
size (e.g., evictProductCaches(Long productId, Long brandId, Integer page,
Integer size) or evictBySizeRange) so implementations can remove exact keys, or
(b) introduce a brand-version/namespace token used by
getProductList/putProductList and bumped by evictProductCaches(brandId) so all
page/size combos are implicitly invalidated; then add tests for
CachedBrandProductPage to assert lists cached at different sizes/pages are all
misses after a putProductDetail/evictProductCaches call.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.java`:
- Around line 27-28: 쿼리 findByBrandIdAndDeletedFalseOrderByLikeCountDesc에서
p.likeCount만으로 정렬하면 동률 레코드의 페이지 경계가 비결정적이므로 ORDER BY에 고정된 tie‑breaker인 p.id를
추가하고 관련 JPA 쿼리를 "ORDER BY p.likeCount DESC, p.id DESC" (또는 원하는 고정 정렬 방향)로 변경하며,
DB 측 인덱스도 같은 순서(같은 컬럼 순서와 방향)로 맞춰 성능/일관성을 확보하고 페이징 안정성 테스트를
findByBrandIdAndDeletedFalseOrderByLikeCountDesc 호출 시 동일 likeCount 그룹에서 각 페이지의
ID 집합이 안정적으로 유지되는지 확인하도록 보강하세요.
In
`@apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceTest.java`:
- Around line 285-346: Add unit tests to verify Redis failures are absorbed and
DB fallback works: add a test in BrandProductCacheTest that stubs
productCacheManager.getProductList(...) to throw (e.g., new
RuntimeException("Redis connection failed")), stubs
productRepository.findByBrandIdWithPaging(...) to return a Page, calls
productAppService.getProductsByBrandIdCached(1L, page, 20), and asserts a
non-null CachedBrandProductPage and that
productRepository.findByBrandIdWithPaging(...) was invoked; similarly add a test
where productCacheManager.putProductList(...) throws to ensure the service still
returns DB results and does not propagate the exception.
In `@apps/commerce-api/src/test/java/com/loopers/domain/common/MoneyTest.java`:
- Around line 96-97: The test currently only checks exception type for
money1.subtract(money2); update the assertion to also verify the CoreException's
error type equals ErrorType.BAD_REQUEST by using
isInstanceOfSatisfying(CoreException.class, e ->
assertThat(e.getErrorType()).isEqualTo(ErrorType.BAD_REQUEST)). Apply the same
pattern to the negative amount creation test (the constructor/factory that
throws CoreException when creating negative Money) to assert the error type is
BAD_REQUEST as well.
---
Outside diff comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/admin/product/AdminProductAppService.java`:
- Around line 53-60: deleteByBrandId currently deletes Product entities via
product.delete() and deleteOptionsByProductId(product.getId()) but does not
evict related cache entries; add cache invalidation calls for each deleted
product (e.g. invoke your product cache service such as
productCacheService.evict(product.getId()) or
cacheService.invalidateProductCache(product.getId())) immediately after
deleteOptionsByProductId(product.getId()), and also invalidate any brand-level
or collection caches (e.g. productListCache.evict(brandId) or similar) so
removed products are not served from cache; update the method
deleteByBrandId(Long brandId) to call these cache eviction methods for symbols
product.delete(), deleteOptionsByProductId(...), and the appropriate cache
service methods.
In `@modules/jpa/src/main/resources/jpa.yml`:
- Around line 34-40: The shared JPA profile currently sets
spring.jpa.hibernate.ddl-auto: update for the local profile — change
spring.jpa.hibernate.ddl-auto to none in the shared JPA config and remove index
DDL from runtime schema auto-update; instead create a versioned migration
(Flyway/Liquibase) that adds idx_products_brand_deleted_likes and run it in
CI/deploy pipelines; if local convenience is required, add a separate profile
(e.g. local-ddl) that sets ddl-auto to update so it does not affect the shared
local profile; add integration tests that (1) start from an empty DB and assert
idx_products_brand_deleted_likes exists with the correct columns via
INFORMATION_SCHEMA.STATISTICS and (2) start commerce-api and commerce-batch in
both orders against the same DB to ensure no runtime DDL diffs occur.
In `@modules/redis/src/main/java/com/loopers/config/redis/RedisConfig.java`:
- Around line 43-46: RedisTemplate bean in RedisConfig is configured with
ReadFrom.REPLICA_PREFERRED, causing stale reads after evictProductCaches();
change injection in RedisProductCacheManager to use the master-only template by
adding `@Qualifier`(REDIS_TEMPLATE_MASTER) (reference the REDIS_TEMPLATE_MASTER
constant and the RedisProductCacheManager class) so reads after evictions go to
the master, and add an integration test that calls evictProductCaches() then
immediately calls getProductDetail() and asserts the deleted/updated value is
not returned (verify master consistency).
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/admin/product/AdminProductAppService.java`:
- Around line 37-43: The update method in AdminProductAppService currently calls
productCacheManager.evictProductCaches(id, product.getBrandId()) inside the
`@Transactional` method, risking cache eviction before commit; refactor to perform
eviction after successful commit by publishing a transactional event (e.g.,
create and publish a ProductUpdatedEvent in update) and handle it in a separate
listener annotated with `@TransactionalEventListener`(phase = AFTER_COMMIT) which
invokes productCacheManager.evictProductCaches using the event payload;
alternatively, if you keep eviction inside update, add an explicit
comment/documentation in AdminProductAppService.update explaining the rollback
vs cache-tradeoff.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/CachedBrandProductPage.java`:
- Around line 21-37: ProductSummary caches a redundant deleted flag (always
false due to DB filtering); remove the deleted field and its getter/builder
usage from CachedBrandProductPage.ProductSummary and its from(Product) mapper,
then update any consumers (notably ProductFacade.toProductInfo) to stop reading
ProductSummary.deleted — either derive deleted from the live Product/DB where
needed or stop passing it altogether; ensure builders, constructors, and imports
are cleaned up after removal.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`:
- Around line 33-40: getProductsByBrand currently fetches cached products before
verifying the brand exists, causing wasted DB/cache work when brandId is
invalid; change the call order in ProductFacade.getProductsByBrand to first
validate the brand via brandAppService.getById(brandId) (or a brand existence
check method) and only then call
productAppService.getProductsByBrandIdCached(brandId, page, size), then map
summaries with toProductInfo(summary, brand) as before to avoid unnecessary
cache/storage operations for non-existent brands.
- Around line 84-96: toProductInfo currently hardcodes ProductInfo.likedByUser
to false, causing API responses to always show not-liked; change the mapping in
toProductInfo(CachedBrandProductPage.ProductSummary summary, Brand brand) to set
likedByUser from the summary (e.g. summary.isLikedByUser() or
summary.getLikedByUser()); if ProductSummary has no liked flag, add one to
CachedBrandProductPage.ProductSummary (and populate it where the cache is built)
so toProductInfo can read it, otherwise remove the likedByUser field from
ProductInfo or explicitly document its always-false behavior in the API docs.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductInfo.java`:
- Around line 48-59: withLikedByUser currently reuses the same options list
reference, risking shared-mutable state; change ProductInfo.withLikedByUser to
defensively copy the options (e.g., use List.copyOf(this.options) or
List.copyOf(Optional.ofNullable(this.options).orElse(Collections.emptyList())))
when passing to the builder and ensure the ProductInfo builder/constructor also
applies List.copyOf to its options field and handles null safely; add/strengthen
a unit test that mutates an original mutable options list after creating
ProductInfo and after calling withLikedByUser to assert neither ProductInfo
instance is affected.
In
`@apps/commerce-api/src/main/java/com/loopers/domain/product/ProductRepository.java`:
- Around line 3-4: ProductRepository currently exposes Spring Data types
(org.springframework.data.domain.Page/Pageable); change the repository contract
to use domain-specific DTOs (e.g., ProductPageQuery and ProductPageResult) and
move the Spring-specific conversions into ProductRepositoryImpl where you
translate ProductPageQuery → Pageable and Page → ProductPageResult; update or
add unit tests that validate ProductRepository behavior using these domain types
(without importing Spring classes) and keep the Spring Data dependencies
confined to ProductRepositoryImpl implementation.
In
`@apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceIntegrationTest.java`:
- Around line 75-92: Add an integration test to verify out-of-range paging
behavior for ProductAppService#getProductsByBrandId: create at least one product
via productRepository.save(...), call
productAppService.getProductsByBrandId(testBrand.getId(), 100, 20) (or another
clearly out-of-range page index), and assert that the returned Page has an empty
content list and that totalElements still equals the actual total (e.g., 1);
place this new test in ProductAppServiceIntegrationTest with a descriptive
`@DisplayName` like "존재하지 않는 페이지 요청 시 빈 결과를 반환한다".
In
`@apps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.java`:
- Around line 126-166: Update ProductFacadeTest to assert paging metadata and
add boundary-case tests: inside getProductsByBrand_success (and/or a new test),
assert that the returned page's page number, page size, and totalPages match
expectations based on the CachedBrandProductPage (use
productFacade.getProductsByBrand result to check getNumber(), getSize(),
getTotalPages(), getTotalElements()); add separate tests that mock
CachedBrandProductPage with totalElements=21 and page size 20 to assert
totalPages == 2 and that requesting page 2 returns the remaining item while
requesting page 3 returns an empty content list; reuse symbols
CachedBrandProductPage, productAppService.getProductsByBrandIdCached(...),
brandAppService.getById(...), and productFacade.getProductsByBrand(...) to
locate where to add/adjust assertions and new test methods.
In
`@apps/commerce-api/src/test/java/com/loopers/interfaces/api/BrandProductApiE2ETest.java`:
- Around line 60-95: Add a new test method getBrandProducts_emptyBrand in
BrandProductApiE2ETest that calls testRestTemplate.exchange on "/api/v1/brands/"
+ testBrand.getId() + "/products?page=0" without creating any Product entities,
and assert the response is HttpStatus.OK, response.getBody().data().products()
is empty, and response.getBody().data().totalElements() == 0; place it alongside
getBrandProducts_success and use the same ParameterizedTypeReference<> and
ApiResponse<ProductDto.BrandProductListResponse> types so the test verifies an
empty product list for a newly registered brand.
- Around line 159-175: Add a boundary test to BrandProductApiE2ETest to verify
the maximum allowed page (50) returns 200 OK: create a new test method
getBrandProducts_maxPage_success that uses testRestTemplate.exchange with
"/api/v1/brands/" + testBrand.getId() + "/products?page=50", the same
HttpMethod.GET and
ParameterizedTypeReference<ProductDto.BrandProductListResponse>, then assert
response.getStatusCode() is HttpStatus.OK (and optionally validate response body
metadata/result if desired) to ensure off-by-one errors are caught.
In `@modules/redis/src/main/java/com/loopers/config/redis/RedisConfig.java`:
- Around line 28-29: The hardcoded Redis timeouts in RedisConfig
(COMMAND_TIMEOUT and CONNECT_TIMEOUT) should be externalized into configurable
RedisProperties with sensible defaults; update RedisConfig to inject
RedisProperties and use properties.getCommandTimeout()/getConnectTimeout()
(falling back to Duration.ofMillis(500) and Duration.ofMillis(300) only as
defaults), add a properties binding unit test to verify values are read from
configuration into RedisProperties, and add/instrument an integration test that
simulates slow Redis responses to assert the fallback/threshold behavior uses
the configured timeouts rather than the hardcoded constants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2947daca-5081-4996-bee0-d07cbf665ae3
📒 Files selected for processing (23)
apps/commerce-api/src/main/java/com/loopers/application/admin/product/AdminProductAppService.javaapps/commerce-api/src/main/java/com/loopers/application/product/CachedBrandProductPage.javaapps/commerce-api/src/main/java/com/loopers/application/product/CachedProductDetail.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductAppService.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductCacheManager.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductInfo.javaapps/commerce-api/src/main/java/com/loopers/domain/common/Money.javaapps/commerce-api/src/main/java/com/loopers/domain/product/Product.javaapps/commerce-api/src/main/java/com/loopers/domain/product/ProductRepository.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/main/java/com/loopers/infrastructure/product/RedisProductCacheManager.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductController.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductDto.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceTest.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/domain/common/MoneyTest.javaapps/commerce-api/src/test/java/com/loopers/domain/product/ProductTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/BrandProductApiE2ETest.javamodules/jpa/src/main/resources/jpa.ymlmodules/redis/src/main/java/com/loopers/config/redis/RedisConfig.java
💤 Files with no reviewable changes (1)
- apps/commerce-api/src/test/java/com/loopers/domain/product/ProductTest.java
apps/commerce-api/src/main/java/com/loopers/application/product/CachedProductDetail.java
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/application/product/ProductAppService.java
Show resolved
Hide resolved
Comment on lines
+7
to
+15
| Optional<CachedBrandProductPage> getProductList(Long brandId, int page, int size); | ||
|
|
||
| void putProductList(Long brandId, int page, int size, CachedBrandProductPage value); | ||
|
|
||
| Optional<CachedProductDetail> getProductDetail(Long productId); | ||
|
|
||
| void putProductDetail(Long productId, CachedProductDetail value); | ||
|
|
||
| void evictProductCaches(Long productId, Long brandId); |
There was a problem hiding this comment.
목록 캐시 무효화 계약이 키 차원을 모두 표현하지 못한다.
getProductList와 putProductList는 (brandId, page, size) 조합으로 엔트리를 만들지만 evictProductCaches는 size와 캐시 대상 페이지 범위를 전달받지 않는다. 이 계약으로는 구현체가 완전한 무효화를 할 수 없어 추정 상수에 의존하게 되고, 운영에서는 관리자 수정/삭제 직후 요청 size에 따라 일부 사용자만 오래된 목록을 TTL 동안 계속 보게 된다. 브랜드 단위 버전 키로 네임스페이스를 끊거나, 최소한 무효화 계약에 페이지/사이즈 정책을 포함해 구현체가 추측하지 않도록 바꾸는 편이 안전하다. 추가 테스트로 서로 다른 size와 캐시 가능 페이지에 저장한 목록 캐시가 수정/삭제 이후 모두 miss 되는지 검증해야 한다.
🤖 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/ProductCacheManager.java`
around lines 7 - 15, evictProductCaches currently can't fully invalidate list
entries because getProductList/putProductList store entries by (brandId, page,
size) but evictProductCaches(Long productId, Long brandId) lacks page/size (or a
brand-level namespace) info; update the contract to either (a) include page and
size (e.g., evictProductCaches(Long productId, Long brandId, Integer page,
Integer size) or evictBySizeRange) so implementations can remove exact keys, or
(b) introduce a brand-version/namespace token used by
getProductList/putProductList and bumped by evictProductCaches(brandId) so all
page/size combos are implicitly invalidated; then add tests for
CachedBrandProductPage to assert lists cached at different sizes/pages are all
misses after a putProductDetail/evictProductCaches call.
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.java
Outdated
Show resolved
Hide resolved
apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceTest.java
Show resolved
Hide resolved
apps/commerce-api/src/test/java/com/loopers/domain/common/MoneyTest.java
Outdated
Show resolved
Hide resolved
- (deleted, like_count DESC): 전체 상품 좋아요 순 조회 - (deleted, created_at DESC): 전체 상품 최신순 조회 - (deleted, base_price ASC): 전체 상품 가격순 조회
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/commerce-api/src/main/java/com/loopers/domain/product/Product.java (1)
18-23: 도메인 엔티티에 인덱스 전략을 직접 두면 운영 스키마 관리 경계가 약해진다.Line [18]-Line [23]처럼 인덱스 정의를 도메인 엔티티에 직접 두면, 운영에서 마이그레이션 기반 스키마 관리와 이중 소스가 되어 환경별 스키마 드리프트 추적이 어려워진다. 수정안은 인덱스 생성/변경 책임을 버전드 마이그레이션으로 일원화하고, 엔티티는 도메인 규칙 중심으로 유지하는 방식이다. 추가 테스트로는 마이그레이션 적용 후
INFORMATION_SCHEMA기준 인덱스명/컬럼 순서/정렬 방향을 검증하는 통합 점검을 권장한다.
As per coding guidelines '도메인 규칙과 인프라 관심사가 섞이면 분리하도록 제안'을 근거로 한 제안이다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/product/Product.java` around lines 18 - 23, The Product entity currently embeds DB index definitions in the `@Table`(indexes=...) annotation which mixes infra concerns into the domain model; remove the indexes array from the Product class/@Table annotation and instead create versioned DB migration scripts that create the same indexes (with the same names, columns, and sort directions), and update deployment docs to run those migrations; also add an integration test that, after applying migrations, queries INFORMATION_SCHEMA (or the DB's metadata) to assert index names, column order, and sort directions match expected values to prevent schema drift.
🤖 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/domain/product/Product.java`:
- Around line 19-22: The `@Index` annotations on Product.java
(idx_products_brand_deleted_likes, idx_products_deleted_likes,
idx_products_deleted_created, idx_products_deleted_price) are not created in
production because ddl-auto is none; create a SQL migration (via your chosen
migrator: Flyway or Liquibase) that executes CREATE INDEX statements for each of
those four index names on the products table (matching the column lists in the
annotations), add the migration file to the repository and ensure it runs at
deploy time, and update the deployment pipeline to run a post-deploy check
(e.g., SHOW INDEX FROM products WHERE Key_name IN (...)) to fail the deploy if
any index is missing.
---
Nitpick comments:
In `@apps/commerce-api/src/main/java/com/loopers/domain/product/Product.java`:
- Around line 18-23: The Product entity currently embeds DB index definitions in
the `@Table`(indexes=...) annotation which mixes infra concerns into the domain
model; remove the indexes array from the Product class/@Table annotation and
instead create versioned DB migration scripts that create the same indexes (with
the same names, columns, and sort directions), and update deployment docs to run
those migrations; also add an integration test that, after applying migrations,
queries INFORMATION_SCHEMA (or the DB's metadata) to assert index names, column
order, and sort directions match expected values to prevent schema drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a845be3d-ac83-409c-9787-5c8e5d07857b
📒 Files selected for processing (1)
apps/commerce-api/src/main/java/com/loopers/domain/product/Product.java
Comment on lines
+19
to
+22
| @Index(name = "idx_products_brand_deleted_likes", columnList = "brand_id, deleted, like_count DESC"), | ||
| @Index(name = "idx_products_deleted_likes", columnList = "deleted, like_count DESC"), | ||
| @Index(name = "idx_products_deleted_created", columnList = "deleted, created_at DESC"), | ||
| @Index(name = "idx_products_deleted_price", columnList = "deleted, base_price ASC") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 1) DDL 자동 생성 설정 확인 =="
fd -a 'application.*' | xargs -r rg -n --no-heading 'ddl-auto|hibernate\.hbm2ddl\.auto'
echo
echo "== 2) 인덱스 마이그레이션 존재 여부 확인 =="
fd -a '.*\.(sql|java|kt)$' | xargs -r rg -n --no-heading \
'idx_products_brand_deleted_likes|idx_products_deleted_likes|idx_products_deleted_created|idx_products_deleted_price|create[[:space:]]+index.*products'
echo
echo "== 3) Product 엔티티의 인덱스 선언 재확인 =="
fd -a 'Product.java' | xargs -r rg -n --no-heading '@Table|@Index|columnList'Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 저장소 구조 탐색 =="
find . -type f -name "*.properties" -o -name "*.yml" -o -name "*.yaml" | head -20
echo
echo "== Product.java 파일 위치 =="
find . -type f -name "Product.java" | head -10
echo
echo "== 마이그레이션 디렉토리 찾기 =="
find . -type d \( -name "migration" -o -name "liquibase" -o -name "flyway" -o -name "db" \) | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 868
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== JPA 설정 확인 (jpa.yml) =="
cat ./modules/jpa/src/main/resources/jpa.yml
echo
echo "== commerce-api 설정 확인 (application.yml) =="
cat ./apps/commerce-api/src/main/resources/application.yml
echo
echo "== Product.java 파일 내용 (1-50줄) =="
head -50 ./apps/commerce-api/src/main/java/com/loopers/domain/product/Product.java
echo
echo "== SQL 파일 검색 =="
find . -type f \( -name "*.sql" -o -name "*.ddl" \) | head -20
echo
echo "== Liquibase/Flyway 관련 파일 검색 =="
find . -type f \( -name "*.xml" -o -name "*.conf" \) | grep -i -E 'migration|liquibase|flyway' | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 5133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Gradle 빌드 스크립트 검색 (index 관련) =="
find . -name "*.gradle" -o -name "*.gradle.kts" | xargs -r grep -i 'index\|ddl\|migration' || echo "No matches found"
echo
echo "== Docker/초기화 스크립트 검색 =="
find . -type f \( -name "*.sh" -o -name "Dockerfile*" -o -name "docker-compose*" \) | head -20
echo
echo "== 데이터베이스 초기화 스크립트 =="
find . -type f -name "*.sh" | xargs -r grep -l 'index\|CREATE.*TABLE' || echo "No matches found"
echo
echo "== 전체 repository에서 CREATE INDEX 검색 =="
rg -i 'create\s+index' --type-list | head -5 || echo "No matches found"
echo
echo "== 전체 repository에서 ddl-auto 검색 =="
rg 'ddl-auto' --all-textRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== 전체 repository에서 ddl-auto 검색 =="
rg 'ddl-auto'
echo
echo "== 전체 repository에서 CREATE INDEX 검색 =="
rg 'CREATE.*INDEX'
echo
echo "== Docker compose 파일 목록 =="
find . -name "*docker*compose*" -o -name "Dockerfile*"
echo
echo "== SQL 초기화 파일 검색 =="
find . -type f \( -name "*.sql" -o -name "init*.sh" -o -name "setup*.sh" \)Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 341
운영 환경에서 @Index 선언이 실제 DB 인덱스로 생성되지 않는 문제를 반드시 해결해야 한다.
jpa.yml 설정을 확인한 결과, 운영(prd), QA, 개발(dev) 환경의 ddl-auto가 기본값 none으로 설정되어 있다. Product.java 19-22줄에 선언된 4개의 @Index 주석(idx_products_brand_deleted_likes, idx_products_deleted_likes, idx_products_deleted_created, idx_products_deleted_price)은 Hibernate가 자동으로 생성하지 않으므로, 실제 DB에 인덱스가 없어 조회 성능이 심각하게 회귀한다.
현재 저장소에는 마이그레이션 파일, SQL 스크립트, Liquibase/Flyway 설정이 전혀 없다. 동일 인덱스를 생성하는 SQL 마이그레이션 파일을 작성하여 배포 전에 반드시 실행되도록 하고, 배포 파이프라인에 인덱스 존재 확인(SHOW INDEX FROM products) 단계를 추가해야 한다.
🤖 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/Product.java`
around lines 19 - 22, The `@Index` annotations on Product.java
(idx_products_brand_deleted_likes, idx_products_deleted_likes,
idx_products_deleted_created, idx_products_deleted_price) are not created in
production because ddl-auto is none; create a SQL migration (via your chosen
migrator: Flyway or Liquibase) that executes CREATE INDEX statements for each of
those four index names on the products table (matching the column lists in the
annotations), add the migration file to the repository and ensure it runs at
deploy time, and update the deployment pipeline to run a post-deploy check
(e.g., SHOW INDEX FROM products WHERE Key_name IN (...)) to fail the deploy if
any index is missing.
Contributor
Author
There was a problem hiding this comment.
@coderabbitai 해당 인덱스 생성 부분은 JPA ddl-auto에 의존하지 않고, 내부 데이터베이스 마이그레이션 파이프라인(별도 DB 스크립트)을 통해 관리 및 반영 중입니다.
There was a problem hiding this comment.
@madirony, 내부 DB 마이그레이션 파이프라인을 통해 인덱스가 별도로 관리되고 있음을 확인하였다. 해당 파이프라인의 존재가 저장소 내에서 파악되지 않아 제기된 지적이므로, 내용을 기록하겠다.
(∪･ω･∪)
✏️ Learnings added
Learnt from: madirony
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 199
File: apps/commerce-api/src/main/java/com/loopers/domain/product/Product.java:19-22
Timestamp: 2026-03-13T18:25:54.998Z
Learning: In the Loopers-dev-lab/loop-pack-be-l2-vol3-java project, database index creation (including JPA Index annotations on Product.java) is NOT managed by JPA ddl-auto. Instead, it is handled via an internal, separate DB migration pipeline (external DB scripts). Do not flag missing Flyway/Liquibase migration files or ddl-auto settings as issues for this repository, as the DB schema changes are applied through a different internal process.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yoon-yoo-tak
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 189
File: apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductV1Controller.java:52-55
Timestamp: 2026-03-13T08:29:17.394Z
Learning: In the loop-pack-be-l2-vol3-java project (commerce-api module), the `Product` entity declares `brandId` with `Column(nullable = false)` and validates it as non-null in the constructor. Therefore, `ProductReadModel.brandId()` is structurally guaranteed to be non-null, and null checks on `brandId` in controller or application layer code (e.g., `ProductV1Controller.getById`) are unnecessary defensive code.
Learnt from: ukukdin
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 203
File: apps/commerce-api/src/main/java/com/loopers/application/product/ProductQueryService.java:53-55
Timestamp: 2026-03-13T05:44:03.072Z
Learning: In code reviews of Java files under apps/commerce-api/src/main/java, pay attention to the caching strategy used by ProductQueryService (and related services). If you encounter a method like evictProductListAfterCommit() that uses cache.clear() (full cache invalidation) instead of selective eviction by brandId, evaluate whether this trade-off is justified by current workload: TTL of 1 minute and low admin write frequency. Only favor full invalidation when it is intentional and documented; otherwise consider implementing selective eviction by key (e.g., brandId) and instrument cache metrics (e.g., Micrometer) to determine if selective eviction would reduce unnecessary cache misses. If metrics indicate frequent admin modifications or poor hit rates, propose migrating to selective eviction. Ensure that such decisions are backed by automated validation or CI checks before rollout.
Learnt from: yoon-yoo-tak
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 189
File: apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductQueryServiceImpl.java:18-20
Timestamp: 2026-03-13T08:29:47.665Z
Learning: In loop-pack-be-l2-vol3-java (commerce-api module), Transactional declaration is the responsibility of the Application layer (ApplicationService) only. Infrastructure layer implementations such as ProductQueryServiceImpl (in apps/commerce-api/src/main/java/com/loopers/infrastructure/product/) must NOT declare Transactional, even readOnly = true, because: (1) on cache hits there is no DB access, and (2) on cache misses, Spring Data JPA's SimpleJpaRepository already provides Transactional(readOnly = true) by default. Duplicate Transactional declarations in the Infrastructure layer are intentionally avoided to respect layer responsibilities.
Learnt from: jsj1215
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 142
File: apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.java:80-94
Timestamp: 2026-03-06T06:41:42.198Z
Learning: In `ProductRepositoryImpl.java` (Loopers-dev-lab/loop-pack-be-l2-vol3-java), `findOptionByIdWithLock()` uses `entityManager.find(ProductOption.class, optionId, LockModeType.PESSIMISTIC_WRITE)` without a lock timeout hint. The call flow is: `OrderService.prepareOrderItems()` → `findProductOnly()` (loads only Product, not ProductOption) → `deductStock()` → `findOptionByIdWithLock()`. Since ProductOption is never loaded into the 1st-level cache before this point, there is no stale read risk. However, the lock timeout hint (`jakarta.persistence.lock.timeout`) is missing, risking infinite wait. Fix: use the 4-parameter `entityManager.find()` overload with `Map.of("jakarta.persistence.lock.timeout", 3000L)`.
Learnt from: Namjin-kimm
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 152
File: apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java:86-91
Timestamp: 2026-03-06T09:00:23.795Z
Learning: In Loopers-dev-lab/loop-pack-be-l2-vol3-java PR `#152`, OrderFacade.java's deadlock prevention for multi-product orders relies on MySQL InnoDB's implicit behavior of scanning IN-clause PK queries in ascending order (B+Tree clustered index traversal). This is a deliberate design decision acknowledged by the author as a trade-off: InnoDB generally locks rows in PK ascending order for IN queries, but this is not a formal SQL standard guarantee. If the query optimizer changes execution plans (e.g., full table scan), lock order may change and deadlocks could occur. The safe alternative is adding explicit ORDER BY id to the SELECT FOR UPDATE query in ProductJpaRepository.findAllByIdsForUpdate(). InnoDB does auto-detect and roll back deadlocked transactions, so hangs are avoided but transaction failures still occur.
Learnt from: yoon-yoo-tak
Repo: Loopers-dev-lab/loop-pack-be-l2-vol3-java PR: 6
File: apps/commerce-api/src/main/java/com/loopers/interfaces/api/user/UserV1Dto.java:9-10
Timestamp: 2026-02-04T05:52:09.963Z
Learning: In the loop-pack-be-l2-vol3-java project, enforce unified error handling by routing errors through CoreException to ApiControllerAdvice to ensure a consistent response format. Do not introduce MethodArgumentNotValidException handlers or Bean Validation handling, as that would create inconsistent error handling patterns. This guidance applies to all Java files under the commerce-api module (source code under apps/commerce-api/src/main/java/).
…aker - CachedProductDetail.from()에 null-safe 옵션 매핑 추가 - ProductAppService에 validatedPageRequest() 헬퍼 도입 (page/size 검증) - ProductAppService 캐시 조회/저장에 예외 흡수 try-catch 추가 (DB fallback) - 브랜드별 좋아요순 쿼리에 p.id DESC tie-breaker 추가 - ProductAppServiceTest에 Redis 장애 fallback 테스트 추가 - MoneyTest 예외 검증을 isInstanceOfSatisfying으로 ErrorType 상세 검증
Contributor
Author
|
@coderabbitai 이제 다 고침 |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/commerce-api/src/main/java/com/loopers/application/product/ProductAppService.java (1)
53-56:⚠️ Potential issue | 🟠 Majordeep paging 상한을 서비스에서 강제하지 않아 비싼 페이지 조회가 그대로 열린다
validatedPageRequest()가page < 0만 검사하고 있어page=51같은 요청이 그대로 저장소 호출로 이어진다. 운영 관점에서는 OFFSET 비용이 큰 조회가 다시 열려 이번 PR의 성능 보호 장치가 무력화된다. 또한getProductsByBrandIdCached()는 검증 전에 캐시를 먼저 조회하므로 잘못된 요청도 Redis를 한 번 더 타게 되고, 비정상 키가 남아 있으면 잘못된 응답을 반환할 여지도 있다.수정안은 두 공개 메서드 초입에서
Pageable pageable = validatedPageRequest(page, size);로 먼저 검증하고, helper에서page > MAX_PAGE를CoreException(ErrorType.BAD_REQUEST)로 함께 차단한 뒤 같은pageable을 재사용하는 것이다. 상수도MAX_PAGE와MAX_PAGE_SIZE로 분리하는 편이 안전하다.수정 예시
- private static final int MAX_PAGE_SIZE = 50; + private static final int MAX_PAGE = 50; + private static final int MAX_PAGE_SIZE = 50; ... `@Transactional`(readOnly = true) public Page<Product> getProductsByBrandId(Long brandId, int page, int size) { - return productRepository.findByBrandIdWithPaging(brandId, validatedPageRequest(page, size)); + Pageable pageable = validatedPageRequest(page, size); + return productRepository.findByBrandIdWithPaging(brandId, pageable); } ... `@Transactional`(readOnly = true) public CachedBrandProductPage getProductsByBrandIdCached(Long brandId, int page, int size) { + Pageable pageable = validatedPageRequest(page, size); + if (page <= CACHEABLE_PAGE_LIMIT) { try { Optional<CachedBrandProductPage> cached = productCacheManager.getProductList(brandId, page, size); if (cached.isPresent()) { return cached.get(); @@ - Page<Product> products = productRepository.findByBrandIdWithPaging(brandId, validatedPageRequest(page, size)); + Page<Product> products = productRepository.findByBrandIdWithPaging(brandId, pageable); ... private Pageable validatedPageRequest(int page, int size) { - if (page < 0 || size <= 0 || size > MAX_PAGE_SIZE) { - throw new CoreException(ErrorType.BAD_REQUEST, "page는 0 이상, size는 1~" + MAX_PAGE_SIZE + " 범위여야 합니다."); + if (page < 0 || page > MAX_PAGE || size <= 0 || size > MAX_PAGE_SIZE) { + throw new CoreException( + ErrorType.BAD_REQUEST, + "page는 0~" + MAX_PAGE + ", size는 1~" + MAX_PAGE_SIZE + " 범위여야 합니다." + ); } return PageRequest.of(page, size); }추가 테스트로는
page=50허용,page=51거절,page=-1/size=0/size=51거절, 그리고 잘못된 입력일 때productCacheManager가 호출되지 않는 케이스를getProductsByBrandId()와getProductsByBrandIdCached()모두에 넣어야 한다.Also applies to: 83-109, 192-196
🤖 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/ProductAppService.java` around lines 53 - 56, The service currently lets deep-paging requests through and calls the cache before validating, so fix getProductsByBrandId and getProductsByBrandIdCached to compute Pageable pageable = validatedPageRequest(page, size) at the very start (before any productCacheManager calls), have validatedPageRequest check both page < 0, page > MAX_PAGE and size < 1, size > MAX_PAGE_SIZE and throw new CoreException(ErrorType.BAD_REQUEST) for out-of-range values, and then reuse that same pageable when calling productRepository.findByBrandIdWithPaging; introduce/rename constants MAX_PAGE and MAX_PAGE_SIZE and ensure no cache lookup happens if validation fails so invalid requests never hit Redis.
🤖 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/test/java/com/loopers/application/product/ProductAppServiceTest.java`:
- Around line 289-366: Add explicit boundary tests for page=2 to prevent
regression of the CACHEABLE_PAGE_LIMIT behavior: create two new tests that call
productAppService.getProductsByBrandIdCached(1L, 2, 20) — one where
productCacheManager.getProductList(1L, 2, 20) returns a CachedBrandProductPage
and verify productRepository.findByBrandIdWithPaging(...) is never invoked, and
another where getProductList(...) returns Optional.empty() (and
productRepository.findByBrandIdWithPaging(...) returns a Page) and verify
productCacheManager.putProductList(1L, 2, 20, ...) is called; reference
productAppService.getProductsByBrandIdCached,
productCacheManager.getProductList, productCacheManager.putProductList, and
productRepository.findByBrandIdWithPaging to locate the code under test.
---
Duplicate comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductAppService.java`:
- Around line 53-56: The service currently lets deep-paging requests through and
calls the cache before validating, so fix getProductsByBrandId and
getProductsByBrandIdCached to compute Pageable pageable =
validatedPageRequest(page, size) at the very start (before any
productCacheManager calls), have validatedPageRequest check both page < 0, page
> MAX_PAGE and size < 1, size > MAX_PAGE_SIZE and throw new
CoreException(ErrorType.BAD_REQUEST) for out-of-range values, and then reuse
that same pageable when calling productRepository.findByBrandIdWithPaging;
introduce/rename constants MAX_PAGE and MAX_PAGE_SIZE and ensure no cache lookup
happens if validation fails so invalid requests never hit Redis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6cdfb59-8cf7-4f7c-983a-6f786b573f9e
📒 Files selected for processing (5)
apps/commerce-api/src/main/java/com/loopers/application/product/CachedProductDetail.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductAppService.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceTest.javaapps/commerce-api/src/test/java/com/loopers/domain/common/MoneyTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductJpaRepository.java
- apps/commerce-api/src/test/java/com/loopers/domain/common/MoneyTest.java
apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceTest.java (1)
329-387:⚠️ Potential issue | 🟡 Minor캐시 저장 예외 경로 테스트를 추가해야 한다
Line 329~Line 347에서 캐시 조회 예외 fallback은 검증했지만,productCacheManager.putProductList(...)저장 예외 시에도 서비스가 정상 응답하는지 검증이 빠져 있다. 운영 관점에서 Redis 쓰기 장애가 발생하면 읽기 트래픽이 많은 구간에서 장애 전파 가능성이 커지므로, 저장 실패를 흡수하고 DB 결과를 반환하는 회귀 테스트가 필요하다.cachePutException_returnsDbResult같은 테스트를 추가해putProductList를 throw로 스텁하고, 예외 전파 없이 결과 반환 및 Repository 호출을 검증하는 방식으로 보완하면 된다.수정 예시(diff)
+ `@Test` + `@DisplayName`("캐시 저장에서 예외가 발생해도 DB 결과를 반환한다") + void cachePutException_returnsDbResult() { + // given + int page = 0; + given(productCacheManager.getProductList(1L, page, 20)) + .willReturn(Optional.empty()); + + Page<Product> dbPage = new PageImpl<>(List.of(), PageRequest.of(page, 20), 0); + given(productRepository.findByBrandIdWithPaging(eq(1L), any(PageRequest.class))) + .willReturn(dbPage); + + doThrow(new RuntimeException("Redis write timeout")) + .when(productCacheManager) + .putProductList(eq(1L), eq(page), eq(20), any(CachedBrandProductPage.class)); + + // when + CachedBrandProductPage result = productAppService.getProductsByBrandIdCached(1L, page, 20); + + // then + assertThat(result).isNotNull(); + verify(productRepository).findByBrandIdWithPaging(eq(1L), any(PageRequest.class)); + }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/application/product/ProductAppServiceTest.java` around lines 329 - 387, Add a new unit test named cachePutException_returnsDbResult that stubs productCacheManager.getProductList(1L, page, 20) to Optional.empty(), stubs productRepository.findByBrandIdWithPaging(eq(1L), any(PageRequest.class)) to return a Page<Product>, and then stubs productCacheManager.putProductList(eq(1L), eq(page), eq(20), any(CachedBrandProductPage.class)) to throw a RuntimeException; call productAppService.getProductsByBrandIdCached(1L, page, 20) and assert it returns the DB-derived CachedBrandProductPage (or non-null), verify productRepository.findByBrandIdWithPaging was called and ensure the thrown cache put exception does not propagate from productAppService.getProductsByBrandIdCached.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceTest.java`:
- Around line 329-387: Add a new unit test named
cachePutException_returnsDbResult that stubs
productCacheManager.getProductList(1L, page, 20) to Optional.empty(), stubs
productRepository.findByBrandIdWithPaging(eq(1L), any(PageRequest.class)) to
return a Page<Product>, and then stubs
productCacheManager.putProductList(eq(1L), eq(page), eq(20),
any(CachedBrandProductPage.class)) to throw a RuntimeException; call
productAppService.getProductsByBrandIdCached(1L, page, 20) and assert it returns
the DB-derived CachedBrandProductPage (or non-null), verify
productRepository.findByBrandIdWithPaging was called and ensure the thrown cache
put exception does not propagate from
productAppService.getProductsByBrandIdCached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64552f64-d55e-435b-be23-ba6b7044873e
📒 Files selected for processing (1)
apps/commerce-api/src/test/java/com/loopers/application/product/ProductAppServiceTest.java
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
likes테이블 JOIN 집계(최대 500만 행) + fullscan이 발생하여 응답 지연이 심각했고, 캐시 부재로 동일 요청이 매번 DB를 타격하고 있었다.🧭 Context & Decision
문제 정의
likes테이블을 COUNT JOIN하여 집계. 정렬/필터에 인덱스 없이 fullscan 수행.increaseLikeCount()/decreaseLikeCount()도메인 메서드가 JPA 변경 감지 기반이라 동시 요청 시 Lost Update 발생 가능선택지와 결정
① 인덱스 전략 — 유스케이스 트래픽 기반 설계
정렬에 사용되는 컬럼에 인덱스가 없으면 데이터 규모가 커질수록 filesort가 슬로우 쿼리로 직결된다. 유스케이스를 분석하여 정렬 조건에 대해 복합 인덱스를 설계했다:
(brand_id, deleted, like_count DESC)(deleted, like_count DESC)(deleted, created_at DESC)(deleted, base_price ASC)이 중 브랜드별 좋아요 순 인덱스에 대해 Docker 환경에서 1,000만 건까지 스케일업하여 실측했다:
② 좋아요 수 집계 방식
@Modifying+SET likeCount = likeCount + 1로 DB 레벨 원자적 증감하여 Lost Update 원천 차단.③ 캐시 전략 — "왜 Redis + 부분 캐싱인가"
로컬 캐시(Caffeine) vs 분산 캐시(Redis)
로컬 캐시는 단일 인스턴스에서는 빠르지만, Scale-out 시 인스턴스 간 캐시 정합성 문제가 발생한다. 인스턴스 A에서 Admin이 상품을 수정해도 인스턴스 B의 로컬 캐시는 갱신되지 않아 사용자마다 다른 데이터를 보게 된다. Redis 분산 캐시를 선택하여 다중 인스턴스 환경에서도 단일 캐시 소스를 보장, Scale-out 시 정합성 문제를 원천 차단했다.
캐시 패턴 선택
부분 캐싱(Partial Caching) + TTL Jitter
전체 페이지를 무차별 캐싱하면 거의 접근되지 않는 deep page까지 Redis 메모리를 점유하는 Cache Pollution이 발생한다. 실제 사용자 트래픽의 90% 이상은 1-3페이지에 집중되므로,
CACHEABLE_PAGE_LIMIT = 2(page 0~2)로 제한하여 캐시 적중률은 최대화하면서 메모리 낭비는 최소화했다.또한 동일 TTL로 캐시가 동시에 만료되면 순간적으로 DB에 요청이 몰리는 Cache Stampede(Thundering Herd) 가 발생할 수 있다. 이를 방어하기 위해 TTL에 랜덤 Jitter를 추가했다:
④ 이벤트 성격별 캐시 무효화 전략 분리
모든 변경 이벤트를 동일하게 처리하지 않고, 이벤트의 빈도와 정합성 요구 수준에 따라 전략을 분리했다:
트레이드오프:
likeCount와 실제likes테이블 간 미세한 정합성 차이 가능 (eventual consistency)추후 개선 여지:
🏗️ Design Overview
변경 범위
ProductCacheManager(application 인터페이스)RedisProductCacheManager(infrastructure 구현체)CachedProductDetail,CachedBrandProductPage(캐시 전용 DTO)GET /api/v1/brands/{brandId}/products(브랜드별 상품 목록 API)Product.increaseLikeCount()/decreaseLikeCount()도메인 메서드 제거 → DB@Modifying쿼리로 대체Money.subtract()의IllegalArgumentException→CoreException으로 통일주요 컴포넌트 책임
Product: 정렬 조건별 복합 인덱스 4개 선언. 비정규화된likeCount필드 보유.ProductJpaRepository:@Modifying원자적 증감 쿼리 + 브랜드별 페이징 쿼리 (likeCount DESC)ProductCacheManager: 캐시 get/put/evict 계약 정의. application 계층에 위치하여 DIP 적용.RedisProductCacheManager: Redis 기반 구현. TTL Jitter, 예외 흡수(DB fallback), ObjectMapper 캡슐화. 인프라 관심사가 인터페이스 밖으로 누출되지 않음.ProductAppService: Cache-Aside 조회 로직.CACHEABLE_PAGE_LIMIT = 2로 page 0~2만 캐싱하여 Cache Pollution 방지.AdminProductAppService: 상품 수정/삭제 시evictProductCaches()호출하여 캐시 무효화.ProductFacade: 순수 오케스트레이터. Repository 직접 호출 0건.CachedProductDetail기반으로 상세 조회 조합.ProductController: 브랜드별 상품 목록 엔드포인트 추가. Deep Paging 방지 (MAX_PAGE = 50).RedisConfig: Fail-Fast 타임아웃 (COMMAND 500ms, CONNECT 300ms) +REJECT_COMMANDS설정으로 Redis 장애 시 빠른 실패 후 DB fallback 유도.🔁 Flow Diagram
Main Flow — 브랜드별 상품 목록 조회 (캐시 히트)
sequenceDiagram autonumber participant Client participant Controller participant Facade participant AppService participant CacheManager participant Redis Client->>Controller: GET /api/v1/brands/{id}/products?page=0 Controller->>Facade: getProductsByBrand(brandId, page, size) Facade->>AppService: getProductsByBrandIdCached(brandId, page, size) AppService->>CacheManager: getProductList(brandId, page, size) CacheManager->>Redis: GET product:brand:{id}:page:0:size:20 Redis-->>CacheManager: JSON hit CacheManager-->>AppService: Optional.of(CachedBrandProductPage) AppService-->>Facade: CachedBrandProductPage Facade->>Facade: Brand 조회 + ProductInfo 조합 Facade-->>Controller: Page<ProductInfo> Controller-->>Client: ApiResponse<BrandProductListResponse>Cache Miss → DB Fallback
캐시 무효화 (Admin 수정/삭제)
sequenceDiagram autonumber participant Admin participant AdminAppService participant CacheManager participant Redis Admin->>AdminAppService: update(id, name, price) / delete(id) AdminAppService->>AdminAppService: 도메인 로직 수행 AdminAppService->>CacheManager: evictProductCaches(productId, brandId) CacheManager->>Redis: DEL product:detail:{id} CacheManager->>Redis: DEL product:brand:{brandId}:page:0~2✅ Checklist
🔖 Index
ProductJpaRepository에ORDER BY likeCount DESC페이징 쿼리 추가❤️ Structure
Product.likeCount비정규화 필드로 JOIN 제거, 인덱스 정렬 활용@Modifying SET likeCount = likeCount ± 1DB 원자적 증감CASE WHEN likeCount > 0 THEN ... ELSE 0 END⚡ Cache
evictProductCaches()로 즉시 무효화, 좋아요 토글은 TTL 만료에 의존 (이벤트 성격별 전략 분리)Optional.empty()→ DB fallbackDisconnectedBehavior.REJECT_COMMANDS로 장애 전파 차단변경 목적: 브랜드별 상품 목록 조회 시 likes 집계와 정렬 인덱 부재로 발생한 full scan·응답 지연과 반복 요청으로 인한 DB 부담을 복합 인덱스와 Redis Cache-Aside 도입으로 해소하여 읽기 성능을 극대화하고 DB 원자성/캐시 장애 시 폴백을 보장함.
핵심 변경점: Product 엔티티에 4개 복합 인덱스 추가(brand_id, deleted, like_count DESC 등), likeCount 비정규화 및 도메인 증감 메서드 제거 -> @Modifying 쿼리로 DB 원자적 증감; ProductCacheManager 인터페이스 + RedisProductCacheManager 구현(목록 TTL 60s±jitter, 상세 TTL 300s±jitter, CACHEABLE_PAGE_LIMIT=2로 부분 캐싱, Redis 타임아웃 설정 및 예외 흡수); 관리자 수정/삭제 즉시 evict; GET /api/v1/brands/{brandId}/products 엔드포인트/페이징 제한(MAX_PAGE=50) 추가.
리스크/주의사항: 부분 캐싱(페이지 0~2)은 TTL 기반 일관성 지연(목록 최대 약 60s) 발생 가능, 좋아요 토글은 즉시 evict하지 않고 TTL 만료에 의존해 최대 DETAIL_TTL(약 300s) 불일치가 생길 수 있음; Redis 예외 흡수로 인한 장기 캐시 미적중 상태·모니터링 필요. 위 행위(부분 캐싱 범위·TTL·좋아요 무효화 전략)가 비즈니스 요구에 부합하는지 확인 필요.
테스트/검증 방법: 통합·E2E 테스트(ProductAppServiceIntegrationTest, BrandProductApiE2ETest)로 페이징·정렬·브랜드 필터링 및 에러(404/400) 검증, 캐시 행위는 ProductAppServiceTest.BrandProductCacheTest로 히트/미스·예외 폴백·deep page bypass 검증, 실제 트래픽 하 성능(응답시간, DB 쿼리 감소)과 캐시 일관성(좋아요 반영 지연) 측정 권장. 추가로, TTL·CACHEABLE_PAGE_LIMIT 변경 시 성능·일관성 영향 측정이 필요함.