From 1e13d40cbb6b0852ce5dc5ab07e74cdd1cbcf923 Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Tue, 26 Jan 2021 09:58:52 +0100 Subject: [PATCH] Avoid pre-computing base URI in PagedResourceAssemblerArgumentResolver. We now avoid the pre-computation of the base URI in PagedResourceAssemblerArgumentResolver as the actual assembler will fall back to using the URI of the current request by default anyway. The latter makes sure that request parameters, that are contained in the original requests appear in links created. If a controller wants to deviate from that behavior, they can create a dedicated link themselves and hand that to the assembler explicitly. Fixes GH-2173, GH-452. --- ...gedResourcesAssemblerArgumentResolver.java | 45 ++++++------------- ...ateoasAwareSpringDataWebConfiguration.java | 2 +- ...cesAssemblerArgumentResolverUnitTests.java | 35 +-------------- .../web/PagedResourcesAssemblerUnitTests.java | 12 +++++ .../data/web/WebTestUtils.java | 10 ++++- 5 files changed, 37 insertions(+), 67 deletions(-) diff --git a/src/main/java/org/springframework/data/web/PagedResourcesAssemblerArgumentResolver.java b/src/main/java/org/springframework/data/web/PagedResourcesAssemblerArgumentResolver.java index b8273e651..f4f2cc8ef 100644 --- a/src/main/java/org/springframework/data/web/PagedResourcesAssemblerArgumentResolver.java +++ b/src/main/java/org/springframework/data/web/PagedResourcesAssemblerArgumentResolver.java @@ -26,7 +26,6 @@ import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.core.MethodParameter; import org.springframework.core.log.LogMessage; import org.springframework.data.domain.Pageable; -import org.springframework.hateoas.Link; import org.springframework.hateoas.server.MethodLinkBuilderFactory; import org.springframework.hateoas.server.core.MethodParameters; import org.springframework.hateoas.server.mvc.WebMvcLinkBuilderFactory; @@ -35,8 +34,6 @@ import org.springframework.web.bind.support.WebDataBinderFactory; import org.springframework.web.context.request.NativeWebRequest; import org.springframework.web.method.support.HandlerMethodArgumentResolver; import org.springframework.web.method.support.ModelAndViewContainer; -import org.springframework.web.util.UriComponents; -import org.springframework.web.util.UriComponentsBuilder; /** * {@link HandlerMethodArgumentResolver} to allow injection of {@link PagedResourcesAssembler} into Spring MVC @@ -55,7 +52,6 @@ public class PagedResourcesAssemblerArgumentResolver implements HandlerMethodArg private static final String PARAMETER_AMBIGUITY = "Discovered multiple parameters of type Pageable but no qualifier annotations to disambiguate!"; private final HateoasPageableHandlerMethodArgumentResolver resolver; - private final MethodLinkBuilderFactory linkBuilderFactory; /** * Creates a new {@link PagedResourcesAssemblerArgumentResolver} using the given @@ -63,12 +59,23 @@ public class PagedResourcesAssemblerArgumentResolver implements HandlerMethodArg * * @param resolver can be {@literal null}. * @param linkBuilderFactory can be {@literal null}, will be defaulted to a {@link WebMvcLinkBuilderFactory}. + * @deprecated since 2.5, 2.4.4, 2.3.7, to be removed in 3.0 + * @use {@link #PagedResourcesAssemblerArgumentResolver(HateoasPageableHandlerMethodArgumentResolver)} instead. */ + @Deprecated public PagedResourcesAssemblerArgumentResolver(HateoasPageableHandlerMethodArgumentResolver resolver, @Nullable MethodLinkBuilderFactory linkBuilderFactory) { + this(resolver); + } + /** + * Creates a new {@link PagedResourcesAssemblerArgumentResolver} using the given + * {@link PageableHandlerMethodArgumentResolver}. + * + * @param resolver can be {@literal null}. + */ + public PagedResourcesAssemblerArgumentResolver(HateoasPageableHandlerMethodArgumentResolver resolver) { this.resolver = resolver; - this.linkBuilderFactory = linkBuilderFactory == null ? new WebMvcLinkBuilderFactory() : linkBuilderFactory; } /* @@ -89,36 +96,12 @@ public class PagedResourcesAssemblerArgumentResolver implements HandlerMethodArg public Object resolveArgument(MethodParameter parameter, @Nullable ModelAndViewContainer mavContainer, NativeWebRequest webRequest, @Nullable WebDataBinderFactory binderFactory) { - UriComponents fromUriString = resolveBaseUri(parameter); MethodParameter pageableParameter = findMatchingPageableParameter(parameter); if (pageableParameter != null) { - return new MethodParameterAwarePagedResourcesAssembler<>(pageableParameter, resolver, fromUriString); + return new MethodParameterAwarePagedResourcesAssembler<>(pageableParameter, resolver, null); } else { - return new PagedResourcesAssembler<>(resolver, fromUriString); - } - } - - /** - * Eagerly resolve a base URI for the given {@link MethodParameter} to be handed to the assembler. - * - * @param parameter must not be {@literal null}. - * @return the {@link UriComponents} representing the base URI, or {@literal null} if it can't be resolved eagerly. - */ - @Nullable - private UriComponents resolveBaseUri(MethodParameter parameter) { - - Method method = parameter.getMethod(); - - if (method == null) { - throw new IllegalArgumentException(String.format("Could not obtain method from parameter %s!", parameter)); - } - - try { - Link linkToMethod = linkBuilderFactory.linkTo(parameter.getDeclaringClass(), method).withSelfRel(); - return UriComponentsBuilder.fromUriString(linkToMethod.getHref()).build(); - } catch (IllegalArgumentException o_O) { - return null; + return new PagedResourcesAssembler<>(resolver, null); } } diff --git a/src/main/java/org/springframework/data/web/config/HateoasAwareSpringDataWebConfiguration.java b/src/main/java/org/springframework/data/web/config/HateoasAwareSpringDataWebConfiguration.java index bcfd97a65..253372a6a 100644 --- a/src/main/java/org/springframework/data/web/config/HateoasAwareSpringDataWebConfiguration.java +++ b/src/main/java/org/springframework/data/web/config/HateoasAwareSpringDataWebConfiguration.java @@ -99,7 +99,7 @@ public class HateoasAwareSpringDataWebConfiguration extends SpringDataWebConfigu @Bean public PagedResourcesAssemblerArgumentResolver pagedResourcesAssemblerArgumentResolver() { - return new PagedResourcesAssemblerArgumentResolver(pageableResolver.get(), null); + return new PagedResourcesAssemblerArgumentResolver(pageableResolver.get()); } /* diff --git a/src/test/java/org/springframework/data/web/PagedResourcesAssemblerArgumentResolverUnitTests.java b/src/test/java/org/springframework/data/web/PagedResourcesAssemblerArgumentResolverUnitTests.java index d3f078d67..2fde182a9 100755 --- a/src/test/java/org/springframework/data/web/PagedResourcesAssemblerArgumentResolverUnitTests.java +++ b/src/test/java/org/springframework/data/web/PagedResourcesAssemblerArgumentResolverUnitTests.java @@ -18,17 +18,13 @@ package org.springframework.data.web; import static org.assertj.core.api.Assertions.*; import java.lang.reflect.Method; -import java.util.Optional; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; - import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.core.MethodParameter; import org.springframework.data.domain.Pageable; -import org.springframework.test.util.ReflectionTestUtils; import org.springframework.web.bind.annotation.RequestMapping; -import org.springframework.web.util.UriComponents; /** * Unit tests for {@link PagedResourcesAssemblerArgumentResolver}. @@ -46,7 +42,7 @@ class PagedResourcesAssemblerArgumentResolverUnitTests { WebTestUtils.initWebTest(); HateoasPageableHandlerMethodArgumentResolver hateoasPageableHandlerMethodArgumentResolver = new HateoasPageableHandlerMethodArgumentResolver(); - this.resolver = new PagedResourcesAssemblerArgumentResolver(hateoasPageableHandlerMethodArgumentResolver, null); + this.resolver = new PagedResourcesAssemblerArgumentResolver(hateoasPageableHandlerMethodArgumentResolver); } @Test // DATACMNS-418 @@ -112,30 +108,6 @@ class PagedResourcesAssemblerArgumentResolverUnitTests { assertThat(result).isNotNull(); } - @Test // DATACMNS-513 - void detectsMappingOfInvokedSubType() throws Exception { - - Method method = Controller.class.getMethod("methodWithMapping", PagedResourcesAssembler.class); - - // Simulate HandlerMethod.HandlerMethodParameter.getDeclaringClass() - // as it's returning the invoked class as the declared one - MethodParameter methodParameter = new MethodParameter(method, 0) { - public java.lang.Class getDeclaringClass() { - return SubController.class; - } - }; - - Object result = resolver.resolveArgument(methodParameter, null, null, null); - - assertThat(result).isInstanceOf(PagedResourcesAssembler.class); - - Optional uriComponents = (Optional) ReflectionTestUtils.getField(result, "baseUri"); - - assertThat(uriComponents).hasValueSatisfying(it -> { - assertThat(it.getPath()).isEqualTo("/foo/mapping"); - }); - } - private void assertSelectsParameter(Method method, int expectedIndex) { MethodParameter parameter = new MethodParameter(method, 0); @@ -190,9 +162,4 @@ class PagedResourcesAssemblerArgumentResolverUnitTests { @RequestMapping("/mapping") Object methodWithMapping(PagedResourcesAssembler pageable); } - - @RequestMapping("/foo") - interface SubController extends Controller { - - } } diff --git a/src/test/java/org/springframework/data/web/PagedResourcesAssemblerUnitTests.java b/src/test/java/org/springframework/data/web/PagedResourcesAssemblerUnitTests.java index 7408142be..0fdf99cd3 100755 --- a/src/test/java/org/springframework/data/web/PagedResourcesAssemblerUnitTests.java +++ b/src/test/java/org/springframework/data/web/PagedResourcesAssemblerUnitTests.java @@ -38,6 +38,7 @@ import org.springframework.hateoas.PagedModel.PageMetadata; import org.springframework.hateoas.RepresentationModel; import org.springframework.hateoas.server.RepresentationModelAssembler; import org.springframework.hateoas.server.core.EmbeddedWrapper; +import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.web.util.UriComponents; import org.springframework.web.util.UriComponentsBuilder; @@ -257,6 +258,17 @@ class PagedResourcesAssemblerUnitTests { assertThat(resource.getRequiredLink(IanaLinkRelations.SELF).getHref()).endsWith("?page=0&size=1"); } + @Test // #2173 + void keepsRequestParametersOfOriginalRequestUri() { + + WebTestUtils.initWebTest(new MockHttpServletRequest("GET", "/sample?foo=bar")); + + PagedModel> model = assembler.toModel(createPage(1)); + + assertThat(model.getRequiredLink(IanaLinkRelations.FIRST).getHref()) + .isEqualTo("http://localhost/sample?foo=bar&page=0&size=1"); + } + private static Page createPage(int index) { Pageable request = PageRequest.of(index, 1); diff --git a/src/test/java/org/springframework/data/web/WebTestUtils.java b/src/test/java/org/springframework/data/web/WebTestUtils.java index 0fc03b5d9..42effbb74 100644 --- a/src/test/java/org/springframework/data/web/WebTestUtils.java +++ b/src/test/java/org/springframework/data/web/WebTestUtils.java @@ -15,6 +15,8 @@ */ package org.springframework.data.web; +import javax.servlet.http.HttpServletRequest; + import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockServletContext; import org.springframework.web.context.WebApplicationContext; @@ -33,8 +35,14 @@ public class WebTestUtils { * Initializes web tests. Will register a {@link MockHttpServletRequest} for the current thread. */ public static void initWebTest() { + initWebTest(new MockHttpServletRequest()); + } + + /** + * Initializes web tests for the given {@link HttpServletRequest} which will registered for the current thread. + */ + public static void initWebTest(HttpServletRequest request) { - MockHttpServletRequest request = new MockHttpServletRequest(); ServletRequestAttributes requestAttributes = new ServletRequestAttributes(request); RequestContextHolder.setRequestAttributes(requestAttributes); }