Skip to content
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: 내가 받은 리뷰 목록에 페이지네이션 적용 #683

Merged
merged 41 commits into from
Sep 26, 2024

Conversation

nayonsoso
Copy link
Contributor

@nayonsoso nayonsoso commented Sep 24, 2024

🚧이 PR 은 아루의 세션 머지 후, 세션 부분을 반영한 뒤에야 머지할 수 있습니다.🚧


🚀 어떤 기능을 구현했나요 ?

  • 내가 받은 리뷰 목록 조회에 페이지네이션을 적용했습니다.

🔥 어떻게 해결했나요 ?

  • 먼저 RestDocs 를 작성해서 API 의 뼈대를 완성하고
  • 레포지토리에 페이지네이션 함수를 만들고 테스트코드를 작성한 후
  • 서비스코드에서 그 함수를 호출하도록 하고 테스트코드를 작성했습니다.

📝 어떤 부분에 집중해서 리뷰해야 할까요?

혼란을 방지하기 위해서 설명을 드리자면, 제가 커밋이나 코드에 사용한페이지네이션 이라는 용어는 데이터를 나누어 전달하는 기술 자체를 의미합니다. 따라서 "무한 스크롤 & 커서 방식인데 왜 페이지를 적용했냐?!" 라는 오해는 없으시기 바랍니다!

📚 참고 자료, 할 말

참고로 제가 처음에 헷갈려서
❗️아루가 만든 session 브랜치에서 작업을 하다가, PR 직전에 브랜치가 잘못되었다는 것을 깨달아 체리픽을 했었읍니다.. ㅠㅠ❗️
따라서 커밋을 따라가며 Session 같은 부분이 보인다면 이러한 이유때문임을 말씀드립니다.
마지막에 체리픽 충돌을 해결하며 우선 이 PR 에서는 세션 관련 부분을 지웠으니 안심하세요!🙋🏻‍♀️

Copy link

github-actions bot commented Sep 24, 2024

Test Results

102 tests  +12   102 ✅ +12   4s ⏱️ -1s
 36 suites + 2     0 💤 ± 0 
 36 files   + 2     0 ❌ ± 0 

Results for commit 7954cc4. ± Comparison against base commit 731f537.

This pull request removes 1 and adds 13 tests. Note that renamed tests count towards both.
reviewme.api.ReviewApiTest ‑ 세션으로_자신이_받은_리뷰_목록을_조회한다()
reviewme.api.ReviewApiTest ‑ 자신이_받은_리뷰_목록을_조회한다()
reviewme.review.repository.ReviewRepositoryTest$리뷰그룹_아이디에_해당하는_리뷰를_생성일_기준_내림차순으로_페이징하여_불러온다 ‑ 마지막_리뷰_아이디가_주어지지_않으면_가장_최신순으로_리뷰를_반환한다()
reviewme.review.repository.ReviewRepositoryTest$리뷰그룹_아이디에_해당하는_리뷰를_생성일_기준_내림차순으로_페이징하여_불러온다 ‑ 마지막_리뷰_아이디를_기준으로_그보다_전에_적힌_리뷰를_반환한다()
reviewme.review.repository.ReviewRepositoryTest$리뷰그룹_아이디에_해당하는_리뷰를_생성일_기준_내림차순으로_페이징하여_불러온다 ‑ 마지막으로_온_리뷰_전에_작성된_리뷰가_없으면_빈_리스트를_반환한다()
reviewme.review.repository.ReviewRepositoryTest$리뷰그룹_아이디에_해당하는_리뷰를_생성일_기준_내림차순으로_페이징하여_불러온다 ‑ 페이징_크기보다_적은_수의_리뷰가_등록되었으면_그_크기만큼의_리뷰만_반환한다()
reviewme.review.repository.ReviewRepositoryTest$리뷰그룹_아이디에_해당하는_리뷰를_생성일_기준_내림차순으로_페이징하여_불러온다 ‑ 페이징_크기보다_큰_수의_리뷰가_등록되었으면_페이징_크기만큼의_리뷰를_반환한다()
reviewme.review.service.PageSizeTest ‑ [1] size=0
reviewme.review.service.PageSizeTest ‑ [2] size=-1
reviewme.review.service.PageSizeTest ‑ [3] size=51
reviewme.review.service.PageSizeTest ‑ null이_들어오면_기본값으로_설정한다()
…

♻️ This comment has been updated with latest results.

@donghoony donghoony changed the title [BE] 내가 받은 리뷰 목록에 페이지네이션 적용 [BE] feat: 내가 받은 리뷰 목록에 페이지네이션 적용 Sep 24, 2024
Copy link
Contributor

@skylar1220 skylar1220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

산쵸쵸 수고했어요!! 몇 개 질문 남겼어요~
산페최(해석은 산쵸맘)📖

Comment on lines 58 to 59
public ReceivedReviewsResponseWithPagination getReceivedReviewsWithPagination(String reviewRequestCode,
long lastReviewId, int limit) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

질문) 페이지의 게시물 수를 나타내는 인자를 컨트롤러에서는 size로 서비스부터는 limit로 표현하고 있는데 용어를 다르게 한 이유가 있을까요?

Copy link
Contributor Author

@nayonsoso nayonsoso Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 부분 잘 짚어주셨네요🙇🏻‍♀️
둘 다 의미상으로는 '몇개의 리뷰를 한번에 보여줄 것인가' 를 의미하긴 하지만, 사용되는 곳에 따라서 다른 이름을 붙여줬어요.

쿼리 파라미터에서는 페이징의 개념과 잘어울리는 size 라는 이름을 사용했고,
sql 에서는 limit 라는 키워드에 사용되기 때문에 변수명도 limit 로 사용했어요.

일단 제 의도는 이러한데, 커비처럼 헷갈릴 수 있으니 아래처럼 바꿔볼게요!

  • 컨트롤러 ~ 서비스 : 변수명으로 size 사용
  • 레포지토리 ~ 네이티브 쿼리 : 변수명으로 limit 사용

Comment on lines 63 to 64
List<ReviewListElementResponse> elements
= reviewListMapper.mapToReviewListWithPagination(reviewGroup, lastReviewId, limit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elements보다 더 의미가 담긴 변수명은 어떨까요?
reviewListElements도 길지만 괜찮다고 생각해요!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영했습니다~

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elementsvalue와 비슷한 맥락이라고 생각해요. reviewListResponse, responses로 명명을 바꾸면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깜짝 제안 :
previewPreviewResponse 는 어떠신지요!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.adoc의 리뷰 목록, 단건 조회 문서명이 서로 바뀌었는데 아루 세션 pr에서 수정되었네요!
저는 보면서 헷갈렸어서 여기도 남겨는 놓을게요~

FieldDescriptor[] responseFieldDescriptors = {
fieldWithPath("revieweeName").description("리뷰이 이름"),
fieldWithPath("projectName").description("프로젝트 이름"),
fieldWithPath("totalSize").description("받은 리뷰 전채 개수"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

작은 오타 수정입니다:) 전채 → 전체

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

fieldWithPath("revieweeName").description("리뷰이 이름"),
fieldWithPath("projectName").description("프로젝트 이름"),
fieldWithPath("totalSize").description("받은 리뷰 전채 개수"),
fieldWithPath("lastReviewId").description("페이지의 마지막 리뷰 아이디"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*리뷰 아이디 → 리뷰 ID

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🫡

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰_요청_코드가_존재하지_않는_경우_예외가_발생한다 이 테스트를 이번 페이지네이션 적용한 메서드를 검증하도록 추가하면 좋겠습니다~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

적용했습니다👍

Comment on lines 139 to 145
// then
assertThat(response.reviews())
.hasSize(2)
.extracting("reviewId")
.containsExactly(review3.getId(), review2.getId());
assertThat(response.totalSize())
.isEqualTo(3);
Copy link
Contributor

@skylar1220 skylar1220 Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newLastReviewId을 구하는 것도 메서드 내에 구현된 로직이니까 이것에 대한 확인을 추가하는 건 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

놓친 부분 잘 캐치해주셨네요🥹
추가했습니다~

@@ -59,4 +59,83 @@ class ReviewRepositoryTest {
// then
assertThat(actual).containsExactly(review2, review1);
}

@Nested
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

중복 데이터 삽입을 안해도 되는 것 좋네요👍👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하하~ 신경 좀 썼습니다 Nested 아름답죵? ㅎㅎ

@GetMapping("/v3/reviews")
public ResponseEntity<ReceivedReviewsResponseWithPagination> findReceivedReviewsWithPagination(
@RequestParam(required = false) Long lastReviewId,
@RequestParam(defaultValue = "5") int size,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

페이지당 게시글 수를 프론트에서 보내면 그걸 쓰고 아니면 우리가 정한 기본 5개를 쓰는 것인데,
이 게시글 수를 쿼리 파라미터를 통해 열어놓는 것 그리고 기본 사이즈를 컨트롤러에서 관리하는 것이 맞나? 하는 고민이 들었어요🤔

  1. 웬만하면 우리가 정한 default size를 쓸 것인데 왜 열어둬야할까?
    • 평소엔 실제로 받아서 쓰지는 않는 &size= 이런식의 쿼리 파라미터를 계속 받아야하는 게 불필요하게 느껴짐
  2. 만약 size가 바뀌는 경우 서비스 코드의 변경을 최소화하기 위해서라면?
    • 그렇다면 프론트에서 &size=변경된size크기를 매번 보내줘야하는데 결국엔 서비스 코드를 바꿔야하지 않나?
  3. 패이지네이션의 크기가 컨트롤러에 있다는 것이 정책적인 규약이 컨트롤러에 있는 것처럼 느껴짐
    • 만약 변경이 생긴다고 할 때 관련 서비스가 아닌 컨트롤러를 먼저 열어봐야겠다고 생각이 들까? (이건 제가 페이지네이션을 잘 몰라서 페이지네이션 크기 변경 = 컨트롤러 국룰! 이런게 있다면 알려주세요ㅎㅎ)

자체 반박)
하지만 이런 장점이 있을 수 있으려나요..!

만약 프론트에서 5개 너무 많아서 4개로 줄인다는 요구사항이 들어왔을 때,
당분간은 프론트에서 size 쿼리 파라미터로 4개 보내주고, 백엔드가 defaultValue를 4로 고친 후 프론트에서는 다시 size 쿼리 파라미터를 빈값으로 보냄. 이런 변경사항 반영에 백엔드가 시간을 벌 수 있다?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 책임에 대한 고민! 좋은 것 같아요👑

패이지네이션의 크기가 컨트롤러에 있다는 것이 정책적인 규약이 컨트롤러에 있는 것처럼 느껴짐

이건 관점의 차이일 수 있겠네요.
저는 페이지네이션의 크기를 정책이라 보지 않고, '프론트가 조절할 수 있는 변수'로 봤어요.
그렇기 때문이 쿼리 파리미터로 관리를 해야 한다 생각했습니다.
더 유연한 API 를 제공할 수 있다는 점을 중요하게 봤던 것 같아요.

이 부분은 정말로 관점의 차이이니, 다른 분들의 의견도 들어보고 싶어요!
@donghoony @Kimprodp

Copy link
Contributor Author

@nayonsoso nayonsoso Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로, 커비가 질문주셔서 저도 장단점에 대해 더 고민해보게 되었는데요!
10000개가 달린 리뷰방에 대해서 악의적인 사용자가 ?size=10000 로 한다면 서버 부하가 올 수 있을 거에요.

따라서 아루 & 테드가 프론트에서 받는 방법에 동의한다면,
size 크기를 100으로 제한하는 등의 로직도 추가해보겠습니다
e.g. size = Math.min(size, 100)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래에 코멘트 달겠습니다!

Copy link
Contributor

@donghoony donghoony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

산초 페이지이잉~ 수고했습니다. 몇 가지 객체지향적인 관점을 챙기려고 해보았어요. 확인해주세요~!

public List<ReviewListElementResponse> mapToReviewListWithPagination(ReviewGroup reviewGroup,
long lastReviewId, int limit) {
List<OptionItem> categoryOptionIds = optionItemRepository.findAllByOptionType(OptionType.CATEGORY);
return reviewRepository.findByReviewGroupIdWithPagination(reviewGroup.getId(), lastReviewId, limit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pagination은 구체적인 구현 방식 중 하나라고 느껴지는데, findByReviewGroupIdWithLimit은 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool~ 좋아요 이게 훨씬 좋네요

= reviewListMapper.mapToReviewListWithPagination(reviewGroup, lastReviewId, limit);
int totalSize = reviewRepository.countByReviewGroupId(reviewGroup.getId());
long newLastReviewId = calculateLastReviewId(elements);
return new ReceivedReviewsResponseWithPagination(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PagedReceivedReviewsResponse는 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이것도 반영했습니다!

Comment on lines 63 to 64
List<ReviewListElementResponse> elements
= reviewListMapper.mapToReviewListWithPagination(reviewGroup, lastReviewId, limit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elementsvalue와 비슷한 맥락이라고 생각해요. reviewListResponse, responses로 명명을 바꾸면 어떨까요?

Comment on lines 66 to 68
if (lastReviewId == null) {
lastReviewId = Long.MAX_VALUE;
}
Copy link
Contributor

@donghoony donghoony Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if/else 분기를 컨트롤러에서 가져가지 말고, 서비스 내부로 챙겨가면 어떨까요? 혹은 저 size를 감싼 값 객체를 만들어 null 들어오는 생성자일 때 어떻게 할지, 더 큰 값이 들어오면 줄여줄 지를 하나의 책임으로 볼 수 있고요

  • new PageSize(size)로 만들고, sizenull인 경우 기본값을 가지게, 더 큰값이 들어오면 바꾸게 (큰 값을 작게 만드는 것은 서비스가 해야 하는 일수도 있겠습니다, 도메인이 아니라요)
  • Service에서 PageSize를 받도록
  • 컨트롤러에서 분기처리하지 않도록
    수정하면 좋겠네요!

@GetMapping("/v3/reviews")
public ResponseEntity<ReceivedReviewsResponseWithPagination> findReceivedReviewsWithPagination(
@RequestParam(required = false) Long lastReviewId,
@RequestParam(defaultValue = "5") int size,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래에 코멘트 달겠습니다!

- 페이지네이션 구현을 구분하기 위해 지었던 이름을 일반적인 이름으로 변경
- 페이지네이션 구현을 구분하기 위해 지었던 이름을 일반적인 이름으로 변경
@nayonsoso
Copy link
Contributor Author

nayonsoso commented Sep 26, 2024

이얍!!!!! 세션 적용한 2페이즈 리뷰 부탁드립니다!!!

추가된 커밋만 모아보기

image

Copy link
Contributor

@donghoony donghoony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

간단한 수정사항이라 어프룹합니다만 내용은 확인해주세요~

Comment on lines 39 to 44
@RequestParam(required = false) Long lastReviewId,
@RequestParam(required = false) Integer size,
@SessionAttribute("reviewRequestCode") String reviewRequestCode
) {
ReceivedReviewsResponse response = reviewListLookupService.getReceivedReviews(reviewRequestCode);
ReceivedReviewsResponse response = reviewListLookupService.getReceivedReviews(
reviewRequestCode, lastReviewId, size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

파라미터 순서 맞춰주세요! 컨트롤러가 바꾸면 될 듯!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼아루 고마웡~

그 그런데, findReceivedReviewDetail 에서 리리코가 뒤에 선언되어있어서 여기에서도 맞추고 싶어요!
그래서 서비스 코드의 인자 순서를 바꿔줬습니당~

Comment on lines 24 to 25
public ReceivedReviewsResponse getReceivedReviews(String reviewRequestCode,
Long lastReviewId, Integer size) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불필요한 개행 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼아루 고마웡~ 🙆🏻‍♀️

Comment on lines +40 to +45
private long calculateLastReviewId(List<ReviewListElementResponse> elements) {
if (elements.isEmpty()) {
return 0;
}
return elements.get(elements.size() - 1).reviewId();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

책임 가질 객체 나올 것 같은데 나중에 고민해볼게요

@@ -5,6 +5,8 @@
public record ReceivedReviewsResponse(
String revieweeName,
String projectName,
int totalSize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totalSize 말고 reviewCount와 같은 건 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 반영하려면 프론트와 상의해야 해요

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요청-응답간에 크기를 나타내는 용어로 'size'가 사용되고 있어서 이것도 size 로 끝나면 좋겠어요🥹
totalSize가 명시적이지 않다면 totalReviewSize 정도는 어떠신가요?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size도 count도 지금 보면 괜찮은 듯 하기도 하고... total이 들어갈 이유는 아직 모르겠긴 해요!

@chysis
Copy link
Contributor

chysis commented Sep 26, 2024

고생했어요~~

- 컨트롤러 함수의 인자와 순서 통일을 위해 변경해주었음
Copy link
Contributor

@skylar1220 skylar1220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

산쵸쵸쵸 완~~전 사소한 것들이 대부분인 리체리체!

Comment on lines 12 to 17
void 유효한_값이_들어오면_그_값을_설정한다() {
int size = 10;

PageSize pageSize = new PageSize(size);

assertEquals(size, pageSize.getSize());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given, when, then 추가를 소소하게 요청합니다~

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영완!


private final int size;

PageSize(Integer size) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 default로 해도 되겠곤요👍


@Test
void 유효한_값이_들어오면_그_값을_설정한다() {
int size = 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아주 쬐~~~그만 수정 요청)
경계값인 50으로 설정하는 건 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영완!

Comment on lines 8 to 9
private static final int DEFAULT_SIZE = 5;
private static final int MAX_SIZE = 50;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 두가지가 도메인의 특성보다는 정책적인 특성이라고 생각해서(=변경 가능성 있음) 서비스로 올리고 주입받는 건 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

설득완!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

디폴트가 5인 이유는 무엇인가요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 ReviewListLookupService 에서 리뷰 목록 조회만의 기능을 가지고 있는데 따로 클래스로 분리한 이유가 있을까요?? 안에서 해도 될 것 같다는 생각..

@@ -61,7 +62,7 @@ class ReviewListLookupServiceTest {

@Test
void 리뷰_요청_코드가_존재하지_않는_경우_예외가_발생한다() {
assertThatThrownBy(() -> reviewListLookupService.getReceivedReviews("abc"))
assertThatThrownBy(() -> reviewListLookupService.getReceivedReviews("abc", 1L, 1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아주 쬐~~~그만 제안)
이 클래스에서는 lastReviewId가 모두 테스트에 영향을 주지 않아야하는 경우 같아요!
lastReviewId가 테스트에 영향을 주지 않아야하는 경우 = integer.MAX_VALUE로 통일해서
lastReviewId가 영향을 주나? 하는 생각해야하는 리소스를 줄여보는 건 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영완!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5초컷 given,when,then 추가 요청😋

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영완!

Copy link
Contributor

@skylar1220 skylar1220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

산초 믿어~~~ 쑤쑤랑 친하게 지내~~~

Copy link
Contributor

@Kimprodp Kimprodp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totalSize 관련 리뷰 썼었는데 바뀌면서 없어졌군요..
페이지네이션 적용 👍

코멘트 확인 부탁해요~~

PageSize pageSize = new PageSize(size);
List<ReviewListElementResponse> reviewListResponse
= reviewListMapper.mapToReviewList(reviewGroup, lastId.getId(), pageSize.getSize());
int totalSize = reviewRepository.countByReviewGroupId(reviewGroup.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totalSize를 먼저 확인해서 0일경우 별다른 로직을 타지 않고 반환하는건 어때요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 지금은 outdated 인 것 같아요
이번 배포에서 totalSize를 아예 내려주지 않도록 이야기되었습니다..🥲
image

Comment on lines 8 to 9
private static final int DEFAULT_SIZE = 5;
private static final int MAX_SIZE = 50;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

디폴트가 5인 이유는 무엇인가요?

ORDER BY r.created_at DESC
LIMIT :limit
""", nativeQuery = true)
List<Review> findByReviewGroupIdWithLimit(long reviewGroupId, long lastReviewId, int limit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AND (:lastReviewId IS NULL OR r.id < :lastReviewId)

이렇게 하면 LastReviewId 클래스 필요 없을 것 같아요

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오! 적용해볼게요!

@@ -17,15 +18,27 @@ public class ReviewListLookupService {

private final ReviewGroupRepository reviewGroupRepository;
private final ReviewListMapper reviewListMapper;
private final ReviewRepository reviewRepository;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용하지 않는 것 같슴다

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

반영했습니다🫡

Comment on lines 8 to 9
private static final int DEFAULT_SIZE = 5;
private static final int MAX_SIZE = 50;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 ReviewListLookupService 에서 리뷰 목록 조회만의 기능을 가지고 있는데 따로 클래스로 분리한 이유가 있을까요?? 안에서 해도 될 것 같다는 생각..

List<OptionItem> categoryOptionIds = optionItemRepository.findAllByOptionType(OptionType.CATEGORY);
return reviewRepository.findAllByGroupId(reviewGroup.getId())
return reviewRepository.findByReviewGroupIdWithLimit(reviewGroup.getId(), lastReviewId, size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~WithLimit 보다는 페이지네이션을 명시해서 확실하게 사용처를 알 수 있게 해도 좋을 것 같습니다

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repository 입장에서 Pagination이라는 단어가 너무 추상적이지 않을까요? 몇 개를 내려줄 것이라는 걸 강조하기 위해 제가 제안했어요 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 pagination을 사용하는 경우가 있고 없고, 이렇다면 pagination이 명시되는게 맞다고 생각하지만!
지금은 pagination을 사용하는게 default이고 유일한 경우니 굳이 명시하지 않아도 된다고 생각해요!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이하동문입니다~.~

Optional<Review> findByIdAndReviewGroupId(long reviewId, long reviewGroupId);

int countByReviewGroupId(long id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용하지 않습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

삭제했습니다!

Copy link
Contributor

@Kimprodp Kimprodp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빠른 반영 좋네요 페이지네이션 얼른 사용해보죠~~(❁´◡`❁)

@nayonsoso nayonsoso merged commit 1f1a7da into develop Sep 26, 2024
2 checks passed
@skylar1220
Copy link
Contributor

쑤쑤짱

@donghoony donghoony deleted the be/feature/679-pagenation branch September 26, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
5 participants