-
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: 회원이 등록한 리뷰 그룹들을 조회하는 기능 구현 #1098
base: develop
Are you sure you want to change the base?
Conversation
Test Results173 tests +7 170 ✅ +7 5s ⏱️ ±0s Results for commit 10ac07c. ± Comparison against base commit 44f413b. This pull request removes 7 and adds 14 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
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.
기능상으로 문제가 있을 것 같아서 RC 합니당~
@Column(name = "created_at", nullable = false) | ||
private LocalDateTime createdAt; |
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.
사소한 부분이지만 정렬 순서를 통일해봐요!
Column 다 나오고 Embedded 오는게 좋겠네요~
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.
저는 엔티티 필드의 정렬 순서가 어노테이션 기준이 아니라, 사용빈도나 중요도에 따라 논리적인 기준이 되어야 한다고 생각해요. 따라서 생성일은 엔티티의 본질적인 필드가 아니므로 맨 마지막에 배치하는 것이 맞다고 생각해요.
다른 필드들을 봐도 현재 어노테이션 별로 정렬되어 있진 않아요. (Column과 JoinColumn 혼재)
산초는 어노테이션 기준으로 정렬하는 것을 선호하는 이유가 있을까요?
-- 리뷰 그룹에 created_at을 추가합니다. | ||
|
||
ALTER TABLE review_group ADD COLUMN created_at TIMESTAMP(6) NOT NULL; |
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.
DATETIME(6)
은 지금은 사용되지 않는 (구) review
테이블입니다!
지금 사용하는 테이블은 new_reivew
이고(현 시점에는 네이밍이 review로 변경됨), TIMESTAMP(6)
으로 등록되어 있어요.
@@ -1,4 +1,4 @@ | |||
package reviewme.review.service; | |||
package reviewme.util; |
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.
패키지 변경 좋네요 ㅎㅎ
// @LoginMemberSession LoginMember loginMember | ||
) { | ||
// TODO : 머지 전, 리졸버 PR 머지되면 리베이스 후 삭제 예정 | ||
long memberId = 1L; | ||
ReviewGroupPageResponse response = reviewGroupLookupService.getMyReviewGroups(lastReviewGroupId, size, memberId); |
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.
의도가 명확하게 todo로 표시해되어서 잘 이해할 수 있었어요😊
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.
리베이스 후 변경 사항 적용했어요~
AND ( | ||
(:lastReviewGroupId IS NULL AND :lastCreatedAt IS NULL) | ||
OR (rg.createdAt < :lastCreatedAt) | ||
OR (rg.createdAt = :lastCreatedAt AND rg.id < :lastReviewGroupId) | ||
) |
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.
혹시 이 부분에서 왜 lastCreatedAt 이 사용되는지 설명해줄 수 있나요?
제가 이해가 부족하네요😓
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.
지금 응답(노출)에 대한 정렬 기준은 생성일(리뷰 작성일) 기준으로 하고 있는데요,
동시성 상황에서는 더 높은 id가 더 최신의 생성일을 가진다고 보장할 수 없기 때문에 응답을 id 기준으로만 했을 때 의도한대로 동작하지 않을 수 있어요.
id가 생성일을 보장하지 않는 이유는
- 생성일은 엔티티가 생성될 때, 생성자에서 설정되고,
- id는 트랜잭션이 커밋되고 영속화 될 때 지정되니
- 여러 요청이 멀티스레 상황에서는 처리하는 순서에 따라 생성일이 뒤바뀔 수 있어요.
따라서 lastCreatedAt도 함께 전달해서 기준을 강화하려는 목적이에요.
하지만 이렇게 사용할 경우, 프론트에서 lastId 뿐만 아니라 lastCreatedAt 까지 같이 전달받아야 해요. (순전히 lastId만 받으면 이전에 응답한 id가 응답될 경우가 존재해요.)
쓰다보니 프론트에서 lastCreatedAt을 요청에 포함해달라고 하는 것도 지금 상황에서는 힘들 것 같고, 다른 방법으로 해결을 할 수 있을 것 같아서, 이슈 정리하고 PR 요청할게요.
해당 조회 로직에서는 lastId만 사용하는 것으로 변경하겠습니다!
public record ReviewGroupPageElementResponse( | ||
|
||
long reviewGroupId, |
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.
이게 있어야 사용자가 클릭했을 때 정보가 전달될 수 있겠네요👍
if (!isLastPage) { | ||
elements.subList(0, elements.size()); | ||
} |
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.
elements.subList(0, pageSize.size()); 여야 되지 않을까요?
그리고 elements 에 하나 잘린게 재할당되어야 합니다!
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.
오 큰일날뻔 했네요! 👍👍
55be95f
to
805e8a8
Compare
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.
리반완!
// @LoginMemberSession LoginMember loginMember | ||
) { | ||
// TODO : 머지 전, 리졸버 PR 머지되면 리베이스 후 삭제 예정 | ||
long memberId = 1L; | ||
ReviewGroupPageResponse response = reviewGroupLookupService.getMyReviewGroups(lastReviewGroupId, size, memberId); |
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.
리베이스 후 변경 사항 적용했어요~
@Column(name = "created_at", nullable = false) | ||
private LocalDateTime createdAt; |
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.
저는 엔티티 필드의 정렬 순서가 어노테이션 기준이 아니라, 사용빈도나 중요도에 따라 논리적인 기준이 되어야 한다고 생각해요. 따라서 생성일은 엔티티의 본질적인 필드가 아니므로 맨 마지막에 배치하는 것이 맞다고 생각해요.
다른 필드들을 봐도 현재 어노테이션 별로 정렬되어 있진 않아요. (Column과 JoinColumn 혼재)
산초는 어노테이션 기준으로 정렬하는 것을 선호하는 이유가 있을까요?
if (!isLastPage) { | ||
elements.subList(0, elements.size()); | ||
} |
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.
오 큰일날뻔 했네요! 👍👍
-- 리뷰 그룹에 created_at을 추가합니다. | ||
|
||
ALTER TABLE review_group ADD COLUMN created_at TIMESTAMP(6) NOT NULL; |
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.
DATETIME(6)
은 지금은 사용되지 않는 (구) review
테이블입니다!
지금 사용하는 테이블은 new_reivew
이고(현 시점에는 네이밍이 review로 변경됨), TIMESTAMP(6)
으로 등록되어 있어요.
AND ( | ||
(:lastReviewGroupId IS NULL AND :lastCreatedAt IS NULL) | ||
OR (rg.createdAt < :lastCreatedAt) | ||
OR (rg.createdAt = :lastCreatedAt AND rg.id < :lastReviewGroupId) | ||
) |
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.
지금 응답(노출)에 대한 정렬 기준은 생성일(리뷰 작성일) 기준으로 하고 있는데요,
동시성 상황에서는 더 높은 id가 더 최신의 생성일을 가진다고 보장할 수 없기 때문에 응답을 id 기준으로만 했을 때 의도한대로 동작하지 않을 수 있어요.
id가 생성일을 보장하지 않는 이유는
- 생성일은 엔티티가 생성될 때, 생성자에서 설정되고,
- id는 트랜잭션이 커밋되고 영속화 될 때 지정되니
- 여러 요청이 멀티스레 상황에서는 처리하는 순서에 따라 생성일이 뒤바뀔 수 있어요.
따라서 lastCreatedAt도 함께 전달해서 기준을 강화하려는 목적이에요.
하지만 이렇게 사용할 경우, 프론트에서 lastId 뿐만 아니라 lastCreatedAt 까지 같이 전달받아야 해요. (순전히 lastId만 받으면 이전에 응답한 id가 응답될 경우가 존재해요.)
쓰다보니 프론트에서 lastCreatedAt을 요청에 포함해달라고 하는 것도 지금 상황에서는 힘들 것 같고, 다른 방법으로 해결을 할 수 있을 것 같아서, 이슈 정리하고 PR 요청할게요.
해당 조회 로직에서는 lastId만 사용하는 것으로 변경하겠습니다!
테스트 + 약간의 수정 적용해서 푸시했습니다~ |
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
구현을 위해서는 3가지 방법을 생각했습니다.
결론적으로 3번 방법으로 구현했고, 이유는 다음과 같습니다.
추가로, ReviewGroup의 페이지네이션과 정렬을 정확하게 하기 위해서는 기준이 되는 생성일자(CreatedAt) 가 필요합니다. 엔티티에 해당 컬럼이 존재하지 않기 때문에 추가했습니다.
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말