feat(webMvcConfig): Added custom argument resolver for PageableHandle… #8031
Conversation
…rMethodArgumentResolver and new ErrorMessage
WalkthroughThe pull request introduces enhancements to pagination and error handling in the GreenCity project. The changes focus on customizing the Changes
Sequence DiagramsequenceDiagram
participant Client
participant WebMvcConfig
participant CustomResolver
participant Controller
Client->>CustomResolver: Request with pagination params
CustomResolver->>CustomResolver: Validate size parameter
alt Invalid size
CustomResolver-->>Client: Throw IllegalArgumentException
else Valid size
CustomResolver->>Controller: Resolve pageable argument
Controller->>Client: Process request
end
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
core/src/main/java/greencity/config/WebMvcConfig.java (3)
66-66: Consider keeping the method name in singular formThe method returns a single
LocaleResolverinstance, so the singular formlocaleResolver()would be more appropriate thanlocaleResolvers().- public LocaleResolver localeResolvers() { + public LocaleResolver localeResolver() {
104-104: Document the reason for removing existing resolversConsider adding a comment explaining why the existing
PageableHandlerMethodArgumentResolverinstances are being removed to help future maintainers understand the rationale.+ // Remove default PageableHandlerMethodArgumentResolver to apply custom implementation resolvers.removeIf(resolver -> resolver instanceof PageableHandlerMethodArgumentResolver);
115-116: Extract magic numbers as constantsConsider extracting the default values for page and size as named constants to improve maintainability and documentation.
+ private static final int DEFAULT_PAGE = 0; + private static final int DEFAULT_SIZE = 20; + private static final int MAX_SIZE = 100; // Add appropriate maximum size @Override public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) { // ... existing code ... - int page = 0; - int size = 20; + int page = DEFAULT_PAGE; + int size = DEFAULT_SIZE;service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
230-231: Enhance error messages with specific constraintsConsider making the error messages more specific by including the actual constraints:
- public static final String INVALID_SIZE_VALUE_EXCEPTION = "Invalid value for size: must be an integer"; - public static final String NEGATIVE_SIZE_VALUE_EXCEPTION = "Size must be a positive"; + public static final String INVALID_SIZE_VALUE_EXCEPTION = "Invalid value for size: must be a positive integer"; + public static final String NEGATIVE_SIZE_VALUE_EXCEPTION = "Size must be greater than 0 and not exceed " + MAX_SIZE;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/greencity/config/WebMvcConfig.java(4 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| try { | ||
| if (pageParam != null) { | ||
| page = Integer.parseInt(pageParam); | ||
| } | ||
| if (sizeParam != null) { | ||
| if (!sizeParam.matches("\\d+")) { | ||
| throw new NumberFormatException(ErrorMessage.NEGATIVE_SIZE_VALUE_EXCEPTION); | ||
| } | ||
| size = Integer.parseInt(sizeParam); | ||
| } | ||
| } catch (NumberFormatException ex) { | ||
| throw new IllegalArgumentException(ErrorMessage.INVALID_SIZE_VALUE_EXCEPTION); | ||
| } |
There was a problem hiding this comment.
Enhance input validation for pagination parameters
The current implementation has several validation gaps:
- No validation for negative page numbers
- No upper bound for size parameter
- Incorrect error message (NEGATIVE_SIZE_VALUE_EXCEPTION) when size is not a digit
- Missing validation to prevent resource exhaustion
Consider applying this enhanced validation:
try {
if (pageParam != null) {
page = Integer.parseInt(pageParam);
+ if (page < 0) {
+ throw new IllegalArgumentException("Page index must not be less than zero");
+ }
}
if (sizeParam != null) {
if (!sizeParam.matches("\\d+")) {
- throw new NumberFormatException(ErrorMessage.NEGATIVE_SIZE_VALUE_EXCEPTION);
+ throw new NumberFormatException(ErrorMessage.INVALID_SIZE_VALUE_EXCEPTION);
}
size = Integer.parseInt(sizeParam);
+ if (size <= 0) {
+ throw new IllegalArgumentException(ErrorMessage.NEGATIVE_SIZE_VALUE_EXCEPTION);
+ }
+ if (size > MAX_SIZE) {
+ throw new IllegalArgumentException("Page size must not be greater than " + MAX_SIZE);
+ }
}
} catch (NumberFormatException ex) {
throw new IllegalArgumentException(ErrorMessage.INVALID_SIZE_VALUE_EXCEPTION);
}Committable suggestion skipped: line range outside the PR's diff.
|
Please cover the new code with tests |
| if (sizeParam != null) { | ||
| if (!sizeParam.matches("\\d+")) { | ||
| throw new NumberFormatException(ErrorMessage.NEGATIVE_SIZE_VALUE_EXCEPTION); | ||
| } | ||
| size = Integer.parseInt(sizeParam); | ||
| } |
There was a problem hiding this comment.
you have code duplication
please move it to the method
There was a problem hiding this comment.
Okay, I'm gonna put it in a separate method.
| size = Integer.parseInt(sizeParam); | ||
| } | ||
| } catch (NumberFormatException ex) { | ||
| throw new IllegalArgumentException(ErrorMessage.INVALID_SIZE_VALUE_EXCEPTION); |
There was a problem hiding this comment.
we can throw this exception if an invalid page as well, not only an invalid size
There was a problem hiding this comment.
I wonder if we really need to throw an exception here?
It is better to use default values
There was a problem hiding this comment.
This is the core of the task: if the size field is entered incorrectly, adjust the setting to the default value and return a response about the incorrectly entered field
| @@ -96,6 +106,37 @@ public void addInterceptors(InterceptorRegistry registry) { | |||
|
|
|||
| @Override | |||
| public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) { | |||
| resolvers.removeIf(resolver -> resolver instanceof PageableHandlerMethodArgumentResolver); | |||
There was a problem hiding this comment.
why do you remove PageableHandlerMethodArgumentResolvers here?
There was a problem hiding this comment.
In this way, I exclude the default implementation and overwrite it with my own, but maybe there is another way to do it?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
core/src/main/java/greencity/config/CustomPageableHandlerMethodArgumentResolver.java (2)
18-18: Add class-level JavaDoc documentation.Consider adding JavaDoc to describe the purpose and behavior of this custom resolver. This will help other developers understand when and how to use this class.
+/** + * Custom implementation of PageableHandlerMethodArgumentResolver that adds + * validation for page size and provides specific error messages for invalid inputs. + */ public class CustomPageableHandlerMethodArgumentResolver extends PageableHandlerMethodArgumentResolver {
19-31: Document override behavior and consider sort parameters.Two suggestions to improve this implementation:
- Add method documentation to explain how this differs from the parent implementation.
- Consider handling sort parameters by calling the parent implementation.
@Override + /** + * Resolves pagination parameters with custom validation. + * @throws BadRequestException if page size exceeds maximum allowed + * @throws IllegalArgumentException if page or size parameters are invalid + */ public Pageable resolveArgument(MethodParameter methodParameter, ModelAndViewContainer mavContainer, NativeWebRequest webRequest, WebDataBinderFactory binderFactory) { + // Get sort parameters from parent implementation + Pageable pageable = super.resolveArgument(methodParameter, mavContainer, webRequest, binderFactory); int page = parseParameter(webRequest.getParameter(PAGE), DEFAULT_PAGE, PAGE); int size = parseParameter(webRequest.getParameter(SIZE), DEFAULT_PAGE_SIZE, SIZE); if (size > MAX_PAGE_SIZE) { throw new BadRequestException(ErrorMessage.MAX_PAGE_SIZE_EXCEPTION); } - return PageRequest.of(page, size); + return PageRequest.of(page, size, pageable.getSort()); }core/src/test/java/greencity/config/CustomPageableHandlerMethodArgumentResolverTest.java (7)
23-34: Consider enhancing test class documentation and setup.The test class structure is well-organized, but could benefit from:
- Adding class-level JavaDoc to describe the test suite's purpose
- Using constructor injection instead of field initialization for the resolver
Here's a suggested improvement:
@ExtendWith(MockitoExtension.class) +/** + * Test suite for CustomPageableHandlerMethodArgumentResolver. + * Validates pagination parameter handling and error scenarios. + */ public class CustomPageableHandlerMethodArgumentResolverTest { - private CustomPageableHandlerMethodArgumentResolver resolver; + private final CustomPageableHandlerMethodArgumentResolver resolver; @Mock private NativeWebRequest webRequest; + public CustomPageableHandlerMethodArgumentResolverTest() { + this.resolver = new CustomPageableHandlerMethodArgumentResolver(); + } + @BeforeEach void setUp() { MockitoAnnotations.openMocks(this); - resolver = new CustomPageableHandlerMethodArgumentResolver(); }
36-44: Enhance default parameters test coverage.The test effectively validates default behavior, but could be more comprehensive by:
- Using a more descriptive test name
- Adding individual assertions for page and size values
Here's a suggested improvement:
@Test -void shouldReturnDefaultPageableWhenNoParametersProvidedTest() { +void shouldCreatePageableWithDefaultValuesWhenParametersAreNull() { when(webRequest.getParameter("page")).thenReturn(null); when(webRequest.getParameter("size")).thenReturn(null); Pageable pageable = resolver.resolveArgument(null, null, webRequest, null); assertEquals(PageRequest.of(DEFAULT_PAGE, DEFAULT_PAGE_SIZE), pageable); + assertEquals(DEFAULT_PAGE, pageable.getPageNumber()); + assertEquals(DEFAULT_PAGE_SIZE, pageable.getPageSize()); }
46-54: Expand test coverage for valid parameters.The test effectively validates basic functionality, but consider:
- Using constants (PAGE, SIZE) for parameter names
- Adding edge case tests for maximum valid values
Here's a suggested improvement:
@Test void shouldReturnCustomPageableWhenValidParametersProvidedTest() { - when(webRequest.getParameter("page")).thenReturn("2"); - when(webRequest.getParameter("size")).thenReturn("10"); + when(webRequest.getParameter(PAGE)).thenReturn("2"); + when(webRequest.getParameter(SIZE)).thenReturn("10"); Pageable pageable = resolver.resolveArgument(null, null, webRequest, null); assertEquals(PageRequest.of(2, 10), pageable); } + +@Test +void shouldAcceptMaximumValidValues() { + when(webRequest.getParameter(PAGE)).thenReturn(Integer.toString(Integer.MAX_VALUE)); + when(webRequest.getParameter(SIZE)).thenReturn("50"); // Assuming 50 is max allowed size + + Pageable pageable = resolver.resolveArgument(null, null, webRequest, null); + + assertEquals(Integer.MAX_VALUE, pageable.getPageNumber()); + assertEquals(50, pageable.getPageSize()); +}
56-75: Consider using parameterized tests for negative values.The tests effectively validate negative value handling, but could be more comprehensive using:
- JUnit's
@ParameterizedTestfor testing multiple negative values- Boundary value testing
Here's a suggested improvement:
+import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + -@Test -void shouldThrowExceptionWhenPageIsNegativeTest() { +@ParameterizedTest +@ValueSource(strings = {"-1", "-100", "-2147483648"}) +void shouldThrowExceptionWhenPageIsNegative(String pageValue) { - when(webRequest.getParameter(PAGE)).thenReturn("-1"); + when(webRequest.getParameter(PAGE)).thenReturn(pageValue); IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> resolver.resolveArgument(null, null, webRequest, null)); assertEquals(ErrorMessage.NEGATIVE_PAGE_VALUE_EXCEPTION, exception.getMessage()); } -@Test -void shouldThrowExceptionWhenSizeIsNegativeTest() { +@ParameterizedTest +@ValueSource(strings = {"-1", "-50", "-2147483648"}) +void shouldThrowExceptionWhenSizeIsNegative(String sizeValue) { when(webRequest.getParameter(PAGE)).thenReturn("1"); - when(webRequest.getParameter(SIZE)).thenReturn("-10"); + when(webRequest.getParameter(SIZE)).thenReturn(sizeValue); IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> resolver.resolveArgument(null, null, webRequest, null));
77-86: Enhance maximum size limit testing.The test effectively validates size limit enforcement, but consider:
- Using a constant for the maximum size value
- Testing boundary values (MAX_PAGE_SIZE + 1)
Here's a suggested improvement:
+private static final int MAX_PAGE_SIZE = 100; // Move to PageableConstants + @Test void shouldThrowExceptionWhenSizeExceedsMaxLimitTest() { when(webRequest.getParameter(PAGE)).thenReturn("2"); - when(webRequest.getParameter(SIZE)).thenReturn("200"); + when(webRequest.getParameter(SIZE)).thenReturn(String.valueOf(MAX_PAGE_SIZE + 1)); BadRequestException exception = assertThrows(BadRequestException.class, () -> resolver.resolveArgument(null, null, webRequest, null)); assertEquals(ErrorMessage.MAX_PAGE_SIZE_EXCEPTION, exception.getMessage()); } + +@Test +void shouldAcceptMaximumPageSize() { + when(webRequest.getParameter(PAGE)).thenReturn("0"); + when(webRequest.getParameter(SIZE)).thenReturn(String.valueOf(MAX_PAGE_SIZE)); + + Pageable pageable = resolver.resolveArgument(null, null, webRequest, null); + + assertEquals(MAX_PAGE_SIZE, pageable.getPageSize()); +}
88-107: Expand invalid input test coverage.The tests effectively validate non-numeric inputs, but could be enhanced by:
- Using parameterized tests for various invalid inputs
- Testing edge cases like empty strings and special characters
Here's a suggested improvement:
-@Test -void shouldThrowExceptionWhenPageIsInvalidTest() { +@ParameterizedTest +@ValueSource(strings = {"abc", "", " ", "12.34", "1e5", "#@!"}) +void shouldThrowExceptionWhenPageIsInvalid(String pageValue) { - when(webRequest.getParameter("page")).thenReturn("abc"); + when(webRequest.getParameter(PAGE)).thenReturn(pageValue); IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> resolver.resolveArgument(null, null, webRequest, null)); assertEquals(ErrorMessage.INVALID_PAGE_VALUE_EXCEPTION, exception.getMessage()); } -@Test -void shouldThrowExceptionWhenSizeInvalidTest() { +@ParameterizedTest +@ValueSource(strings = {"abc", "", " ", "12.34", "1e5", "#@!"}) +void shouldThrowExceptionWhenSizeIsInvalid(String sizeValue) { when(webRequest.getParameter(PAGE)).thenReturn("2"); - when(webRequest.getParameter(SIZE)).thenReturn("abc"); + when(webRequest.getParameter(SIZE)).thenReturn(sizeValue); IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> resolver.resolveArgument(null, null, webRequest, null));
23-109: Consider adding tests for edge cases.The test suite is comprehensive, but consider adding tests for:
- Null handling for the webRequest parameter
- Non-null MethodParameter scenarios
Here's a suggested addition:
@Test void shouldThrowExceptionWhenWebRequestIsNull() { assertThrows(IllegalArgumentException.class, () -> resolver.resolveArgument(null, null, null, null)); } @Test void shouldHandleNonNullMethodParameter() { MethodParameter methodParameter = mock(MethodParameter.class); when(webRequest.getParameter(PAGE)).thenReturn("0"); when(webRequest.getParameter(SIZE)).thenReturn("10"); Pageable pageable = resolver.resolveArgument(methodParameter, null, webRequest, null); assertEquals(PageRequest.of(0, 10), pageable); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/greencity/config/CustomPageableHandlerMethodArgumentResolver.java(1 hunks)core/src/main/java/greencity/config/WebMvcConfig.java(2 hunks)core/src/main/java/greencity/constant/PageableConstants.java(1 hunks)core/src/test/java/greencity/config/CustomPageableHandlerMethodArgumentResolverTest.java(1 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/greencity/constant/PageableConstants.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/greencity/config/WebMvcConfig.java
- service-api/src/main/java/greencity/constant/ErrorMessage.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
core/src/main/java/greencity/config/CustomPageableHandlerMethodArgumentResolver.java (2)
33-53: Document helper method and add test coverage.The helper method looks good but could use some improvements:
- Add method documentation
- Consider creating unit tests as requested in PR comments
+ /** + * Parses and validates pagination parameters. + * @param paramValue the string value to parse + * @param defaultValue the default value if paramValue is null + * @param param the parameter name (PAGE or SIZE) + * @return the parsed integer value + * @throws IllegalArgumentException if value is negative or not a number + */ private int parseParameter(String paramValue, int defaultValue, String param) {Let's check if tests exist for this class:
✅ Verification successful
Documentation addition is sufficient, test coverage is comprehensive
The class already has excellent test coverage through
CustomPageableHandlerMethodArgumentResolverTest.java. The tests thoroughly verify all scenarios including:
- Default parameter handling
- Valid parameter parsing
- Negative value validation
- Invalid format handling
- Size limit validation
Just add the suggested documentation to improve code maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files related to CustomPageableHandlerMethodArgumentResolver fd -e java -e kt "CustomPageableHandlerMethodArgumentResolver.*Test"Length of output: 159
Script:
#!/bin/bash # Check content of the test file cat core/src/test/java/greencity/config/CustomPageableHandlerMethodArgumentResolverTest.javaLength of output: 4289
18-54: Verify thread safety of the implementation.The implementation looks solid overall. Since this resolver will be used in a web environment with concurrent requests, ensure that it's thread-safe. The current implementation appears to be stateless, which is good for thread safety, but it would be valuable to document this characteristic.
Consider adding a note in the class documentation:
/** * Custom implementation of PageableHandlerMethodArgumentResolver that adds * validation for page size and provides specific error messages for invalid inputs. + * This implementation is stateless and thread-safe. */✅ Verification successful
Implementation confirmed thread-safe ✅
The implementation is inherently thread-safe due to its stateless nature. All variables are method-scoped, operations are pure computations, and it extends Spring's thread-safe
PageableHandlerMethodArgumentResolver. No additional thread safety measures are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check for any instance variables or static fields in the class and parent class references ast-grep --pattern 'class $_ { $$$private $_$$$}' # Look for ErrorMessage class definition ast-grep --pattern 'class ErrorMessage { $$$ }' # Find any usage of synchronized keyword or concurrent utilities rg -i "synchronized|concurrent|atomic|volatile" -g "*.java"Length of output: 849
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/greencity/constant/PageableConstants.java (1)
7-11: Constants are well-defined with reasonable default values.The pagination constants follow best practices:
- Zero-based pagination with
DEFAULT_PAGE = 0- Reasonable default page size of 20 items
- Clear parameter name constants
- Sensible maximum page size limit of 100 to prevent excessive resource usage
Consider adding Javadoc comments to document the purpose and usage of these constants, especially for the maximum page size limit rationale.
@UtilityClass public class PageableConstants { + /** + * Default page number for pagination (0-based indexing). + */ public static final Integer DEFAULT_PAGE = 0; + /** + * Default number of items per page. + */ public static final Integer DEFAULT_PAGE_SIZE = 20; + /** + * Request parameter name for page number. + */ public static final String PAGE = "page"; + /** + * Request parameter name for page size. + */ public static final String SIZE = "size"; + /** + * Maximum allowed page size to prevent excessive resource usage. + */ public static final Integer MAX_PAGE_SIZE = 100; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/greencity/constant/PageableConstants.java(1 hunks)core/src/test/java/greencity/config/CustomPageableHandlerMethodArgumentResolverTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/greencity/config/CustomPageableHandlerMethodArgumentResolverTest.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
core/src/main/java/greencity/constant/PageableConstants.java (2)
5-6: Well-structured utility class with appropriate annotation!Good use of
@UtilityClassfrom Lombok to enforce utility class constraints and reduce boilerplate code.
1-12: Verify the integration with CustomPageableHandlerMethodArgumentResolver.The constants look good, but let's verify they're properly utilized in the custom resolver.
✅ Verification successful
Integration with CustomPageableHandlerMethodArgumentResolver is properly implemented
The resolver correctly utilizes all constants with proper validation:
- Applies default values (DEFAULT_PAGE, DEFAULT_PAGE_SIZE) when parameters are missing
- Enforces MAX_PAGE_SIZE limit
- Uses PAGE and SIZE constants consistently for parameter handling
- Includes comprehensive error handling for invalid inputs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of PageableConstants in the custom resolver # Expected: Find references to these constants in the resolver implementation # Search for usage of the constants rg -l "PageableConstants\." core/src/main/java/greencity/config/Length of output: 151
Script:
#!/bin/bash # Get the complete implementation of CustomPageableHandlerMethodArgumentResolver ast-grep --pattern 'class CustomPageableHandlerMethodArgumentResolver { $$$ }' core/src/main/java/greencity/config/CustomPageableHandlerMethodArgumentResolver.java # Also get direct references to PageableConstants rg "PageableConstants\." -A 2 core/src/main/java/greencity/config/CustomPageableHandlerMethodArgumentResolver.javaLength of output: 705
Script:
#!/bin/bash # Get the method implementations to verify constant usage rg -U "public class CustomPageableHandlerMethodArgumentResolver.*?\n}$" -A 1000 core/src/main/java/greencity/config/CustomPageableHandlerMethodArgumentResolver.javaLength of output: 166
Script:
#!/bin/bash # Get the file content with simpler pattern rg -A 50 "^public class CustomPageableHandlerMethodArgumentResolver" core/src/main/java/greencity/config/CustomPageableHandlerMethodArgumentResolver.java # Also search for specific usage of each constant rg "DEFAULT_PAGE|DEFAULT_PAGE_SIZE|MAX_PAGE_SIZE|PAGE|SIZE" core/src/main/java/greencity/config/CustomPageableHandlerMethodArgumentResolver.javaLength of output: 2797
| int size = parseParameter(webRequest.getParameter(SIZE), DEFAULT_PAGE_SIZE, SIZE); | ||
|
|
||
| if (size > MAX_PAGE_SIZE) { | ||
| throw new BadRequestException(ErrorMessage.MAX_PAGE_SIZE_EXCEPTION); |
There was a problem hiding this comment.
it is better to use the default page size instead of throwing Exception. But it depends on the requirements. If you have to throw an exception it is ok
There was a problem hiding this comment.
It will not be very clear to the user if he form a size value 10000 and is returned a value of 20
| return PageRequest.of(page, size); | ||
| } | ||
|
|
||
| private int parseParameter(String paramValue, int defaultValue, String param) { |
There was a problem hiding this comment.
can be optimized
private int parseParameter(NativeWebRequest webRequest, String param, int defaultValue) {
String paramValue = webRequest.getParameter(param);
if (paramValue == null) {
return defaultValue;
}
try {
int value = Integer.parseInt(paramValue);
if (value < 0) {
throw new IllegalArgumentException(String.format(NEGATIVE_VALUE_EXCEPTION, param));
}
return value;
} catch (NumberFormatException e) {
throw new IllegalArgumentException(String.format(INVALID_VALUE_EXCEPTION, param), e);
}
}
public static final String INVALID_VALUE_EXCEPTION = "Invalid value for %s: must be an integer";
public static final String NEGATIVE_VALUE_EXCEPTION = "%s must be a positive number";
And usage of this method
@Override
public Pageable resolveArgument(MethodParameter methodParameter,
ModelAndViewContainer mavContainer,
NativeWebRequest webRequest,
WebDataBinderFactory binderFactory) {
int page = parseParameter(webRequest, PAGE, DEFAULT_PAGE);
int size = parseParameter(webRequest, SIZE, DEFAULT_PAGE_SIZE);
if (size > MAX_PAGE_SIZE) {
throw new BadRequestException(ErrorMessage.MAX_PAGE_SIZE_EXCEPTION);
}
return PageRequest.of(page, size);
}
|
Cr1stal423
deleted the
bugfix/6389/invalid-response-when-size-is-incorrect
branch
😎 GreenCity PR😎
Issue Link:
6389
Changes:
Add custom argument resolver for PageableHandlerMethodArgumentResolver
Add new error messages
Previous Condition:
Current Condition:
Summary by CodeRabbit
New Features
Bug Fixes
Tests