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

페이지네이션 개선 완료 #355

Merged
merged 19 commits into from
May 29, 2024
Merged

페이지네이션 개선 완료 #355

merged 19 commits into from
May 29, 2024

Conversation

mingmingmon
Copy link
Collaborator

Summary

#354

페이지네이션에 정렬 기준을 직접 커스텀 할 수 있습니다.

Tasks

  • 페이지네이션 정렬 기준 추가

ETC

Screenshot

image

@mingmingmon mingmingmon added the ✨ Feature 새로운 기능 명세 및 개발 label May 26, 2024
@mingmingmon mingmingmon self-assigned this May 26, 2024
@mingmingmon mingmingmon requested a review from limehee as a code owner May 26, 2024 07:47
@limehee
Copy link
Collaborator

limehee commented May 26, 2024

기존 작동하던 API를 모두 호환시키면서 확장한 점이 인상 깊고, 많은 고민의 흔적이 보입니다.
정렬 기준이 파라미터로 넘어오지 않았을 때 @RequestParam에서 defaultValue를 통해 지정하는 방법이 더 좋지 않을까하여 코멘트 남깁니다.
defaultValue를 사용하면 Optional로 기본값을 처리하는 과정도 줄어들고 코드 가독성도 더 향상될 것으로 여겨집니다.

기존

    public ApiResponse<PagedResponseDto<MemberResponseDto>> getMembersByConditions(
            @RequestParam(name = "id", required = false) String id,
            @RequestParam(name = "name", required = false) String name,
            @RequestParam(name = "page", defaultValue = "0") int page,
            @RequestParam(name = "size", defaultValue = "20") int size,
            @RequestParam(name = "sortBy", required = false) Optional<List<String>> sortBy,
            @RequestParam(name = "sortDirection", required = false) Optional<List<String>> sortDirection
    ) throws SortingArgumentException {
        List<String> sortByList = sortBy.orElse(List.of("createdAt"));
        List<String> sortDirectionList = sortDirection.orElse(List.of("desc"));
        Pageable pageable = PageableUtils.createPageable(page, size, sortByList, sortDirectionList, Member.class);
        PagedResponseDto<MemberResponseDto> members = memberService.getMembersByConditions(id, name, pageable);
        return ApiResponse.success(members);
    }

수정 예시

    public ApiResponse<PagedResponseDto<MemberResponseDto>> getMembersByConditions(
            @RequestParam(name = "id", required = false) String id,
            @RequestParam(name = "name", required = false) String name,
            @RequestParam(name = "page", defaultValue = "0") int page,
            @RequestParam(name = "size", defaultValue = "20") int size,
            @RequestParam(name = "sortBy", defaultValue = "createdAt") List<String> sortBy,
            @RequestParam(name = "sortDirection", defaultValue = "desc") List<String> sortDirection
    ) throws SortingArgumentException {
        Pageable pageable = PageableUtils.createPageable(page, size, sortBy, sortDirection, Member.class);
        PagedResponseDto<MemberResponseDto> members = memberService.getMembersByConditions(id, name, pageable);
        return ApiResponse.success(members);
    }

@limehee
Copy link
Collaborator

limehee commented May 26, 2024

사용자 정의 예외 SortingArgumentException 외에도, 개선된 페이지네이션 로직 내에서 발생할 수 있는 다음의 예외도 GlobalExceptionHandler에 추가할 필요가 있어보입니다.
노션 'CoreTeam > Docs > 서버 발생 예외 및 응답'에 추가된 예외에 대해 기록하고, 팀원 모두에게 변경사항 전달해주시면 되겠습니다.

org.hibernate.query.sqm.UnknownPathException
org.springframework.dao.InvalidDataAccessApiUsageException

@limehee
Copy link
Collaborator

limehee commented May 26, 2024

Accuse 도메인의 '신고 내역 조회' API는 accuse_target 테이블에 쿼리를 보내 요청을 처리하기 때문에 아래 코드를 다음과 같이 수정할 필요가 있어보입니다.

기존

Pageable pageable = PageableUtils.createPageable(page, size, sortByList, sortDirectionList, Accuse.class);

수정

Pageable pageable = PageableUtils.createPageable(page, size, sortByList, sortDirectionList, AccuseTarget.class);

추가로, 아래와 같이 요청을 보냈을 때 에러가 발생하고 있습니다. 확인 부탁드립니다.
Request: /api/v1/accuses?countOrder=false&page=0&size=20&sortBy=accuseCount&sortDirection=desc
Error: Resolved [page.clab.api.global.exception.NotFoundException: accuseCount라는 칼럼이 존재하지 않습니다.]

Request: /api/v1/accuses?countOrder=false&page=0&size=20&sortBy=accuse_count&sortDirection=desc
Error: Resolved [page.clab.api.global.exception.NotFoundException: accuse_count라는 칼럼이 존재하지 않습니다.]

@limehee
Copy link
Collaborator

limehee commented May 26, 2024

테이블에서 칼럼을 찾지 못했을 때 page.clab.api.global.exception.NotFoundException이 발생하고 있습니다. 오류가 발생하였을 때 클라이언트에서 구분 가능하도록 별도의 예외를 정의해서 사용하는게 좋아보입니다.

@limehee
Copy link
Collaborator

limehee commented May 26, 2024

description에 정렬가능한 칼럼도 적어주면 클라이언트에서 API 요청할 때 훨씬 수월할 것 같네요.

@mingmingmon
Copy link
Collaborator Author

피드백 감사합니다! 전부 적용했습니다!

@limehee limehee linked an issue May 29, 2024 that may be closed by this pull request
26 tasks
@limehee
Copy link
Collaborator

limehee commented May 29, 2024

적용한 내용 모두 확인했습니다. 피드백 수용해주셔서 감사합니다.

@limehee limehee merged commit 2c9bfb9 into develop May 29, 2024
1 check failed
@limehee limehee deleted the feat/#354 branch May 29, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feature 새로운 기능 명세 및 개발
Projects
None yet
Development

Successfully merging this pull request may close these issues.

페이지네이션 개선
2 participants