Conversation
|
Warning Gemini encountered an error creating the summary. You can try again by commenting |
Summary of Changes이 Pull Request는 사용자가 브랜드명으로 브랜드를 검색할 수 있는 새로운 API 엔드포인트를 도입하여 브랜드 탐색 기능을 확장합니다. 기존의 정렬 및 필터링 옵션을 통합하고 페이지네이션을 지원함으로써, 사용자는 더욱 유연하고 효율적으로 원하는 브랜드를 찾을 수 있게 됩니다. 이는 전반적인 사용자 경험을 향상시키고 애플리케이션의 검색 기능을 강화하는 데 기여합니다. Highlights
Changelog
Activity
|
|
/gemini review |
|
/re-review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 브랜드 검색 API를 추가하고 페이지네이션을 적용하는 중요한 기능 개선을 포함하고 있습니다. 전반적으로 기능 구현은 잘 되었지만, 몇 가지 심각한 성능 문제를 발견했습니다. 특히, searchMatchingBrands 메소드에서 N+1 쿼리 문제와 모든 데이터를 메모리에 로드하여 처리하는 방식은 서비스 확장성에 큰 걸림돌이 될 수 있습니다. 이 부분은 데이터베이스 레벨에서 처리하도록 리팩토링이 시급하며, 이는 기존 코드 리뷰 규칙과도 일치합니다. 또한, 태그 필터링 로직의 효율성을 개선할 수 있는 부분도 제안했습니다.
자세한 내용은 각 코드 리뷰 코멘트를 참고해주세요.
| List<Long> brandIds = brandHistories.stream() | ||
| .map(h -> h.getBrand().getId()) | ||
| .toList(); | ||
| Map<Long, List<String>> brandDescribeTagMap = brandIds.stream() | ||
| .collect(Collectors.toMap( | ||
| brandId -> brandId, | ||
| brandId -> brandDescribeTagRepository.findAllByBrandId(brandId).stream() | ||
| .map(BrandDescribeTag::getBrandDescribeTag) | ||
| .toList() | ||
| )); |
There was a problem hiding this comment.
brandDescribeTagMap을 생성하는 과정에서 N+1 쿼리 문제가 발생합니다. brandIds 리스트를 순회하며 각 brandId에 대해 brandDescribeTagRepository.findAllByBrandId(brandId)를 호출하고 있어 브랜드 수만큼 쿼리가 실행됩니다. 이로 인해 심각한 성능 저하가 발생할 수 있습니다.
모든 태그를 한 번의 쿼리로 가져온 후 메모리에서 그룹화하는 방식으로 수정해야 합니다.
BrandDescribeTagRepository에 findAllByBrandIdIn(List<Long> brandIds) 메소드를 추가하고, 이를 사용하여 아래와 같이 수정하는 것을 권장합니다.
// 1. BrandDescribeTagRepository.java 에 다음 메소드 추가
List<BrandDescribeTag> findAllByBrandIdIn(List<Long> brandIds);
// 2. MatchServiceImpl.java 수정
List<Long> brandIds = brandHistories.stream()
.map(h -> h.getBrand().getId())
.toList();
List<BrandDescribeTag> allTags = brandDescribeTagRepository.findAllByBrandIdIn(brandIds);
Map<Long, List<String>> brandDescribeTagMap = allTags.stream()
.collect(Collectors.groupingBy(
tag -> tag.getBrand().getId(),
Collectors.mapping(BrandDescribeTag::getBrandDescribeTag, Collectors.toList())
));References
- To prevent N+1 query issues, fetch multiple items in a single batch query (e.g., using an 'IN' clause) instead of querying for each item within a loop.
| List<MatchBrandResponseDto.BrandDto> filteredBrands = brandHistories.stream() | ||
| .filter(history -> filterBrandByCategory(history.getBrand(), category)) | ||
| .filter(history -> filterBrandByTitle(history.getBrand(), title)) | ||
| .filter(history -> filterBrandByTags(history.getBrand().getId(), tags, brandDescribeTagMap)) | ||
| .sorted(getBrandHistoryComparator(sortBy, brandLikeCountMap)) | ||
| .map(history -> toMatchBrandDtoFromHistory(history, likedBrandIds, recruitingBrandIds, brandDescribeTagMap)) | ||
| .toList(); | ||
|
|
||
| int total = filteredBrands.size(); | ||
| int safePage = Math.max(page, 0); | ||
| int safeSize = size <= 0 ? 20 : size; | ||
| int fromIndex = safePage * safeSize; | ||
| int toIndex = Math.min(fromIndex + safeSize, total); | ||
| List<MatchBrandResponseDto.BrandDto> pagedBrands = | ||
| fromIndex >= total ? List.of() : filteredBrands.subList(fromIndex, toIndex); |
There was a problem hiding this comment.
필터링, 정렬, 페이지네이션을 모두 메모리에서 처리하고 있습니다. brandHistories에서 모든 브랜드 정보를 가져온 후 애플리케이션에서 처리하는 방식은 데이터 양이 증가하면 심각한 성능 저하와 메모리 부족(OOM)을 유발할 수 있습니다.
이러한 작업은 데이터베이스 레벨에서 처리하는 것이 효율적입니다. Specification이나 QueryDSL을 사용하거나, 복잡한 JPQL 쿼리를 작성하여 데이터베이스가 필터링, 정렬, 페이지네이션을 모두 처리하도록 수정하는 것을 강력히 권장합니다. 이렇게 하면 애플리케이션의 부하를 크게 줄이고 확장성을 확보할 수 있습니다.
References
- Avoid filtering large collections in memory; instead, push filtering logic down to the database layer by adding specific query methods to the repository for better performance.
| private boolean filterBrandByTags(Long brandId, List<String> tags, Map<Long, List<String>> brandDescribeTagMap) { | ||
| if (tags == null || tags.isEmpty()) { | ||
| return true; | ||
| } | ||
| List<String> brandTags = brandDescribeTagMap.getOrDefault(brandId, List.of()); | ||
| if (brandTags.isEmpty()) { | ||
| return false; | ||
| } | ||
|
|
||
| List<String> normalizedBrandTags = brandTags.stream() | ||
| .filter(StringUtils::hasText) | ||
| .map(tag -> tag.trim().toLowerCase()) | ||
| .toList(); | ||
|
|
||
| return tags.stream() | ||
| .filter(StringUtils::hasText) | ||
| .map(tag -> tag.trim().toLowerCase()) | ||
| .anyMatch(normalizedBrandTags::contains); | ||
| } |
There was a problem hiding this comment.
filterBrandByTags 메소드에서 태그를 비교하는 로직이 비효율적입니다. normalizedBrandTags가 List인 상태에서 contains를 호출하면 내부적으로 반복문이 실행되어 O(N*M)의 시간 복잡도를 가집니다 (N: 필터 태그 수, M: 브랜드 태그 수).
normalizedBrandTags를 Set으로 변환하여 contains 호출 시 O(1)의 시간 복잡도를 갖도록 하거나, Collections.disjoint를 사용하여 두 컬렉션에 공통 요소가 있는지 확인하는 것이 더 효율적이고 코드가 간결해집니다.
private boolean filterBrandByTags(Long brandId, List<String> tags, Map<Long, List<String>> brandDescribeTagMap) {
if (tags == null || tags.isEmpty()) {
return true;
}
List<String> brandTags = brandDescribeTagMap.getOrDefault(brandId, List.of());
if (brandTags.isEmpty()) {
return false;
}
List<String> normalizedFilterTags = tags.stream()
.filter(StringUtils::hasText)
.map(tag -> tag.trim().toLowerCase())
.toList();
List<String> normalizedBrandTags = brandTags.stream()
.filter(StringUtils::hasText)
.map(tag -> tag.trim().toLowerCase())
.toList();
return !java.util.Collections.disjoint(normalizedBrandTags, normalizedFilterTags);
}
|
/re-review |
|
/gemini review @gemini-code-assist 빨리 전반적인 리뷰를 다시 하란말이다!!! 코드 품질 개선할 점이 있는지, 시스템 장애 문제 잠재 가능성이 없는지 확인해. |
3 participants
Summary
/api/v1/brands/search 엔드포인트를 추가해 매칭된 브랜드를 브랜드명(title)로 검색할 수 있게 했고, sortBy, category, tags 필터를 동일하게 적용했습니다. 검색 결과는 페이지네이션(page, size)을 지원합니다.
Changes
Type of Change
Related Issues
Closes #333
참고 사항