[Volume-9] 상품 랭킹 기능 구현 - #389
Open
SuHyun-git wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughChanges일일 상품 랭킹
Sequence Diagram(s)sequenceDiagram
participant CatalogEventsConsumer
participant RankingRepositoryImpl
participant RankingFacade
participant RankingController
CatalogEventsConsumer->>RankingRepositoryImpl: 일일 상품 점수 누적
RankingController->>RankingFacade: 날짜/페이지 랭킹 조회
RankingFacade->>RankingRepositoryImpl: 상위 항목과 전체 개수 조회
RankingFacade-->>RankingController: 상품 정보가 결합된 랭킹 페이지 반환
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 7
🧹 Nitpick comments (4)
apps/commerce-streamer/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.java (1)
21-24: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedis 네트워크 왕복 횟수 증가로 인한 성능 저하가 우려된다.
- 왜 문제인지(운영 관점): 이벤트 처리 시마다
incrementScore와expire가 각각 별도의 Redis 명령으로 실행되어 네트워크 통신 횟수가 불필요하게 두 배로 늘어난다. 이는 대량 트래픽 상황에서 컨슈머의 처리량을 떨어뜨려 지연(Lag)을 유발하는 병목 지점이 된다.- 수정안:
RedisTemplate.executePipelined를 사용하여 두 명령을 하나의 파이프라인으로 묶어 전송하도록 개선해야 한다.- 추가 테스트: 파이프라인 적용 후에도 기존의 단위 테스트(
incrementScore_setsTwoDayTtl)가 정상 통과하여 동작에 이상이 없음을 검증해야 한다.As per path instructions, 대량 데이터에서의 병목을 점검해야 한다.
⚡ 수정안
`@Override` public void incrementScore(String key, Long productId, double score) { - redisTemplate.opsForZSet().incrementScore(key, String.valueOf(productId), score); - redisTemplate.expire(key, TTL); + redisTemplate.executePipelined(new org.springframework.data.redis.core.SessionCallback<Object>() { + `@Override` + public Object execute(org.springframework.data.redis.core.RedisOperations operations) throws org.springframework.dao.DataAccessException { + operations.opsForZSet().incrementScore(key, String.valueOf(productId), score); + operations.expire(key, TTL); + return null; + } + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-streamer/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.java` around lines 21 - 24, Update RankingRepositoryImpl.incrementScore to execute the ZSET increment and key expiration through a single RedisTemplate.executePipelined callback, preserving the existing key, product ID, score, and TTL behavior. Ensure incrementScore_setsTwoDayTtl continues to pass with the pipelined implementation.Source: Path instructions
apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java (2)
31-34: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win빈 리스트 조회 시 불필요한 DB 쿼리 방어 로직이 누락되었다.
왜 문제인지(운영 관점): Redis 랭킹 ZSET에 데이터가 전혀 없거나 오프셋 범위를 초과하여 조회된 항목이 없는 경우,
productIds가 빈 리스트가 된다. 이를 그대로productRepository.findAllByIds에 전달하면 불필요한 데이터베이스 라운드트립이 발생하거나, JPA 구현체 및 버전에 따라 IN 쿼리 구문 오류를 유발하여 애플리케이션 로그에 에러 노이즈를 발생시킬 수 있다.수정안:
productIds가 비어있는 경우 즉시 빈 결과를 반환하여 DB 호출을 생략한다.🛠 제안하는 수정안
List<Long> productIds = entries.stream().map(RankingRepository.RankingEntry::productId).toList(); + if (productIds.isEmpty()) { + return new RankingPageInfo(java.util.List.of(), totalElements, totalPages); + } Map<Long, ProductModel> productMap = productRepository.findAllByIds(productIds).stream() .collect(Collectors.toMap(ProductModel::getId, p -> p, (existing, replacement) -> existing));추가 테스트: 해당 날짜의 랭킹 데이터가 존재하지 않을 때,
productRepository.findAllByIds가 호출되지 않으며(Mock 객체의verify활용) 정상적으로 빈 리스트가 반환되는지 확인하는 단위 테스트를 추가한다.As per path instructions,
**/*Repository*.javarequires checking for missing or excessive query conditions that could cause bottlenecks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java` around lines 31 - 34, Update the ranking flow containing productIds and productRepository.findAllByIds so an empty productIds list immediately returns an empty result before querying the repository. Preserve the existing mapping behavior for non-empty rankings, and add a unit test verifying findAllByIds is not called and an empty list is returned when no ranking entries exist.Source: Path instructions
23-23: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win읽기 전용 트랜잭션 경계 설정이 누락되었다.
왜 문제인지(운영 관점):
RankingFacade.getRankings는 DB에서 상품 정보를 조회(productRepository.findAllByIds)한다. 명시적인@Transactional(readOnly = true)가 없으면 리포지토리 레벨의 기본 트랜잭션이 매 조회마다 생성되며, 복수의 영속성 컨텍스트를 다루거나 커넥션을 관리할 때 불필요한 오버헤드가 발생한다. 또한 데이터베이스의 읽기 전용 복제본(Read Replica) 라우팅 설정이 적용되지 않아 마스터 노드에 부하가 집중될 수 있다.수정안:
RankingFacade클래스 또는getRankings메서드에@Transactional(readOnly = true)를 선언한다.🛠 제안하는 수정안
+ `@org.springframework.transaction.annotation.Transactional`(readOnly = true) public RankingPageInfo getRankings(LocalDate date, int page, int size) {추가 테스트: Spring Boot 통합 테스트 환경에서 트랜잭션 동기화 매니저(TransactionSynchronizationManager)를 통해 트랜잭션이 활성화되고
isCurrentTransactionReadOnly()가true를 반환하는지 검증하는 단위/통합 테스트를 추가한다.As per path instructions,
**/*Service*.javarequires verifying transaction boundaries (@Transactional), propagation, andreadOnlyconfigurations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java` at line 23, Declare `@Transactional`(readOnly = true) on RankingFacade.getRankings (or at the RankingFacade class level) so the ranking and product read operations execute within one read-only transaction. Preserve the existing method behavior and add transaction-boundary coverage only if the project’s test conventions require it.Source: Path instructions
apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingPageInfo.java (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win가변 리스트 주입으로 인한 불변성 훼손 위험이 있다.
왜 문제인지(운영 관점): 생성자로 주입받은
items리스트가 가변(mutable)일 경우, 외부에서 원본 리스트를 변경하면RankingPageInfo객체의 상태도 변경된다. 이는 병렬 처리나 캐싱 등에서 예기치 않은 데이터 오염과 추적하기 어려운 버그를 유발할 수 있다.수정안: 레코드의 컴팩트 생성자에서 방어적 복사(Defensive Copy)를 수행하여 완전한 불변 객체로 만든다.
🛠 제안하는 수정안
-public record RankingPageInfo(List<RankingItemInfo> items, long totalElements, int totalPages) {} +public record RankingPageInfo(List<RankingItemInfo> items, long totalElements, int totalPages) { + public RankingPageInfo { + items = items != null ? java.util.List.copyOf(items) : java.util.List.of(); + } +}추가 테스트:
RankingPageInfo생성 시 전달한 원본 리스트에 요소를 추가하거나 삭제했을 때, 생성된 객체 내부의items리스트 크기 및 데이터가 변경되지 않음을 검증하는 단위 테스트를 추가한다.As per path instructions,
**/*.javarequires checking for null handling, defensive copying, and immutability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingPageInfo.java` at line 5, Update the compact constructor of the RankingPageInfo record to defensively copy the incoming items list into an immutable list, while preserving its contents and existing metadata fields. Ensure subsequent mutations to the source list cannot change RankingPageInfo.items, and add coverage verifying additions or removals from the original list do not affect the record.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`:
- Around line 71-79: Update ProductFacade.getRankSafely to generate the ranking
key with an explicit Asia/Seoul timezone instead of the system default, and log
only e.getMessage() rather than the exception object to avoid stack-trace
output; preserve the existing null fallback when ranking lookup fails.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.java`:
- Around line 29-33: Update the ranking entry mapping in RankingRepositoryImpl
so invalid Redis member IDs that cause Long.valueOf to throw
NumberFormatException are skipped, while valid entries continue to produce
RankingEntry results. Add coverage for getTopN with a non-numeric member such as
"INVALID", verifying no exception is thrown and only valid members are returned.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingController.java`:
- Around line 23-29: Update RankingController to add class-level `@Validated`,
constrain the size and page request parameters with appropriate `@Min/`@Max
bounds, and replace manual LocalDate.parse handling with `@DateTimeFormat-based`
LocalDate binding while preserving the optional current-date default. Ensure
invalid pagination and date inputs are handled as standard 400 responses through
the existing controller advice.
In
`@apps/commerce-streamer/src/main/java/com/loopers/domain/ranking/RankingScorePolicy.java`:
- Around line 19-22: Update RankingScorePolicy.orderScore to return 0.0 whenever
the calculated amount is less than or equal to zero, and only apply Math.log10
for positive amounts. Add a unit test covering negative order amounts and
asserting that orderScore returns 0.0.
In
`@apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogEventsConsumer.java`:
- Line 62: Replace system-default date resolution with the explicit business
timezone Asia/Seoul when generating ranking keys in CatalogEventsConsumer and
OrderEventsConsumer. Update the corresponding date calculations in
CatalogEventsConsumerTest and OrderEventsConsumerTest to use the same timezone,
including their test helpers, so rollover assertions match production behavior.
Affected sites:
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogEventsConsumer.java
lines 62-62;
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/OrderEventsConsumer.java
lines 56-56;
apps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/CatalogEventsConsumerTest.java
lines 104-105;
apps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/OrderEventsConsumerTest.java
lines 48-50.
In
`@apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/OrderEventsConsumer.java`:
- Around line 58-70: Refactor the order-item handling around the visible
consumer loop to extract all product IDs first, load metrics with a single
productMetricsRepository.findAllById call, apply addSalesCount in memory, and
persist with saveAll. Replace per-item rankingRepository.incrementScore calls
with a pipeline-based batch update method, while preserving each item’s price,
quantity, and ranking-score calculation; add integration coverage verifying
batched database and Redis calls.
In
`@apps/commerce-streamer/src/test/java/com/loopers/domain/ranking/RankingScorePolicyTest.java`:
- Around line 31-38: RankingScorePolicyTest의 orderScore 테스트에 음수 amount 경계값 케이스를
추가하고, RankingScorePolicy.orderScore에 음수 금액을 전달했을 때 예외 없이 0.0을 반환하는지 검증하라. 기존 0
금액 테스트는 유지하고, 동일한 검증 방식을 사용하라.
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java`:
- Around line 31-34: Update the ranking flow containing productIds and
productRepository.findAllByIds so an empty productIds list immediately returns
an empty result before querying the repository. Preserve the existing mapping
behavior for non-empty rankings, and add a unit test verifying findAllByIds is
not called and an empty list is returned when no ranking entries exist.
- Line 23: Declare `@Transactional`(readOnly = true) on RankingFacade.getRankings
(or at the RankingFacade class level) so the ranking and product read operations
execute within one read-only transaction. Preserve the existing method behavior
and add transaction-boundary coverage only if the project’s test conventions
require it.
In
`@apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingPageInfo.java`:
- Line 5: Update the compact constructor of the RankingPageInfo record to
defensively copy the incoming items list into an immutable list, while
preserving its contents and existing metadata fields. Ensure subsequent
mutations to the source list cannot change RankingPageInfo.items, and add
coverage verifying additions or removals from the original list do not affect
the record.
In
`@apps/commerce-streamer/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.java`:
- Around line 21-24: Update RankingRepositoryImpl.incrementScore to execute the
ZSET increment and key expiration through a single
RedisTemplate.executePipelined callback, preserving the existing key, product
ID, score, and TTL behavior. Ensure incrementScore_setsTwoDayTtl continues to
pass with the pipelined implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f6977bf5-d4ec-42aa-a741-4459658de957
📒 Files selected for processing (31)
apps/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/application/ranking/RankingFacade.javaapps/commerce-api/src/main/java/com/loopers/application/ranking/RankingItemInfo.javaapps/commerce-api/src/main/java/com/loopers/application/ranking/RankingPageInfo.javaapps/commerce-api/src/main/java/com/loopers/config/WebMvcConfig.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingKeyGenerator.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductDto.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingController.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingDto.javaapps/commerce-api/src/main/java/com/loopers/interfaces/event/order/OrderCreatedEvent.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductCacheServiceIntegrationTest.javaapps/commerce-api/src/test/java/com/loopers/application/product/ProductFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/application/ranking/RankingFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/ranking/RankingRepositoryImplTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/ProductApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/RankingApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/event/order/OrderCreatedEventTest.javaapps/commerce-streamer/src/main/java/com/loopers/domain/ranking/RankingKeyGenerator.javaapps/commerce-streamer/src/main/java/com/loopers/domain/ranking/RankingRepository.javaapps/commerce-streamer/src/main/java/com/loopers/domain/ranking/RankingScorePolicy.javaapps/commerce-streamer/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.javaapps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogEventsConsumer.javaapps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/OrderEventsConsumer.javaapps/commerce-streamer/src/test/java/com/loopers/domain/ranking/RankingKeyGeneratorTest.javaapps/commerce-streamer/src/test/java/com/loopers/domain/ranking/RankingScorePolicyTest.javaapps/commerce-streamer/src/test/java/com/loopers/infrastructure/ranking/RankingRepositoryImplTest.javaapps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/CatalogEventsConsumerTest.javaapps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/OrderEventsConsumerTest.java
Comment on lines
+71
to
+79
| private Long getRankSafely(Long id) { | ||
| try { | ||
| String rankingKey = com.loopers.domain.ranking.RankingKeyGenerator.dailyKey(java.time.LocalDate.now()); | ||
| return rankingRepository.getRank(rankingKey, id).map(r -> r + 1).orElse(null); | ||
| } catch (Exception e) { | ||
| log.warn("[productId={}] 랭킹 조회 실패 — rank 없이 조회를 계속합니다.", id, e); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
시스템 기본 시간대 의존 및 예외 발생 시 로그 폭주 위험이 있다.
왜 문제인지(운영 관점):
LocalDate.now()는 실행 환경의 기본 시간대(예: UTC)에 의존하므로, 분산 서버 환경에서 인스턴스별로 생성되는 랭킹 키(날짜)가 불일치할 수 있다.- Redis 장애 발생 시 트래픽이 높은 상품 상세 조회 API에서 매 요청마다 전체 스택 트레이스를 로깅하게 되면, 디스크 I/O 과부하 및 로그 폭주(Log flood)로 인한 2차 장애가 발생할 수 있다.
- 외부 의존성(Redis) 호출 시 타임아웃 지연이 누적되면 전체 API 스레드가 고갈될 위험이 있다.
수정안:
LocalDate.now(ZoneId.of("Asia/Seoul"))와 같이 비즈니스 기준이 되는 명시적인 시간대를 사용한다.- 로그 기록 시
e대신e.getMessage()를 사용하여 스택 트레이스 출력을 방지한다. - 향후 서킷 브레이커(예: Resilience4j)를 적용하여 실패 시 빠른 실패(Fail-fast)를 통해 애플리케이션 스레드를 보호한다.
추가 테스트:
- 시스템 시간대가 변경되어도 명시적 기준에 따라 일관된 랭킹 키를 참조하는지 검증하는 단위 테스트를 추가한다.
- Redis 예외 상황을 모의(Mocking)하여, 로그 포맷이 의도대로 출력되고 응답 지연 없이
null이 반환되는지 확인하는 테스트 흐름을 점검한다.
As per path instructions, Service 클래스에서는 외부 호출에 대해 서킷브레이커를 고려하고 예외 실패 시 대체 흐름 및 로그 메시지를 점검해야 한다.
🛠 제안하는 수정안
private Long getRankSafely(Long id) {
try {
- String rankingKey = com.loopers.domain.ranking.RankingKeyGenerator.dailyKey(java.time.LocalDate.now());
+ String rankingKey = com.loopers.domain.ranking.RankingKeyGenerator.dailyKey(java.time.LocalDate.now(java.time.ZoneId.of("Asia/Seoul")));
return rankingRepository.getRank(rankingKey, id).map(r -> r + 1).orElse(null);
} catch (Exception e) {
- log.warn("[productId={}] 랭킹 조회 실패 — rank 없이 조회를 계속합니다.", id, e);
+ log.warn("[productId={}] 랭킹 조회 실패 — rank 없이 조회를 계속합니다. 원인: {}", id, e.getMessage());
return null;
}
}📝 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
| private Long getRankSafely(Long id) { | |
| try { | |
| String rankingKey = com.loopers.domain.ranking.RankingKeyGenerator.dailyKey(java.time.LocalDate.now()); | |
| return rankingRepository.getRank(rankingKey, id).map(r -> r + 1).orElse(null); | |
| } catch (Exception e) { | |
| log.warn("[productId={}] 랭킹 조회 실패 — rank 없이 조회를 계속합니다.", id, e); | |
| return null; | |
| } | |
| } | |
| private Long getRankSafely(Long id) { | |
| try { | |
| String rankingKey = com.loopers.domain.ranking.RankingKeyGenerator.dailyKey(java.time.LocalDate.now(java.time.ZoneId.of("Asia/Seoul"))); | |
| return rankingRepository.getRank(rankingKey, id).map(r -> r + 1).orElse(null); | |
| } catch (Exception e) { | |
| log.warn("[productId={}] 랭킹 조회 실패 — rank 없이 조회를 계속합니다. 원인: {}", id, e.getMessage()); | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java`
around lines 71 - 79, Update ProductFacade.getRankSafely to generate the ranking
key with an explicit Asia/Seoul timezone instead of the system default, and log
only e.getMessage() rather than the exception object to avoid stack-trace
output; preserve the existing null fallback when ranking lookup fails.
Source: Path instructions
Comment on lines
+29
to
+33
| return tuples.stream() | ||
| .filter(t -> t.getValue() != null && t.getScore() != null) | ||
| .map(t -> new RankingEntry(Long.valueOf(t.getValue()), t.getScore())) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ID 파싱 시 NumberFormatException 방어 로직이 누락되었다.
왜 문제인지(운영 관점): Redis는 스키마리스(schemaless) 저장소이므로, 타 시스템이나 운영자 실수로 ZSET에 숫자가 아닌 문자열이 삽입될 경우 Long.valueOf() 처리 과정에서 NumberFormatException이 발생한다. 이로 인해 단일 데이터 오염만으로도 전체 랭킹 조회 API가 500 에러를 반환하는 장애가 발생할 수 있다.
수정안: 숫자 변환 시 예외를 잡아 무시(혹은 로깅)하도록 방어 코드를 추가한다.
🛠 제안하는 수정안
- return tuples.stream()
- .filter(t -> t.getValue() != null && t.getScore() != null)
- .map(t -> new RankingEntry(Long.valueOf(t.getValue()), t.getScore()))
- .toList();
+ return tuples.stream()
+ .filter(t -> t.getValue() != null && t.getScore() != null)
+ .map(t -> {
+ try {
+ return new RankingEntry(Long.valueOf(t.getValue()), t.getScore());
+ } catch (NumberFormatException e) {
+ return null; // 로깅 후 무시
+ }
+ })
+ .filter(java.util.Objects::nonNull)
+ .toList();추가 테스트: Redis에 숫자가 아닌 문자열 멤버("INVALID")를 추가한 뒤 getTopN 호출 시 예외 없이 정상 멤버들만 반환되는지 확인하는 테스트 코드를 작성한다.
📝 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
| return tuples.stream() | |
| .filter(t -> t.getValue() != null && t.getScore() != null) | |
| .map(t -> new RankingEntry(Long.valueOf(t.getValue()), t.getScore())) | |
| .toList(); | |
| } | |
| return tuples.stream() | |
| .filter(t -> t.getValue() != null && t.getScore() != null) | |
| .map(t -> { | |
| try { | |
| return new RankingEntry(Long.valueOf(t.getValue()), t.getScore()); | |
| } catch (NumberFormatException e) { | |
| return null; // 로깅 후 무시 | |
| } | |
| }) | |
| .filter(java.util.Objects::nonNull) | |
| .toList(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.java`
around lines 29 - 33, Update the ranking entry mapping in RankingRepositoryImpl
so invalid Redis member IDs that cause Long.valueOf to throw
NumberFormatException are skipped, while valid entries continue to produce
RankingEntry results. Add coverage for getTopN with a non-numeric member such as
"INVALID", verifying no exception is thrown and only valid members are returned.
Comment on lines
+23
to
+29
| @RequestParam(required = false) String date, | ||
| @RequestParam(defaultValue = "20") int size, | ||
| @RequestParam(defaultValue = "1") int page | ||
| ) { | ||
| LocalDate targetDate = (date == null || date.isBlank()) | ||
| ? LocalDate.now() | ||
| : LocalDate.parse(date, DateTimeFormatter.BASIC_ISO_DATE); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
파라미터 검증(Bean Validation) 및 프레임워크 기반 날짜 바인딩이 누락되었다.
왜 문제인지(운영 관점):
- 버그나 악의적인 공격으로 인해
size파라미터에 비정상적으로 큰 값이 인입될 경우, 대량의 데이터 조회로 인해 애플리케이션 OOM(Out of Memory)이나 DB 스레드 풀 병목 현상이 발생해 전체 서비스 장애로 이어진다. LocalDate.parse를 직접 호출하면 잘못된 날짜 형식 인입 시DateTimeParseException이 발생하여 서버 오류(500)로 처리된다. 이는 클라이언트 오류를 시스템 오류로 모니터링하게 만들어 알람 노이즈를 유발하고, 에러 응답 포맷의 일관성을 저해한다.
수정안: 랭킹 컨트롤러 클래스 레벨에 @Validated를 선언하고, 페이징 파라미터에 @Max, @Min 제약을 설정한다. 날짜는 @DateTimeFormat을 사용하여 프레임워크가 바인딩과 예외(400 Bad Request) 처리를 담당하게 한다.
🛠 제안하는 수정안
- public ApiResponse<RankingDto.RankingPageResponse> getRankings(
- `@RequestParam`(required = false) String date,
- `@RequestParam`(defaultValue = "20") int size,
- `@RequestParam`(defaultValue = "1") int page
- ) {
- LocalDate targetDate = (date == null || date.isBlank())
- ? LocalDate.now()
- : LocalDate.parse(date, DateTimeFormatter.BASIC_ISO_DATE);
+ public ApiResponse<RankingDto.RankingPageResponse> getRankings(
+ `@RequestParam`(required = false) `@org.springframework.format.annotation.DateTimeFormat`(pattern = "yyyyMMdd") LocalDate date,
+ `@RequestParam`(defaultValue = "20") `@jakarta.validation.constraints.Max`(100) int size,
+ `@RequestParam`(defaultValue = "1") `@jakarta.validation.constraints.Min`(1) int page
+ ) {
+ LocalDate targetDate = date == null ? LocalDate.now() : date;(참고: Controller 클래스 최상단에 @org.springframework.validation.annotation.Validated 애노테이션이 필요하다.)
추가 테스트:
size파라미터에 허용치(예: 100)를 초과하는 값을 입력했을 때 400 Bad Request가 반환되는지 확인하는 테스트를 추가한다.- 잘못된 날짜 형식 인입 시
@ControllerAdvice를 통한 표준 400 에러 응답이 정상적으로 내려오는지 확인하는 테스트를 추가한다.
As per path instructions, **/*Controller*.java requires Controllers to focus on request validation (Bean Validation) and consistent status code/error response formats relying on @ControllerAdvice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingController.java`
around lines 23 - 29, Update RankingController to add class-level `@Validated`,
constrain the size and page request parameters with appropriate `@Min/`@Max
bounds, and replace manual LocalDate.parse handling with `@DateTimeFormat-based`
LocalDate binding while preserving the optional current-date default. Ensure
invalid pagination and date inputs are handled as standard 400 responses through
the existing controller advice.
Source: Path instructions
Comment on lines
+19
to
+22
| public static double orderScore(long price, long quantity) { | ||
| double amount = (double) price * quantity; | ||
| return ORDER_WEIGHT * Math.log10(amount + 1); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
주문 금액이 음수일 때 NaN 반환으로 인한 장애 발생 위험이 있다.
- 왜 문제인지(운영 관점): 환불이나 보정 이벤트 등으로 인해
price나quantity가 음수가 되어amount가 0 이하가 되면Math.log10연산 시NaN또는-Infinity가 반환된다. 비정상적인 값이 Redis ZSET에 전달되면 예외가 발생하여 Kafka 컨슈머의 메시지 처리가 중단되고 Lag이 누적될 수 있다. - 수정안: 방어적 프로그래밍을 적용하여
amount <= 0인 경우0.0을 반환하도록 로직을 개선해야 한다. - 추가 테스트: 금액이 음수일 때
orderScore가0.0을 반환하는 실패 케이스 단위 테스트를 추가해야 한다.
🛡️ 수정안
public static double orderScore(long price, long quantity) {
double amount = (double) price * quantity;
+ if (amount <= 0) {
+ return 0.0;
+ }
return ORDER_WEIGHT * Math.log10(amount + 1);
}📝 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 static double orderScore(long price, long quantity) { | |
| double amount = (double) price * quantity; | |
| return ORDER_WEIGHT * Math.log10(amount + 1); | |
| } | |
| public static double orderScore(long price, long quantity) { | |
| double amount = (double) price * quantity; | |
| if (amount <= 0) { | |
| return 0.0; | |
| } | |
| return ORDER_WEIGHT * Math.log10(amount + 1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-streamer/src/main/java/com/loopers/domain/ranking/RankingScorePolicy.java`
around lines 19 - 22, Update RankingScorePolicy.orderScore to return 0.0
whenever the calculated amount is less than or equal to zero, and only apply
Math.log10 for positive amounts. Add a unit test covering negative order amounts
and asserting that orderScore returns 0.0.
| ProductMetricsModel metrics = productMetricsRepository.findByProductId(productId) | ||
| .orElseGet(() -> ProductMetricsModel.create(productId)); | ||
|
|
||
| String rankingKey = RankingKeyGenerator.dailyKey(LocalDate.now()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
시스템 타임존에 의존적인 날짜 생성으로 랭킹 일자 롤오버 불일치 위험이 있다.
-
왜 문제인지(운영 관점):
LocalDate.now()는 실행 환경의 기본 타임존을 따른다. 컨테이너나 서버 간 타임존 설정이 다르거나 UTC로 구동될 경우, 자정(00:00) 롤오버가 의도치 않은 시점에 발생하여 사용자에게 부정확한 과거 혹은 미래 일자의 랭킹 데이터가 노출될 수 있다. -
수정안: 비즈니스 기준이 되는 타임존을 명시적으로 지정하여 환경 독립성을 보장해야 한다.
-
추가 테스트: 자정 경계를 넘는 시점에 생성된 이벤트가 설정된 타임존의 올바른 날짜 키에 누적되는지 시뮬레이션하는 단위 테스트를 추가해야 한다.
-
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogEventsConsumer.java#L62-L62:LocalDate.now(ZoneId.of("Asia/Seoul"))와 같이 비즈니스 기준 타임존을 명시적으로 지정해야 한다. -
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/OrderEventsConsumer.java#L56-L56: 동일하게 타임존을 명시해야 한다. -
apps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/CatalogEventsConsumerTest.java#L104-L105: 테스트 코드에서도 운영 코드와 동일한 타임존을 명시하여 검증 일관성을 유지해야 한다. -
apps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/OrderEventsConsumerTest.java#L48-L50: 테스트 헬퍼 메서드 역시 명시적인 타임존이 주입되도록 수정해야 한다.
📍 Affects 4 files
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogEventsConsumer.java#L62-L62(this comment)apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/OrderEventsConsumer.java#L56-L56apps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/CatalogEventsConsumerTest.java#L104-L105apps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/OrderEventsConsumerTest.java#L48-L50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogEventsConsumer.java`
at line 62, Replace system-default date resolution with the explicit business
timezone Asia/Seoul when generating ranking keys in CatalogEventsConsumer and
OrderEventsConsumer. Update the corresponding date calculations in
CatalogEventsConsumerTest and OrderEventsConsumerTest to use the same timezone,
including their test helpers, so rollover assertions match production behavior.
Affected sites:
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogEventsConsumer.java
lines 62-62;
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/OrderEventsConsumer.java
lines 56-56;
apps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/CatalogEventsConsumerTest.java
lines 104-105;
apps/commerce-streamer/src/test/java/com/loopers/interfaces/consumer/OrderEventsConsumerTest.java
lines 48-50.
Comment on lines
58
to
70
| for (JsonNode item : items) { | ||
| Long productId = item.get("productId").asLong(); | ||
| long quantity = item.get("quantity").asLong(); | ||
| long price = item.path("price").asLong(0); | ||
|
|
||
| ProductMetricsModel metrics = productMetricsRepository.findByProductId(productId) | ||
| .orElseGet(() -> ProductMetricsModel.create(productId)); | ||
|
|
||
| metrics.addSalesCount(quantity); | ||
| productMetricsRepository.save(metrics); | ||
|
|
||
| rankingRepository.incrementScore(rankingKey, productId, RankingScorePolicy.orderScore(price, quantity)); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔴 Critical | 🏗️ Heavy lift
루프 내 데이터베이스 조회 및 저장으로 인한 N+1 병목이 존재한다.
- 왜 문제인지(운영 관점): 주문 상품(items) 수만큼 루프 내에서
productMetricsRepository.findByProductId,save및 Redis 호출이 반복된다. 단일 이벤트에서 N번의 DB 및 Redis I/O가 발생하여 커넥션 풀을 빠르게 고갈시키고, 대규모 주문 이벤트 유입 시 컨슈머 지연(Lag)을 초래하는 주요 원인이 된다. - 수정안: 루프 밖에서 전체
productId목록을 먼저 추출해findAllById로 한 번에 조회하고, 메모리상에서addSalesCount를 적용한 뒤saveAll로 일괄 저장하도록 구조를 변경해야 한다. Redis 업데이트 역시 파이프라인 기반의 일괄 처리 메서드를 추가하여 횟수를 최소화해야 한다. - 추가 테스트: 다수의 상품이 포함된 주문 이벤트를 처리할 때, DB 쿼리와 Redis 네트워크 호출이 각각 N번이 아닌 일괄 처리되어 최소 횟수로 수행되는지 통합 테스트에서 철저히 검증해야 한다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/OrderEventsConsumer.java`
around lines 58 - 70, Refactor the order-item handling around the visible
consumer loop to extract all product IDs first, load metrics with a single
productMetricsRepository.findAllById call, apply addSalesCount in memory, and
persist with saveAll. Replace per-item rankingRepository.incrementScore calls
with a pipeline-based batch update method, while preserving each item’s price,
quantity, and ranking-score calculation; add integration coverage verifying
batched database and Redis calls.
Comment on lines
+31
to
+38
| @DisplayName("가격이 0이어도 orderScore는 예외 없이 0을 반환한다.") | ||
| @Test | ||
| void orderScore_returnsZero_whenAmountIsZero() { | ||
| double score = RankingScorePolicy.orderScore(0L, 1L); | ||
|
|
||
| assertThat(score).isEqualTo(0.0); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
음수 금액 발생 시의 실패 케이스에 대한 단위 테스트가 누락되었다.
- 왜 문제인지(운영 관점): 비정상적인 데이터 유입 시 시스템이 의도대로 방어하여 예외를 방지하는지 보장할 수 없으므로 잠재적 운영 장애 위험이 남아있게 된다.
- 수정안:
amount가 음수일 때 방어 로직이 정상 작동하여0.0을 반환하는지 확인하는 경계값 테스트를 추가해야 한다. - 추가 테스트: 아래와 같이 실패 케이스 단위 테스트를 추가해야 한다.
As per path instructions, 단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검해야 한다.
🧪 수정안
`@DisplayName`("가격이 0이어도 orderScore는 예외 없이 0을 반환한다.")
`@Test`
void orderScore_returnsZero_whenAmountIsZero() {
double score = RankingScorePolicy.orderScore(0L, 1L);
assertThat(score).isEqualTo(0.0);
}
+
+ `@DisplayName`("금액이 음수일 때 orderScore는 0을 반환한다.")
+ `@Test`
+ void orderScore_returnsZero_whenAmountIsNegative() {
+ double score = RankingScorePolicy.orderScore(-100L, 1L);
+
+ assertThat(score).isEqualTo(0.0);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @DisplayName("가격이 0이어도 orderScore는 예외 없이 0을 반환한다.") | |
| @Test | |
| void orderScore_returnsZero_whenAmountIsZero() { | |
| double score = RankingScorePolicy.orderScore(0L, 1L); | |
| assertThat(score).isEqualTo(0.0); | |
| } | |
| } | |
| `@DisplayName`("가격이 0이어도 orderScore는 예외 없이 0을 반환한다.") | |
| `@Test` | |
| void orderScore_returnsZero_whenAmountIsZero() { | |
| double score = RankingScorePolicy.orderScore(0L, 1L); | |
| assertThat(score).isEqualTo(0.0); | |
| } | |
| `@DisplayName`("금액이 음수일 때 orderScore는 0을 반환한다.") | |
| `@Test` | |
| void orderScore_returnsZero_whenAmountIsNegative() { | |
| double score = RankingScorePolicy.orderScore(-100L, 1L); | |
| assertThat(score).isEqualTo(0.0); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-streamer/src/test/java/com/loopers/domain/ranking/RankingScorePolicyTest.java`
around lines 31 - 38, RankingScorePolicyTest의 orderScore 테스트에 음수 amount 경계값 케이스를
추가하고, RankingScorePolicy.orderScore에 음수 금액을 전달했을 때 예외 없이 0.0을 반환하는지 검증하라. 기존 0
금액 테스트는 유지하고, 동일한 검증 방식을 사용하라.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🧭 Context & Decision
문제 정의
catalog-events/order-events) →commerce-streamer파이프라인은product_metrics(DB)만 갱신하고 있었다. R8에서 Redis Sorted Set을 대기열 순서 관리에 써봤지만, 랭킹에는 아직 활용되지 않았다. 유저에게 "오늘 인기 상품"을 보여줄 방법이 없었다.GROUP BY+ORDER BY로 랭킹을 매번 계산하면 데이터가 쌓일수록 느려지고, 조회 빈도가 매우 높은 지면(홈, Top-N)이라 DB 과부하로 이어질 수 있다.선택지와 결정
ORDER BY기반 실시간 집계 — 정합성은 높지만 데이터가 쌓일수록 느려지고 조회 부하가 큼ranking:all:{yyyyMMdd}) 채택. 날짜별 Key 분리 + TTL 2일.Score = W(view)*1 + W(like)*1 + W(order)*log10(price*quantity+1), 가중치는view=0.1 / like=0.2 / order=0.7(합 1).log10정규화를 적용해 조회/좋아요(소액 다건)와 주문(고액 소건) 사이 스케일 격차를 눌렀다. 단순 상수 비율 곱셈으로는 상대적 격차가 그대로 유지된다는 걸 구체적 숫자로 확인한 뒤 log를 선택했다.product_metrics(DB)는event_handled테이블로 멱등성이 보장되지만, 같은 트랜잭션 안에서 이뤄지는 Redis ZSET 증가는 "DB 커밋 실패 → 재전송" 케이스에서 중복 반영될 수 있다. 이번 라운드에서는 이를 완벽한 멱등성이 아니라 근사치 랭킹으로 받아들이기로 결정했다 (Must-Have 범위에서는 합리적인 트레이드오프라고 판단).@TransactionalEventListener(AFTER_COMMIT)로 DB 커밋 후에만 Redis를 갱신하도록 바꾸면 위 이중 카운팅 문제를 해결할 수 있다 (R7에서 배운 패턴과 동일한 방향).ZUNIONSTORE)로 콜드 스타트 완화, 1시간 단위 랭킹, 실시간 Weight 조절, Kafka 배치 리스너는 Nice-to-Have로 남겨뒀다.🤔 고민한 점 / 막혔던 부분
date파라미터로 과거 날짜를 정상 조회하는 시나리오, (2) Score Carry-Over가 자정 경계에서 전날 key를 참조해야 하는 시스템 내부 요구, 이 두 가지였다.🙋 기타
price필드가 없을 때 NPE로 죽는 poison pill 문제)은 즉시 수정했다.