refactor: @PageableDefault 및 페이지 검증 로직을 ArgumentResolver로 개선 #247
Conversation
There was a problem hiding this comment.
추가)
코드 리뷰를 하다보니,
"스프링에서 기본적으로 제공하는 Pageable 기능을 구현하고 있는" 느낌을 받았어요.
마치 이미 System.out.println();라는 코드를 사용할 수 있음에도
void printNewLine() {
System.out.print("\n");
}이렇게 구현하는 듯한 느낌이요🥲
그래서 이미 만들어진 스프링 코드를 사용하는법이 없을까 찾아보니,
webConfig에 PageableHandlerMethodArgumentResolver 를 추가하는 방법이 있더라고요.
@Configuration
public class WebMvcConfig implements WebMvcConfigurer {
@Override
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) {
PageableHandlerMethodArgumentResolver pageResolver = new PageableHandlerMethodArgumentResolver();
pageResolver.setMaxPageSize(50);
pageResolver.setOneIndexedParameters(true);
pageResolver.setFallbackPageable(PageRequest.of(0, 10));
resolvers.addAll(List.of(pageResolver));
}
}이런식으로 작성한다면, 스프링에서 제공하는 코드를 재사용해서 코드량을 많이 줄일 수 있을 것 같아요!
이것과 별개로, 기존에 남겨둔 코드리뷰는 규혁님이 생각해보실만한 것들이라 남겨둡니다.
| public CustomPageRequest(int page, int size) { | ||
| validatePageParameters(page, size); | ||
| this.page = page; | ||
| this.size = size; | ||
| } | ||
|
|
||
| public CustomPageRequest(int page, int size, Sort sort) { | ||
| this(page, size); | ||
| this.sort = sort; | ||
| } |
There was a problem hiding this comment.
1/
일반적으로 생성자 체이닝(=this나 super를 사용하여 생성자 내부에서 다른 생성자를 호출하는 기법)에서는
모든 인자를 받는 '기본 생성자'가 기준이 됩니다.
지금처럼 CustomPageRequest(int page, int size, Sort sort)가
CustomPageRequest(int page, int size)를 호출하는게 아닌, CustomPageRequest(int page, int size)가
CustomPageRequest(int page, int size, Sort sort) 를 호출하도록 변경되면 좋을 것 같아요.
개발자들끼리 암묵적으로 정한 규칙들을 지키면, 다른 개발자들이 이해하기 더 쉬운 코드가 될것 같습니다!
ref. https://koonsland.tistory.com/270
2/
'기본 생성자'를 사용하도록 코드를 바꾸면,
CustomPageRequest(int page, int size) 생성자를 사용하는 곳은 없다는게 보일거예요.
안쓰는 생성자는 삭제하는게 좋겠습니다~
...main/java/com/example/solidconnection/custom/resolver/CustomPageRequestArgumentResolver.java
Outdated
Show resolved
Hide resolved
...main/java/com/example/solidconnection/custom/resolver/CustomPageRequestArgumentResolver.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/custom/request/CustomPageRequest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/example/solidconnection/custom/request/CustomPageRequest.java
Outdated
Show resolved
Hide resolved
오 이 방식도 좋은 거 같습니다! 그런데 이 방식은 |
범위 검증정도는 해도 좋겠네요! |
Walkthrough이 풀 리퀘스트는 여러 모듈에서 페이지네이션 로직을 리팩토링합니다. 다음은 주요 변경 사항입니다:
Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Dispatcher
participant Resolver
participant Controller
participant Service
Client->>Dispatcher: HTTP 요청과 페이지네이션 매개변수
Dispatcher->>Resolver: 사용자 정의 인자 해석 호출
Resolver-->>Dispatcher: 검증된 Pageable 객체
Dispatcher->>Controller: 페이지블을 엔드포인트 메서드에 전달
Controller->>Service: 페이지블로 서비스 메서드 호출
Service-->>Controller: 데이터 페이지 반환
Controller-->>Dispatcher: 데이터와 함께 HTTP 응답
Dispatcher-->>Client: 응답 전달
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/admin/controller/AdminScoreController.java (1)
34-44: Consider adding API documentation for pagination behavior.Now that pagination is handled by a custom resolver, it would be helpful to document the expected pagination behavior for API consumers, especially regarding:
- Default page size
- Default page number (zero-based or one-based)
- Maximum page size limit
- How invalid pagination parameters are handled
This could be added as JavaDoc comments or annotations if you're using a tool like Swagger/OpenAPI.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/com/example/solidconnection/admin/controller/AdminScoreController.java(2 hunks)src/main/java/com/example/solidconnection/config/web/WebMvcConfig.java(2 hunks)src/main/java/com/example/solidconnection/custom/exception/ErrorCode.java(2 hunks)src/main/java/com/example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolver.java(1 hunks)src/main/java/com/example/solidconnection/util/PagingUtils.java(0 hunks)src/test/java/com/example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolverTest.java(1 hunks)src/test/java/com/example/solidconnection/util/PagingUtilsTest.java(0 hunks)
💤 Files with no reviewable changes (2)
- src/test/java/com/example/solidconnection/util/PagingUtilsTest.java
- src/main/java/com/example/solidconnection/util/PagingUtils.java
🔇 Additional comments (28)
src/main/java/com/example/solidconnection/config/web/WebMvcConfig.java (3)
4-4: No concerns regarding the new import.
19-19: Field injection approach looks consistent with constructor injection.
25-26: Adding the custom pageable resolver is appropriate.src/test/java/com/example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolverTest.java (11)
1-24: New test class setup is clear and well-structured.
26-44: Good use of constants and setup procedure.
It’s beneficial to keep these constants here for quick reference, ensuring the test remains self-descriptive.
46-55: Test default pagination scenario thoroughly.
The coverage for missing parameters is good, ensuring fallback values are utilized.
57-72: Valid parameter test logic is comprehensive.
Properly verifies that the resolver reflects the provided page and size.
74-88: Non-numeric parameter handling is correct.
Using default values when invalid data is given prevents unexpected crashes.
89-99: Exception handling for zero-page scenarios is appropriate.
Covers boundary conditions effectively.
101-111: Negative page parameter coverage is well-validated.
Ensures robust handling of invalid inputs.
113-123: Ensures size below the minimum triggers an exception.
Properly aligns with business rules.
125-135: Maximum size limit test is correct.
Verifies that anything exceeding the limit raises an error.
137-147: Negative size parameter handled properly.
Excellent to confirm all lower-bound edge cases.
149-154: Controller method reference for testing is straightforward.
This lightweight test fixture effectively validates the resolver logic.src/main/java/com/example/solidconnection/custom/exception/ErrorCode.java (2)
8-10: Referencing constants aligns error messages with configurable limits.
This direct reference helps maintain consistency across the codebase.
103-104: Dynamically referencing min and max constraints in error messaging is beneficial.
This approach keeps the error messages accurate and up-to-date with the resolver logic.src/main/java/com/example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolver.java (8)
1-2: Package declaration and spacing look fine.
3-13: Imports are well-organized and relevant.
14-17: Static imports from ErrorCode maintain clarity.
It keeps error handling concise.
19-31: Constructor customization is correct.
Using one-indexed parameters with a fallback ensures consistent behavior across the app.
33-44: Overriding resolveArgument with additional validation is a clean approach.
Ensures the user sees consistent error messages when invalid parameters are passed.
45-54: Validation logic is straightforward and covers boundary checks.
Good job throwing a clear custom exception.
56-66: Graceful handling of non-integer parameters.
Falling back to defaults prevents unexpected errors.
67-68: Class ends properly with no extraneous lines.src/main/java/com/example/solidconnection/admin/controller/AdminScoreController.java (4)
39-42: Controller code is now cleaner and more focused.The removal of the pagination validation and adjustment logic (
PagingUtils.validatePageandPageRequest.of) from the controller simplifies the code and properly delegates pagination handling to the custom argument resolver. This is a good separation of concerns.
58-58: Consistent pagination approach across controller methods.The removal of
@PageableDefault(page = 1)annotation here maintains consistency with the changes made to thesearchGpaScoresmethod above. This ensures a unified approach to pagination throughout the controller.
60-63: Cleaner service method invocation.The simplified direct passing of the
pageableparameter to the service method makes the code more straightforward and easier to maintain. By delegating pagination handling to the argument resolver, the controller can focus on its core responsibility of routing requests.
37-37:Details
✅ Verification successful
Simplified method parameter by removing @PageableDefault.
The removal of
@PageableDefault(page = 1)annotation simplifies the controller parameter, which is good. This change aligns with the PR objective of moving pagination configuration to a centralized ArgumentResolver.Let's confirm that the pagination defaults are properly configured in the custom argument resolver:
🏁 Script executed:
#!/bin/bash # Check if a custom Pageable resolver is properly configured echo "Searching for CustomPageableHandlerMethodArgumentResolver configuration..." rg -A 10 "CustomPageableHandlerMethodArgumentResolver|WebMvcConfig" --type javaLength of output: 12314
Pagination configuration validated and simplified
The removal of the
@PageableDefault(page = 1)annotation is appropriate. Our custom argument resolver (CustomPageableHandlerMethodArgumentResolver), as configured inWebMvcConfig, now correctly provides the default pagination behavior (default page = 1, size = 10), as confirmed by the tests and configuration search.
- Verified that the custom resolver is properly registered in
src/main/java/com/example/solidconnection/config/web/WebMvcConfig.java.- Confirmed default settings and error handling via tests in
src/test/java/com/example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolverTest.java.Good job simplifying the controller parameters and centralizing pagination configuration!
There was a problem hiding this comment.
먼저.. 시간이 조금 지나고 코드를 보니 의견이 달라졌습니다요🥹
그 부분에 대해서 죄송하다는 말씀을 드립니다..
그래서 이번에 리뷰할 때는 정말 열심히 오래 생각해서 제안을 드려봅니다!
시간이 조금 지난 상태에서 코드를 보니 정책이 다소 어색하게 느껴지는 부분이 있는 것 같아요 🥲
지금은 유효하지 않은 입력에 대해서 두가지로 나누어 처리되고 있어요.
- 형식 자체가 유효하지 않는 값 (e.g. 없거나 문자열) -> 기본 값으로 대체
- 형식은 유효하나 범위는 올바르지 않는 값 (e.g. 음수거나 범위 초과) -> 예외 발생
즉, "page=-1"은 예외가 응답되는데, "page=하하"는 정상 응답되는 상태입니다.
차라리 모든 유효하지 않는 값에 대해서 기본값으로 대체하도록 통일하는건 어떨까요?
규혁님이 위에서 ‘클라이언트 입장에서 헷갈릴 것이다’라고 하신 내용을 고려해봐도
통일하는게 좋겠다는 생각을 했어요.
이건 우리끼리 합의하면 되는 내용인데, 합의는 명료할수록 좋잖아요!
다른 서비스들은 페이지 파라미터에 대해서 어떻게 처리하는지 궁금해서 실험을 해봤는데요🤔
- 네이버 카페 & 블로그
- 형식 or 범위 유효하지 않으면 기본 값으로 처리
- 다음 카페
- 형식 or 범위 유효하지 않으면 기본 값으로 처리
- 인프런
- 형식 유효하지 않으면 기본 값으로 처리
- 범위 유효하지 않으면
- 기본 페이지 크기인 10보다 작게 요청하면 그 값만큼 응답
- 최대 페이지 크기인 100보다 크게 요청하면 100만큼만 응답
- 백준
- 형식 or 범위 유효하지 않으면 404
- 에브리타임
- 형식 유효하지 않으면 기본값으로 처리
- 음수는 internal server error 발생
- 멜론
- 놀랍게도 페이지 크기를 1만으로 해도 1만개를 응답한다.. 서버 부하 안오나?
- 형식 유효하지 않으면 ‘페이지를 찾을 수 없습니다’ 응답됨
➡️ 표준이 있는게 아니라, 서비스마다 정책이 다른 것 같아요.
우리 서비스는 페이지에 대해서 민감한 사이트도 아니니,
"모든 유효하지 않은 요청에 대해서 기본 값으로 대체한다"는 단순한 정책을 적용해보는게 어떨지 제안드립니다!
그렇게 되면 테스트코드도 훨씬 깔끔해질 것 같아요.
|
너무 좋은 거 같습니다! 통일성도 있고 클라이언트 입장에서 오해도 없을 거 같네요! |
...com/example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolver.java
Outdated
Show resolved
Hide resolved
...com/example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolver.java
Outdated
Show resolved
Hide resolved
...example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolverTest.java
Outdated
Show resolved
Hide resolved
...example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolverTest.java
Outdated
Show resolved
Hide resolved
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
작동은 하니 approve! 하고 의견은 코멘트로 따로 남겨뒀습니다 ㅎㅎ
| @Override | ||
| public Pageable resolveArgument(MethodParameter methodParameter, | ||
| ModelAndViewContainer mavContainer, | ||
| NativeWebRequest webRequest, | ||
| WebDataBinderFactory binderFactory) { | ||
| return super.resolveArgument(methodParameter, mavContainer, webRequest, binderFactory); | ||
| } |
There was a problem hiding this comment.
부모 클래스의 함수를 그대로 호출하는 함수를 Override 할 필요는 없어보입니다. 이 override 함수는 없애는게 좋겠습니다😊
| assertThat(pageable.getPageSize()).isEqualTo(expectedSize); | ||
| } | ||
|
|
||
| static Stream<Arguments> provideInvalidParameters() { |
There was a problem hiding this comment.
(이 부분을 짚을까 고민이 되었지만..!
규혁님이 앞으로도 테스트 코드 리팩터링을 할 예정이니, 생각해볼만한 지점이 될것 같다 생각해서 말씀드립니다.
다만 이 부분은 제가 너무 과하게 생각하는걸수도 있어요..😓 아니다 싶으면 규혁님 의견대로 가도 됩니다!)
제 생각에 이 테스트 코드는 "페이지와 사이즈의 정책을 동시에 테스트하는 함수" 같아요.
테스트 코드는 작은 영역을 테스트할수록 좋아요.
그 목적이 명확해질 뿐 아니라, 변경에도 유연하게 대처할 수 있기 때문입니다.
예를 들어, '페이지는 기본 값으로 대체되지만, 사이즈는 예외를 던지게 한다' 처럼 정책이 수정된다면,
이 함수는 두개로 나뉘어야 할거예요.
그래서 저라면 '페이지_파라미터가_유효하지_않으면' 함수와 '사이즈_파라미터가_유효하지_않으면' 함수 둘로 나눌 것 같아요.
그렇게 되면 95~100 라인은 중복되는 테스트이니 없앨 수도 있고요!
그리고 동시에 두가지를 테스트하려고하니, 테스트 함수에 전해줘야 하는 인자도
(String testName, String pageParam, String sizeParam, int expectedPage, int expectedSize)
이렇게 다섯개나 되어 가독성이 떨어진다는 점도 생각해볼만 한 것 같아요~
cf. 만약에 "유효하지 않은 경우"를 두개로 분리하시기로 했다면, "유효한 경우"도 분리하는게 통일감있을 것 같습니다.
There was a problem hiding this comment.
저도 통일성이 있어야할 거 같다고 생각합니다!
사실 그래서 성공 케이스에서 페이지랑 사이즈를 동시에 확인하고 있으니 실패 케이스도 그렇게 하는 게 통일감이 있겠다 싶어서 그랬습니다. 😓 그러다보니 자연스레 인자도 늘어나버렸네요.
말씀해주신 대로 분리를 하는 게 나중에 정책이 바뀌었을 때 더 유연하게 대처할 수 있을 거 같다고 생각해요! 반영해놓겠습니다!
| Arguments.of("Size 0", "2", "0", 1, DEFAULT_SIZE), | ||
| Arguments.of("Size 음수", "2", "-1", 1, DEFAULT_SIZE), | ||
| Arguments.of("Size 문자열", "2", "invalid", 1, DEFAULT_SIZE), | ||
| Arguments.of("Size 최대값 초과", "2", String.valueOf(MAX_SIZE + 1), 1, MAX_SIZE), |
There was a problem hiding this comment.
93 라인은 특이 케이스이니 따로 분리해봐도 좋겠네요!
| private static final int DEFAULT_SIZE = 10; | ||
|
|
||
| public CustomPageableHandlerMethodArgumentResolver() { | ||
| setMaxPageSize(MAX_SIZE); |
There was a problem hiding this comment.
처음엔 이 설정을 유지한 채
if (request != null) {
validateParameters(request);
}
이 부분을 없애면 음수나 문자열 같은 잘못된 값들이 부모쪽에서 다 처리를 해줄거라 생각했는데 size를 초과하는 값을 넣으면 기본값인 10이 아니라 최대 size로 반환을 하더라구요. 그래서 이건 지우고 따로 검증하는 방식으로 수정하였습니다.
...com/example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolver.java
Outdated
Show resolved
Hide resolved
...example/solidconnection/custom/resolver/CustomPageableHandlerMethodArgumentResolverTest.java
Outdated
Show resolved
Hide resolved
| assertThat(pageable.getPageSize()).isEqualTo(expectedSize); | ||
| } | ||
|
|
||
| static Stream<Arguments> provideInvalidParameters() { |
There was a problem hiding this comment.
저도 통일성이 있어야할 거 같다고 생각합니다!
사실 그래서 성공 케이스에서 페이지랑 사이즈를 동시에 확인하고 있으니 실패 케이스도 그렇게 하는 게 통일감이 있겠다 싶어서 그랬습니다. 😓 그러다보니 자연스레 인자도 늘어나버렸네요.
말씀해주신 대로 분리를 하는 게 나중에 정책이 바뀌었을 때 더 유연하게 대처할 수 있을 거 같다고 생각해요! 반영해놓겠습니다!
nayonsoso
left a comment
nayonsoso
left a comment
There was a problem hiding this comment.
가독성이 많이 좋아졌네요! 😄😁
한번 더 approve 합니다~
2 participants
관련 이슈
작업 내용
페이지를 사용하는 곳 마다
위의 내용을 코드로 구현하였습니다.
특이 사항
리뷰 요구사항 (선택)
기존 컨벤션엔 Custom을 안붙이는 거로 확인했는데 Spring 자체에 PageRequest가 있어서 Custom을 붙였는데 다른 추천 명칭이 있다면 고치겠습니다!
최대 페이지 번호 제한은 필요가 없는 거 같아서 뺐는데 괜찮을까요? 추가로 최대 size를 초과하면 예외를 던지게 구현하였는데 초과 시 최대 사이즈로 조회되게하는 게 나을까요 지금처럼 예외를 던지는 게 나을까요?
테스트 코드 작성 시 CustomPageRequest이 Null일 수 있다고 IDE에서 경고를 주는데 테스트코드마다 assertThat(pageRequest).isNotNull();을 추가하는 게 좋을까요?