-
Notifications
You must be signed in to change notification settings - Fork 2
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
[BE] feat: 리뷰 단건 조회 API 업데이트 #294
Conversation
"SELECT o.* FROM option_item o " + | ||
"LEFT JOIN checkbox_answer ca " + | ||
"LEFT JOIN checkbox_answer_selected_option c " + | ||
"ON c.checkbox_answer_id = ca.id " + | ||
"WHERE ca.review_id = :reviewId " + | ||
"AND ca.question_id = :questionId " + | ||
"AND c.selected_option_id = o.id " + | ||
"ORDER BY o.position", | ||
nativeQuery = true) |
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.
여러 개를 Join해올 때에는 id가 겹쳐서, SELECT o.*
와 같이 앞쪽 와일드카드를 사용해야 해요
import reviewme.review.domain.exception.TextAnswerNotFoundException; | ||
|
||
@Slf4j | ||
public class TextAnswers { |
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.
서비스에서 낼 수 있는 예외를 도메인 클래스를 생성해 위임했습니다. 따로 엔티티는 아니예요!
@OneToMany(fetch = FetchType.EAGER, cascade = CascadeType.ALL, orphanRemoval = true) | ||
@JoinColumn(name = "checkbox_answer_id", nullable = false, updatable = false) | ||
private List<CheckBoxAnswerSelectedOptionId> selectedOptionIds; |
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.
@CollectionTable
을 사용하면DatabaseCleaner
가 이를 찾아내지 못합니다 (Table
어노테이션 활용)- 나아가 저희가 이야기한
@CollectionTable
의 단점들도 있기에 (전체 삭제 등), 이를@OneToMany
+@JoinColumn(updatable = false)
로 해결할 수 있다고 보았어요.
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.
식별자 없는 테이블보다 이게 더 좋다고 생각됩니다 👍
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.
그리고 이 부분도 필드명을 selectedOptions 로 가는게 좋겠네요!
@Column(name = "section_id", nullable = false, insertable = false, updatable = false) | ||
private long sectionId; |
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.
FK를 가지지 않아도 되지만, 네이티브 쿼리로 짤 일이 많다보니 테이블 열과 일치할 필요가 있었어요.
찾아보니 이런 식으로 작성하면 직접 생성할 때 값이 없어도, 포함하는 테이블에서 Persist할 때 함께 등록됩니다.
if (question.isSelectable()) { | ||
questionResponses.add(getCheckboxAnswerResponse(review, question)); | ||
continue; | ||
} | ||
questionResponses.add(getTextAnswerResponse(question, textAnswers)); |
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.
이런 식으로 분기하는 게 적절할까요? 어쩔 수 없었을까요?
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.
상속이나 인터페이스를 만들지 않은 지금은 이 방법밖에 업다 생각합니다
@GetMapping("/v2/reviews/{id}") | ||
public ResponseEntity<TemplateAnswerResponse> findReceivedReviewDetailV2( | ||
@PathVariable long id, | ||
@HeaderProperty(GROUP_ACCESS_CODE_HEADER) String groupAccessCode) { | ||
TemplateAnswerResponse response = reviewDetailLookupService.getReviewDetail(groupAccessCode, id); | ||
return ResponseEntity.ok(response); | ||
} |
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.
임시로 V2로 두었는데, 이거 기존 API를 두고 컨트롤러를 사악 올려도 될 것 같아요 😋
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.
좋은 생각입니당~
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.
테스트 격리 문제가 중간 테이블 적용으로 해결되었군요👍👍👍
깊게 읽으려면 시간이 너무 걸릴 것 같아서 일단 표면적인 것 먼저 달았습니다!
long formId, | ||
String revieweeName, | ||
String projectName, | ||
List<SectionAnswerResponse> sections |
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.
LocalDate createAt이 추가되어야 하겠네요!
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.
이거 우선 생성자에서 now()
를 하고, JPA Audit을 쓰는 게 깔끔할 것 같아요
public record TemplateAnswerResponse( | ||
long formId, |
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.
템플릿은 말그대로 템플릿으로서의 의미가 크게 느껴지기도 해서 TemplateAnswer라는 DTO명이 낮설게 느껴지기도 합니다!
id명도 formId니까 FormAnswerResponse은 어떤가요?
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.
전체 네이밍을 바꾸자는 제안인가요 ? Template -> Form?
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.
아니요 이 DTO 이름만 해당하는 제안이었습니다~
#296 (comment)
이 코멘트와도 연결이 되어있어요!
// given | ||
TextAnswers textAnswers = new TextAnswers(List.of(new TextAnswer(1, "답변"))); | ||
// when | ||
TextAnswer actual = textAnswers.getAnswerByQuestionId(1); | ||
// then | ||
assertThat(actual.getContent()).isEqualTo("답변"); |
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.
given, when, then 개행이 필요합니다~
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.
👍🏻
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.
오우..... 고생했어요 일단 코멘트 확인 북탁함니다~
.orElseThrow(() -> new ReviewGroupNotFoundByGroupAccessCodeException(groupAccessCode)); | ||
|
||
Review2 review = reviewRepository.findByIdAndReviewGroupId(reviewId, reviewGroup.getId()) | ||
.orElseThrow(() -> new ReviewGroupNotFoundByGroupAccessCodeException(groupAccessCode)); |
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.
리뷰 그룹을 찾고 난 후 조회하려는 리뷰와 일치하지 않는 경우 인 것 같아요. (그룹 액세스 코드는 검증했지만, 볼 수 없는 리뷰인 경우?)
ReviewGroupNotFoundByGroupAccessCodeException 보다는 다른 메세지가 담긴 예외가 나을 것 같아요
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.
사용자 입장에서는 존재하지 않는 예외로 내보내는 게 좋겠습니다
long templateId = review.getTemplateId(); | ||
|
||
Set<Long> selectedOptionItemIds = optionItemRepository.findSelectedOptionItemIdsByReviewId(reviewId); | ||
List<SectionAnswerResponse> sectionResponses = sectionRepository.findAllByTemplateId(templateId) |
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.
template 레포에서 template를 찾고 sectionIds 로 찾는 것과 네이티브 쿼리로 한방에 찾는 것 중 어떤 방법이 더 좋을지 고민되네요..
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.
쿼리 개수로 따지면 이 쪽이 훨씬 효율적이라고 느낍니다. 한 번에 가져오는 게 낫다고 판단했어요
Set<Long> selectedOptionItemIds = optionItemRepository.findSelectedOptionItemIdsByReviewId(reviewId); | ||
List<SectionAnswerResponse> sectionResponses = sectionRepository.findAllByTemplateId(templateId) | ||
.stream() | ||
.filter(section -> section.isVisibleBySelectedOptionIds(selectedOptionItemIds)) |
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.
always와 작성된 섹션만 가져오는 것 좋네요👍👍
|
||
Optional<OptionGroup> findByQuestionId(long questionId); | ||
|
||
default OptionGroup getByQuestionId(long questionId) { |
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.
default 메서드명 컨벤션을 어떻게 통일하면 좋을까요..
지금 get000By000Id 로 되어있는 것들도 있고 이런 경우도 있는 것 같아요.
개인적으로는 findByIdorThrow 처럼 안에 검증이 포함되어있다 라는 것을 명시한 네이밍도 좋다고 생각해요
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.
오? getOrElseThrow()
로 바깥에서 예외를 넣어주는 것도 좋겠는데요 ㅋㅋ
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.
통일성 있게 사용 가능하면 이 방법도 논의 해보면 좋겠네요
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.
아루 고생 많았어요~~🥳
어려운 부분을 맡았었네요! 고생한 흔적이 보입니다🥹
궁금한 부분들 코멘트 남겼으니 확인 부탁해요!
backend/src/main/java/reviewme/review/service/dto/response/detail/QuestionAnswerResponse.java
Show resolved
Hide resolved
backend/src/main/java/reviewme/template/repository/SectionRepository.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/review/repository/QuestionRepository2.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/reviewme/question/domain/exception/OptionGroupNotFoundException.java
Outdated
Show resolved
Hide resolved
@OneToMany(fetch = FetchType.EAGER, cascade = CascadeType.ALL, orphanRemoval = true) | ||
@JoinColumn(name = "checkbox_answer_id", nullable = false, updatable = false) | ||
private List<CheckBoxAnswerSelectedOptionId> selectedOptionIds; |
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.
그리고 이 부분도 필드명을 selectedOptions 로 가는게 좋겠네요!
backend/src/main/java/reviewme/review/service/ReviewDetailService.java
Outdated
Show resolved
Hide resolved
public TextAnswer getAnswerByQuestionId(long questionId) { | ||
if (!textAnswers.containsKey(questionId)) { | ||
throw new TextAnswerNotFoundException(questionId); | ||
} | ||
return textAnswers.get(questionId); | ||
} |
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.
이 객체는 무엇을 목적으로 만드신 건지 궁금해요! 🙋🏻♀️
서비스 코드와 함께 봤을 때, (제가 이해하기로는 )
- Template 안의 Section 안의
questionIds
에 대응하는 - Review 안의 TextQuestion 안의
questionId
가 있는지
검증하기 위함 같은데, 그럼 저장 과정 중에 questionId 가 잘못 저장되는 경우를 염두에 두신 건가요?
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.
- 가져오는 걸 get get 하는 것이 별로라고 생각했어요
- 넣을 때 검증도 있지만, 빼낼 때의 검증을 하지 않아서 나쁠 게 없다고 생각했어요 🤔
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.
그렇다면 이때 발생하는 예외는 ERROR 레벨이어야 하겠네요,
제약조건 준수가 되지 않는 데이터가 들어가있는 상황이니까요😲
if (question.isSelectable()) { | ||
questionResponses.add(getCheckboxAnswerResponse(review, question)); | ||
continue; | ||
} | ||
questionResponses.add(getTextAnswerResponse(question, textAnswers)); |
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.
상속이나 인터페이스를 만들지 않은 지금은 이 방법밖에 업다 생각합니다
@GetMapping("/v2/reviews/{id}") | ||
public ResponseEntity<TemplateAnswerResponse> findReceivedReviewDetailV2( | ||
@PathVariable long id, | ||
@HeaderProperty(GROUP_ACCESS_CODE_HEADER) String groupAccessCode) { | ||
TemplateAnswerResponse response = reviewDetailLookupService.getReviewDetail(groupAccessCode, id); | ||
return ResponseEntity.ok(response); | ||
} |
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.
좋은 생각입니당~
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.
일프로브!!
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.
어푸푸푸푸르루브 🏊🏻
커밋 따라서 읽으시는 걸 추천해요
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
@CollectionTable
으로 생성되는 테이블은 데이터베이스 격리가 불가능해 이를 빼냈습니다.📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말