[DDING-65] Feed 조회 API 구현 및 커서 기반 페이지네이션 적용 #206
Conversation
Walkthrough이 풀 리퀘스트는 피드(Feed) 도메인의 API 및 서비스 계층에 대한 광범위한 리팩토링을 포함합니다. 주요 변경 사항은 기존의 피드 조회 방식에서 페이지네이션 기반의 접근 방식으로의 전환에 중점을 두고 있습니다. 클럽별 피드 조회, 최신 피드 조회 등의 메서드가 커서 기반 페이지네이션을 지원하도록 수정되었으며, 응답 구조와 쿼리 메서드도 이에 맞춰 재설계되었습니다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant FeedController
participant FacadeFeedService
participant FeedRepository
Client->>FeedController: 클럽별 피드 페이지 요청
FeedController->>FacadeFeedService: getFeedPageByClub(clubId, size, cursorId)
FacadeFeedService->>FeedRepository: findPageByClubIdOrderById()
FeedRepository-->>FacadeFeedService: Slice<Feed>
FacadeFeedService-->>FeedController: ClubFeedPageQuery
FeedController-->>Client: ClubFeedPageResponse
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (21)
src/test/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepositoryTest.java (4)
31-35: 테스트 데이터 정리 로직 개선 필요테스트 격리를 위해
feedRepository만 정리하고 있지만, 연관된clubRepository도 함께 정리하는 것이 좋습니다.@BeforeEach void setUp() { + clubRepository.deleteAll(); feedRepository.deleteAll(); + clubRepository.flush(); feedRepository.flush(); }
37-98: 테스트 케이스 문서화 및 엣지 케이스 보완 필요테스트가 잘 구성되어 있지만, 다음 사항들을 보완하면 좋겠습니다:
- 테스트 데이터의 의도를 명확히 하는 주석 추가
- 페이지 크기가 전체 클럽 수보다 큰 경우에 대한 테스트 케이스 추가
- 빈 결과를 반환하는 경우에 대한 테스트 케이스 추가
예시 코드:
@Test @DisplayName("전체 클럽 수보다 큰 size를 요청하면 전체 클럽의 최신 피드만 반환한다") void findNewestPerClubPage_WhenSizeIsLargerThanTotalClubs() { // 테스트 구현 } @Test @DisplayName("피드가 없는 경우 빈 페이지를 반환한다") void findNewestPerClubPage_WhenNoFeeds() { // 테스트 구현 }
101-145: 메서드 이름 컨벤션 통일 필요테스트 메서드 이름이 한글(
페이지네이션_남은_개수가_사이즈보다_적은경우)로 작성되어 있어 다른 메서드들과 일관성이 없습니다. 영문으로 통일하는 것이 좋겠습니다.-void 페이지네이션_남은_개수가_사이즈보다_적은경우() +void findPageByClubIdOrderById_WhenRemainingItemsLessThanSize()
147-192: 커서 기반 페이지네이션 동작 설명 보완 필요테스트는 잘 구현되어 있으나, 커서 기반 페이지네이션의 동작 방식에 대한 설명이 부족합니다. 다음 사항들을 보완하면 좋겠습니다:
- 커서 ID의 의미와 동작 방식 설명
- 정렬 순서에 대한 명시적인 설명
- 페이지네이션 경계값 테스트 추가
주석 예시:
/** * 커서 기반 페이지네이션 테스트 * - cursorId: 현재 페이지의 기준이 되는 ID * - 정렬 순서: ID 기준 내림차순 * - size: 한 페이지당 반환할 피드 수 */src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedListQuery.java (1)
10-11: 필드 분리에 대한 문서화 제안CDN URL과 Origin URL을 분리한 것은 좋은 설계입니다. 각 필드의 용도와 차이점을 명확히 하기 위해 JavaDoc 주석 추가를 고려해 보시기 바랍니다.
예시:
+/** + * @param thumbnailCdnUrl CDN을 통해 제공되는 썸네일 URL + * @param thumbnailOriginUrl 원본 서버의 썸네일 URL + */src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java (2)
8-8: 메서드명 개선 제안: 반환값에 따른 복수형 사용메서드명
getFeedPageByClubId를getFeedsPageByClubId로 수정하여 다수의 피드를 반환함을 명확히 나타내는 것을 제안합니다.
10-10: 메서드명 개선 제안: 일관성 유지 및 명확성 향상메서드명
getNewestFeedPerClubPage를getNewestFeedsPerClubPage로 수정하여 반환되는 피드들이 여러 개임을 명확히 표현하고, 다른 메서드들과의 명명 규칙 일관성을 유지하는 것을 제안합니다.src/main/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepository.java (2)
12-22: [메소드 이름 수정 제안]메소드
findNewestAll()은 각 클럽별로 최신 피드 하나씩을 조회하는 쿼리를 실행하고 있습니다. 그러나 메소드 이름이 이러한 기능을 정확히 반영하지 않아 혼동을 줄 수 있습니다. 메소드 이름을findNewestFeedPerClub()등으로 변경하여 메소드의 목적을 명확히 표현하는 것을 제안합니다.
40-55: [쿼리 중복 제거 및 메소드 통합 제안]
findNewestPerClubPage메소드의 쿼리는findNewestAll()메소드의 쿼리와 유사하지만 페이징 기능이 추가되었습니다. 기능의 중복을 줄이기 위해findNewestAll()메소드를 제거하고findNewestPerClubPage메소드로 통합하는 것을 고려해보세요.src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (1)
52-61: [buildSlice 메소드의 불필요한 복사 제거 제안]
buildSlice메소드에서ArrayList를 사용하여 콘텐츠를 복사하고 있습니다. 불변 리스트를 사용하거나 서브리스트를 반환하여 불필요한 복사를 줄이는 것을 고려해보세요.src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/PagingQuery.java (1)
9-11: 정적 팩토리 메서드에 문서화 추가 필요
of메서드에 대한 JavaDoc 문서화가 누락되었습니다. 메서드의 목적과 매개변수에 대한 설명을 추가하면 코드의 가독성이 향상될 것입니다./** * PagingQuery 인스턴스를 생성합니다. * * @param currentCursorId 현재 페이지의 커서 ID * @param nextCursorId 다음 페이지의 커서 ID * @param hasNext 다음 페이지 존재 여부 * @return 새로운 PagingQuery 인스턴스 * @throws IllegalArgumentException nextCursorId가 currentCursorId보다 작은 경우 */ public static PagingQuery of(Long currentCursorId, Long nextCursorId, boolean hasNext)src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/ClubFeedPageQuery.java (1)
10-12: 정적 팩토리 메서드 문서화 및 방어적 복사 추가 필요
of메서드에 대한 문서화가 필요하며, 리스트의 불변성을 보장하기 위한 방어적 복사가 필요합니다./** * ClubFeedPageQuery 인스턴스를 생성합니다. * * @param feedListQueries 피드 목록 쿼리 리스트 * @param pagingQuery 페이징 쿼리 * @return 새로운 ClubFeedPageQuery 인스턴스 * @throws IllegalArgumentException feedListQueries가 null이거나 비어있는 경우 */ public static ClubFeedPageQuery of(List<FeedListQuery> feedListQueries, PagingQuery pagingQuery) { return new ClubFeedPageQuery(List.copyOf(feedListQueries), pagingQuery); }src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/PagingResponse.java (1)
7-12: Swagger 스키마 설명 개선 필요현재 스키마 설명이 예시 값("9", "10", "true")으로 되어 있어 명확하지 않습니다. 각 필드의 의미와 용도를 자세히 설명하는 것이 좋습니다.
public record PagingResponse( @Schema( name = "현재 커서 ID", description = "현재 페이지의 마지막 항목 ID", example = "9" ) Long currentCursorId, @Schema( name = "다음 커서 ID", description = "다음 페이지의 첫 번째 항목 ID", example = "10" ) Long nextCursorId, @Schema( name = "다음 페이지 존재 여부", description = "다음 페이지가 존재하는지 여부를 나타내는 플래그", example = "true" ) boolean hasNext )src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/ClubFeedPageResponse.java (1)
28-31: Schema 예시 URL을 더 현실적인 값으로 수정해주세요.현재 URL 예시가 포맷 문자열(
%s)을 포함하고 있어 API 문서 사용자에게 혼란을 줄 수 있습니다.다음과 같이 실제 URL 형식으로 수정하는 것을 제안합니다:
- @Schema(description = "피드 썸네일 CDN URL", example = "https://%s.s3.%s.amazonaws.com/%s/%s/%s") + @Schema(description = "피드 썸네일 CDN URL", example = "https://cdn.ddingdong.com/feeds/thumbnails/123.jpg") - @Schema(description = "피드 썸네일 S3 URL", example = "https://%s.s3.%s.amazonaws.com/%s/%s/%s") + @Schema(description = "피드 썸네일 S3 URL", example = "https://ddingdong.s3.ap-northeast-2.amazonaws.com/feeds/thumbnails/123.jpg")src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/NewestFeedPerClubPageResponse.java (1)
26-26: 불필요한 public 제어자를 제거해주세요.중첩된 record는 이미 외부 record의 일부이므로 public 제어자가 불필요합니다.
- public record NewestFeedListResponse( + record NewestFeedListResponse(src/main/java/ddingdong/ddingdongBE/domain/feed/api/FeedApi.java (1)
29-30: 페이지네이션 파라미터에 대한 설명을 추가해주세요.size와 currentCursorId 파라미터에 대한 @Schema 어노테이션이 누락되어 있습니다.
@PathVariable Long clubId, - @RequestParam(value = "size", defaultValue = "9") int size, - @RequestParam(value = "currentCursorId", defaultValue = "-1") Long currentCursorId + @RequestParam(value = "size", defaultValue = "9") + @Schema(description = "페이지당 피드 수", example = "9", minimum = "1", maximum = "100") int size, + @RequestParam(value = "currentCursorId", defaultValue = "-1") + @Schema(description = "현재 커서 ID (첫 페이지는 -1)", example = "-1") Long currentCursorIdsrc/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/FeedResponse.java (2)
33-35: 동일한 @Schema 설명 수정 필요'profileImageOriginUrl'과 'profileImageCdnUrl' 필드의 @Schema 'description'이 동일합니다. 각 필드의 의미를 정확히 전달하기 위해 'description'을 구체적으로 수정하는 것이 좋습니다.
다음과 같이 수정할 수 있습니다:
- @Schema(description = "동아리 프로필 이미지 url", example = "https://%s.s3.%s.amazonaws.com/%s/%s/%s") + @Schema(description = "동아리 프로필 이미지 Origin URL", example = "https://example.com/origin/image.jpg") String profileImageOriginUrl, - @Schema(description = "동아리 프로필 이미지 url", example = "https://%s.s3.%s.amazonaws.com/%s/%s/%s") + @Schema(description = "동아리 프로필 이미지 CDN URL", example = "https://cdn.example.com/image.jpg") String profileImageCdnUrl,
53-56: @Schema 'example' 필드에 구체적인 URL 제공 권장'originUrl'과 'cdnUrl' 필드의 @Schema 'example' 값이 'url'로 설정되어 있습니다. 더 구체적인 예시 URL을 제공하면 API 문서 사용자가 이해하기 쉽습니다.
예를 들어, 다음과 같이 수정할 수 있습니다:
- @Schema(description = "원본 url", example = "url") + @Schema(description = "원본 url", example = "https://example.com/origin/file.jpg") String originUrl, - @Schema(description = "cdn url", example = "url") + @Schema(description = "cdn url", example = "https://cdn.example.com/file.jpg") String cdnUrl,src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedService.java (1)
87-87: 에러 메시지의 괄호 불균형 수정 필요'ResourceNotFound' 예외의 에러 메시지에 괄호가 올바르게 짝지어져 있지 않습니다. 메시지를 수정하여 가독성을 높이는 것이 좋습니다.
다음과 같이 수정할 수 있습니다:
-.orElseThrow(() -> new ResourceNotFound("해당 FileMetaData(feedId: " + id + ")를 찾을 수 없습니다.)")); +.orElseThrow(() -> new ResourceNotFound("해당 FileMetaData(feedId: " + id + ")를 찾을 수 없습니다."));src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedFileUrlQuery.java (1)
9-11: 정적 팩토리 메서드 사용이 적절합니다만, 문서화가 필요합니다.메서드의 용도와 매개변수에 대한 설명을 JavaDoc으로 추가하면 좋을 것 같습니다.
+ /** + * FeedFileUrlQuery 인스턴스를 생성합니다. + * + * @param id 파일의 고유 식별자 + * @param originUrl 원본 파일 URL + * @param cdnUrl CDN 파일 URL + * @return 새로운 FeedFileUrlQuery 인스턴스 + */ public static FeedFileUrlQuery of(String id, String originUrl, String cdnUrl) {src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedQuery.java (1)
12-15: 필드 순서 개선이 필요합니다.연관된 필드들을 그룹화하여 배치하면 가독성이 향상될 것 같습니다.
public record FeedQuery( Long id, + ClubProfileQuery clubProfileQuery, String activityContent, + FeedFileUrlQuery feedFileUrlQuery, String feedType, - LocalDate createdDate, - FeedFileUrlQuery feedFileUrlQuery, - ClubProfileQuery clubProfileQuery + LocalDate createdDate ) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
src/main/java/ddingdong/ddingdongBE/domain/feed/api/FeedApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/controller/FeedController.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/ClubFeedPageResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/FeedListResponse.java(0 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/FeedResponse.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/NewestFeedListResponse.java(0 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/NewestFeedPerClubPageResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/PagingResponse.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepository.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedService.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/ClubFeedPageQuery.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/ClubProfileQuery.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedFileUrlQuery.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedListQuery.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedQuery.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/NewestFeedPerClubPageQuery.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/PagingQuery.java(1 hunks)src/test/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepositoryTest.java(3 hunks)src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java(0 hunks)src/test/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedServiceTest.java(2 hunks)
💤 Files with no reviewable changes (3)
- src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/FeedListResponse.java
- src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/NewestFeedListResponse.java
- src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (11)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedListQuery.java (2)
4-4: 적절한 import 구문 추가 확인!
UploadedVideoUrlQuery의 import 구문이 적절하게 추가되었습니다.
15-21: 팩토리 메서드 개선 확인 및 제안메서드 이름을 'of'로 변경하고 URL 처리를 분리한 것은 좋은 개선입니다. 다만,
urlQuery의 null 체크를 고려해보시면 좋겠습니다.다음과 같은 방어적 코드 추가를 검토해 보시기 바랍니다:
public static FeedListQuery of(Feed feed, UploadedVideoUrlQuery urlQuery) { + if (feed == null) { + throw new IllegalArgumentException("feed must not be null"); + } + if (urlQuery == null) { + throw new IllegalArgumentException("urlQuery must not be null"); + } return FeedListQuery.builder()src/main/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepository.java (1)
25-38: [쿼리 성능 및 인덱스 검토 필요]
findPageByClubIdOrderById메소드의 쿼리에서id와club_id컬럼에 대한 조건이 사용되고 있습니다. 해당 컬럼들에 인덱스가 설정되어 있는지 확인하여 쿼리 속도를 향상시킬 수 있습니다.인덱스 설정 여부를 확인하기 위해 다음 스크립트를 실행해보세요:
(데이터베이스 정보는 실제 환경에 맞게 대체하여 사용하세요.)
✅ Verification successful
[인덱스 설정 상태 확인 완료]
현재
feed테이블의 인덱스 상태:
id컬럼은 PRIMARY KEY로 자동 인덱싱되어 있습니다.club_id컬럼은 외래 키 제약조건으로 인해 자동으로 인덱스가 생성되어 있습니다.현재 설정으로도 쿼리 성능이 적절할 것으로 예상되나, 실제 운영 환경에서 성능 이슈가 발생하는 경우 추가 인덱스 설정을 고려해보시기 바랍니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 설명: 'feed' 테이블의 'id'와 'club_id' 컬럼에 인덱스가 있는지 확인합니다. # MySQL 사용 시: echo "SHOW INDEX FROM feed WHERE Column_name='id' OR Column_name='club_id';" | mysql -u [username] -p[password] [database_name]Length of output: 243
Script:
#!/bin/bash # Find Feed entity definition echo "=== Feed Entity Definition ===" ast-grep --pattern 'class Feed {$$$}' echo -e "\n=== Schema Files ===" fd -e sql echo -e "\n=== Index Annotations ===" rg -i "@index|@table.*indexes" -A 5Length of output: 3133
Script:
#!/bin/bash echo "=== Feed Table Creation ===" cat src/main/resources/db/migration/V4__Create_feed.sql echo -e "\n=== Key Convention Changes ===" cat src/main/resources/db/migration/V8__apply_key_name_convention.sqlLength of output: 2943
src/main/java/ddingdong/ddingdongBE/domain/feed/controller/FeedController.java (2)
21-27: [메소드 및 DTO 명명 규칙 적절성 확인]
getFeedPageByClub메소드는 클럽의 피드를 페이징하여 반환하며, DTO와 메소드 명명이 일관되고 이해하기 쉽게 구성되었습니다.
31-36: [메소드 및 DTO 명명 규칙 적절성 확인]
getNewestFeedPerClub메소드는 각 클럽의 최신 피드를 페이징하여 반환하며, DTO와 메소드 명명이 적절하게 사용되었습니다.src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (2)
23-25: [페이징 로직 구현 정확성 확인]
getFeedPageByClubId메소드에서 페이징을 위해size + 1개의 데이터를 가져와hasNext여부를 판단하는 로직이 올바르게 구현되었습니다.
28-30: [페이징 로직 구현 정확성 확인]
getNewestFeedPerClubPage메소드에서도 동일한 페이징 로직이 적용되어 일관성이 유지되었습니다.src/main/java/ddingdong/ddingdongBE/domain/feed/controller/dto/response/FeedResponse.java (1)
68-76: 변경 사항 검토 결과 문제 없음'FeedResponse.from' 메서드의 변경 사항이 적절하며, 새로운 필드들이 올바르게 매핑되었습니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/ClubProfileQuery.java (1)
9-10: 변경 사항 검토 결과 문제 없음'profileImageOriginUrl'과 'profileImageCdnUrl' 필드 추가가 적절하며, 필요한 정보가 잘 반영되었습니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedFileUrlQuery.java (1)
3-7: 레코드 구조가 명확하고 적절합니다!파일 URL 관련 정보를 독립적인 DTO로 분리한 것이 좋은 설계 결정입니다. 불변성이 보장되어 안전한 데이터 전달이 가능합니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/dto/query/FeedQuery.java (1)
17-25: 정적 팩토리 메서드 구현이 깔끔합니다!Feed 엔티티로부터 DTO를 생성하는 로직이 명확하고, 새로 추가된 FeedFileUrlQuery의 통합이 잘 되어있습니다. Builder 패턴의 사용도 적절합니다.
| public record PagingQuery( | ||
| Long currentCursorId, | ||
| Long nextCursorId, | ||
| boolean hasNext | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
커서 기반 페이지네이션 필드에 대한 유효성 검증 추가 필요
현재 구현에서는 currentCursorId와 nextCursorId에 대한 유효성 검증이 없습니다. 음수 값이나 잘못된 순서의 커서 ID가 입력될 수 있습니다.
다음과 같은 검증 로직을 추가하는 것을 고려해보세요:
public record PagingQuery(
@NotNull(message = "현재 커서 ID는 null일 수 없습니다")
@PositiveOrZero(message = "현재 커서 ID는 0 이상이어야 합니다")
Long currentCursorId,
@NotNull(message = "다음 커서 ID는 null일 수 없습니다")
@PositiveOrZero(message = "다음 커서 ID는 0 이상이어야 합니다")
Long nextCursorId,
boolean hasNext
) {
public PagingQuery {
if (nextCursorId < currentCursorId) {
throw new IllegalArgumentException("다음 커서 ID는 현재 커서 ID보다 작을 수 없습니다");
}
}
}| public record ClubFeedPageQuery( | ||
| List<FeedListQuery> feedListQueries, | ||
| PagingQuery pagingQuery | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
클럽 피드 페이지 쿼리 필드에 대한 유효성 검증 추가 필요
feedListQueries가 null이거나 비어있는 경우에 대한 처리가 누락되었습니다.
public record ClubFeedPageQuery(
@NotNull(message = "피드 목록 쿼리는 null일 수 없습니다")
@NotEmpty(message = "피드 목록 쿼리는 비어있을 수 없습니다")
List<FeedListQuery> feedListQueries,
@NotNull(message = "페이징 쿼리는 null일 수 없습니다")
PagingQuery pagingQuery
) {
public ClubFeedPageQuery {
feedListQueries = List.copyOf(feedListQueries); // 불변성 보장
}
}| public record NewestFeedPerClubPageQuery( | ||
| List<FeedListQuery> feedListQueries, | ||
| PagingQuery pagingQuery | ||
| ) { | ||
|
|
||
| public static NewestFeedPerClubPageQuery of(List<FeedListQuery> feedListQueries, PagingQuery pagingQuery) { | ||
| return new NewestFeedPerClubPageQuery(feedListQueries, pagingQuery); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
코드 중복 제거를 위한 상위 클래스 도입 고려
ClubFeedPageQuery와 동일한 구조를 가지고 있어 코드 중복이 발생했습니다. 공통 동작을 추상화하여 코드 재사용성을 높일 수 있습니다.
// 공통 상위 클래스 도입
public abstract record BasePageQuery(
List<FeedListQuery> feedListQueries,
PagingQuery pagingQuery
) {
protected BasePageQuery {
feedListQueries = List.copyOf(feedListQueries);
}
}
// 구체적인 구현
public record NewestFeedPerClubPageQuery(
List<FeedListQuery> feedListQueries,
PagingQuery pagingQuery
) extends BasePageQuery(feedListQueries, pagingQuery) {
public static NewestFeedPerClubPageQuery of(List<FeedListQuery> feedListQueries, PagingQuery pagingQuery) {
return new NewestFeedPerClubPageQuery(feedListQueries, pagingQuery);
}
}| public static PagingResponse from(PagingQuery pagingQuery) { | ||
| return new PagingResponse(pagingQuery.currentCursorId(), pagingQuery.nextCursorId(), pagingQuery.hasNext()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
from 메서드에 null 체크 추가 필요
pagingQuery가 null인 경우에 대한 처리가 누락되었습니다.
public static PagingResponse from(PagingQuery pagingQuery) {
Objects.requireNonNull(pagingQuery, "PagingQuery must not be null");
return new PagingResponse(
pagingQuery.currentCursorId(),
pagingQuery.nextCursorId(),
pagingQuery.hasNext()
);
}| @DisplayName("입력한 파라미터의 정보에 따라 해당하는 페이지의 정보를 반환한다.") | ||
| @Test | ||
| void getFeedPageByClubId() { | ||
| // given | ||
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | ||
| .set("name", "카우") | ||
| .set("user", null) | ||
| .set("score", Score.from(BigDecimal.ZERO)) | ||
| .set("clubMembers", null) | ||
| .sample(); | ||
| Club savedClub = clubRepository.save(club); | ||
|
|
||
| Feed feed1 = fixtureMonkey.giveMeBuilder(Feed.class) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용1") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| Feed feed2 = fixtureMonkey.giveMeBuilder(Feed.class) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용2") | ||
| .set("feedType", FeedType.VIDEO) | ||
| .sample(); | ||
| Feed feed3 = fixtureMonkey.giveMeBuilder(Feed.class) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용3") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| Feed feed4 = fixtureMonkey.giveMeBuilder(Feed.class) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용4") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| feedRepository.saveAll(List.of(feed1, feed2, feed3, feed4)); | ||
|
|
||
| Long clubId = savedClub.getId(); | ||
| int size = 2; | ||
| Long cursorId = -1L; | ||
| // when | ||
| Slice<Feed> page = feedService.getFeedPageByClubId(clubId, size, cursorId); | ||
| // then | ||
|
|
||
| assertThat(page.getContent().size()).isEqualTo(2); | ||
| assertThat(page.getNumber()).isEqualTo(0); | ||
| assertThat(page.getContent().get(page.getContent().size() - 1).getId()).isEqualTo(3); | ||
| assertThat(page.hasNext()).isTrue(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
페이지네이션 테스트 케이스를 보완해주세요.
현재 테스트는 기본적인 케이스만 다루고 있습니다. 다음과 같은 엣지 케이스들도 검증이 필요합니다:
- 빈 결과
- 마지막 페이지
- 잘못된 커서 ID
- 최대 size 초과
또한 매직 넘버를 상수로 추출하여 테스트의 가독성을 높이는 것을 추천드립니다:
+ private static final int PAGE_SIZE = 2;
+ private static final long INITIAL_CURSOR = -1L;
+ private static final int EXPECTED_CONTENT_SIZE = 2;
+ private static final int EXPECTED_PAGE_NUMBER = 0;
+ private static final long EXPECTED_LAST_FEED_ID = 3;
@Test
void getFeedPageByClubId() {
...
- int size = 2;
- Long cursorId = -1L;
+ int size = PAGE_SIZE;
+ Long cursorId = INITIAL_CURSOR;
...
- assertThat(page.getContent().size()).isEqualTo(2);
- assertThat(page.getNumber()).isEqualTo(0);
- assertThat(page.getContent().get(page.getContent().size() - 1).getId()).isEqualTo(3);
+ assertThat(page.getContent().size()).isEqualTo(EXPECTED_CONTENT_SIZE);
+ assertThat(page.getNumber()).isEqualTo(EXPECTED_PAGE_NUMBER);
+ assertThat(page.getContent().get(page.getContent().size() - 1).getId()).isEqualTo(EXPECTED_LAST_FEED_ID);📝 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.
| @DisplayName("입력한 파라미터의 정보에 따라 해당하는 페이지의 정보를 반환한다.") | |
| @Test | |
| void getFeedPageByClubId() { | |
| // given | |
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | |
| .set("name", "카우") | |
| .set("user", null) | |
| .set("score", Score.from(BigDecimal.ZERO)) | |
| .set("clubMembers", null) | |
| .sample(); | |
| Club savedClub = clubRepository.save(club); | |
| Feed feed1 = fixtureMonkey.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용1") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed2 = fixtureMonkey.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용2") | |
| .set("feedType", FeedType.VIDEO) | |
| .sample(); | |
| Feed feed3 = fixtureMonkey.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용3") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed4 = fixtureMonkey.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용4") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| feedRepository.saveAll(List.of(feed1, feed2, feed3, feed4)); | |
| Long clubId = savedClub.getId(); | |
| int size = 2; | |
| Long cursorId = -1L; | |
| // when | |
| Slice<Feed> page = feedService.getFeedPageByClubId(clubId, size, cursorId); | |
| // then | |
| assertThat(page.getContent().size()).isEqualTo(2); | |
| assertThat(page.getNumber()).isEqualTo(0); | |
| assertThat(page.getContent().get(page.getContent().size() - 1).getId()).isEqualTo(3); | |
| assertThat(page.hasNext()).isTrue(); | |
| } | |
| private static final int PAGE_SIZE = 2; | |
| private static final long INITIAL_CURSOR = -1L; | |
| private static final int EXPECTED_CONTENT_SIZE = 2; | |
| private static final int EXPECTED_PAGE_NUMBER = 0; | |
| private static final long EXPECTED_LAST_FEED_ID = 3; | |
| @DisplayName("입력한 파라미터의 정보에 따라 해당하는 페이지의 정보를 반환한다.") | |
| @Test | |
| void getFeedPageByClubId() { | |
| // given | |
| Club club = fixtureMonkey.giveMeBuilder(Club.class) | |
| .set("name", "카우") | |
| .set("user", null) | |
| .set("score", Score.from(BigDecimal.ZERO)) | |
| .set("clubMembers", null) | |
| .sample(); | |
| Club savedClub = clubRepository.save(club); | |
| Feed feed1 = fixtureMonkey.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용1") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed2 = fixtureMonkey.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용2") | |
| .set("feedType", FeedType.VIDEO) | |
| .sample(); | |
| Feed feed3 = fixtureMonkey.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용3") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed4 = fixtureMonkey.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용4") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| feedRepository.saveAll(List.of(feed1, feed2, feed3, feed4)); | |
| Long clubId = savedClub.getId(); | |
| int size = PAGE_SIZE; | |
| Long cursorId = INITIAL_CURSOR; | |
| // when | |
| Slice<Feed> page = feedService.getFeedPageByClubId(clubId, size, cursorId); | |
| // then | |
| assertThat(page.getContent().size()).isEqualTo(EXPECTED_CONTENT_SIZE); | |
| assertThat(page.getNumber()).isEqualTo(EXPECTED_PAGE_NUMBER); | |
| assertThat(page.getContent().get(page.getContent().size() - 1).getId()).isEqualTo(EXPECTED_LAST_FEED_ID); | |
| assertThat(page.hasNext()).isTrue(); | |
| } |
| List<Feed> feeds = feedPage.getContent(); | ||
|
|
||
| List<FeedListQuery> feedListQueries = buildFeedListQuery(feeds); | ||
| PagingQuery pagingQuery = PagingQuery.of(currentCursorId, feeds.get(feeds.size() -1).getId(), feedPage.hasNext()); |
There was a problem hiding this comment.
빈 결과에 대한 예외 처리 필요
'feeds' 리스트가 비어 있는 경우, 'feeds.get(feeds.size() -1).getId()' 호출 시 IndexOutOfBoundsException이 발생할 수 있습니다. 빈 결과에 대한 처리가 필요합니다.
다음과 같이 수정할 수 있습니다:
+ if (!feeds.isEmpty()) {
PagingQuery pagingQuery = PagingQuery.of(currentCursorId, feeds.get(feeds.size() -1).getId(), feedPage.hasNext());
return NewestFeedPerClubPageQuery.of(feedListQueries, pagingQuery);
+ } else {
+ // 빈 결과에 대한 처리 로직 추가
+ }Committable suggestion skipped: line range outside the PR's diff.
| List<Feed> feeds = feedPage.getContent(); | ||
|
|
||
| List<FeedListQuery> feedListQueries = buildFeedListQuery(feeds); | ||
| PagingQuery pagingQuery = PagingQuery.of(currentCursorId, feeds.get(feeds.size() -1).getId(), feedPage.hasNext()); |
There was a problem hiding this comment.
빈 결과에 대한 예외 처리 필요
'feeds' 리스트가 비어 있는 경우, 'feeds.get(feeds.size() -1).getId()' 호출 시 IndexOutOfBoundsException이 발생할 수 있습니다. 빈 결과에 대한 처리가 필요합니다.
다음과 같이 수정할 수 있습니다:
+ if (!feeds.isEmpty()) {
PagingQuery pagingQuery = PagingQuery.of(currentCursorId, feeds.get(feeds.size() -1).getId(), feedPage.hasNext());
return ClubFeedPageQuery.of(feedListQueries, pagingQuery);
+ } else {
+ // 빈 결과에 대한 처리 로직 추가
+ }Committable suggestion skipped: line range outside the PR's diff.
| FileMetaData fileMetaData = getFileMetaData(feed.getFeedType().getDomainType(), feed.getId()); | ||
| UploadedVideoUrlQuery urlQuery = s3FileService.getUploadedVideoUrl(fileMetaData.getFileKey()); | ||
| return FeedListQuery.of(feed, urlQuery); |
There was a problem hiding this comment.
Feed 타입에 따른 URL 처리 로직 개선 필요
'buildFeedListQuery' 메서드에서 모든 피드에 대해 'getUploadedVideoUrl'을 호출하고 있습니다. 피드 타입이 이미지인 경우에도 영상 URL을 가져오는 것은 부적절합니다. 피드 타입에 따라 적절한 URL을 가져오도록 로직을 수정해야 합니다.
다음과 같이 수정할 수 있습니다:
FileMetaData fileMetaData = getFileMetaData(feed.getFeedType().getDomainType(), feed.getId());
+ if (feed.isImage()) {
+ UploadedFileUrlQuery urlQuery = s3FileService.getUploadedFileUrl(fileMetaData.getFileKey());
+ return FeedListQuery.of(feed, urlQuery);
+ } else if (feed.isVideo()) {
UploadedVideoUrlQuery urlQuery = s3FileService.getUploadedVideoUrl(fileMetaData.getFileKey());
return FeedListQuery.of(feed, urlQuery);
+ } else {
+ throw new IllegalArgumentException("지원하지 않는 FeedType입니다.");
+ }Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepository.java (1)
11-24: 쿼리 최적화 및 가독성 개선이 필요합니다.다음 사항들을 개선하면 좋을 것 같습니다:
-1과 같은 매직 넘버는 상수로 정의하는 것이 좋습니다.- 인덱스 힌트를 추가하여 쿼리 성능을 개선할 수 있습니다.
다음과 같이 수정을 제안드립니다:
@Query(value = """ select /*+ INDEX(f feed_id_idx) */ * from feed f - where (:currentCursorId = -1 or id < :currentCursorId) + where (:currentCursorId = :INITIAL_CURSOR_ID or id < :currentCursorId) and f.club_id = :clubId and deleted_at is null order by f.id DESC limit :size """, nativeQuery = true) Slice<Feed> findPageByClubIdOrderById( @Param("clubId") Long clubId, @Param("size") int size, - @Param("currentCursorId") Long currentCursorId + @Param("currentCursorId") Long currentCursorId, + @Param("INITIAL_CURSOR_ID") Long INITIAL_CURSOR_ID );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepository.java(1 hunks)
🔇 Additional comments (1)
src/main/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepository.java (1)
4-4: Slice 임포트 추가가 적절합니다!커서 기반 페이지네이션을 위한 Slice 사용이 올바르게 적용되었습니다.
| @Query(value = """ | ||
| select * from feed f | ||
| where f.id in | ||
| (select max(id) | ||
| from feed | ||
| where deleted_at is null | ||
| GROUP BY club_id) | ||
| and (:currentCursorId = -1 or id < :currentCursorId) | ||
| ORDER BY id DESC | ||
| limit :size | ||
| """, | ||
| nativeQuery = true) | ||
| Slice<Feed> findNewestPerClubPage( | ||
| @Param("size") int size, | ||
| @Param("currentCursorId") Long currentCursorId | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
성능 최적화가 필요한 쿼리입니다.
현재 구현은 다음과 같은 개선이 필요합니다:
- 서브쿼리 대신 Window 함수를 사용하면 성능이 개선될 수 있습니다.
- 이전 쿼리와 동일하게 매직 넘버 처리가 필요합니다.
다음과 같은 쿼리 개선을 제안드립니다:
@Query(value = """
- select * from feed f
- where f.id in
- (select max(id)
- from feed
- where deleted_at is null
- GROUP BY club_id)
- and (:currentCursorId = -1 or id < :currentCursorId)
- ORDER BY id DESC
- limit :size
+ WITH RankedFeeds AS (
+ SELECT f.*,
+ ROW_NUMBER() OVER (PARTITION BY club_id ORDER BY id DESC) as rn
+ FROM feed f
+ WHERE deleted_at is null
+ AND (:currentCursorId = :INITIAL_CURSOR_ID or id < :currentCursorId)
+ )
+ SELECT * FROM RankedFeeds
+ WHERE rn = 1
+ ORDER BY id DESC
+ LIMIT :size
""",
nativeQuery = true)
Slice<Feed> findNewestPerClubPage(
@Param("size") int size,
- @Param("currentCursorId") Long currentCursorId
+ @Param("currentCursorId") Long currentCursorId,
+ @Param("INITIAL_CURSOR_ID") Long INITIAL_CURSOR_ID
);이렇게 수정하면 다음과 같은 이점이 있습니다:
- Window 함수를 사용하여 쿼리 성능 개선
- 매직 넘버 제거
- 더 명확한 쿼리 의도 전달
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java (1)
Line range hint
119-127: 응답 검증이 불충분합니다.현재 기본적인 필드만 검증하고 있습니다. 파일 URL 관련 필드에 대한 검증이 필요합니다.
다음과 같은 검증을 추가해주세요:
assertThat(info.activityContent()).isEqualTo(savedFeed.getActivityContent()); assertThat(info.feedType()).isEqualTo(savedFeed.getFeedType().toString()); assertThat(info.createdDate()).isEqualTo(LocalDate.from(now)); +assertThat(info.clubProfileQuery().profileImageOriginUrl()).isEqualTo("original-url"); +assertThat(info.clubProfileQuery().profileImageCdnUrl()).isEqualTo("cdn-url"); +assertThat(info.imageUrls()).isNotEmpty(); +assertThat(info.imageUrls().get(0).originUrl()).isEqualTo("original-url"); +assertThat(info.imageUrls().get(0).cdnUrl()).isEqualTo("cdn-url");
🧹 Nitpick comments (6)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java (2)
11-11: 최신 피드 조회 메서드 개선 제안
getNewestFeedPerClubPage메서드가 클럽별 최신 피드를 조회하는 것으로 보입니다. 성능 최적화를 위해 다음 사항들을 고려해보시면 좋을 것 같습니다:
- 캐싱 전략 도입:
@Cacheable(value = "newestFeeds", key = "#currentCursorId") Slice<Feed> getNewestFeedPerClubPage(int size, Long currentCursorId);
- 결과 정렬 기준 명시:
Slice<Feed> getNewestFeedPerClubPage(int size, Long currentCursorId, Sort sort);
9-11: 페이지네이션 관련 문서화 제안두 페이지네이션 메서드에 대한 자세한 동작 방식과 사용법을 문서화하면 좋을 것 같습니다.
다음과 같은 Javadoc 추가를 제안드립니다:
/** * 클럽별 피드를 커서 기반 페이지네이션으로 조회합니다. * @param clubId 조회할 클럽의 ID * @param size 페이지당 피드 수 * @param currentCursorId 현재 커서 ID (첫 페이지 조회 시 null) * @return 피드 목록과 다음 페이지 존재 여부 */ Slice<Feed> getFeedPageByClubId(Long clubId, int size, Long currentCursorId); /** * 클럽별 최신 피드를 커서 기반 페이지네이션으로 조회합니다. * @param size 페이지당 피드 수 * @param currentCursorId 현재 커서 ID (첫 페이지 조회 시 null) * @return 최신 피드 목록과 다음 페이지 존재 여부 */ Slice<Feed> getNewestFeedPerClubPage(int size, Long currentCursorId);src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java (1)
77-87: 파일 상태에 따른 테스트 케이스 추가가 필요합니다.현재
FileStatus.COUPLED상태만 테스트하고 있습니다. 다양한 파일 상태에 대한 테스트가 필요합니다.다음과 같은 테스트 케이스 추가를 제안드립니다:
FileStatus.PENDING상태의 파일FileStatus.DELETED상태의 파일- FileMetaData가 없는 경우
Also applies to: 99-109
src/test/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepositoryTest.java (2)
39-101: 페이지네이션 테스트 케이스를 보완해주세요.현재 구현된 테스트는 기본적인 시나리오만 검증하고 있습니다. 다음 사항들을 추가로 검증하면 좋을 것 같습니다:
Slice.hasNext()를 통한 다음 페이지 존재 여부 검증- 다양한 커서 값에 대한 엣지 케이스 테스트
예시 코드:
// then List<Feed> feeds = newestFeeds.getContent(); assertThat(feeds.size()).isEqualTo(2); assertThat(feeds.get(0).getId()).isEqualTo(6); assertThat(feeds.get(1).getId()).isEqualTo(4); +assertThat(newestFeeds.hasNext()).isTrue();
103-151: 테스트 메소드 이름을 영어로 통일해주세요.다른 테스트 메소드들은 영어로 작성되어 있는데, 이 메소드만 한글로 작성되어 있어 일관성이 떨어집니다.
-void 페이지네이션_남은_개수가_사이즈보다_적은경우() { +void findPageWhenRemainingItemsLessThanSize() {src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (1)
24-31: 페이지네이션 메서드 구현이 깔끔하나 문서화 필요메서드 구현이 명확하고 단일 책임 원칙을 잘 따르고 있습니다. 다만, 다음 사항들에 대한 JavaDoc 문서화를 추가하면 좋을 것 같습니다:
- 파라미터 size의 최대/최소 제한값
- currentCursorId가 null일 때의 동작
- 반환되는 Slice 객체의 구조와 사용 방법
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java(3 hunks)src/test/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepositoryTest.java(3 hunks)src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java(3 hunks)
🔇 Additional comments (6)
src/main/java/ddingdong/ddingdongBE/domain/feed/service/FeedService.java (2)
4-4: Spring Slice 사용이 적절합니다!커서 기반 페이지네이션을 위해
Page대신Slice를 사용한 것이 효율적입니다.Slice는 전체 카운트를 계산하지 않아 성능상 이점이 있습니다.
9-9: 메서드 시그니처가 명확합니다.
getFeedPageByClubId메서드의 이름과 매개변수가 기능을 잘 설명하고 있습니다. 다만, 페이지네이션 관련 매개변수에 대한 제약조건 검증이 필요할 수 있습니다.다음 사항들을 확인해 주시기 바랍니다:
- size 파라미터의 최소/최대값 제한
- currentCursorId가 null일 때의 처리
src/test/java/ddingdong/ddingdongBE/domain/feed/service/FacadeFeedServiceTest.java (1)
16-22: 파일 저장소 서비스 변경이 적절히 반영되었습니다.로컬 파일 처리에서 S3 기반 저장소로의 전환이 잘 이루어졌습니다. 관련 의존성과 저장소가 적절히 추가되었습니다.
Also applies to: 48-52
src/test/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepositoryTest.java (1)
31-37: 테스트 환경 설정이 적절합니다!각 테스트 실행 전에 데이터베이스를 정리하는 방식이 외래 키 제약 조건을 고려하여 올바르게 구현되었습니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (2)
6-6: 페이지네이션 구현을 위한 적절한 임포트 추가커서 기반 페이지네이션 구현에 필요한 적절한 클래스들이 임포트되었습니다.
Also applies to: 10-12
25-25: Repository 메서드 구현 확인 필요다음 메서드들의 구현을 확인해 주시기 바랍니다:
- findPageByClubIdOrderById
- findNewestPerClubPage
특히 다음 사항들을 확인해 주세요:
- 인덱스 사용 여부
- 커서 기반 쿼리의 성능
- ORDER BY 절의 적절성
Also applies to: 30-30
✅ Verification successful
Repository 구현이 적절함을 확인했습니다
- 커서 기반 페이지네이션이 올바르게 구현되어 있습니다
- id 기반 인덱싱이 적절히 사용되고 있습니다
- 테스트 케이스를 통해 정확성이 검증되었습니다
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Repository 구현 확인 ast-grep --pattern 'interface FeedRepository { $$$ Slice<Feed> findPageByClubIdOrderById($$$); Slice<Feed> findNewestPerClubPage($$$); $$$ }'Length of output: 151
Script:
#!/bin/bash # 1. Find repository files fd -e java "FeedRepository" # 2. If found, examine the content fd -e java "FeedRepository" -x cat {} # 3. Search for query method implementations rg -A 5 "findPageByClubIdOrderById|findNewestPerClubPage" --type javaLength of output: 15815
| @BeforeEach | ||
| void setUp() { | ||
| feedRepository.deleteAll(); | ||
| feedRepository.flush(); | ||
| clubRepository.deleteAll(); | ||
| clubRepository.flush(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
테스트 격리를 위한 데이터 정리가 필요합니다.
현재 Feed와 Club 데이터만 정리하고 있습니다. FileMetaData도 테스트 간 격리가 필요합니다.
다음과 같이 FileMetaData 정리 로직을 추가해주세요:
@BeforeEach
void setUp() {
feedRepository.deleteAll();
feedRepository.flush();
clubRepository.deleteAll();
clubRepository.flush();
+ fileMetaDataRepository.deleteAll();
+ fileMetaDataRepository.flush();
}📝 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.
| @BeforeEach | |
| void setUp() { | |
| feedRepository.deleteAll(); | |
| feedRepository.flush(); | |
| clubRepository.deleteAll(); | |
| clubRepository.flush(); | |
| @BeforeEach | |
| void setUp() { | |
| feedRepository.deleteAll(); | |
| feedRepository.flush(); | |
| clubRepository.deleteAll(); | |
| clubRepository.flush(); | |
| fileMetaDataRepository.deleteAll(); | |
| fileMetaDataRepository.flush(); |
| BDDMockito.given(s3FileService.getUploadedFileUrl(any())) | ||
| .willReturn(new UploadedFileUrlQuery(null, null, null)); | ||
| BDDMockito.given(s3FileService.getUploadedVideoUrl(any())) | ||
| .willReturn(new UploadedVideoUrlQuery(null, null, null,null)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
S3 URL 응답에 대한 구체적인 테스트가 필요합니다.
현재 모든 URL 응답이 null로 설정되어 있어 실제 동작을 검증하기 어렵습니다.
다음과 같이 실제 URL을 포함한 응답으로 수정해주세요:
-BDDMockito.given(s3FileService.getUploadedFileUrl(any()))
- .willReturn(new UploadedFileUrlQuery(null, null, null));
-BDDMockito.given(s3FileService.getUploadedVideoUrl(any()))
- .willReturn(new UploadedVideoUrlQuery(null, null, null, null));
+BDDMockito.given(s3FileService.getUploadedFileUrl(any()))
+ .willReturn(new UploadedFileUrlQuery(
+ "original-url",
+ "cdn-url",
+ "thumbnail-url"
+ ));
+BDDMockito.given(s3FileService.getUploadedVideoUrl(any()))
+ .willReturn(new UploadedVideoUrlQuery(
+ "video-url",
+ "cdn-url",
+ "thumbnail-url",
+ "preview-url"
+ ));📝 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.
| BDDMockito.given(s3FileService.getUploadedFileUrl(any())) | |
| .willReturn(new UploadedFileUrlQuery(null, null, null)); | |
| BDDMockito.given(s3FileService.getUploadedVideoUrl(any())) | |
| .willReturn(new UploadedVideoUrlQuery(null, null, null,null)); | |
| BDDMockito.given(s3FileService.getUploadedFileUrl(any())) | |
| .willReturn(new UploadedFileUrlQuery( | |
| "original-url", | |
| "cdn-url", | |
| "thumbnail-url" | |
| )); | |
| BDDMockito.given(s3FileService.getUploadedVideoUrl(any())) | |
| .willReturn(new UploadedVideoUrlQuery( | |
| "video-url", | |
| "cdn-url", | |
| "thumbnail-url", | |
| "preview-url" | |
| )); |
| .set("id", 1L) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용1") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| Feed feed2 = fixture.giveMeBuilder(Feed.class) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용2") | ||
| .set("feedType", FeedType.VIDEO) | ||
| .sample(); | ||
| .set("id", 2L) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용2") | ||
| .set("feedType", FeedType.VIDEO) | ||
| .sample(); | ||
| Feed feed3 = fixture.giveMeBuilder(Feed.class) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용3") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| feedRepository.save(feed1); | ||
| feedRepository.save(feed2); | ||
| feedRepository.save(feed3); | ||
| .set("id", 3L) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용3") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| Feed feed4 = fixture.giveMeBuilder(Feed.class) | ||
| .set("id", 4L) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용4") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); |
There was a problem hiding this comment.
ID를 수동으로 설정하는 방식을 개선해주세요.
테스트 데이터의 ID를 수동으로 설정하면 데이터베이스의 ID 생성 전략과 충돌이 발생할 수 있습니다. ID 설정을 제거하고 데이터베이스가 생성한 ID를 사용하는 것이 좋습니다.
Feed feed1 = fixture.giveMeBuilder(Feed.class)
- .set("id", 1L)
.set("club", savedClub)
.set("activityContent", "내용1")
.set("feedType", FeedType.IMAGE)
.sample();📝 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.
| .set("id", 1L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용1") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed2 = fixture.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용2") | |
| .set("feedType", FeedType.VIDEO) | |
| .sample(); | |
| .set("id", 2L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용2") | |
| .set("feedType", FeedType.VIDEO) | |
| .sample(); | |
| Feed feed3 = fixture.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용3") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| feedRepository.save(feed1); | |
| feedRepository.save(feed2); | |
| feedRepository.save(feed3); | |
| .set("id", 3L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용3") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed4 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 4L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용4") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| .set("club", savedClub) | |
| .set("activityContent", "내용1") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed2 = fixture.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용2") | |
| .set("feedType", FeedType.VIDEO) | |
| .sample(); | |
| Feed feed3 = fixture.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용3") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed4 = fixture.giveMeBuilder(Feed.class) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용4") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); |
| @DisplayName("cursorId보다 작은 Feed를 size 개수만큼 페이지로 반환한다.") | ||
| @Test | ||
| void findPageByClubIdOrderById() { | ||
| // given | ||
| Club club = fixture.giveMeBuilder(Club.class) | ||
| .set("name", "카우") | ||
| .set("user", null) | ||
| .set("score", Score.from(BigDecimal.ZERO)) | ||
| .set("clubMembers", null) | ||
| .sample(); | ||
| Club savedClub = clubRepository.save(club); | ||
|
|
||
| Feed feed1 = fixture.giveMeBuilder(Feed.class) | ||
| .set("id", 1L) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용1") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| Feed feed2 = fixture.giveMeBuilder(Feed.class) | ||
| .set("id", 2L) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용2") | ||
| .set("feedType", FeedType.VIDEO) | ||
| .sample(); | ||
| Feed feed3 = fixture.giveMeBuilder(Feed.class) | ||
| .set("id", 3L) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용3") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| Feed feed4 = fixture.giveMeBuilder(Feed.class) | ||
| .set("id", 4L) | ||
| .set("club", savedClub) | ||
| .set("activityContent", "내용4") | ||
| .set("feedType", FeedType.IMAGE) | ||
| .sample(); | ||
| feedRepository.saveAll(List.of(feed1, feed2, feed3, feed4)); | ||
|
|
||
| Long clubId = savedClub.getId(); | ||
| int size = 2; | ||
| Long cursorId = 4L; | ||
| // when | ||
| Slice<Feed> page = feedRepository.findPageByClubIdOrderById(clubId, size, cursorId); | ||
| // then | ||
| Assertions.assertThat(feeds.get(0).getActivityContent()).isEqualTo("내용3"); | ||
| Assertions.assertThat(feeds.get(0).getId()).isEqualTo(3L); | ||
| Assertions.assertThat(feeds.get(1).getActivityContent()).isEqualTo("내용2"); | ||
| Assertions.assertThat(feeds.get(1).getId()).isEqualTo(2L); | ||
| Assertions.assertThat(feeds.get(2).getActivityContent()).isEqualTo("내용1"); | ||
| Assertions.assertThat(feeds.get(2).getId()).isEqualTo(1L); | ||
| List<Feed> feeds = page.getContent(); | ||
| assertThat(feeds.size()).isEqualTo(2); | ||
| assertThat(feeds.get(0).getId()).isEqualTo(feed3.getId()); | ||
| assertThat(feeds.get(0).getActivityContent()).isEqualTo(feed3.getActivityContent()); | ||
| assertThat(feeds.get(1).getId()).isEqualTo(feed2.getId()); | ||
| assertThat(feeds.get(1).getActivityContent()).isEqualTo(feed2.getActivityContent()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
페이지네이션 메타데이터 검증을 추가해주세요.
현재 테스트는 컨텐츠 검증에 중점을 두고 있지만, 페이지네이션 관련 메타데이터도 함께 검증하면 좋을 것 같습니다. 또한, ID 설정 이슈도 개선이 필요합니다.
// then
List<Feed> feeds = page.getContent();
assertThat(feeds.size()).isEqualTo(2);
assertThat(feeds.get(0).getId()).isEqualTo(feed3.getId());
assertThat(feeds.get(0).getActivityContent()).isEqualTo(feed3.getActivityContent());
assertThat(feeds.get(1).getId()).isEqualTo(feed2.getId());
assertThat(feeds.get(1).getActivityContent()).isEqualTo(feed2.getActivityContent());
+assertThat(page.hasNext()).isTrue();
+assertThat(page.isFirst()).isTrue();
+assertThat(page.getSize()).isEqualTo(size);📝 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.
| @DisplayName("cursorId보다 작은 Feed를 size 개수만큼 페이지로 반환한다.") | |
| @Test | |
| void findPageByClubIdOrderById() { | |
| // given | |
| Club club = fixture.giveMeBuilder(Club.class) | |
| .set("name", "카우") | |
| .set("user", null) | |
| .set("score", Score.from(BigDecimal.ZERO)) | |
| .set("clubMembers", null) | |
| .sample(); | |
| Club savedClub = clubRepository.save(club); | |
| Feed feed1 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 1L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용1") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed2 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 2L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용2") | |
| .set("feedType", FeedType.VIDEO) | |
| .sample(); | |
| Feed feed3 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 3L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용3") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed4 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 4L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용4") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| feedRepository.saveAll(List.of(feed1, feed2, feed3, feed4)); | |
| Long clubId = savedClub.getId(); | |
| int size = 2; | |
| Long cursorId = 4L; | |
| // when | |
| Slice<Feed> page = feedRepository.findPageByClubIdOrderById(clubId, size, cursorId); | |
| // then | |
| Assertions.assertThat(feeds.get(0).getActivityContent()).isEqualTo("내용3"); | |
| Assertions.assertThat(feeds.get(0).getId()).isEqualTo(3L); | |
| Assertions.assertThat(feeds.get(1).getActivityContent()).isEqualTo("내용2"); | |
| Assertions.assertThat(feeds.get(1).getId()).isEqualTo(2L); | |
| Assertions.assertThat(feeds.get(2).getActivityContent()).isEqualTo("내용1"); | |
| Assertions.assertThat(feeds.get(2).getId()).isEqualTo(1L); | |
| List<Feed> feeds = page.getContent(); | |
| assertThat(feeds.size()).isEqualTo(2); | |
| assertThat(feeds.get(0).getId()).isEqualTo(feed3.getId()); | |
| assertThat(feeds.get(0).getActivityContent()).isEqualTo(feed3.getActivityContent()); | |
| assertThat(feeds.get(1).getId()).isEqualTo(feed2.getId()); | |
| assertThat(feeds.get(1).getActivityContent()).isEqualTo(feed2.getActivityContent()); | |
| @DisplayName("cursorId보다 작은 Feed를 size 개수만큼 페이지로 반환한다.") | |
| @Test | |
| void findPageByClubIdOrderById() { | |
| // given | |
| Club club = fixture.giveMeBuilder(Club.class) | |
| .set("name", "카우") | |
| .set("user", null) | |
| .set("score", Score.from(BigDecimal.ZERO)) | |
| .set("clubMembers", null) | |
| .sample(); | |
| Club savedClub = clubRepository.save(club); | |
| Feed feed1 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 1L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용1") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed2 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 2L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용2") | |
| .set("feedType", FeedType.VIDEO) | |
| .sample(); | |
| Feed feed3 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 3L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용3") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| Feed feed4 = fixture.giveMeBuilder(Feed.class) | |
| .set("id", 4L) | |
| .set("club", savedClub) | |
| .set("activityContent", "내용4") | |
| .set("feedType", FeedType.IMAGE) | |
| .sample(); | |
| feedRepository.saveAll(List.of(feed1, feed2, feed3, feed4)); | |
| Long clubId = savedClub.getId(); | |
| int size = 2; | |
| Long cursorId = 4L; | |
| // when | |
| Slice<Feed> page = feedRepository.findPageByClubIdOrderById(clubId, size, cursorId); | |
| // then | |
| List<Feed> feeds = page.getContent(); | |
| assertThat(feeds.size()).isEqualTo(2); | |
| assertThat(feeds.get(0).getId()).isEqualTo(feed3.getId()); | |
| assertThat(feeds.get(0).getActivityContent()).isEqualTo(feed3.getActivityContent()); | |
| assertThat(feeds.get(1).getId()).isEqualTo(feed2.getId()); | |
| assertThat(feeds.get(1).getActivityContent()).isEqualTo(feed2.getActivityContent()); | |
| assertThat(page.hasNext()).isTrue(); | |
| assertThat(page.isFirst()).isTrue(); | |
| assertThat(page.getSize()).isEqualTo(size); |
| private Slice<Feed> buildSlice(Slice<Feed> originalSlice, int size) { | ||
| List<Feed> content = new ArrayList<>(originalSlice.getContent()); | ||
| boolean hasNext = content.size() > size; | ||
|
|
||
| if (hasNext) { | ||
| content.remove(content.size() - 1); | ||
| } | ||
|
|
||
| return new SliceImpl<>(content, PageRequest.of(originalSlice.getNumber(), size), hasNext); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
buildSlice 메서드에 입력값 검증 추가 필요
현재 구현은 올바르게 동작하지만, 다음과 같은 방어적 코드를 추가하면 좋을 것 같습니다:
- size 파라미터가 음수인 경우 검증
- originalSlice가 null인 경우 검증
- content가 비어있는 경우 처리
다음과 같이 수정을 제안드립니다:
private Slice<Feed> buildSlice(Slice<Feed> originalSlice, int size) {
+ if (originalSlice == null) {
+ throw new IllegalArgumentException("originalSlice must not be null");
+ }
+ if (size < 0) {
+ throw new IllegalArgumentException("size must not be negative");
+ }
+
List<Feed> content = new ArrayList<>(originalSlice.getContent());
boolean hasNext = content.size() > size;
if (hasNext) {
content.remove(content.size() - 1);
}
return new SliceImpl<>(content, PageRequest.of(originalSlice.getNumber(), size), hasNext);
}📝 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.
| private Slice<Feed> buildSlice(Slice<Feed> originalSlice, int size) { | |
| List<Feed> content = new ArrayList<>(originalSlice.getContent()); | |
| boolean hasNext = content.size() > size; | |
| if (hasNext) { | |
| content.remove(content.size() - 1); | |
| } | |
| return new SliceImpl<>(content, PageRequest.of(originalSlice.getNumber(), size), hasNext); | |
| } | |
| private Slice<Feed> buildSlice(Slice<Feed> originalSlice, int size) { | |
| if (originalSlice == null) { | |
| throw new IllegalArgumentException("originalSlice must not be null"); | |
| } | |
| if (size < 0) { | |
| throw new IllegalArgumentException("size must not be negative"); | |
| } | |
| List<Feed> content = new ArrayList<>(originalSlice.getContent()); | |
| boolean hasNext = content.size() > size; | |
| if (hasNext) { | |
| content.remove(content.size() - 1); | |
| } | |
| return new SliceImpl<>(content, PageRequest.of(originalSlice.getNumber(), size), hasNext); | |
| } |
src/main/java/ddingdong/ddingdongBE/domain/feed/api/FeedApi.java
Outdated
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/feed/api/FeedApi.java
Outdated
Show resolved
Hide resolved
| List<Feed> feeds = feedPage.getContent(); | ||
|
|
||
| List<FeedListQuery> feedListQueries = buildFeedListQuery(feeds); | ||
| PagingQuery pagingQuery = PagingQuery.of(currentCursorId, feeds.get(feeds.size() -1).getId(), feedPage.hasNext()); |
There was a problem hiding this comment.
p2)
조회된 feed가 없을 경우 feeds.get(feeds.size() -1) 해당 코드에서 OutOfBound exception이 발생할 수 있어 처리가 필요할거 같습니다!
There was a problem hiding this comment.
FeedService 내에서 Slice반환하기 전, 빈 콘텐츠 값일 때 예외 처리 하였습니다
7374d2f
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/test/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepositoryTest.java (2)
65-94:⚠️ Potential issueID를 수동으로 설정하는 방식을 개선해주세요.
테스트 데이터의 ID를 수동으로 설정하면 데이터베이스의 ID 생성 전략과 충돌이 발생할 수 있습니다. ID 설정을 제거하고 데이터베이스가 생성한 ID를 사용하는 것이 좋습니다.
다음과 같이 수정해주세요:
Feed feed1 = fixture.giveMeBuilder(Feed.class) - .set("id", 1L) .set("club", savedClub) .set("activityContent", "내용1") .sample();Also applies to: 171-194
97-107: 🛠️ Refactor suggestion페이지네이션 메타데이터 검증을 추가해주세요.
현재 테스트는 컨텐츠 검증에만 중점을 두고 있습니다. 페이지네이션의 완전한 검증을 위해 hasNext, isFirst, size 등의 메타데이터도 검증해주세요.
다음과 같이 각 테스트 메소드의 검증부를 보완해주세요:
assertThat(feeds.size()).isEqualTo(2); assertThat(feeds.get(0).getId()).isEqualTo(feed3.getId()); assertThat(feeds.get(0).getActivityContent()).isEqualTo(feed3.getActivityContent()); +assertThat(page.hasNext()).isTrue(); +assertThat(page.isFirst()).isTrue(); +assertThat(page.getSize()).isEqualTo(size);Also applies to: 151-156, 201-208
🧹 Nitpick comments (4)
src/main/java/ddingdong/ddingdongBE/domain/feed/api/FeedApi.java (3)
22-31: 페이지네이션 파라미터에 대한 제약 조건 추가 검토가 필요합니다.size 파라미터에 대한 최대값 제한이 없어 과도한 요청이 발생할 수 있습니다.
다음과 같이
@Max어노테이션을 추가하는 것을 고려해보세요:- @RequestParam(value = "size", defaultValue = "9") int size, + @RequestParam(value = "size", defaultValue = "9") @Max(value = 100, message = "페이지 크기는 최대 100을 초과할 수 없습니다.") int size,
33-41: 페이지네이션 구현이 일관성 있게 잘 되었습니다.이전 엔드포인트와 동일한 페이지네이션 로직을 사용하여 일관성을 유지하고 있습니다. 다만, 앞서 언급한 size 파라미터 제약 조건은 여기에도 동일하게 적용되면 좋을 것 같습니다.
22-41: 커서 기반 페이지네이션 구현에 대한 아키텍처 제안현재 구현은 잘 되어있지만, 다음 사항들을 고려해보시면 좋을 것 같습니다:
- 페이지네이션 관련 파라미터들을 별도의 DTO로 분리하여 재사용성을 높이는 것
- 커서 값의 유효성을 검증하는 로직 추가
- 응답에 다음 페이지 존재 여부를 나타내는 필드 포함
이러한 개선사항들은 API의 확장성과 유지보수성을 향상시킬 수 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (1)
Line range hint
24-71: 커서 기반 페이지네이션 구현에 대한 아키텍처 제안현재 구현은 잘 작동하지만, 다음과 같은 개선을 고려해 보시면 좋겠습니다:
페이지네이션 관련 상수 추출
- 최대 페이지 크기
- 기본 페이지 크기
커서 처리를 위한 별도의 DTO 도입 고려
- 현재 커서 상태
- 다음 페이지 커서
- 페이지 메타데이터
이러한 구조화를 통해 페이지네이션 로직의 재사용성과 유지보수성을 높일 수 있습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/ddingdong/ddingdongBE/domain/feed/api/FeedApi.java(1 hunks)src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java(3 hunks)src/test/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepositoryTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (7)
src/main/java/ddingdong/ddingdongBE/domain/feed/api/FeedApi.java (2)
3-17: LGTM - 깔끔한 import 구성입니다!페이지네이션 응답 타입에 대한 import가 잘 구성되어 있습니다.
43-48: 단일 피드 조회 API가 명확하게 구현되었습니다.
@PathVariable에 파라미터 이름을 명시하여 Swagger 문서화가 잘 될 수 있도록 구현되었습니다.src/test/java/ddingdong/ddingdongBE/domain/feed/repository/FeedRepositoryTest.java (2)
31-37: 테스트 설정이 잘 구현되었습니다!각 테스트 실행 전에 데이터를 정리하는 방식이 외래 키 제약 조건을 고려하여 적절하게 구현되었습니다.
39-41: 테스트 케이스 구성이 잘되어있습니다!커서 기반 페이지네이션의 주요 시나리오들을 잘 커버하고 있습니다:
- 최신 피드 조회
- 남은 데이터가 요청 크기보다 작은 경우
- 기본 커서 기반 페이지네이션
Also applies to: 109-111, 159-161
src/main/java/ddingdong/ddingdongBE/domain/feed/service/GeneralFeedService.java (3)
6-6: 페이지네이션 관련 import 구문이 적절히 추가되었습니다.커서 기반 페이지네이션 구현을 위한 필수 클래스들이 잘 import 되었습니다.
Also applies to: 10-12
58-71: buildSlice 메서드 개선 필요
입력값 검증 추가 필요
빈 목록 처리 방식 재고려
빈 목록을 예외로 처리하는 것보다, 빈 Slice를 반환하는 것이 페이지네이션의 일반적인 패턴에 더 부합할 수 있습니다.private Slice<Feed> buildSlice(Slice<Feed> originalSlice, int size) { + if (originalSlice == null) { + throw new IllegalArgumentException("originalSlice must not be null"); + } + if (size <= 0) { + throw new IllegalArgumentException("size must be positive"); + } List<Feed> content = new ArrayList<>(originalSlice.getContent()); - if (content.isEmpty()) { - throw new ResourceNotFound("Feed 페이지 내 콘텐츠를 찾을 수 없습니다."); - } boolean hasNext = content.size() > size; if (hasNext) { content.remove(content.size() - 1); } return new SliceImpl<>(content, PageRequest.of(originalSlice.getNumber(), size), hasNext); }
29-31: 🛠️ Refactor suggestion입력값 검증 로직 추가 필요
위와 동일한 맥락에서 size와 currentCursorId에 대한 검증이 필요합니다.
public Slice<Feed> getNewestFeedPerClubPage(int size, Long currentCursorId) { + if (size <= 0) { + throw new IllegalArgumentException("size must be positive"); + } Slice<Feed> feedPages = feedRepository.findNewestPerClubPage(size + 1, currentCursorId); return buildSlice(feedPages, size); }Likely invalid or redundant comment.
| public Slice<Feed> getFeedPageByClubId(Long clubId, int size, Long currentCursorId) { | ||
| Slice<Feed> feedPages = feedRepository.findPageByClubIdOrderById(clubId, size + 1, currentCursorId); | ||
| return buildSlice(feedPages, size); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
입력값 검증 로직 추가 필요
다음과 같은 입력값 검증을 추가하는 것이 좋겠습니다:
- clubId null 체크
- size가 양수인지 검증
- currentCursorId null 허용 여부 명시적 처리
public Slice<Feed> getFeedPageByClubId(Long clubId, int size, Long currentCursorId) {
+ if (clubId == null) {
+ throw new IllegalArgumentException("clubId must not be null");
+ }
+ if (size <= 0) {
+ throw new IllegalArgumentException("size must be positive");
+ }
Slice<Feed> feedPages = feedRepository.findPageByClubIdOrderById(clubId, size + 1, currentCursorId);
return buildSlice(feedPages, size);
}📝 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.
| public Slice<Feed> getFeedPageByClubId(Long clubId, int size, Long currentCursorId) { | |
| Slice<Feed> feedPages = feedRepository.findPageByClubIdOrderById(clubId, size + 1, currentCursorId); | |
| return buildSlice(feedPages, size); | |
| } | |
| public Slice<Feed> getFeedPageByClubId(Long clubId, int size, Long currentCursorId) { | |
| if (clubId == null) { | |
| throw new IllegalArgumentException("clubId must not be null"); | |
| } | |
| if (size <= 0) { | |
| throw new IllegalArgumentException("size must be positive"); | |
| } | |
| Slice<Feed> feedPages = feedRepository.findPageByClubIdOrderById(clubId, size + 1, currentCursorId); | |
| return buildSlice(feedPages, size); | |
| } |
|
🚀 작업 내용
🤔 고민했던 내용
💬 리뷰 중점사항
Summary by CodeRabbit
새로운 기능
버그 수정
리팩토링