-
Notifications
You must be signed in to change notification settings - Fork 8
chore: 전체 게시글 조회 시 첫 번째 이미지 url에 해당하는 변수명 변경 #433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: 전체 게시글 조회 시 첫 번째 이미지 url에 해당하는 변수명 변경 #433
Conversation
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/com/example/solidconnection/community/post/dto/PostListResponse.java (2)
18-18: 2) API 응답 필드명 변경에 따른 호환성 확인 요청
- 이 변경은 외부 응답 스키마를 바꾸므로, 기존 클라이언트가
url을 기대한다면 깨질 수 있습니다.- 브레이킹 체인지가 의도된 것인지 확인해 주세요. 그리고 API 문서/브루노 스위트/프런트 코드 동시 반영을 권장합니다.
- 컨트랙트 테스트(예: MockMvc + JSONPath)로
postThumbnailUrl필드 노출을 검증하면 회귀를 막을 수 있습니다.
18-18: 3) null 필드 응답 정책 정리 제안
- 이미지가 없을 때 null을 반환합니다. API 응답에서 null 필드를 숨기고 싶다면 Jackson 설정을 통해 간단히 제어할 수 있습니다.
- 레코드에 다음 어노테이션을 적용하면 응답에서 null 필드를 제거할 수 있습니다.
// import com.fasterxml.jackson.annotation.JsonInclude; @JsonInclude(JsonInclude.Include.NON_NULL) public record PostListResponse(...) { ... }src/test/java/com/example/solidconnection/community/post/service/PostQueryServiceTest.java (2)
175-177: 2) ‘첫 번째’의 정의를 명확히 하여 테스트 플래키니스 방지
getFirstImageUrl(post)는 컬렉션의 순서에 의존합니다. 컬렉션이 List(삽입 순서 보장)인지, JPA에서@OrderBy로 정렬이 보장되는지 확인이 필요합니다.- 보장이 없다면 아래 중 하나로 결정성을 확보해 주세요.
- 엔티티 컬렉션에
@OrderBy("createdAt asc")또는@OrderBy("id asc")추가.- 조회 쿼리에서 ORDER BY로 정렬.
- 코드에서
sorted(Comparator.comparing(PostImage::getId))후findFirst()적용.// 예시: 코드에서 결정적 정렬 보장 return post.getPostImageList().stream() .sorted(Comparator.comparing(PostImage::getId)) // 혹은 createdAt .map(PostImage::getUrl) .findFirst() .orElse(null);
179-199: 3) 직렬화 스키마 검증 테스트(컨트롤러 레벨) 추가 제안
- 본 테스트는 서비스 레이어 객체를 검증합니다. 실제 HTTP 응답에서 JSON 필드명이
postThumbnailUrl로 노출되는지까지 확인하면 더 견고해집니다.- MVC 슬라이스 테스트(WebMvcTest)로 목록 API를 호출하고
jsonPath("$[?(@.id==%s)].postThumbnailUrl")형태로 필드 존재/값을 검증하는 테스트를 추가하는 것을 권장합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/example/solidconnection/community/post/dto/PostListResponse.java(1 hunks)src/test/java/com/example/solidconnection/community/post/service/PostQueryServiceTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
src/main/java/com/example/solidconnection/community/post/dto/PostListResponse.java (1)
18-18: 1) DTO 필드명 변경: postThumbnailUrl로 명확성 향상의도가 분명해져 가독성이 좋아졌습니다. 네이밍 일관성 면에서도 👍입니다.
src/test/java/com/example/solidconnection/community/post/service/PostQueryServiceTest.java (1)
170-199: 1) 첫 번째 이미지 썸네일 검증 테스트 추가: 의도를 정확히 커버
- 두 장의 이미지를 추가하고, 첫 번째 URL을 썸네일로 검증하는 핵심 시나리오를 잘 담았습니다.
- 이미지가 없는 게시글에 대해 null을 검증한 점도 좋아요.
Gyuhyeok99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인했습니다!
| ZonedDateTime createdAt, | ||
| ZonedDateTime updatedAt, | ||
| String postCategory, | ||
| String url | ||
| String postThumbnailUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 pr과 관련은 없지만 불필요한 Long타입 나중에 한 번 싹 고치는 pr 제가 올리겠습니다!
nayonsoso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 PR이 머지되면 bruno PR 또한 올리겠습니다 !
꼼꼼한 체크 감사드립니다🙇🏻♀️
| // then | ||
| PostListResponse post1Response = actualResponses.stream() | ||
| .filter(p -> p.id().equals(post1.getId())) | ||
| .findFirst() | ||
| .orElseThrow(); | ||
|
|
||
| PostListResponse post3Response = actualResponses.stream() | ||
| .filter(p -> p.id().equals(post3.getId())) | ||
| .findFirst() | ||
| .orElseThrow(); | ||
|
|
||
| assertAll( | ||
| () -> assertThat(post1Response.postThumbnailUrl()).isEqualTo(firstImageUrl), | ||
| () -> assertThat(post3Response.postThumbnailUrl()).isNull() | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조금 사소한 부분일수도 있는데요..!
지금의 테스트는 두가지 동작을 테스트하고 있는 것 같아요.
- 게시글에 여러개의 이미지가 있으면 첫번째 이미지 썸네일을 반환한다.
- 게시글에 이미지가 없다면, 썸네일을 null로 반환한다.
이 두가지를 각각의 테스트로 나누어 "테스트가 한가지만 검증하도록" 하는 것도 좋을 것 같습니다~
하지만 치명적인 부분은 아니라 생각해서 approve 드립니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아핫 그러네요 저도 분리하는 것이 더 적합하다고 생각합니다 !!
관련 이슈
작업 내용
전체 게시글 조회 시 첨부된 이미지들 중 첫 번째 이미지를 가져오는 로직은 이미 구현되어 있었습니다.
다만 응답에서 변수명이
url로 작성되었고, 혼동의 여지가 있기 때문에,postThumbnailUrl로 변수명을 변경하였습니다.또한 첫 번째 이미지를 가져오는지 확인하는 테스트 코드를 추가로 작성하였습니다.
이 PR이 머지되면 bruno PR 또한 올리겠습니다 !
특이 사항
리뷰 요구사항 (선택)