Conversation
- ✨feat: 내 리뷰 조회 UseCase 메서드 구현 및 QueryRequest 추가 - ✨feat: 내 리뷰 목록 조회 요청 DTO 추가 및 QueryRequest 변환 메서드 작성 - ✨feat: 리뷰 조회 쿼리 프로젝션 및 QueryRepository 인터페이스 정의 - ✨feat: 리뷰 조회 QueryService 구현 - ✨feat: 리뷰 조회 QueryDSL 구현체 작성 - ✨feat: 내 리뷰 목록 응답 DTO 정의 및 매핑 로직 추가 - ✨feat: 내 리뷰 조회 API 정의 및 컨트롤러 구현
870c625 to
57e45ec
Compare
| @Bean | ||
| fun modelResolver(objectMapper: ObjectMapper): ModelResolver { | ||
| objectMapper.propertyNamingStrategy = PropertyNamingStrategies.SNAKE_CASE | ||
| return ModelResolver(objectMapper) |
There was a problem hiding this comment.
이런 방법이 가능했군요..! 이 방법을 몰라서 고생했는데 하나 또 배워갑니다 ㅎㅎ
There was a problem hiding this comment.
P5: 이것도 질문인데 SNAKE_CASE를 적용해야하는 이유가 있는 건가요? 오히려 명시적으로 camel Case를 적용하면 안되는 것일까요?
There was a problem hiding this comment.
@dnjsals45 이부분은 성민님이 답변해주시면 좋을 것 같아요. (#15)
dnjsals45
left a comment
There was a problem hiding this comment.
계층 부분에 대해서 많이 고민해주신 부분이 확실히 드러난 PR이라고 생각합니다. 덕분에 저도 조금 더 생각해보고 고민해볼 수 있었던 것 같아요.
Pageable이 변경되었을 때 파급을 막기 위한 방법으로는
- Service 에서 UseCase로 데이터 전달 시, 별도의 dto로 감싸서 데이터만 보낸다
- Pageable 기술을 사용하지 않고 직접 쿼리로 전체 요소 개수 등을 파악한다
정도가 생각이 드는 것 같아요. 별도의 dto로 감싸게 되면 추후 변경되어도 service <-> repository 부분만 수정하면 되니까 조금 덜해지지 않을까 생각이 드는데, 번거롭게 과정이 하나 추가되는 단점이 존재하는 것 같아요
또한, 쿼리로 직접 모든 데이터를 구하려면 추가적인 쿼리가 발생한다는 단점이 있을 것 같아요.
| data class MyReviewListItem | ||
| @QueryProjection | ||
| constructor( | ||
| val id: Long, | ||
| val title: String, | ||
| val organizer: OrganizerType, | ||
| val activityType: String, | ||
| val createdAt: LocalDateTime, | ||
| val approvalStatus: ReviewApprovalStatus, | ||
| ) |
There was a problem hiding this comment.
p3: 지금 이 dto의 경우, application 계층에서 QueryDSL이라는 기술에 종속되어 있는 상태인 것 같아요. PR에 작성해주신 내용에 따르면 infrastructure -> application으로 진행하면서 application 계층에서는 infrastructure 계층에 대해 몰라야 하는 방향인 것 같은데, @QueryProjection을 사용하면 infrastructure 계층에서 변경점이 생기면 같이 변경되어야 할 것 같아요. Projections.contructor() 메소드를 RespositoryImpl 내부에서 사용하는 게 더 적절하지 않을까 생각해봅니다!
There was a problem hiding this comment.
이 부분도 고민이 많았습니다.
먼저 말씀드리고 싶은 건 단순히 저수준 모듈에서의 코드 복잡성을 고려해서 @QueryProjection을 사용했던 것은 아니었습니다.
제가 해당 모델이 기술에 독립적이지 못한 선택을 했던 것은 크게 2가지 이유가 있었습니다.
- 그렇게 따지면
@Entity도 JPA 기술에 강하게 의존하고 있는 것 또한 계층 분리를 수행하여야 한다. - QueryDSL이라는 기술스택이 우리에게 가져다주는 가장 큰 장점인 타입안정성을 포기해야한다.
제가 생각했을 때 QueryDSL 기술의 핵심 장점인 타입 안정성이 계층 순수성 보장이라는 이유로 포기하고 JPQL처럼 개발자의 실수가 런타임에 발견되는 패널티를 가져가야 한다. 라는 선택지에서 의구심이 들었습니다.
순수성을 가져갔을 때 얻어지는 이점과 포기해야하는 기술의 핵심 장점과의 트레이드 오프가 발생하였는데,
저는 핵심 장점이 더 크다고 여겨졌습니다.
QueryDSL을 사용하지 않고 다른 기술을 선택해서 재구현할 확률 보다 해당 쿼리를 유지보수 할일이 더 많아 효율적이라고 판단했던 것 같습니다.
There was a problem hiding this comment.
원경님 의견 듣고 저도 좀 조사를 해보니까, Q클래스를 생성하면서 컴파일 타임에서 안정성을 가져갈 수 있군요.
기존에 생각처럼 의존성을 계속 분리하는 방향으로 생각하면 할 수록, JPA를 적용하지 않았어야 했나.. 라는 생각까지 들게 되네요
지금처럼 일정 부분의 연관성을 타협을 하면서 진행해가는 방향으로 정하면 좋을 것 같네요. 의견 감사합니다!
| @Min(1) | ||
| @Schema(description = "요청 페이지 (1부터 시작)") | ||
| val page: Int = 1, |
There was a problem hiding this comment.
P5: 왜 1부터 시작하나요? 0부터 시작해도 되지 않나 생각이 들어서 질문드립니다~
There was a problem hiding this comment.
1부터 시작한 가장 큰 이유는 클라이언트를 배려였습니다.
ui화면에서 첫페이지가 1이고 2, 3, 4 .. 로 시작을 1로 시작하고 있습니다. 하지만 만약 API스펙을 0으로 바꾼다면 API 호출할 때는 1을 빼서 0번부터 호출해야 합니다.
우리의 DB의 쿼리 시스템이 0부터 시작이니 API에서 page 관련 호출은 1을 미리 빼서 호출해주세요 라는 맥락과 약속이 필요하게 됩니다.
그 책임을 누가갖든 상관없이 구현이 가능하지만, FE는 있는 그대로 요청하고 있는 그대로 반환하는 형태의 앱이 가장 이상적이라고 생각했던 것 같습니다.
간단한 연산이라도 클라이언트 쪽에서 하면 보안적인 이유에서도 그렇고 별로 좋지 못하다는 이야기를 줏어듣기도 했습니다..🙁
이러한 배경지식으로 1을 빼야하는 책임을 백엔드의 책임이라고 결정하였습니다.
|
|
||
| @Service | ||
| class ReviewQueryService( |
There was a problem hiding this comment.
P5: 네이밍에 대해서 우리가 같이 생각을 해보면 좋을 것 같아요 ReviewService의 경우 ReviewQueryRepository를 의존하고 있는 서비스들로 이해가 되는데 사실 제가 현업에서 이렇게 네이밍을 가지고 갈때 모든 조회 기능이 여기에 들어가면서 코드가 몇백줄이 넘어가는데 응집성은 딱히 없더라구요 차라리 기능별로 서비스를 분리하는 것은 어떨까 의견드립니다.
There was a problem hiding this comment.
그냥 ReviewService가 아닌ReviewQueryService를 말씀해주신게 맞겠죠?!
복합조회 관련 쿼리들이 이 객체에 많이 몰린다면 관련없는 각종 조인쿼리들이 많이 생길텐데 응집성이 떨어질 것 같다는 의견에 동의합니다.
하지만 또 이 도메인에서 응답에 필요로할 조회를 고려했을 때,
약간의 DTO 프로젝션만 수정하면 재사용이 가능할 것 같아서
각 기능별로 1:1로 서비스를 분리할 만큼의 복잡도는 또 아닌 것 같아서 조금 망설여 집니다.
추가될 조회기능으로는 특정 활동에서의 리뷰 리스트 조회 정도로 예상하고 있습니다.
말씀 주신대로 성격이 좀 달라보이는 조회쿼리가 올 가능성을 염두해두고 이름 변경을 해보겠습니다!
| @Bean | ||
| fun modelResolver(objectMapper: ObjectMapper): ModelResolver { | ||
| objectMapper.propertyNamingStrategy = PropertyNamingStrategies.SNAKE_CASE | ||
| return ModelResolver(objectMapper) |
There was a problem hiding this comment.
P5: 이것도 질문인데 SNAKE_CASE를 적용해야하는 이유가 있는 건가요? 오히려 명시적으로 camel Case를 적용하면 안되는 것일까요?
Tianea2160
left a comment
There was a problem hiding this comment.
수고 많으셨습니다! 몇몇가지 질문은 달았지만 나머지는 큰 이견이 없는 것 같아요~
PR 설명
✨feat: PageResponse 응답 포맷 및 리뷰 성공 코드 추가
제네릭을 활용한 공통 PageResponse 클래스를 정의했습니다.
✨feat:내 리뷰 조회 API 구현
리뷰 포인트
Page<Review>로 연관 데이터를 조회하는 방식은 N+1 문제를 유발할 수 있어, DTO Projection을 선택합니다.ReviewQueryRepositoryImpl은 가장 저수준 모듈에 해당하지만,Pageable에서 정렬 프로퍼티를OrderSpecifier로 매핑하는 로직이 코드 흐름을 방해하지 않도록 하고,private메서드로 분리하였습니다.2025-07-20 추가 변경 사항
Command→QueryRequest로 이름을 변경하였습니다.🔧config: swagger 문서에 snake_case 필드명으로 적용되도록 ModelResolver 설정
property naming strategy를snake_case로 변경하여 실제 응답 필드명과 문서 표기가 일치하도록 수정2025-07-22 추가 변경 사항
♻️refactor: 리뷰 도메인의 복합 조회 객체 이름을 명시적으로 변경
ReviewQueryService→ReviewOverviewQueryServiceReviewQueryRepository→ReviewOverviewQueryRepositoryReviewQueryRepositoryImpl→ReviewOverviewQueryRepositoryImpl