From 2d2726b8f77d7139c2dfdd0e4c0bc1b7591282d7 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 3 May 2022 12:41:52 +0100 Subject: [PATCH] Add RequestBody resolver and minor improvements Support for RequestBody arguments. List supported arguments on HttpExchange. Improve null handling. See gh-28386 --- .../web/service/annotation/HttpExchange.java | 47 +++++- .../invoker/HttpMethodArgumentResolver.java | 12 +- .../service/invoker/HttpRequestValues.java | 2 +- .../service/invoker/HttpServiceMethod.java | 8 + .../invoker/HttpServiceProxyFactory.java | 5 +- .../invoker/RequestBodyArgumentResolver.java | 86 +++++++++++ ...Resolver.java => UrlArgumentResolver.java} | 17 ++- .../HttpMethodArgumentResolverTests.java | 15 +- .../RequestBodyArgumentResolverTests.java | 141 ++++++++++++++++++ ...sts.java => UrlArgumentResolverTests.java} | 23 ++- 10 files changed, 330 insertions(+), 26 deletions(-) create mode 100644 spring-web/src/main/java/org/springframework/web/service/invoker/RequestBodyArgumentResolver.java rename spring-web/src/main/java/org/springframework/web/service/invoker/{HttpUrlArgumentResolver.java => UrlArgumentResolver.java} (74%) create mode 100644 spring-web/src/test/java/org/springframework/web/service/invoker/RequestBodyArgumentResolverTests.java rename spring-web/src/test/java/org/springframework/web/service/invoker/{HttpUrlArgumentResolverTests.java => UrlArgumentResolverTests.java} (68%) diff --git a/spring-web/src/main/java/org/springframework/web/service/annotation/HttpExchange.java b/spring-web/src/main/java/org/springframework/web/service/annotation/HttpExchange.java index 304e32cb6fc..e135fc14662 100644 --- a/spring-web/src/main/java/org/springframework/web/service/annotation/HttpExchange.java +++ b/spring-web/src/main/java/org/springframework/web/service/annotation/HttpExchange.java @@ -24,6 +24,7 @@ import java.lang.annotation.Target; import org.springframework.core.annotation.AliasFor; import org.springframework.web.bind.annotation.Mapping; +import org.springframework.web.service.invoker.UrlArgumentResolver; /** @@ -49,16 +50,52 @@ import org.springframework.web.bind.annotation.Mapping; * * Method Argument * Description + * Resolver * * - * {@link org.springframework.http.HttpMethod} - * Set the HTTP method for the request, overriding the annotation - * {@link #method()} attribute value + * {@link java.net.URI URI} + * Dynamically set the URL for the request, overriding the annotation + * {@link #url()} attribute + * {@link UrlArgumentResolver + * HttpUrlArgumentResolver} + * + * + * {@link org.springframework.http.HttpMethod HttpMethod} + * Dynamically set the HTTP method for the request, overriding the annotation + * {@link #method()} attribute + * {@link org.springframework.web.service.invoker.HttpMethodArgumentResolver + * HttpMethodArgumentResolver} + * + * + * {@link org.springframework.web.bind.annotation.RequestHeader @RequestHeader} + * Add a request header + * {@link org.springframework.web.service.invoker.RequestHeaderArgumentResolver + * RequestHeaderArgumentResolver} * * * {@link org.springframework.web.bind.annotation.PathVariable @PathVariable} - * Provide a path variable to expand the URI template with. This may be an - * individual value or a Map of values. + * Add a path variable for the URI template + * {@link org.springframework.web.service.invoker.PathVariableArgumentResolver + * PathVariableArgumentResolver} + * + * + * {@link org.springframework.web.bind.annotation.RequestBody @RequestBody} + * Set the body of the request + * {@link org.springframework.web.service.invoker.RequestBodyArgumentResolver + * RequestBodyArgumentResolver} + * + * + * {@link org.springframework.web.bind.annotation.RequestParam @RequestParam} + * Add a request parameter, either form data if {@code "Content-Type"} is + * {@code "application/x-www-form-urlencoded"} or query params otherwise + * {@link org.springframework.web.service.invoker.RequestParamArgumentResolver + * RequestParamArgumentResolver} + * + * + * {@link org.springframework.web.bind.annotation.CookieValue @CookieValue} + * Add a cookie + * {@link org.springframework.web.service.invoker.CookieValueArgumentResolver + * CookieValueArgumentResolver} * * * diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpMethodArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpMethodArgumentResolver.java index 5de5c14868c..67db0e554f2 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpMethodArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpMethodArgumentResolver.java @@ -40,15 +40,19 @@ public class HttpMethodArgumentResolver implements HttpServiceArgumentResolver { public boolean resolve( @Nullable Object argument, MethodParameter parameter, HttpRequestValues.Builder requestValues) { - if (argument instanceof HttpMethod httpMethod) { + if (!parameter.getParameterType().equals(HttpMethod.class)) { + return false; + } + + if (argument != null) { + HttpMethod httpMethod = (HttpMethod) argument; + requestValues.setHttpMethod(httpMethod); if (logger.isTraceEnabled()) { logger.trace("Resolved HTTP method to: " + httpMethod.name()); } - requestValues.setHttpMethod(httpMethod); - return true; } - return false; + return true; } } diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpRequestValues.java b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpRequestValues.java index f022b1e7642..82afcba6c36 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpRequestValues.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpRequestValues.java @@ -343,7 +343,7 @@ public final class HttpRequestValues { *

This is mutually exclusive with, and resets any previously set * {@link #setBodyValue(Object) body value}. */ - public > void setBody(Publisher

body, ParameterizedTypeReference elementTye) { + public > void setBody(P body, ParameterizedTypeReference elementTye) { this.body = body; this.bodyElementType = elementTye; this.bodyValue = null; diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java index 01ac5ff233f..3d756434462 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceMethod.java @@ -110,14 +110,22 @@ final class HttpServiceMethod { Assert.isTrue(arguments.length == this.parameters.length, "Method argument mismatch"); for (int i = 0; i < arguments.length; i++) { Object value = arguments[i]; + boolean resolved = false; for (HttpServiceArgumentResolver resolver : this.argumentResolvers) { if (resolver.resolve(value, this.parameters[i], requestValues)) { + resolved = true; break; } } + Assert.state(resolved, formatArgumentError(this.parameters[i], "No suitable resolver")); } } + private static String formatArgumentError(MethodParameter param, String message) { + return "Could not resolve parameter [" + param.getParameterIndex() + "] in " + + param.getExecutable().toGenericString() + (StringUtils.hasText(message) ? ": " + message : ""); + } + /** * Factory for an {@link HttpRequestValues} with values extracted from diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceProxyFactory.java b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceProxyFactory.java index cebb149f862..388169f2f59 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceProxyFactory.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/HttpServiceProxyFactory.java @@ -188,10 +188,11 @@ public final class HttpServiceProxyFactory { private List initArgumentResolvers(ConversionService conversionService) { List resolvers = new ArrayList<>(this.customResolvers); resolvers.add(new RequestHeaderArgumentResolver(conversionService)); + resolvers.add(new RequestBodyArgumentResolver(this.reactiveAdapterRegistry)); resolvers.add(new PathVariableArgumentResolver(conversionService)); - resolvers.add(new CookieValueArgumentResolver(conversionService)); resolvers.add(new RequestParamArgumentResolver(conversionService)); - resolvers.add(new HttpUrlArgumentResolver()); + resolvers.add(new CookieValueArgumentResolver(conversionService)); + resolvers.add(new UrlArgumentResolver()); resolvers.add(new HttpMethodArgumentResolver()); return resolvers; } diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/RequestBodyArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/RequestBodyArgumentResolver.java new file mode 100644 index 00000000000..5463bc3ba81 --- /dev/null +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/RequestBodyArgumentResolver.java @@ -0,0 +1,86 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.web.service.invoker; + +import org.reactivestreams.Publisher; + +import org.springframework.core.MethodParameter; +import org.springframework.core.ParameterizedTypeReference; +import org.springframework.core.ReactiveAdapter; +import org.springframework.core.ReactiveAdapterRegistry; +import org.springframework.lang.Nullable; +import org.springframework.util.Assert; +import org.springframework.web.bind.annotation.RequestBody; + + +/** + * {@link HttpServiceArgumentResolver} for {@link RequestBody @RequestBody} + * annotated arguments. + * + * @author Rossen Stoyanchev + * @since 6.0 + */ +public class RequestBodyArgumentResolver implements HttpServiceArgumentResolver { + + private final ReactiveAdapterRegistry reactiveAdapterRegistry; + + + public RequestBodyArgumentResolver(ReactiveAdapterRegistry reactiveAdapterRegistry) { + Assert.notNull(reactiveAdapterRegistry, "ReactiveAdapterRegistry is required"); + this.reactiveAdapterRegistry = reactiveAdapterRegistry; + } + + + @Override + public boolean resolve( + @Nullable Object argument, MethodParameter parameter, HttpRequestValues.Builder requestValues) { + + RequestBody annot = parameter.getParameterAnnotation(RequestBody.class); + if (annot == null) { + return false; + } + + if (argument != null) { + ReactiveAdapter reactiveAdapter = this.reactiveAdapterRegistry.getAdapter(parameter.getParameterType()); + if (reactiveAdapter != null) { + setBody(argument, parameter, reactiveAdapter, requestValues); + } + else { + requestValues.setBodyValue(argument); + } + } + + return true; + } + + private void setBody( + Object argument, MethodParameter parameter, ReactiveAdapter reactiveAdapter, + HttpRequestValues.Builder requestValues) { + + String message = "Async type for @RequestBody should produce value(s)"; + Assert.isTrue(!reactiveAdapter.isNoValue(), message); + + parameter = parameter.nested(); + Class elementClass = parameter.getNestedParameterType(); + Assert.isTrue(elementClass != Void.class, message); + ParameterizedTypeReference typeRef = ParameterizedTypeReference.forType(parameter.getNestedGenericParameterType()); + Publisher publisher = reactiveAdapter.toPublisher(argument); + + requestValues.setBody(publisher, typeRef); + } + +} diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpUrlArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java similarity index 74% rename from spring-web/src/main/java/org/springframework/web/service/invoker/HttpUrlArgumentResolver.java rename to spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java index 7b65280b823..729954b6ceb 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/HttpUrlArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java @@ -19,29 +19,32 @@ package org.springframework.web.service.invoker; import java.net.URI; import org.springframework.core.MethodParameter; -import org.springframework.http.HttpMethod; import org.springframework.lang.Nullable; /** - * {@link HttpServiceArgumentResolver} that resolves the target - * request's URL from an {@link HttpMethod} argument. + * {@link HttpServiceArgumentResolver} that resolves the URL for the request + * from an {@link URI} argument. * * @author Rossen Stoyanchev * @since 6.0 */ -public class HttpUrlArgumentResolver implements HttpServiceArgumentResolver { +public class UrlArgumentResolver implements HttpServiceArgumentResolver { @Override public boolean resolve( @Nullable Object argument, MethodParameter parameter, HttpRequestValues.Builder requestValues) { - if (argument instanceof URI uri) { - requestValues.setUri(uri); + if (!parameter.getParameterType().equals(URI.class)) { + return false; + } + + if (argument != null) { + requestValues.setUri((URI) argument); return true; } - return false; + return true; } } diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/HttpMethodArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/HttpMethodArgumentResolverTests.java index 5753e894d46..625e0ab627c 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/HttpMethodArgumentResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/HttpMethodArgumentResolverTests.java @@ -22,6 +22,7 @@ import org.springframework.http.HttpMethod; import org.springframework.web.service.annotation.GetExchange; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; /** @@ -44,14 +45,18 @@ public class HttpMethodArgumentResolverTests { } @Test - void ignoreOtherArgumentTypes() { - this.service.execute("test"); - assertThat(getActualMethod()).isEqualTo(HttpMethod.GET); + void notHttpMethod() { + assertThatIllegalStateException() + .isThrownBy(() -> this.service.executeNotHttpMethod("test")) + .withMessage("Could not resolve parameter [0] in " + + "public abstract void org.springframework.web.service.invoker." + + "HttpMethodArgumentResolverTests$Service.executeNotHttpMethod(java.lang.String): " + + "No suitable resolver"); } @Test void ignoreNull() { - this.service.execute((HttpMethod) null); + this.service.execute(null); assertThat(getActualMethod()).isEqualTo(HttpMethod.GET); } @@ -66,7 +71,7 @@ public class HttpMethodArgumentResolverTests { void execute(HttpMethod method); @GetExchange - void execute(String test); + void executeNotHttpMethod(String test); } diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/RequestBodyArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/RequestBodyArgumentResolverTests.java new file mode 100644 index 00000000000..8d9c78de7c5 --- /dev/null +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/RequestBodyArgumentResolverTests.java @@ -0,0 +1,141 @@ +/* + * Copyright 2002-2022 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.web.service.invoker; + +import io.reactivex.rxjava3.core.Completable; +import io.reactivex.rxjava3.core.Single; +import org.junit.jupiter.api.Test; +import org.reactivestreams.Publisher; +import reactor.core.publisher.Mono; + +import org.springframework.core.ParameterizedTypeReference; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.service.annotation.GetExchange; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; + + +/** + * Unit tests for {@link RequestBodyArgumentResolver}. + * + * @author Rossen Stoyanchev + */ +public class RequestBodyArgumentResolverTests { + + private final TestHttpClientAdapter client = new TestHttpClientAdapter(); + + private final Service service = HttpServiceProxyFactory.builder(this.client).build().createClient(Service.class); + + + @Test + void stringBody() { + String body = "bodyValue"; + this.service.execute(body); + + assertThat(getRequestValues().getBodyValue()).isEqualTo(body); + assertThat(getRequestValues().getBody()).isNull(); + } + + @Test + void monoBody() { + Mono bodyMono = Mono.just("bodyValue"); + this.service.executeMono(bodyMono); + + assertThat(getRequestValues().getBodyValue()).isNull(); + assertThat(getRequestValues().getBody()).isSameAs(bodyMono); + assertThat(getRequestValues().getBodyElementType()).isEqualTo(new ParameterizedTypeReference() {}); + } + + @Test + void singleBody() { + String bodyValue = "bodyValue"; + this.service.executeSingle(Single.just(bodyValue)); + + assertThat(getRequestValues().getBodyValue()).isNull(); + assertThat(getRequestValues().getBodyElementType()).isEqualTo(new ParameterizedTypeReference() {}); + + Publisher body = getRequestValues().getBody(); + assertThat(body).isNotNull(); + assertThat(((Mono) body).block()).isEqualTo(bodyValue); + } + + @Test + void monoVoid() { + assertThatIllegalArgumentException() + .isThrownBy(() -> this.service.executeMonoVoid(Mono.empty())) + .withMessage("Async type for @RequestBody should produce value(s)"); + } + + @Test + void completable() { + assertThatIllegalArgumentException() + .isThrownBy(() -> this.service.executeCompletable(Completable.complete())) + .withMessage("Async type for @RequestBody should produce value(s)"); + } + + @Test + void notRequestBody() { + assertThatIllegalStateException() + .isThrownBy(() -> this.service.executeNotRequestBody("value")) + .withMessage("Could not resolve parameter [0] in " + + "public abstract void org.springframework.web.service.invoker." + + "RequestBodyArgumentResolverTests$Service.executeNotRequestBody(java.lang.String): " + + "No suitable resolver"); + } + + @Test + void ignoreNull() { + this.service.execute(null); + + assertThat(getRequestValues().getBodyValue()).isNull(); + assertThat(getRequestValues().getBody()).isNull(); + + this.service.executeMono(null); + + assertThat(getRequestValues().getBodyValue()).isNull(); + assertThat(getRequestValues().getBody()).isNull(); + } + + private HttpRequestValues getRequestValues() { + return this.client.getRequestValues(); + } + + + private interface Service { + + @GetExchange + void execute(@RequestBody String body); + + @GetExchange + void executeMono(@RequestBody Mono body); + + @GetExchange + void executeSingle(@RequestBody Single body); + + @GetExchange + void executeMonoVoid(@RequestBody Mono body); + + @GetExchange + void executeCompletable(@RequestBody Completable body); + + @GetExchange + void executeNotRequestBody(String body); + } + +} diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/HttpUrlArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/UrlArgumentResolverTests.java similarity index 68% rename from spring-web/src/test/java/org/springframework/web/service/invoker/HttpUrlArgumentResolverTests.java rename to spring-web/src/test/java/org/springframework/web/service/invoker/UrlArgumentResolverTests.java index 345d605db55..f0d2b3b5430 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/HttpUrlArgumentResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/UrlArgumentResolverTests.java @@ -23,14 +23,15 @@ import org.junit.jupiter.api.Test; import org.springframework.web.service.annotation.GetExchange; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; /** - * Unit tests for {@link HttpUrlArgumentResolver}. + * Unit tests for {@link UrlArgumentResolver}. * * @author Rossen Stoyanchev */ -public class HttpUrlArgumentResolverTests { +public class UrlArgumentResolverTests { private final TestHttpClientAdapter client = new TestHttpClientAdapter(); @@ -46,6 +47,22 @@ public class HttpUrlArgumentResolverTests { assertThat(getRequestValues().getUriTemplate()).isNull(); } + @Test + void notUrl() { + assertThatIllegalStateException() + .isThrownBy(() -> this.service.executeNotUri("test")) + .withMessage("Could not resolve parameter [0] in " + + "public abstract void org.springframework.web.service.invoker." + + "UrlArgumentResolverTests$Service.executeNotUri(java.lang.String): " + + "No suitable resolver"); + } + + @Test + void ignoreNull() { + this.service.execute(null); + assertThat(getRequestValues().getUri()).isNull(); + } + private HttpRequestValues getRequestValues() { return this.client.getRequestValues(); } @@ -56,6 +73,8 @@ public class HttpUrlArgumentResolverTests { @GetExchange("/path") void execute(URI uri); + @GetExchange + void executeNotUri(String other); } }