Skip to content
Open
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
Expand Up @@ -5,11 +5,18 @@
import com.loopers.domain.brand.BrandService;
import com.loopers.domain.product.Product;
import com.loopers.domain.product.ProductService;
import com.loopers.domain.ranking.ProductRankMonthly;
import com.loopers.domain.ranking.ProductRankWeekly;
import com.loopers.infrastructure.ranking.ProductRankMonthlyJpaRepository;
import com.loopers.infrastructure.ranking.ProductRankWeeklyJpaRepository;
import com.loopers.infrastructure.ranking.RankingCacheService;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;

import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.time.temporal.WeekFields;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
Expand All @@ -19,12 +26,32 @@
@Component
public class RankingFacade {

private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyyMMdd");

private final RankingCacheService rankingCacheService;
private final ProductService productService;
private final BrandService brandService;
private final ProductRankWeeklyJpaRepository productRankWeeklyJpaRepository;
private final ProductRankMonthlyJpaRepository productRankMonthlyJpaRepository;

@Transactional(readOnly = true)
public List<RankingInfo> getRankings(String date, int page, int size) {
public List<RankingInfo> getRankings(String date, int page, int size, RankingType type) {
return switch (type) {
case DAILY -> getDailyRankings(date, page, size);
case WEEKLY -> getWeeklyRankings(date, page, size);
case MONTHLY -> getMonthlyRankings(date, page, size);
};
}

public long getTotalCount(String date, RankingType type) {
return switch (type) {
case DAILY -> rankingCacheService.getTotalCount(date);
case WEEKLY -> productRankWeeklyJpaRepository.countByYearWeek(toYearWeek(date));
case MONTHLY -> productRankMonthlyJpaRepository.countByYearMonth(toYearMonth(date));
};
}

private List<RankingInfo> getDailyRankings(String date, int page, int size) {
int offset = (page - 1) * size;
List<String> productIdStrings = rankingCacheService.getProductIds(date, offset, size);

Expand All @@ -34,32 +61,100 @@ public List<RankingInfo> getRankings(String date, int page, int size) {

List<Long> ids = productIdStrings.stream().map(Long::valueOf).toList();
List<Product> products = productService.getProductsByIds(ids);

List<Long> brandIds = products.stream().map(Product::getBrandId).distinct().toList();
Map<Long, Brand> brandMap = brandService.getBrandsByIds(brandIds).stream()
.collect(Collectors.toMap(Brand::getId, b -> b));

Map<Long, Brand> brandMap = toBrandMap(products);
Map<Long, Product> productMap = products.stream()
.collect(Collectors.toMap(Product::getId, p -> p));

List<RankingInfo> result = new ArrayList<>();
for (int i = 0; i < productIdStrings.size(); i++) {
Long productId = Long.valueOf(productIdStrings.get(i));
Product product = productMap.get(productId);
if (product == null) {
continue;
}
if (product == null) continue;
Brand brand = brandMap.get(product.getBrandId());
if (brand == null) {
continue;
}
ProductInfo productInfo = ProductInfo.from(product, brand);
result.add(RankingInfo.of(offset + i + 1, productInfo));
if (brand == null) continue;
result.add(RankingInfo.of(offset + i + 1, ProductInfo.from(product, brand)));
}
return result;
}

public long getTotalCount(String date) {
return rankingCacheService.getTotalCount(date);
private List<RankingInfo> getWeeklyRankings(String date, int page, int size) {
String yearWeek = toYearWeek(date);
List<ProductRankWeekly> ranked = productRankWeeklyJpaRepository
.findByYearWeekOrderByRankPosition(yearWeek);

int offset = (page - 1) * size;
List<ProductRankWeekly> paged = ranked.stream()
.skip(offset).limit(size).toList();

if (paged.isEmpty()) {
return List.of();
}

List<Long> ids = paged.stream().map(ProductRankWeekly::getProductId).toList();
List<Product> products = productService.getProductsByIds(ids);
Map<Long, Brand> brandMap = toBrandMap(products);
Map<Long, Product> productMap = products.stream()
.collect(Collectors.toMap(Product::getId, p -> p));

return paged.stream()
.map(r -> {
Product product = productMap.get(r.getProductId());
if (product == null) return null;
Brand brand = brandMap.get(product.getBrandId());
if (brand == null) return null;
return RankingInfo.of(r.getRankPosition(), ProductInfo.from(product, brand));
})
.filter(r -> r != null)
.toList();
}

private List<RankingInfo> getMonthlyRankings(String date, int page, int size) {
String yearMonth = toYearMonth(date);
List<ProductRankMonthly> ranked = productRankMonthlyJpaRepository
.findByYearMonthOrderByRankPosition(yearMonth);

int offset = (page - 1) * size;
List<ProductRankMonthly> paged = ranked.stream()
.skip(offset).limit(size).toList();

if (paged.isEmpty()) {
return List.of();
}

List<Long> ids = paged.stream().map(ProductRankMonthly::getProductId).toList();
List<Product> products = productService.getProductsByIds(ids);
Map<Long, Brand> brandMap = toBrandMap(products);
Map<Long, Product> productMap = products.stream()
.collect(Collectors.toMap(Product::getId, p -> p));

return paged.stream()
.map(r -> {
Product product = productMap.get(r.getProductId());
if (product == null) return null;
Brand brand = brandMap.get(product.getBrandId());
if (brand == null) return null;
return RankingInfo.of(r.getRankPosition(), ProductInfo.from(product, brand));
})
.filter(r -> r != null)
.toList();
}

private Map<Long, Brand> toBrandMap(List<Product> products) {
List<Long> brandIds = products.stream().map(Product::getBrandId).distinct().toList();
return brandService.getBrandsByIds(brandIds).stream()
.collect(Collectors.toMap(Brand::getId, b -> b));
}

private String toYearWeek(String date) {
LocalDate localDate = LocalDate.parse(date, DATE_FORMATTER);
WeekFields weekFields = WeekFields.ISO;
int week = localDate.get(weekFields.weekOfWeekBasedYear());
int year = localDate.get(weekFields.weekBasedYear());
return year + "-W" + String.format("%02d", week);
}

private String toYearMonth(String date) {
LocalDate localDate = LocalDate.parse(date, DATE_FORMATTER);
return localDate.format(DateTimeFormatter.ofPattern("yyyy-MM"));
}
Comment on lines +148 to 159
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

잘못된 date 형식 입력 시 DateTimeParseException이 그대로 전파된다.

date 파라미터가 yyyyMMdd 형식이 아닌 경우 LocalDate.parse에서 DateTimeParseException이 발생하여 500 에러로 응답된다. Controller 또는 Facade에서 적절한 예외 변환(400 Bad Request)이 필요하다.

🛡️ 예외 처리 추가 예시
 private String toYearWeek(String date) {
+    try {
         LocalDate localDate = LocalDate.parse(date, DATE_FORMATTER);
         WeekFields weekFields = WeekFields.ISO;
         int week = localDate.get(weekFields.weekOfWeekBasedYear());
         int year = localDate.get(weekFields.weekBasedYear());
         return year + "-W" + String.format("%02d", week);
+    } catch (DateTimeParseException e) {
+        throw new CoreException(ErrorType.INVALID_DATE_FORMAT, "date 형식이 올바르지 않습니다: " + date);
+    }
 }
🤖 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/ranking/RankingFacade.java`
around lines 148 - 159, Both toYearWeek and toYearMonth call
LocalDate.parse(date, DATE_FORMATTER) and let DateTimeParseException bubble up
causing 500 responses; wrap the parse call in a try-catch for
DateTimeParseException inside each method (toYearWeek and toYearMonth) and
convert it into a client-facing exception (e.g., throw new
IllegalArgumentException or a custom InvalidDateFormatException with a clear
message that includes the invalid input) so the controller can map it to a 400
Bad Request.

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

public enum RankingType {
DAILY, WEEKLY, MONTHLY
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.loopers.infrastructure.ranking;

import com.loopers.domain.ranking.ProductRankMonthly;
import org.springframework.data.jpa.repository.JpaRepository;

import java.util.List;

public interface ProductRankMonthlyJpaRepository extends JpaRepository<ProductRankMonthly, Long> {

List<ProductRankMonthly> findByYearMonthOrderByRankPosition(String yearMonth);

long countByYearMonth(String yearMonth);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package com.loopers.infrastructure.ranking;

import com.loopers.domain.ranking.ProductRankWeekly;
import org.springframework.data.jpa.repository.JpaRepository;

import java.util.List;

public interface ProductRankWeeklyJpaRepository extends JpaRepository<ProductRankWeekly, Long> {

List<ProductRankWeekly> findByYearWeekOrderByRankPosition(String yearWeek);

long countByYearWeek(String yearWeek);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import com.loopers.application.ranking.RankingFacade;
import com.loopers.application.ranking.RankingInfo;
import com.loopers.application.ranking.RankingType;
import com.loopers.interfaces.api.ApiResponse;
import lombok.RequiredArgsConstructor;
import org.springframework.web.bind.annotation.GetMapping;
Expand All @@ -25,13 +26,15 @@ public class RankingV1Controller {
@GetMapping
public ApiResponse<RankingV1Dto.RankingPageResponse> getRankings(
@RequestParam(required = false) String date,
@RequestParam(defaultValue = "daily") String type,
@RequestParam(defaultValue = "20") int size,
@RequestParam(defaultValue = "1") int page
) {
String rankingDate = (date != null) ? date : LocalDate.now().format(DATE_FORMATTER);
RankingType rankingType = RankingType.valueOf(type.toUpperCase());
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

RankingType.valueOf() 예외 처리 누락으로 일관성 없는 에러 응답 발생 가능

type.toUpperCase()가 유효하지 않은 값(예: "invalid")일 경우 IllegalArgumentException이 발생하며, 이는 CoreException을 통한 표준 에러 처리 경로를 우회하여 일관성 없는 에러 응답을 반환한다.

운영 관점에서 클라이언트가 잘못된 type 파라미터를 전달했을 때 500 에러가 아닌 명확한 400 Bad Request 응답이 필요하다.

수정안
-        RankingType rankingType = RankingType.valueOf(type.toUpperCase());
+        RankingType rankingType;
+        try {
+            rankingType = RankingType.valueOf(type.toUpperCase());
+        } catch (IllegalArgumentException e) {
+            throw new CoreException(ErrorType.INVALID_INPUT_VALUE, "Invalid ranking type: " + type);
+        }

또는 RankingType에 정적 팩토리 메서드를 추가하여 CoreException을 던지도록 구현할 수 있다.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingV1Controller.java`
at line 34, 현재 RankingV1Controller에서 RankingType rankingType =
RankingType.valueOf(type.toUpperCase()); 호출 시 잘못된 type 값이면
IllegalArgumentException이 발생하여 CoreException 기반의 일관된 에러 처리 경로를 우회합니다; 수정 방법은
RankingV1Controller에서 valueOf 호출을 try-catch로 감싸고 IllegalArgumentException을 잡아
CoreException(또는 적절한 400 Bad Request 상태를 표현하는 예외)으로 변환해 던지거나, 대안으로 RankingType에
정적 팩토리 메서드(e.g., RankingType.from(String))를 추가해 유효성 검증 후 유효하지 않으면 CoreException을
던지도록 구현해 일관된 400 응답이 반환되게 하세요.


List<RankingInfo> rankings = rankingFacade.getRankings(rankingDate, page, size);
long totalElements = rankingFacade.getTotalCount(rankingDate);
List<RankingInfo> rankings = rankingFacade.getRankings(rankingDate, page, size, rankingType);
long totalElements = rankingFacade.getTotalCount(rankingDate, rankingType);

List<RankingV1Dto.RankingItemResponse> content = rankings.stream()
.map(RankingV1Dto.RankingItemResponse::from)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
package com.loopers.application.coupon;

import com.loopers.domain.coupon.CouponTemplate;
import com.loopers.domain.coupon.CouponIssueRequest;
import com.loopers.domain.coupon.CouponService;
import com.loopers.domain.coupon.CouponTemplateRepository;
import com.loopers.domain.coupon.CouponType;
import com.loopers.domain.coupon.IssuedCoupon;
import com.loopers.domain.coupon.IssuedCouponRepository;
import com.loopers.domain.outbox.OutboxEventPublisher;
import com.loopers.domain.user.User;
import com.loopers.domain.user.UserService;
import com.loopers.support.error.CoreException;
Expand All @@ -17,9 +17,6 @@
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import java.time.ZonedDateTime;
import java.util.Optional;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
Expand All @@ -29,106 +26,90 @@
class CouponFacadeTest {

@Mock
private CouponTemplateRepository couponTemplateRepository;
private CouponService couponService;

@Mock
private IssuedCouponRepository issuedCouponRepository;
private UserService userService;

@Mock
private UserService userService;
private OutboxEventPublisher outboxEventPublisher;

private CouponFacade couponFacade;
@Mock
private CouponTemplateRepository couponTemplateRepository;

@Mock
private IssuedCouponRepository issuedCouponRepository;

private static final ZonedDateTime FUTURE = ZonedDateTime.now().plusDays(30);
private CouponFacade couponFacade;

@BeforeEach
void setUp() {
couponFacade = new CouponFacade(couponTemplateRepository, issuedCouponRepository, userService);
couponFacade = new CouponFacade(couponService, userService, outboxEventPublisher,
couponTemplateRepository, issuedCouponRepository);
}

@DisplayName("쿠폰 발급")
@DisplayName("쿠폰 발급 요청")
@Nested
class IssueCoupon {
class RequestIssue {

@DisplayName("유효한 쿠폰 템플릿으로 발급 요청하면, 발급된 쿠폰을 반환한다.")
@DisplayName("유효한 쿠폰으로 발급 요청하면, 발급 요청 정보를 반환한다.")
@Test
void returnsIssuedCoupon_whenTemplateIsValid() {
void returnsCouponIssueInfo_whenCouponIsValid() {
// Arrange
String loginId = "user1";
String rawPassword = "Test1234!";
User user = new User(loginId, "encoded", "홍길동", "19900101", "test@naver.com");
CouponTemplate template = new CouponTemplate("10% 할인", CouponType.RATE, 10, null, FUTURE);
CouponIssueRequest request = new CouponIssueRequest(1L, user.getId());

given(userService.authenticate(loginId, rawPassword)).willReturn(user);
given(couponTemplateRepository.findById(1L)).willReturn(Optional.of(template));
given(issuedCouponRepository.existsByUserIdAndCouponTemplateId(any(), any())).willReturn(false);
given(issuedCouponRepository.save(any())).willAnswer(inv -> inv.getArgument(0));
given(couponService.createRequest(1L, user.getId())).willReturn(request);

Comment on lines +62 to 66
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

인자 검증이 느슨해 잘못된 사용자/요청 매핑 회귀를 놓칠 수 있다

운영 관점에서 현재 스텁은 any() 사용과 불명확한 userId 설정 때문에 파사드가 잘못된 인자를 넘겨도 테스트가 통과할 수 있어 장애를 사전에 차단하지 못한다. 수정안으로 userId를 명시적으로 고정하고 eq(...) 기반 스텁/검증으로 호출 인자를 강하게 고정하는 것이 필요하다. 추가 테스트로 verify(couponService).createRequest(1L, userId) 및 잘못된 템플릿 ID 전달 시 실패 검증을 추가하는 것이 좋다.

권장 수정 예시
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
+import org.springframework.test.util.ReflectionTestUtils;

 User user = new User(loginId, "encoded", "홍길동", "19900101", "test@naver.com");
-CouponIssueRequest request = new CouponIssueRequest(1L, user.getId());
+ReflectionTestUtils.setField(user, "id", 10L);
+Long userId = 10L;
+CouponIssueRequest request = new CouponIssueRequest(1L, userId);

-given(couponService.createRequest(1L, user.getId())).willReturn(request);
+given(couponService.createRequest(eq(1L), eq(userId))).willReturn(request);
 ...
+verify(couponService).createRequest(1L, userId);

-given(couponService.createRequest(any(), any()))
+given(couponService.createRequest(eq(999L), eq(userId)))
     .willThrow(new CoreException(ErrorType.NOT_FOUND, "쿠폰을 찾을 수 없습니다."));

As per coding guidelines **/*Test*.java: "Mock 남용으로 의미가 약해지면 테스트 방향을 재정렬하도록 제안한다."

Also applies to: 83-84, 106-110

🤖 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/coupon/CouponFacadeTest.java`
around lines 62 - 66, The test uses loose stubbing allowing wrong args to
pass—replace any()/implicit user ids by declaring a concrete userId variable and
use eq(userId)/eq(1L) in the stubs for userService.authenticate(...) and
couponService.createRequest(...), then add explicit verifications like
verify(couponService).createRequest(eq(1L), eq(userId)) and add a negative test
case that asserts a call with an incorrect template id fails; update references
in this file to CouponIssueRequest, userService.authenticate,
couponService.createRequest and the verify(couponService).createRequest
assertion accordingly.

// Act
CouponInfo.IssuedInfo result = couponFacade.issueCoupon(loginId, rawPassword, 1L);
CouponIssueInfo result = couponFacade.requestIssue(loginId, rawPassword, 1L);

// Assert
assertThat(result).isNotNull();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

성공 케이스 단언이 notNull만 있어 회귀 탐지력이 낮다

운영 관점에서 Line [71], Line [115]의 단언은 결과 객체가 비어 있지 않다는 사실만 확인하므로 필드 매핑 오류나 잘못된 상태 반환이 배포 전 탐지되지 않는다. 수정안으로 CouponIssueInfo의 핵심 필드(예: 요청 ID, 쿠폰 템플릿 ID, 사용자 ID, 상태)를 정확 값으로 단언해야 한다. 추가 테스트로 의도적으로 다른 요청 데이터를 반환하도록 스텁한 뒤 필드 단언이 실패하는지 확인하는 케이스를 넣는 것이 좋다.

As per coding guidelines **/*Test*.java: "단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다."

Also applies to: 115-115

🤖 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/coupon/CouponFacadeTest.java`
at line 71, The test currently only asserts result is not null in
CouponFacadeTest; replace and augment these weak assertions by asserting
CouponIssueInfo's core fields (e.g., requestId, couponTemplateId, userId,
status) equal the expected values from the fixture/stub used to produce the
result (reference the result variable and the method under test in
CouponFacadeTest), and add a complementary negative test that stubs the
dependency to return mismatched/different request data and verifies those field
assertions fail (or that the facade returns the correct error/exception) to
cover regression and edge/failure flows.

}

@DisplayName("존재하지 않는 쿠폰 템플릿으로 발급 요청하면, NOT_FOUND 예외가 발생한다.")
@DisplayName("존재하지 않는 쿠폰으로 발급 요청하면, NOT_FOUND 예외가 발생한다.")
@Test
void throwsNotFound_whenTemplateDoesNotExist() {
void throwsNotFound_whenCouponDoesNotExist() {
// Arrange
String loginId = "user1";
String rawPassword = "Test1234!";
User user = new User(loginId, "encoded", "홍길동", "19900101", "test@naver.com");

given(userService.authenticate(loginId, rawPassword)).willReturn(user);
given(couponTemplateRepository.findById(999L)).willReturn(Optional.empty());
given(couponService.createRequest(any(), any()))
.willThrow(new CoreException(ErrorType.NOT_FOUND, "쿠폰을 찾을 수 없습니다."));

// Act
CoreException exception = assertThrows(CoreException.class,
() -> couponFacade.issueCoupon(loginId, rawPassword, 999L));
() -> couponFacade.requestIssue(loginId, rawPassword, 999L));

// Assert
assertThat(exception.getErrorType()).isEqualTo(ErrorType.NOT_FOUND);
}

@DisplayName("이미 발급받은 쿠폰을 다시 발급 요청하면, CONFLICT 예외가 발생한다.")
@Test
void throwsConflict_whenCouponAlreadyIssued() {
// Arrange
String loginId = "user1";
String rawPassword = "Test1234!";
User user = new User(loginId, "encoded", "홍길동", "19900101", "test@naver.com");
CouponTemplate template = new CouponTemplate("10% 할인", CouponType.RATE, 10, null, FUTURE);

given(userService.authenticate(loginId, rawPassword)).willReturn(user);
given(couponTemplateRepository.findById(1L)).willReturn(Optional.of(template));
given(issuedCouponRepository.existsByUserIdAndCouponTemplateId(any(), any())).willReturn(true);

// Act
CoreException exception = assertThrows(CoreException.class,
() -> couponFacade.issueCoupon(loginId, rawPassword, 1L));

// Assert
assertThat(exception.getErrorType()).isEqualTo(ErrorType.CONFLICT);
}
}

@DisplayName("쿠폰 목록 조회")
@DisplayName("쿠폰 발급 결과 조회")
@Nested
class GetMyCoupons {
class GetIssueResult {

@DisplayName("인증된 유저의 쿠폰 목록을 반환한다.")
@DisplayName("인증된 유저가 발급 요청 결과를 조회하면 결과를 반환한다.")
@Test
void returnsMyCoupons_whenAuthenticated() {
void returnsIssueResult_whenAuthenticated() {
// Arrange
String loginId = "user1";
String rawPassword = "Test1234!";
User user = new User(loginId, "encoded", "홍길동", "19900101", "test@naver.com");
CouponIssueRequest request = new CouponIssueRequest(1L, user.getId());

given(userService.authenticate(loginId, rawPassword)).willReturn(user);
given(issuedCouponRepository.findAllByUserId(any())).willReturn(java.util.List.of());
given(couponService.getRequest(1L)).willReturn(request);

// Act
var result = couponFacade.getMyCoupons(loginId, rawPassword);
CouponIssueInfo result = couponFacade.getIssueResult(loginId, rawPassword, 1L);

// Assert
assertThat(result).isNotNull();
Expand Down
Loading