From cc4a4de892314ed656a5329625905c0e756ca2d7 Mon Sep 17 00:00:00 2001 From: Oliver Gierke Date: Wed, 9 Oct 2013 02:19:09 +0900 Subject: [PATCH] DATACMNS-408 - Handle invalid pagination and sorting parameters gracefully. The PageableHandlerMethodArgumentResolver and SortHandlerMethodArgumentResolver now transparently ignore invalid values provided for page number, size and sort. We fall back to ignore the invalid values (for sort) and fall back to the defaults where appropriate (page number and size). Used and adapted test cases from pull request #48. --- ...PageableHandlerMethodArgumentResolver.java | 27 ++++++-- .../SortHandlerMethodArgumentResolver.java | 8 ++- ...andlerMethodArgumentResolverUnitTests.java | 43 +++++++++++- ...andlerMethodArgumentResolverUnitTests.java | 66 +++++++++++++++++++ 4 files changed, 136 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/springframework/data/web/PageableHandlerMethodArgumentResolver.java b/src/main/java/org/springframework/data/web/PageableHandlerMethodArgumentResolver.java index 8e16515d0..890ddcecf 100644 --- a/src/main/java/org/springframework/data/web/PageableHandlerMethodArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/PageableHandlerMethodArgumentResolver.java @@ -234,10 +234,10 @@ public class PageableHandlerMethodArgumentResolver implements HandlerMethodArgum String pageString = webRequest.getParameter(getParameterNameToUse(pageParameterName, methodParameter)); String pageSizeString = webRequest.getParameter(getParameterNameToUse(sizeParameterName, methodParameter)); - int page = StringUtils.hasText(pageString) ? Integer.parseInt(pageString) - (oneIndexedParameters ? 1 : 0) - : defaultOrFallback.getPageNumber(); - int pageSize = StringUtils.hasText(pageSizeString) ? Integer.parseInt(pageSizeString) : defaultOrFallback - .getPageSize(); + int page = StringUtils.hasText(pageString) ? parseAndApplyBoundaries(pageString, 0, Integer.MAX_VALUE) + - (oneIndexedParameters ? 1 : 0) : defaultOrFallback.getPageNumber(); + int pageSize = StringUtils.hasText(pageSizeString) ? parseAndApplyBoundaries(pageSizeString, 0, maxPageSize) + : defaultOrFallback.getPageSize(); // Limit lower bound pageSize = pageSize < 1 ? defaultOrFallback.getPageSize() : pageSize; @@ -301,4 +301,23 @@ public class PageableHandlerMethodArgumentResolver implements HandlerMethodArgum return new PageRequest(defaultPageNumber, defaultPageSize, defaults.direction(), defaults.sort()); } + + /** + * Tries to parse the given {@link String} into an integer and applies the given boundaries. Will return the lower + * boundary if the {@link String} cannot be parsed. + * + * @param parameter + * @param lower + * @param upper + * @return + */ + private static int parseAndApplyBoundaries(String parameter, int lower, int upper) { + + try { + int parsed = Integer.parseInt(parameter); + return parsed < lower ? lower : parsed > upper ? upper : parsed; + } catch (NumberFormatException e) { + return lower; + } + } } diff --git a/src/main/java/org/springframework/data/web/SortHandlerMethodArgumentResolver.java b/src/main/java/org/springframework/data/web/SortHandlerMethodArgumentResolver.java index 228a60736..e9361e83a 100644 --- a/src/main/java/org/springframework/data/web/SortHandlerMethodArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/SortHandlerMethodArgumentResolver.java @@ -251,7 +251,13 @@ public class SortHandlerMethodArgumentResolver implements HandlerMethodArgumentR continue; } - allOrders.add(new Order(direction, elements[i])); + String property = elements[i]; + + if (!StringUtils.hasText(property)) { + continue; + } + + allOrders.add(new Order(direction, property)); } } diff --git a/src/test/java/org/springframework/data/web/PageableHandlerMethodArgumentResolverUnitTests.java b/src/test/java/org/springframework/data/web/PageableHandlerMethodArgumentResolverUnitTests.java index 4b14801f2..0e27ed6b6 100644 --- a/src/test/java/org/springframework/data/web/PageableHandlerMethodArgumentResolverUnitTests.java +++ b/src/test/java/org/springframework/data/web/PageableHandlerMethodArgumentResolverUnitTests.java @@ -15,6 +15,8 @@ */ package org.springframework.data.web; +import static org.springframework.data.web.PageableHandlerMethodArgumentResolver.*; + import org.junit.Before; import org.junit.Test; import org.springframework.beans.factory.annotation.Qualifier; @@ -97,8 +99,7 @@ public class PageableHandlerMethodArgumentResolverUnitTests extends PageableDefa request.addParameter("page", "0"); request.addParameter("size", "0"); - assertSupportedAndResult(supportedMethodParameter, PageableHandlerMethodArgumentResolver.DEFAULT_PAGE_REQUEST, - request); + assertSupportedAndResult(supportedMethodParameter, DEFAULT_PAGE_REQUEST, request); } /** @@ -112,7 +113,43 @@ public class PageableHandlerMethodArgumentResolverUnitTests extends PageableDefa exception.expect(IllegalStateException.class); exception.expectMessage("invalidDefaultPageSize"); - assertSupportedAndResult(parameter, PageableHandlerMethodArgumentResolver.DEFAULT_PAGE_REQUEST); + assertSupportedAndResult(parameter, DEFAULT_PAGE_REQUEST); + } + + /** + * @see DATACMNS-408 + */ + @Test + public void fallsBackToFirstPageIfNegativePageNumberIsGiven() throws Exception { + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("page", "-1"); + + assertSupportedAndResult(supportedMethodParameter, DEFAULT_PAGE_REQUEST, request); + } + + /** + * @see DATACMNS-408 + */ + @Test + public void pageParamIsNotNumeric() throws Exception { + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("page", "a"); + + assertSupportedAndResult(supportedMethodParameter, DEFAULT_PAGE_REQUEST, request); + } + + /** + * @see DATACMNS-408 + */ + @Test + public void sizeParamIsNotNumeric() throws Exception { + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("size", "a"); + + assertSupportedAndResult(supportedMethodParameter, DEFAULT_PAGE_REQUEST, request); } @Override diff --git a/src/test/java/org/springframework/data/web/SortHandlerMethodArgumentResolverUnitTests.java b/src/test/java/org/springframework/data/web/SortHandlerMethodArgumentResolverUnitTests.java index ed0a80ea9..0d5dab5e7 100644 --- a/src/test/java/org/springframework/data/web/SortHandlerMethodArgumentResolverUnitTests.java +++ b/src/test/java/org/springframework/data/web/SortHandlerMethodArgumentResolverUnitTests.java @@ -17,7 +17,11 @@ package org.springframework.data.web; import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.*; +import static org.springframework.data.domain.Sort.Direction.*; +import javax.servlet.http.HttpServletRequest; + +import org.junit.BeforeClass; import org.junit.Test; import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.core.MethodParameter; @@ -40,6 +44,13 @@ import org.springframework.web.context.request.ServletWebRequest; */ public class SortHandlerMethodArgumentResolverUnitTests extends SortDefaultUnitTests { + static MethodParameter PARAMETER; + + @BeforeClass + public static void setUp() throws Exception { + PARAMETER = new MethodParameter(Controller.class.getMethod("supportedMethod", Sort.class), 0); + } + /** * @see DATACMNS-351 */ @@ -126,6 +137,61 @@ public class SortHandlerMethodArgumentResolverUnitTests extends SortDefaultUnitT assertThat(result, is(new Sort(Direction.ASC, "firstname", "lastname"))); } + /** + * @see DATACMNS-408 + */ + @Test + public void parsesEmptySortToNull() throws Exception { + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("sort", ""); + + assertThat(resolveSort(request, PARAMETER), is(nullValue())); + } + + /** + * @see DATACMNS-408 + */ + @Test + public void sortParamIsInvalidProperty() throws Exception { + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("sort", ",DESC"); + + assertThat(resolveSort(request, PARAMETER), is(nullValue())); + } + + /** + * @see DATACMNS-408 + */ + @Test + public void sortParamIsInvalidPropertyWhenMultiProperty() throws Exception { + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("sort", "property1,,DESC"); + + assertThat(resolveSort(request, PARAMETER), is(new Sort(DESC, "property1"))); + } + + /** + * @see DATACMNS-408 + */ + @Test + public void sortParamIsEmptyWhenMultiParams() throws Exception { + + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("sort", "property,DESC"); + request.addParameter("sort", ""); + + assertThat(resolveSort(request, PARAMETER), is(new Sort(DESC, "property"))); + } + + private static Sort resolveSort(HttpServletRequest request, MethodParameter parameter) throws Exception { + + SortHandlerMethodArgumentResolver resolver = new SortHandlerMethodArgumentResolver(); + return resolver.resolveArgument(parameter, null, new ServletWebRequest(request), null); + } + private static void assertSupportedAndResolvedTo(NativeWebRequest request, MethodParameter parameter, Sort sort) { SortHandlerMethodArgumentResolver resolver = new SortHandlerMethodArgumentResolver();