From 9702527786323fae73b09b68322fad0f9475485e Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Fri, 18 Jun 2021 14:48:07 +0200 Subject: [PATCH] #1548 - Delay request parameter variable lookup until necessary. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We pulled up the parameter name lookup for request parameters to avoid repeated lookups but unfortunately missed that the lookup will fail e.g. for @RequestParam Map<…> as we now strictly tried to find a request parameter name that doesn't actually exist in this case. --- .../hateoas/server/core/MethodParameters.java | 2 +- .../server/core/SpringAffordanceBuilder.java | 2 ++ .../hateoas/server/core/WebHandler.java | 25 ++++++++++++--- .../server/mvc/WebMvcLinkBuilderUnitTest.java | 31 +++++++++++++++++++ 4 files changed, 54 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/springframework/hateoas/server/core/MethodParameters.java b/src/main/java/org/springframework/hateoas/server/core/MethodParameters.java index f137a518..e2006730 100644 --- a/src/main/java/org/springframework/hateoas/server/core/MethodParameters.java +++ b/src/main/java/org/springframework/hateoas/server/core/MethodParameters.java @@ -42,7 +42,7 @@ import org.springframework.util.ConcurrentReferenceHashMap; */ public class MethodParameters { - private static final ParameterNameDiscoverer DISCOVERER = new DefaultParameterNameDiscoverer(); + private static ParameterNameDiscoverer DISCOVERER = new DefaultParameterNameDiscoverer(); private static final Map CACHE = new ConcurrentReferenceHashMap<>(); private final List parameters; diff --git a/src/main/java/org/springframework/hateoas/server/core/SpringAffordanceBuilder.java b/src/main/java/org/springframework/hateoas/server/core/SpringAffordanceBuilder.java index c310c065..87aaa082 100644 --- a/src/main/java/org/springframework/hateoas/server/core/SpringAffordanceBuilder.java +++ b/src/main/java/org/springframework/hateoas/server/core/SpringAffordanceBuilder.java @@ -18,6 +18,7 @@ package org.springframework.hateoas.server.core; import java.lang.reflect.Method; import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.function.Function; import java.util.stream.Collectors; @@ -96,6 +97,7 @@ public class SpringAffordanceBuilder { .orElse(ResolvableType.NONE); List queryMethodParameters = parameters.getParametersWith(RequestParam.class).stream() // + .filter(it -> !Map.class.isAssignableFrom(it.getParameterType())) .map(QueryParameter::of) // .collect(Collectors.toList()); diff --git a/src/main/java/org/springframework/hateoas/server/core/WebHandler.java b/src/main/java/org/springframework/hateoas/server/core/WebHandler.java index ff96e8fe..15d961c9 100644 --- a/src/main/java/org/springframework/hateoas/server/core/WebHandler.java +++ b/src/main/java/org/springframework/hateoas/server/core/WebHandler.java @@ -132,7 +132,10 @@ public class WebHandler { bindRequestParameters(builder, parameter, arguments, conversionService); - if (SKIP_VALUE.equals(parameter.getVerifiedValue(arguments))) { + boolean isSkipValue = SKIP_VALUE.equals(parameter.getVerifiedValue(arguments)); + boolean isMapParameter = Map.class.isAssignableFrom(parameter.parameter.getParameterType()); + + if (isSkipValue && !isMapParameter) { values.put(parameter.getVariableName(), SKIP_VALUE); @@ -186,7 +189,7 @@ public class WebHandler { return; } - String key = parameter.getVariableName(); + Class parameterType = parameter.parameter.getParameterType(); if (value instanceof MultiValueMap) { @@ -198,7 +201,11 @@ public class WebHandler { } } - } else if (value instanceof Map) { + return; + + } + + if (value instanceof Map) { Map requestParams = (Map) value; @@ -206,14 +213,22 @@ public class WebHandler { builder.queryParam(requestParamEntry.getKey(), encodeParameter(requestParamEntry.getValue())); } - } else if (value instanceof Collection) { + return; + } + + if (Map.class.isAssignableFrom(parameterType) && SKIP_VALUE.equals(value)) { + return; + } + + String key = parameter.getVariableName(); + + if (value instanceof Collection) { for (Object element : (Collection) value) { if (key != null) { builder.queryParam(key, encodeParameter(element)); } } - } else if (SKIP_VALUE.equals(value)) { if (parameter.isRequired()) { diff --git a/src/test/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderUnitTest.java b/src/test/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderUnitTest.java index 34cd8cef..19ae9d8e 100644 --- a/src/test/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderUnitTest.java +++ b/src/test/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderUnitTest.java @@ -20,8 +20,11 @@ import static org.springframework.hateoas.server.mvc.WebMvcLinkBuilder.*; import java.lang.reflect.Method; import java.util.Arrays; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.springframework.hateoas.IanaLinkRelations; @@ -29,8 +32,10 @@ import org.springframework.hateoas.Link; import org.springframework.hateoas.TemplateVariable; import org.springframework.hateoas.TemplateVariable.VariableType; import org.springframework.hateoas.TestUtils; +import org.springframework.hateoas.server.core.MethodParameters; import org.springframework.http.HttpEntity; import org.springframework.http.ResponseEntity; +import org.springframework.test.util.ReflectionTestUtils; import org.springframework.util.MultiValueMap; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -634,6 +639,27 @@ class WebMvcLinkBuilderUnitTest extends TestUtils { linkTo(methodOn(ControllerWithHandlerMethodParameterThatNeedsConversion.class).method(41L)).withSelfRel(); } + @Test // #1548 + void mapsRequestParamMap() { + + Object original = ReflectionTestUtils.getField(MethodParameters.class, "DISCOVERER"); + + try { + + ReflectionTestUtils.setField(MethodParameters.class, "DISCOVERER", null); + + Stream.of(null, new HashMap()).forEach(it -> { + + Link link = linkTo(methodOn(ControllerWithMethods.class).methodWithMapRequestParam(it)).withSelfRel(); + + assertThat(link.getHref()).endsWith("/with-map"); + }); + + } finally { + ReflectionTestUtils.setField(MethodParameters.class, "DISCOVERER", original); + } + } + private static UriComponents toComponents(Link link) { return UriComponentsBuilder.fromUriString(link.expand().getHref()).build(); } @@ -714,6 +740,11 @@ class WebMvcLinkBuilderUnitTest extends TestUtils { HttpEntity methodWithJdk8Optional(@RequestParam Optional value) { return null; } + + @RequestMapping(path = "/with-map") // #1548 + HttpEntity methodWithMapRequestParam(@RequestParam Map params) { + return null; + } } @RequestMapping("/parent")