From f891f3be6dc0882fc8a1b12604e6bff74a22e1b1 Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Fri, 27 Aug 2021 17:41:47 +0200 Subject: [PATCH] #1598 - Fully prepare handler method values in link creation. We now eagerly convert all request parameters and path variables into their String representation using the MVC ConversionService to make sure we can use all formatting metadata available on the parameters. --- .../hateoas/server/core/WebHandler.java | 110 +++++++++++++----- .../server/mvc/WebMvcLinkBuilderUnitTest.java | 54 ++++++++- 2 files changed, 132 insertions(+), 32 deletions(-) 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 beb14285..ed9e66d6 100644 --- a/src/main/java/org/springframework/hateoas/server/core/WebHandler.java +++ b/src/main/java/org/springframework/hateoas/server/core/WebHandler.java @@ -21,16 +21,8 @@ import static org.springframework.web.util.UriComponents.UriTemplateVariables.*; import java.lang.annotation.Annotation; import java.lang.reflect.Method; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.Iterator; -import java.util.List; -import java.util.Map; +import java.util.*; import java.util.Map.Entry; -import java.util.Optional; import java.util.concurrent.ConcurrentHashMap; import java.util.function.BiFunction; import java.util.function.Function; @@ -66,6 +58,8 @@ import org.springframework.web.util.UriTemplate; */ public class WebHandler { + private static final TypeDescriptor STRING_DESCRIPTOR = TypeDescriptor.valueOf(String.class); + public interface LinkBuilderCreator { T createBuilder(UriComponents components, TemplateVariables variables, List affordances); } @@ -114,19 +108,27 @@ public class WebHandler { Iterator classMappingParameters = invocations.getObjectParameters(); while (classMappingParameters.hasNext()) { + String name = names.next(); TemplateVariable variable = TemplateVariable.segment(name); - values.put(name, variable.prepareAndEncode(classMappingParameters.next())); + Object source = classMappingParameters.next(); + + values.put(name, variable.prepareAndEncode( + HandlerMethodParameter.prepareValue(source, conversionService, TypeDescriptor.forObject(source)))); } Method method = invocation.getMethod(); HandlerMethodParameters parameters = HandlerMethodParameters.of(method); Object[] arguments = invocation.getArguments(); - ConversionService resolved = conversionService; for (HandlerMethodParameter parameter : parameters.getParameterAnnotatedWith(PathVariable.class, arguments)) { + TemplateVariable variable = TemplateVariable.segment(parameter.getVariableName()); - values.put(variable.getName(), variable.prepareAndEncode(parameter.getValueAsString(arguments, resolved))); + Object verifiedValue = parameter.getVerifiedValue(arguments); + Object preparedValue = verifiedValue == null ? verifiedValue + : parameter.prepareValue(verifiedValue, conversionService); + + values.put(variable.getName(), variable.prepareAndEncode(preparedValue)); } List optionalEmptyParameters = new ArrayList<>(); @@ -196,7 +198,7 @@ public class WebHandler { if (value instanceof MultiValueMap) { - Map> requestParams = (Map>) value; + Map> requestParams = (Map>) parameter.prepareValue(value, conversionService); for (Entry> entry : requestParams.entrySet()) { for (Object element : entry.getValue()) { @@ -210,7 +212,7 @@ public class WebHandler { if (value instanceof Map) { - Map requestParams = (Map) value; + Map requestParams = (Map) parameter.prepareValue(value, conversionService); for (Entry entry : requestParams.entrySet()) { @@ -232,11 +234,13 @@ public class WebHandler { if (value instanceof Collection) { + Collection collection = (Collection) parameter.prepareValue(value, conversionService); + if (parameter.isNonComposite()) { - builder.queryParam(key, variable.prepareAndEncode(value)); + builder.queryParam(key, variable.prepareAndEncode(collection)); } else { - for (Object element : (Collection) value) { + for (Object element : (Collection) collection) { if (key != null) { builder.queryParam(key, variable.prepareAndEncode(element)); } @@ -252,11 +256,31 @@ public class WebHandler { } else { if (key != null) { - builder.queryParam(key, variable.prepareAndEncode(parameter.getValueAsString(arguments, conversionService))); + builder.queryParam(key, variable.prepareAndEncode(parameter.prepareValue(value, conversionService))); } } } + private static Function getFormatter(ConversionService conversionService, TypeDescriptor descriptor) { + + return source -> { + + if (String.class.isInstance(source)) { + return (String) source; + } + + Object result = conversionService.canConvert(descriptor, STRING_DESCRIPTOR) + ? conversionService.convert(source, descriptor, STRING_DESCRIPTOR) + : source == null ? null : source.toString(); + + if (result == null) { + throw new IllegalArgumentException(String.format("Conversion of value %s resulted in null!", source)); + } + + return (String) result; + }; + } + private static class HandlerMethodParameters { private static final List> ANNOTATIONS = Arrays.asList(RequestParam.class, @@ -310,7 +334,6 @@ public class WebHandler { private abstract static class HandlerMethodParameter { - private static final TypeDescriptor STRING_DESCRIPTOR = TypeDescriptor.valueOf(String.class); private static final Map, Function> FACTORY; private static final String NO_PARAMETER_NAME = "Could not determine name of parameter %s! Make sure you compile with parameter information or explicitly define a parameter name in %s."; @@ -396,29 +419,54 @@ public class WebHandler { return variableName; } - public String getValueAsString(Object[] values, ConversionService conversionService) { + public Object prepareValue(Object value, ConversionService conversionService) { - Object value = values[parameter.getParameterIndex()]; + Object result = prepareValue(value, conversionService, typeDescriptor); - if (value == null) { - throw new IllegalArgumentException("Cannot turn null value into required String!"); - } + return result == null ? value : result; + } - if (String.class.isInstance(value)) { - return (String) value; + @Nullable + @SuppressWarnings("unchecked") + public static Object prepareValue(@Nullable Object value, ConversionService conversionService, + @Nullable TypeDescriptor descriptor) { + + if (descriptor == null || value == null) { + return value; } value = ObjectUtils.unwrapOptional(value); - Object result = conversionService.canConvert(typeDescriptor, STRING_DESCRIPTOR) - ? conversionService.convert(value, typeDescriptor, STRING_DESCRIPTOR) - : value == null ? null : value.toString(); + if (Collection.class.isInstance(value)) { - if (result == null) { - throw new IllegalArgumentException(String.format("Conversion of value %s resulted in null!", value)); + List prepared = new ArrayList<>(); + + for (Object element : (Collection) value) { + + TypeDescriptor elementTypeDescriptor = descriptor.elementTypeDescriptor(element); + prepared.add(prepareValue(element, conversionService, elementTypeDescriptor)); + } + + return prepared; } - return (String) result; + if (Map.class.isInstance(value)) { + + Map prepared = new LinkedHashMap<>(); + + for (Entry entry : ((Map) value).entrySet()) { + + TypeDescriptor keyTypeDescriptor = descriptor.getMapKeyTypeDescriptor(entry.getKey()); + TypeDescriptor elementTypeDescriptor = descriptor.elementTypeDescriptor(entry.getValue()); + + prepared.put(prepareValue(entry.getKey(), conversionService, keyTypeDescriptor), + prepareValue(entry.getValue(), conversionService, elementTypeDescriptor)); + } + + return prepared; + } + + return getFormatter(conversionService, descriptor).apply(value); } private String determineVariableName() { 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 7e147fcf..e2ea9965 100644 --- a/src/test/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderUnitTest.java +++ b/src/test/java/org/springframework/hateoas/server/mvc/WebMvcLinkBuilderUnitTest.java @@ -23,6 +23,7 @@ import java.time.OffsetDateTime; import java.time.ZoneId; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -30,6 +31,8 @@ import java.util.Optional; import java.util.stream.Stream; import org.junit.jupiter.api.Test; +import org.springframework.core.convert.converter.Converter; +import org.springframework.core.convert.support.ConfigurableConversionService; import org.springframework.format.annotation.DateTimeFormat; import org.springframework.format.annotation.DateTimeFormat.ISO; import org.springframework.hateoas.IanaLinkRelations; @@ -39,8 +42,10 @@ 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.hateoas.server.mvc.WebMvcLinkBuilderUnitTest.Sample.SampleConverter; import org.springframework.http.HttpEntity; import org.springframework.http.ResponseEntity; +import org.springframework.lang.NonNull; import org.springframework.test.util.ReflectionTestUtils; import org.springframework.util.MultiValueMap; import org.springframework.web.bind.annotation.GetMapping; @@ -653,6 +658,22 @@ class WebMvcLinkBuilderUnitTest extends TestUtils { .doesNotContain(":", "+"); } + @Test // #1598 + void usesRegisteredConverterForCollectionValues() { + + ConfigurableConversionService conversionService = // + (ConfigurableConversionService) ReflectionTestUtils.getField(WebMvcLinkBuilderFactory.class, + "FALLBACK_CONVERSION_SERVICE"); + + conversionService.addConverter(SampleConverter.INSTANCE); + + Link result = linkTo( + methodOn(ControllerWithMethods.class).methodWithCustomEnum(Collections.singletonList(Sample.ENUM))) + .withSelfRel(); + + assertThat(result.getHref()).endsWith("?param=first"); + } + private static UriComponents toComponents(Link link) { return UriComponentsBuilder.fromUriString(link.expand().getHref()).build(); } @@ -753,6 +774,11 @@ class WebMvcLinkBuilderUnitTest extends TestUtils { HttpEntity methodWithOffsetDateTime(@RequestParam @DateTimeFormat(iso = ISO.DATE_TIME) OffsetDateTime date) { return null; } + + @RequestMapping("/custom-enum") + HttpEntity methodWithCustomEnum(@RequestParam List param) { + return null; + } } @RequestMapping("/parent") @@ -801,10 +827,36 @@ class WebMvcLinkBuilderUnitTest extends TestUtils { } } - // #??? + // #118 interface ControllerWithHandlerMethodParameterThatNeedsConversion { @GetMapping("/{id}") HttpEntity method(@PathVariable Long id); } + + // #1598 + enum Sample { + + ENUM("first"); + + String label; + + /** + * @param label + */ + private Sample(String label) { + this.label = label; + } + + enum SampleConverter implements Converter { + + INSTANCE; + + @NonNull + @Override + public String convert(Sample source) { + return source.label; + } + } + } }