Issue#308/fix/course#311
Conversation
- 기존: 리뷰 목록 순회하며 개별 조회 (N+1 문제) - 개선: (placeId, userId) 쌍을 추출하여 배치 조회 후 메모리 매핑
- Edge case 테스트 추가 - 비로그인 사용자(user null) - 장소가 없는 루트 - 사진이 없는 장소 - 검색 결과 없음(서비스 호출 검증 추가)
- 전용 상수 클래스 PaginationConstants를 만들어 모든 LIMIT 상수 한 곳에 집중 - PLACE_PHOTO_LIMIT: 장소 별 사진 개수 - REVIEW_PHOTO_LIMIT: 리뷰 별 사진 개수 - FOLDER_THUMBNAIL_LIMIT: 폴더 썸네일 사진 개수 - PLACE_REVIEW_LIMIT: 장소 별 리뷰 개수
Summary of ChangesHello @lovelyAlien, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves application performance and maintainability by tackling N+1 query problems and centralizing configuration. It optimizes data retrieval for photos and route places, ensuring more efficient database interactions. The introduction of a dedicated constants file streamlines the management of pagination limits, making the codebase cleaner and easier to adapt. Additionally, the changes include minor bug fixes and more robust testing for search functionalities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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
This pull request primarily focuses on optimizing data fetching to address N+1 issues and centralizing pagination-related constants. Key changes include the introduction of a PaginationConstants class to manage limits for photos and reviews across various services like AdminFacade, FolderFacade, PhotoService, ReviewService, and RouteFacade. The PhotoRepository was enhanced with a new native PostgreSQL query utilizing ROW_NUMBER() and unnest to efficiently fetch limited photos per place and user in a single batch, which PhotoService now leverages through a new getPhotosByPlaceAndUserBatch method. The DateSchedService and RouteRepository were updated to use LEFT JOIN FETCH for eager loading of RoutePlace entities, preventing N+1 problems when fetching routes. Additionally, the Route entity's removePlace method was corrected to compare Place entities by their ID for reliability. Review comments highlighted the need to declare the newly introduced limit fields as static final constants for better adherence to Java conventions and suggested using a dedicated key object (like a Java record) instead of a concatenated string for map keys in getPhotosByPlaceAndUserBatch for improved type safety and readability. A comment also noted the PhotoRepository's native query's strong dependency on PostgreSQL, recommending a comment to acknowledge this database-specific implementation. Finally, the SearchFacadeTest was expanded to include more comprehensive tests for route search, covering various scenarios and verifying interactions with the new photo fetching logic.
| private final RouteService routeService; | ||
| private final FolderService folderService; | ||
| private final static int PLACE_DEFAULT_PHOTO_LIMIT = 30; | ||
| private final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; |
There was a problem hiding this comment.
This field is intended to be a constant. According to Java conventions, constants should be declared as static final. While it works as an instance field in a singleton service, making it static final clarifies its nature as a compile-time constant and is consistent with best practices.
| private final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; | |
| private static final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; |
| private final FolderService folderService; | ||
| private final PlaceService placeService; | ||
| private final PhotoService photoService; | ||
| private final int FOLDER_THUMBNAIL_LIMIT = PaginationConstants.FOLDER_THUMBNAIL_LIMIT; |
There was a problem hiding this comment.
This field is intended to be a constant. According to Java conventions, constants should be declared as static final. While it works as an instance field in a singleton service, making it static final clarifies its nature as a compile-time constant and is consistent with best practices.
| private final int FOLDER_THUMBNAIL_LIMIT = PaginationConstants.FOLDER_THUMBNAIL_LIMIT; | |
| private static final int FOLDER_THUMBNAIL_LIMIT = PaginationConstants.FOLDER_THUMBNAIL_LIMIT; |
| @Query(value = "SELECT p.* FROM ( " + | ||
| " SELECT p.*, " + | ||
| " ROW_NUMBER() OVER(PARTITION BY p.place_id, p.user_id ORDER BY p.created_at DESC) as rn " + | ||
| " FROM photo p " + | ||
| " WHERE p.type = :type " + | ||
| " AND EXISTS ( " + | ||
| " SELECT 1 FROM unnest(CAST(:placeIds AS bigint[]), CAST(:userIds AS bigint[])) AS t(place_id, user_id) " + | ||
| " WHERE p.place_id = t.place_id AND p.user_id = t.user_id " + | ||
| " ) " + | ||
| ") p " + | ||
| "WHERE p.rn <= :limit", nativeQuery = true) | ||
| List<Photo> findLimitedPhotosPerPlaceAndUser(@Param("placeIds") List<Long> placeIds, | ||
| @Param("userIds") List<Long> userIds, | ||
| @Param("type") String type, | ||
| @Param("limit") int limit); |
There was a problem hiding this comment.
The native query is highly optimized for PostgreSQL using unnest and window functions, which is great for performance. However, this creates a hard dependency on PostgreSQL, reducing portability. If the application might need to support other databases in the future, this could become a maintenance challenge. It would be good practice to add a comment mentioning this database-specific implementation.
| private final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; | ||
| private final int REVIEW_PHOTO_LIMIT = PaginationConstants.REVIEW_PHOTO_LIMIT; |
There was a problem hiding this comment.
These fields are intended to be constants. According to Java conventions, constants should be declared as static final. While it works as an instance field in a singleton service, making them static final clarifies their nature as compile-time constants and is consistent with best practices.
| private final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; | |
| private final int REVIEW_PHOTO_LIMIT = PaginationConstants.REVIEW_PHOTO_LIMIT; | |
| private static final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; | |
| private static final int REVIEW_PHOTO_LIMIT = PaginationConstants.REVIEW_PHOTO_LIMIT; |
| return photos.stream() | ||
| .collect(Collectors.groupingBy( | ||
| photo -> photo.getPlace().getId() + ":" + photo.getUser().getId() | ||
| )); |
There was a problem hiding this comment.
Using a concatenated string photo.getPlace().getId() + ":" + photo.getUser().getId() as a map key is functional but can be fragile and less readable. A more robust and type-safe approach is to use a dedicated key object, such as a Java record. This improves code clarity and maintainability.
For example, you could define a record:
private record PlaceUserKey(Long placeId, Long userId) {}And then use it for grouping:
return photos.stream()
.collect(Collectors.groupingBy(
photo -> new PlaceUserKey(photo.getPlace().getId(), photo.getUser().getId())
));This would require changing the return type to Map<PlaceUserKey, List<Photo>> and adjusting the calling code in RouteFacade accordingly.
| private final ReviewRepository reviewRepository; | ||
| private final PaginationService paginationService; | ||
| private final static int PLACE_DEFAULT_REVIEW_LIMIT = 5; | ||
| private final int PLACE_REVIEW_LIMIT = PaginationConstants.PLACE_REVIEW_LIMIT; |
There was a problem hiding this comment.
This field is intended to be a constant. According to Java conventions, constants should be declared as static final. While it works as an instance field in a singleton service, making it static final clarifies its nature as a compile-time constant and is consistent with best practices.
| private final int PLACE_REVIEW_LIMIT = PaginationConstants.PLACE_REVIEW_LIMIT; | |
| private static final int PLACE_REVIEW_LIMIT = PaginationConstants.PLACE_REVIEW_LIMIT; |
| */ | ||
| public void removePlace(Place place) { | ||
| this.routePlaces.removeIf(rp -> rp.getPlace().equals(place)); | ||
| this.routePlaces.removeIf(rp -> rp.getPlace().getId().equals(place.getId())); |
There was a problem hiding this comment.
Using equals() to compare JPA entities can be unreliable due to proxy objects and entity state. Comparing by ID is a much safer and more robust approach. This is a great correction.
| private final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; | ||
| private final int REVIEW_PHOTO_LIMIT = PaginationConstants.REVIEW_PHOTO_LIMIT; | ||
| private final int PLACE_REVIEW_LIMIT = PaginationConstants.PLACE_REVIEW_LIMIT; |
There was a problem hiding this comment.
These fields are intended to be constants. According to Java conventions, constants should be declared as static final. While it works as an instance field in a singleton service, making them static final clarifies their nature as compile-time constants and is consistent with best practices.
| private final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; | |
| private final int REVIEW_PHOTO_LIMIT = PaginationConstants.REVIEW_PHOTO_LIMIT; | |
| private final int PLACE_REVIEW_LIMIT = PaginationConstants.PLACE_REVIEW_LIMIT; | |
| private static final int PLACE_PHOTO_LIMIT = PaginationConstants.PLACE_PHOTO_LIMIT; | |
| private static final int REVIEW_PHOTO_LIMIT = PaginationConstants.REVIEW_PHOTO_LIMIT; | |
| private static final int PLACE_REVIEW_LIMIT = PaginationConstants.PLACE_REVIEW_LIMIT; |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
No description provided.