Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package com.loopers.application.ranking;

import com.loopers.application.product.ProductLikeSummary;
import com.loopers.ranking.RankingEntry;

import java.time.LocalDate;
import java.util.List;

public record RankingQueryResponse(
LocalDate date,
List<RankingEntry> rankingEntries,
List<ProductLikeSummary> productLikeSummary
) {
public static RankingQueryResponse of(LocalDate date, List<RankingEntry> rankingEntries, List<ProductLikeSummary> productLikeSummary) {
return new RankingQueryResponse(date, rankingEntries, productLikeSummary);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package com.loopers.application.ranking;

import com.loopers.application.like.event.ProductLikeEvent;
import com.loopers.application.product.ProductLikeSummary;
import com.loopers.application.product.ProductQueryService;
import com.loopers.domain.product.ProductSortType;
import com.loopers.ranking.DailyRankingResponse;
import com.loopers.ranking.RankingEntry;
import com.loopers.ranking.RankingZSetRepository;
import com.loopers.support.error.CoreException;
import com.loopers.support.error.ErrorType;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.time.LocalDate;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.time.format.DateTimeFormatter;
import java.util.ArrayList;
import java.util.List;
import java.util.OptionalDouble;

@Slf4j
@Service
@RequiredArgsConstructor
public class RankingQueryService {
private final RankingZSetRepository rankingZSetRepository;
private final ProductQueryService productQueryService;

@Transactional(readOnly = true)
public RankingQueryResponse getDailyPopularProducts(String date, int size) {
LocalDate target = initLocalDate(date);

int limit = (size <= 0) ? 20 : Math.min(size, 100);

List<RankingEntry> rankingEntries = rankingZSetRepository.findTopDailyAllByLimit(target, limit);
List<ProductLikeSummary> productLikeSummaries = findProductSummaryFrom(rankingEntries);

return new RankingQueryResponse(
target,
rankingEntries,
productLikeSummaries
);
}

@Transactional(readOnly = true)
public OptionalDouble getDailyRankingScore(Long productId) {
LocalDate now = LocalDate.now(ZoneId.systemDefault());
return rankingZSetRepository.findDailyRanking(now, productId);
}

private boolean hasValidDate(String date) {
return date == null || date.isBlank();
}

private LocalDate initLocalDate(String date) {
return (hasValidDate(date))
? LocalDate.now(ZoneId.systemDefault())
: LocalDate.parse(date, DateTimeFormatter.BASIC_ISO_DATE);

}

private List<ProductLikeSummary> findProductSummaryFrom(List<RankingEntry> rankingEntries) {
List<ProductLikeSummary> result = new ArrayList<>();

for(RankingEntry rankingEntry : rankingEntries) {
ProductLikeSummary summary;
try {
summary = productQueryService.getProductLikeSummary(rankingEntry.productId());
} catch (CoreException e) {
if(e.getErrorType() == ErrorType.NOT_FOUND) {
log.error("Could not find product like summary for {}", rankingEntry.productId());
}
summary = null;
}
result.add(summary);
}
return result;
}
Comment on lines +65 to +81
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

N+1 쿼리 νŒ¨ν„΄ 및 null 처리 κ°œμ„  ν•„μš”

  1. N+1 쿼리: 각 λž­ν‚Ή ν•­λͺ©λ§ˆλ‹€ κ°œλ³„ DB μ‘°νšŒκ°€ λ°œμƒν•©λ‹ˆλ‹€. productId λͺ©λ‘μœΌλ‘œ 일괄 μ‘°νšŒν•˜λŠ” 것이 νš¨μœ¨μ μž…λ‹ˆλ‹€.

  2. Null 처리: μ°Ύμ§€ λͺ»ν•œ μƒν’ˆμ— λŒ€ν•΄ null을 λ¦¬μŠ€νŠΈμ— μΆ”κ°€ν•˜λ©΄ ν΄λΌμ΄μ–ΈνŠΈμ—μ„œ NPEκ°€ λ°œμƒν•  수 μžˆμŠ΅λ‹ˆλ‹€. ν•„ν„°λ§ν•˜κ±°λ‚˜ λͺ…μ‹œμ μΈ "μ‚­μ œλ¨" ν‘œμ‹œλ₯Ό μ‚¬μš©ν•˜μ„Έμš”.

  3. μ˜ˆμ™Έ 처리: NOT_FOUNDκ°€ μ•„λ‹Œ λ‹€λ₯Έ μ˜ˆμ™Έλ„ λ¬΄μ‹œλ˜κ³  μžˆμŠ΅λ‹ˆλ‹€.

πŸ”Ž μ œμ•ˆλœ μˆ˜μ •
     private List<ProductLikeSummary> findProductSummaryFrom(List<RankingEntry> rankingEntries) {
-        List<ProductLikeSummary> result = new ArrayList<>();
-
-        for(RankingEntry rankingEntry : rankingEntries) {
-            ProductLikeSummary summary;
-            try {
-                summary = productQueryService.getProductLikeSummary(rankingEntry.productId());
-            } catch (CoreException e) {
-                if(e.getErrorType() == ErrorType.NOT_FOUND) {
-                    log.error("Could not find product like summary for {}", rankingEntry.productId());
-                }
-                summary =  null;
-            }
-            result.add(summary);
+        List<Long> productIds = rankingEntries.stream()
+                .map(RankingEntry::productId)
+                .toList();
+        
+        // TODO: ProductQueryService에 일괄 쑰회 λ©”μ„œλ“œ μΆ”κ°€ κ³ λ €
+        List<ProductLikeSummary> result = new ArrayList<>();
+        for (RankingEntry rankingEntry : rankingEntries) {
+            try {
+                ProductLikeSummary summary = productQueryService.getProductLikeSummary(rankingEntry.productId());
+                result.add(summary);
+            } catch (CoreException e) {
+                if (e.getErrorType() == ErrorType.NOT_FOUND) {
+                    log.warn("Product not found for ranking entry: {}", rankingEntry.productId());
+                    // null λŒ€μ‹  κ±΄λ„ˆλ›°κΈ°
+                } else {
+                    throw e; // NOT_FOUNDκ°€ μ•„λ‹Œ μ˜ˆμ™ΈλŠ” μ „νŒŒ
+                }
+            }
         }
         return result;
     }
πŸ€– Prompt for AI Agents
In
apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingQueryService.java
around lines 65-81, the current loop causes N+1 queries, inserts nulls for
missing products and swallows non-NOT_FOUND exceptions; change to collect all
productIds from rankingEntries and call productQueryService to fetch summaries
in bulk, build a Map<productId,ProductLikeSummary>, iterate rankingEntries to
map to summaries while skipping or marking missing products (do not add nulls),
and for exceptions other than NOT_FOUND either log with details and rethrow or
propagate so failures aren't silently ignored; preserve original ranking order
in the final list.

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

import com.loopers.application.product.ProductLikeSummary;
import com.loopers.application.product.ProductQueryService;
import com.loopers.application.ranking.RankingQueryService;
import com.loopers.domain.product.ProductSortType;
import com.loopers.ranking.RankingEntry;
import com.loopers.support.tracking.general.UserActionType;
import com.loopers.interfaces.api.ApiResponse;
import com.loopers.support.tracking.annotation.TrackUserAction;
Expand All @@ -12,11 +14,14 @@
import org.springframework.data.web.PageableDefault;
import org.springframework.web.bind.annotation.*;

import java.util.OptionalDouble;

@RequiredArgsConstructor
@RestController
@RequestMapping("/api/v1/products")
public class ProductV1Controller implements ProductV1ApiSpec{
private final ProductQueryService productQueryService;
private final RankingQueryService rankingQueryService;

@Override
@GetMapping
Expand All @@ -41,7 +46,7 @@ public ApiResponse<ProductV1Dto.ProductListResponse<ProductLikeSummary>> getProd
)
public ApiResponse<ProductV1Dto.ProductDetailResponse<ProductLikeSummary>> getProductDetail(@PathVariable("productId") Long productId) {
ProductLikeSummary productLikeSummary = productQueryService.getProductLikeSummary(productId);

return ApiResponse.success(ProductV1Dto.ProductDetailResponse.of(productLikeSummary));
OptionalDouble rankingEntry = rankingQueryService.getDailyRankingScore(productId);
return ApiResponse.success(ProductV1Dto.ProductDetailResponse.of(productLikeSummary, rankingEntry));
}
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package com.loopers.interfaces.api.product;

import com.loopers.ranking.RankingEntry;
import org.springframework.data.domain.Page;

import java.util.List;
import java.util.OptionalDouble;

public class ProductV1Dto {

Expand All @@ -28,10 +30,15 @@ public static <T> ProductListResponse<T> of(Page<T> page, List<T> content) {
}

public record ProductDetailResponse<T>(
T content
T content,
OptionalDouble rankingScore
){
static <T> ProductDetailResponse<T> of(T content) {
return new ProductDetailResponse<>(content);
return new ProductDetailResponse<>(content, null);
}

static <T> ProductDetailResponse<T> of(T content, OptionalDouble rankingScore) {
return new ProductDetailResponse<>(content, rankingScore);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.loopers.interfaces.api.ranking;

import com.loopers.application.ranking.RankingQueryResponse;
import com.loopers.application.ranking.RankingQueryService;
import lombok.RequiredArgsConstructor;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RequestMapping;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@RequiredArgsConstructor
@RestController
@RequestMapping("/api/v1/ranking")
public class RankingV1Controller {

private final RankingQueryService rankingQueryService;

@GetMapping
public RankingQueryResponse getDailyRanking(
@RequestParam(required = false, name = "date") String date,
@RequestParam(defaultValue = "20", name = "size") int size
) {
return rankingQueryService.getDailyPopularProducts(date, size);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package com.loopers.application.ranking;

import com.loopers.ranking.RankingKey;
import lombok.RequiredArgsConstructor;

import org.springframework.data.redis.connection.RedisZSetCommands;
import org.springframework.data.redis.core.RedisCallback;
import org.springframework.data.redis.core.StringRedisTemplate;
import org.springframework.scheduling.annotation.Scheduled;
import org.springframework.stereotype.Service;

import java.time.*;

@Service
@RequiredArgsConstructor
public class RankingAggregationService {
private static final double LIKE_WEIGHT = 0.2d;
private static final Duration TTL = Duration.ofDays(2);
private static final double CARRY_OVER_WEIGHT = 0.1d;

private final StringRedisTemplate redisTemplate;

public void applyLike(long productId, Instant occurredAt) {
LocalDate day = occurredAt.atZone(ZoneId.systemDefault()).toLocalDate();
String key = RankingKey.dailyAll(day);
String member = String.valueOf(productId);

// Score = Weight * 1
redisTemplate.opsForZSet().incrementScore(key, member, LIKE_WEIGHT);

setTTLOnlyOnce(key);
}

/**
* 일별 λž­ν‚Ήμ— λŒ€ν•œ μ „μΌμž Carry Over μŠ€μΌ€μ€„λŸ¬
* - 맀일 23:30λΆ„ μˆ˜ν–‰ν•©λ‹ˆλ‹€.
*/
@Scheduled(cron = "0 30 23 * * *")
public void carryOverDailyRanking() {
LocalDate today = LocalDate.now(ZoneId.systemDefault());
LocalDate tomorrow = today.plusDays(1);

String sourceKey = RankingKey.dailyAll(today);
String targetKey = RankingKey.dailyAll(tomorrow);

carryOver(sourceKey, targetKey, CARRY_OVER_WEIGHT);
}

private void carryOver(String sourceKey, String targetKey, double weight) {
redisTemplate.execute((RedisCallback<Long>) connection -> {
byte[] target = redisTemplate.getStringSerializer().serialize(targetKey);
byte[] source = redisTemplate.getStringSerializer().serialize(sourceKey);
// ZUNIONSTORE dest 1 src WEIGHTS 0.1 AGGREGATE SUM
Long result = (Long) connection.execute(
"ZUNIONSTORE",
target,
"1".getBytes(),
source,
"WEIGHTS".getBytes(),
String.valueOf(weight).getBytes(),
"AGGREGATE".getBytes(),
"SUM".getBytes()
);
// μˆœμ„œ μ£Όμ˜ν•  것
// ZUNIONSTORE target ... β†’ ν‚€ μž¬μƒμ„± λ˜λ―€λ‘œ 이전 TTL μ†Œλ©Έλ¨
connection.expire(target, TTL.getSeconds());
return result;
});
}

private void setTTLOnlyOnce(String key) {
Boolean exists = redisTemplate.hasKey(key);
if (Boolean.FALSE.equals(exists)) {
redisTemplate.expire(key, TTL);
}
}
Comment on lines +71 to +76
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | πŸ”΄ Critical

TTL이 μ„€μ •λ˜μ§€ μ•ŠλŠ” μ‹¬κ°ν•œ 버그

setTTLOnlyOnceκ°€ incrementScore 이후에 ν˜ΈμΆœλ˜λ―€λ‘œ, 이 μ‹œμ μ—μ„œ ν‚€λŠ” 항상 μ‘΄μž¬ν•©λ‹ˆλ‹€. λ”°λΌμ„œ hasKeyκ°€ 항상 trueλ₯Ό λ°˜ν™˜ν•˜μ—¬ TTL이 μ ˆλŒ€ μ„€μ •λ˜μ§€ μ•ŠμŠ΅λ‹ˆλ‹€.

πŸ”Ž μ œμ•ˆλœ μˆ˜μ •
-    private void setTTLOnlyOnce(String key) {
-        Boolean exists = redisTemplate.hasKey(key);
-        if (Boolean.FALSE.equals(exists)) {
-            redisTemplate.expire(key, TTL);
-        }
-    }
+    /**
+     * TTL이 μ„€μ •λ˜μ§€ μ•Šμ€ κ²½μš°μ—λ§Œ TTL을 μ„€μ •ν•©λ‹ˆλ‹€.
+     */
+    private void setTTLOnlyOnce(String key) {
+        Long currentTtl = redisTemplate.getExpire(key);
+        // TTL이 -1이면 만료 μ‹œκ°„μ΄ μ„€μ •λ˜μ§€ μ•Šμ€ μƒνƒœ
+        if (currentTtl != null && currentTtl == -1) {
+            redisTemplate.expire(key, TTL);
+        }
+    }
πŸ“ 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 void setTTLOnlyOnce(String key) {
Boolean exists = redisTemplate.hasKey(key);
if (Boolean.FALSE.equals(exists)) {
redisTemplate.expire(key, TTL);
}
}
/**
* TTL이 μ„€μ •λ˜μ§€ μ•Šμ€ κ²½μš°μ—λ§Œ TTL을 μ„€μ •ν•©λ‹ˆλ‹€.
*/
private void setTTLOnlyOnce(String key) {
Long currentTtl = redisTemplate.getExpire(key);
// TTL이 -1이면 만료 μ‹œκ°„μ΄ μ„€μ •λ˜μ§€ μ•Šμ€ μƒνƒœ
if (currentTtl != null && currentTtl == -1) {
redisTemplate.expire(key, TTL);
}
}
πŸ€– Prompt for AI Agents
In
apps/commerce-streamer/src/main/java/com/loopers/application/ranking/RankingAggregationService.java
around lines 71-76, setTTLOnlyOnce currently checks hasKey after incrementScore,
so the key always exists and TTL is never set; change the logic to set TTL when
the key is newly created β€” e.g., after performing redis increment, check the
returned incremented value and call redisTemplate.expire(key, TTL) only if the
increment result equals 1 (meaning the key was just created), or alternatively
call a set-if-absent before increment and set TTL when that returns true; update
setTTLOnlyOnce or incrementScore accordingly so TTL is applied exactly once when
the key is created.

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.fasterxml.jackson.databind.ObjectMapper;
import com.loopers.application.idempotency.EventHandledService;
import com.loopers.application.metrics.MetricsAggregationService;
import com.loopers.application.ranking.RankingAggregationService;
import com.loopers.contract.event.ProductLikeEvent;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
Expand All @@ -22,6 +23,7 @@ public class ProductLikeEventConsumer {
private final ObjectMapper objectMapper;
private final EventHandledService eventHandledService;
private final MetricsAggregationService metricsAggregationService;
private final RankingAggregationService rankingAggregationService;

@KafkaListener(topics = "product-like-events", groupId = "${spring.kafka.consumer.group-id}")
public void consume( @Header(KafkaHeaders.RECEIVED_KEY) String key,
Expand All @@ -42,6 +44,7 @@ public void consume( @Header(KafkaHeaders.RECEIVED_KEY) String key,
}

metricsAggregationService.handleProductLiked(event.productId());
rankingAggregationService.applyLike(event.productId(), event.occurredAt());

ack.acknowledge();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,52 @@
import com.loopers.domain.ProductLikeMetricsModel;
import com.loopers.infrastructure.EventHandleRepository;
import com.loopers.infrastructure.ProductLikeMetricsRepository;
import com.loopers.testcontainers.KafkaTestContainersConfig;
import com.loopers.utils.KafkaCleanUp;
import org.apache.kafka.clients.consumer.ConsumerConfig;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.autoconfigure.kafka.KafkaProperties;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.context.annotation.Import;
import org.springframework.kafka.core.KafkaTemplate;
import org.testcontainers.junit.jupiter.Testcontainers;
import org.springframework.test.context.ActiveProfiles;

import java.util.Map;

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

@Testcontainers
@ActiveProfiles("local")
@Import(KafkaTestContainersConfig.class)
@SpringBootTest
class EventHandledServiceTest {

@Autowired
private KafkaCleanUp kafkaCleanUp;

@Autowired
KafkaTemplate<Object,Object> kafkaTemplate;
@Autowired
ProductLikeMetricsRepository metricsRepo;
@Autowired
EventHandleRepository handledRepo;

@Autowired
private KafkaProperties kafkaProperties;

@BeforeEach
void setUp() {
// @BeforeEachλŠ” 이미 SpringBootTest μ»¨ν…μŠ€νŠΈ + KafkaListener μ»¨ν…Œμ΄λ„ˆκ°€ λ‹€ μ˜¬λΌκ°„ 뒀에 μ‹€ν–‰
// kafkaCleanUp.resetAllTestTopics();
kafkaCleanUp.resetAllConsumerGroups();
}

@Test
@DisplayName("λ©±λ“±μ„± ν…ŒμŠ€νŠΈ")
void duplicate_message_should_be_applied_once() throws Exception {

long productId = 1L;
ProductLikeMetricsModel metrics = metricsRepo.findById(productId)
.orElseGet(() -> metricsRepo.save(ProductLikeMetricsModel.of(productId)));
Expand All @@ -42,6 +66,9 @@ void duplicate_message_should_be_applied_once() throws Exception {
// TODO - μΉ΄ν”„μΉ΄ 브둜컀/컨슈머 ν…ŒμŠ€νŠΈν™˜κ²½μ—μ„œ μ„€μ •ν•„μš”
Thread.sleep(1500);

Map<String, Object> consumerProps = kafkaProperties.buildConsumerProperties();
consumerProps.put(ConsumerConfig.GROUP_ID_CONFIG, "${spring.kafka.consumer.group-id}");

Comment on lines +69 to +71
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟑 Minor

consumer properties 섀정이 μ˜¬λ°”λ₯΄μ§€ μ•ŠμŠ΅λ‹ˆλ‹€.

Line 70μ—μ„œ "${spring.kafka.consumer.group-id}" λ¬Έμžμ—΄ λ¦¬ν„°λŸ΄μ„ μ‚¬μš©ν•˜κ³  μžˆλŠ”λ°, μ΄λŠ” Spring ν”Œλ ˆμ΄μŠ€ν™€λ” λ¬Έλ²•μœΌλ‘œ λŸ°νƒ€μž„μ— ν•΄μ„λ˜μ§€ μ•ŠμŠ΅λ‹ˆλ‹€. λ˜ν•œ μ΄λ ‡κ²Œ μƒμ„±ν•œ consumerProps 맡이 μ‹€μ œλ‘œ μ‚¬μš©λ˜μ§€ μ•Šκ³  μžˆμŠ΅λ‹ˆλ‹€.

μ‹€μ œ group-id 값을 μ‚¬μš©ν•˜λ €λ©΄ λ‹€μŒκ³Ό 같이 μˆ˜μ •ν•˜μ„Έμš”:

πŸ”Ž μ œμ•ˆν•˜λŠ” μˆ˜μ •
-        Map<String, Object> consumerProps = kafkaProperties.buildConsumerProperties();
-        consumerProps.put(ConsumerConfig.GROUP_ID_CONFIG, "${spring.kafka.consumer.group-id}");
+        // ν…ŒμŠ€νŠΈμ—μ„œ μ‚¬μš©ν•  μ‹€μ œ consumer group id
+        String actualGroupId = kafkaProperties.getConsumer().getGroupId();

λ§Œμ•½ 이 μ½”λ“œκ°€ μ‹€μ œλ‘œ ν•„μš”ν•˜μ§€ μ•Šλ‹€λ©΄ μ œκ±°ν•˜λŠ” 것을 ꢌμž₯ν•©λ‹ˆλ‹€.

Committable suggestion skipped: line range outside the PR's diff.

πŸ€– Prompt for AI Agents
In
apps/commerce-streamer/src/test/java/com/loopers/application/idempotency/EventHandledServiceTest.java
around lines 69 to 71, the test sets
consumerProps.put(ConsumerConfig.GROUP_ID_CONFIG,
"${spring.kafka.consumer.group-id}") which incorrectly uses a Spring placeholder
string (and the consumerProps map is never used); either remove the unused
consumerProps block entirely, or replace the placeholder with the real group-id
value retrieved from the KafkaProperties object (e.g.,
kafkaProperties.getConsumer().getGroupId()) or from the Environment, and ensure
the populated consumerProps map is actually passed into the consumer setup so
the test uses the correct group-id at runtime.


// then
// like μΉ΄μš΄νŠΈλŠ” 1번만 증가
Expand Down
Loading