From 00e80d7eb387adab8dfd009da5a7177e645c5243 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 5 Aug 2024 15:30:10 +0200 Subject: [PATCH 1/2] Reject not optional null arguments. --- .../ROOT/pages/integration/rest-clients.adoc | 3 + .../modules/ROOT/pages/rsocket.adoc | 3 +- .../service/PayloadArgumentResolver.java | 17 ++- .../service/PayloadArgumentResolverTests.java | 109 ++++++++++++++++-- .../AbstractNamedValueArgumentResolver.java | 3 +- .../invoker/HttpMethodArgumentResolver.java | 24 ++-- .../invoker/RequestBodyArgumentResolver.java | 16 ++- .../UriBuilderFactoryArgumentResolver.java | 15 ++- .../service/invoker/UrlArgumentResolver.java | 16 ++- .../HttpMethodArgumentResolverTests.java | 36 +++++- .../NamedValueArgumentResolverTests.java | 12 +- .../RequestBodyArgumentResolverTests.java | 99 +++++++++++++++- ...riBuilderFactoryArgumentResolverTests.java | 51 +++++++- .../invoker/UrlArgumentResolverTests.java | 50 +++++++- 14 files changed, 414 insertions(+), 40 deletions(-) diff --git a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc index fed6f923528e..f24850ff57dc 100644 --- a/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc +++ b/framework-docs/modules/ROOT/pages/integration/rest-clients.adoc @@ -977,6 +977,9 @@ method parameters: `MultiValueMap` with multiple cookies, a `Collection` of values, or an individual value. Type conversion is supported for non-String values. +The parameters can't be null unless they are set as not required through the annotation, +annotated with `@Nullable` or they are `Optional`. + |=== diff --git a/framework-docs/modules/ROOT/pages/rsocket.adoc b/framework-docs/modules/ROOT/pages/rsocket.adoc index 402c213898df..6a6654a314f6 100644 --- a/framework-docs/modules/ROOT/pages/rsocket.adoc +++ b/framework-docs/modules/ROOT/pages/rsocket.adoc @@ -1115,7 +1115,8 @@ method parameters: | `@Payload` | Set the input payload(s) for the request. This can be a concrete value, or any producer of values that can be adapted to a Reactive Streams `Publisher` via - `ReactiveAdapterRegistry` + `ReactiveAdapterRegistry`. The payload can't be null unless it's set as not required +through the annotation, annotated with `@Nullable` or it is `Optional`. | `Object`, if followed by `MimeType` | The value for a metadata entry in the input payload. This can be any `Object` as long diff --git a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolver.java b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolver.java index 8511f675f25f..4998b109a953 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolver.java +++ b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2024 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. @@ -16,6 +16,8 @@ package org.springframework.messaging.rsocket.service; +import java.util.Optional; + import org.springframework.core.MethodParameter; import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.ReactiveAdapter; @@ -29,6 +31,7 @@ * annotated arguments. * * @author Rossen Stoyanchev + * @author Olga Maciaszek-Sharma * @since 6.0 */ public class PayloadArgumentResolver implements RSocketServiceArgumentResolver { @@ -54,8 +57,15 @@ public boolean resolve( return false; } + parameter = parameter.nestedIfOptional(); + + if (argument instanceof Optional optionalValue) { + argument = optionalValue.orElse(null); + } + if (argument != null) { - ReactiveAdapter reactiveAdapter = this.reactiveAdapterRegistry.getAdapter(parameter.getParameterType()); + ReactiveAdapter reactiveAdapter = this.reactiveAdapterRegistry + .getAdapter(parameter.getNestedParameterType()); if (reactiveAdapter == null) { requestValues.setPayloadValue(argument); } @@ -70,7 +80,10 @@ public boolean resolve( reactiveAdapter.toPublisher(argument), ParameterizedTypeReference.forType(nestedParameter.getNestedGenericParameterType())); } + return true; } + boolean required = (annot == null || annot.required()) && !parameter.isOptional(); + Assert.isTrue(!required, () -> "Missing payload"); return true; } diff --git a/spring-messaging/src/test/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolverTests.java b/spring-messaging/src/test/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolverTests.java index d3ba5ba03113..16c90c42de3d 100644 --- a/spring-messaging/src/test/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolverTests.java +++ b/spring-messaging/src/test/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolverTests.java @@ -16,6 +16,8 @@ package org.springframework.messaging.rsocket.service; +import java.util.Optional; + import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Single; import org.junit.jupiter.api.Test; @@ -25,6 +27,7 @@ import org.springframework.core.MethodParameter; import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.ReactiveAdapterRegistry; +import org.springframework.lang.Nullable; import org.springframework.messaging.handler.annotation.Payload; import static org.assertj.core.api.Assertions.assertThat; @@ -34,7 +37,9 @@ * Tests for {@link PayloadArgumentResolver}. * * @author Rossen Stoyanchev + * @author Olga Maciaszek-Sharma */ +@SuppressWarnings("DataFlowIssue") class PayloadArgumentResolverTests extends RSocketServiceArgumentResolverTestSupport { @Override @@ -47,9 +52,7 @@ void stringPayload() { String payload = "payloadValue"; boolean resolved = execute(payload, initMethodParameter(Service.class, "execute", 0)); - assertThat(resolved).isTrue(); - assertThat(getRequestValues().getPayloadValue()).isEqualTo(payload); - assertThat(getRequestValues().getPayload()).isNull(); + assertPayload(resolved, payload); } @Test @@ -57,10 +60,7 @@ void monoPayload() { Mono payloadMono = Mono.just("payloadValue"); boolean resolved = execute(payloadMono, initMethodParameter(Service.class, "executeMono", 0)); - assertThat(resolved).isTrue(); - assertThat(getRequestValues().getPayloadValue()).isNull(); - assertThat(getRequestValues().getPayload()).isSameAs(payloadMono); - assertThat(getRequestValues().getPayloadElementType()).isEqualTo(new ParameterizedTypeReference() {}); + assertPayloadMono(resolved, payloadMono); } @Test @@ -92,7 +92,7 @@ void completable() { } @Test - void notRequestBody() { + void notPayload() { MethodParameter parameter = initMethodParameter(Service.class, "executeNotAnnotated", 0); boolean resolved = execute("value", parameter); @@ -100,23 +100,108 @@ void notRequestBody() { } @Test - void ignoreNull() { - boolean resolved = execute(null, initMethodParameter(Service.class, "execute", 0)); + void nullPayload() { + assertThatIllegalArgumentException() + .isThrownBy(() -> execute(null, initMethodParameter(Service.class, "execute", 0))) + .withMessage("Missing payload"); + + assertThatIllegalArgumentException() + .isThrownBy(() -> execute(null, initMethodParameter(Service.class, "executeMono", 0))) + .withMessage("Missing payload"); + } + + @Test + void nullPayloadWithNullable() { + boolean resolved = execute(null, initMethodParameter(Service.class, "executeNullable", 0)); + assertNullValues(resolved); + + boolean resolvedMono = execute(null, initMethodParameter(Service.class, "executeNullableMono", 0)); + assertNullValues(resolvedMono); + } + + @Test + void nullPayloadWithNotRequired() { + boolean resolved = execute(null, initMethodParameter(Service.class, "executeNotRequired", 0)); + assertNullValues(resolved); + + boolean resolvedMono = execute(null, initMethodParameter(Service.class, "executeNotRequiredMono", 0)); + assertNullValues(resolvedMono); + } + + @Test + void nullPayloadWithOptional() { + boolean resolved = execute(null, initMethodParameter(Service.class, "executeOptional", 0)); + assertNullValues(resolved); + + boolean resolvedMono = execute(null, initMethodParameter(Service.class, "executeOptionalMono", 0)); + assertNullValues(resolvedMono); + } + + @Test + void emptyPayloadWithOptional() { + boolean resolved = execute(Optional.empty(), initMethodParameter(Service.class, "executeOptional", 0)); + assertNullValues(resolved); + + boolean resolvedMono = execute(Optional.empty(), initMethodParameter(Service.class, "executeOptionalMono", 0)); + assertNullValues(resolvedMono); + } + + @Test + void optionalStringPayload() { + String payload = "payloadValue"; + boolean resolved = execute(Optional.of(payload), initMethodParameter(Service.class, "executeOptional", 0)); + + assertPayload(resolved, payload); + } + + @Test + void optionalMonoPayload() { + Mono payloadMono = Mono.just("payloadValue"); + boolean resolved = execute(Optional.of(payloadMono), initMethodParameter(Service.class, "executeOptionalMono", 0)); + + assertPayloadMono(resolved, payloadMono); + } + + + private void assertPayload(boolean resolved, String payload) { + assertThat(resolved).isTrue(); + assertThat(getRequestValues().getPayloadValue()).isEqualTo(payload); + assertThat(getRequestValues().getPayload()).isNull(); + } + + private void assertPayloadMono(boolean resolved, Mono payloadMono) { + assertThat(resolved).isTrue(); + assertThat(getRequestValues().getPayloadValue()).isNull(); + assertThat(getRequestValues().getPayload()).isSameAs(payloadMono); + assertThat(getRequestValues().getPayloadElementType()).isEqualTo(new ParameterizedTypeReference() { }); + } + private void assertNullValues(boolean resolved) { assertThat(resolved).isTrue(); assertThat(getRequestValues().getPayloadValue()).isNull(); assertThat(getRequestValues().getPayload()).isNull(); assertThat(getRequestValues().getPayloadElementType()).isNull(); } - - @SuppressWarnings("unused") + @SuppressWarnings({"unused", "OptionalUsedAsFieldOrParameterType"}) private interface Service { void execute(@Payload String body); + void executeNotRequired(@Payload(required = false) String body); + + void executeNullable(@Nullable @Payload String body); + void executeMono(@Payload Mono body); + void executeOptional(@Payload Optional body); + + void executeNullableMono(@Nullable @Payload Mono body); + + void executeNotRequiredMono(@Payload(required = false) Mono body); + + void executeOptionalMono(@Payload Optional> body); + void executeSingle(@Payload Single body); void executeMonoVoid(@Payload Mono body); diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/AbstractNamedValueArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/AbstractNamedValueArgumentResolver.java index de8ad19ce2a7..97288c2b5ea3 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/AbstractNamedValueArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/AbstractNamedValueArgumentResolver.java @@ -39,6 +39,7 @@ * request header, path variable, cookie, and others. * * @author Rossen Stoyanchev + * @author Olga Maciaszek-Sharma * @since 6.0 */ public abstract class AbstractNamedValueArgumentResolver implements HttpServiceArgumentResolver { @@ -145,7 +146,7 @@ private NamedValueInfo updateNamedValueInfo(MethodParameter parameter, NamedValu .formatted(parameter.getNestedParameterType().getName())); } } - boolean required = (info.required && !parameter.getParameterType().equals(Optional.class)); + boolean required = (info.required && !parameter.isOptional()); String defaultValue = (ValueConstants.DEFAULT_NONE.equals(info.defaultValue) ? null : info.defaultValue); return info.update(name, required, defaultValue); } 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 9a6f966f08fe..daaaf7c1fa44 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2024 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. @@ -16,6 +16,8 @@ package org.springframework.web.service.invoker; +import java.util.Optional; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -41,17 +43,25 @@ public class HttpMethodArgumentResolver implements HttpServiceArgumentResolver { public boolean resolve( @Nullable Object argument, MethodParameter parameter, HttpRequestValues.Builder requestValues) { - if (!parameter.getParameterType().equals(HttpMethod.class)) { + parameter = parameter.nestedIfOptional(); + + if (!parameter.getNestedParameterType().equals(HttpMethod.class)) { return false; } - Assert.notNull(argument, "HttpMethod is required"); - HttpMethod httpMethod = (HttpMethod) argument; - requestValues.setHttpMethod(httpMethod); - if (logger.isTraceEnabled()) { - logger.trace("Resolved HTTP method to: " + httpMethod.name()); + if (argument instanceof Optional optionalValue) { + argument = optionalValue.orElse(null); } + if(argument != null) { + HttpMethod httpMethod = (HttpMethod) argument; + requestValues.setHttpMethod(httpMethod); + if (logger.isTraceEnabled()) { + logger.trace("Resolved HTTP method to: " + httpMethod.name()); + } + return true; + } + Assert.isTrue(parameter.isOptional(), "HttpMethod is required"); return true; } 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 index cdd3c6f0c05f..2126853a81bc 100644 --- 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -16,6 +16,8 @@ package org.springframework.web.service.invoker; +import java.util.Optional; + import org.springframework.core.MethodParameter; import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.ReactiveAdapter; @@ -30,6 +32,7 @@ * annotated arguments. * * @author Rossen Stoyanchev + * @author Olga Maciaszek-Sharma * @since 6.0 */ public class RequestBodyArgumentResolver implements HttpServiceArgumentResolver { @@ -68,9 +71,16 @@ public boolean resolve( return false; } + parameter = parameter.nestedIfOptional(); + + if (argument instanceof Optional optionalValue) { + argument = optionalValue.orElse(null); + } + if (argument != null) { if (this.reactiveAdapterRegistry != null) { - ReactiveAdapter adapter = this.reactiveAdapterRegistry.getAdapter(parameter.getParameterType()); + ReactiveAdapter adapter = this.reactiveAdapterRegistry + .getAdapter(parameter.getNestedParameterType()); if (adapter != null) { MethodParameter nestedParameter = parameter.nested(); @@ -93,7 +103,9 @@ public boolean resolve( // Not a reactive type requestValues.setBodyValue(argument); + return true; } + Assert.isTrue(!annot.required() || parameter.isOptional(), "RequestBody is required"); return true; } diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolver.java index d5d40f0bad78..ffefc07c24a1 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -17,9 +17,11 @@ package org.springframework.web.service.invoker; import java.net.URL; +import java.util.Optional; import org.springframework.core.MethodParameter; import org.springframework.lang.Nullable; +import org.springframework.util.Assert; import org.springframework.web.util.UriBuilderFactory; import org.springframework.web.util.UriTemplate; @@ -42,14 +44,23 @@ public class UriBuilderFactoryArgumentResolver implements HttpServiceArgumentRes public boolean resolve( @Nullable Object argument, MethodParameter parameter, HttpRequestValues.Builder requestValues) { - if (!parameter.getParameterType().equals(UriBuilderFactory.class)) { + parameter = parameter.nestedIfOptional(); + + if (!parameter.getNestedParameterType().equals(UriBuilderFactory.class)) { return false; } + if (argument instanceof Optional optionalValue) { + argument = optionalValue.orElse(null); + } + if (argument != null) { requestValues.setUriBuilderFactory((UriBuilderFactory) argument); + return true; } + Assert.isTrue(parameter.isOptional(), "UriBuilderFactory is required"); + return true; } } diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java index 3ca43c85051e..1a7090bd161d 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2022 the original author or authors. + * Copyright 2002-2024 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. @@ -17,15 +17,18 @@ package org.springframework.web.service.invoker; import java.net.URI; +import java.util.Optional; import org.springframework.core.MethodParameter; import org.springframework.lang.Nullable; +import org.springframework.util.Assert; /** * {@link HttpServiceArgumentResolver} that resolves the URL for the request * from a {@link URI} argument. * * @author Rossen Stoyanchev + * @author Olga Maciaszek-Sharma * @since 6.0 */ public class UrlArgumentResolver implements HttpServiceArgumentResolver { @@ -34,14 +37,23 @@ public class UrlArgumentResolver implements HttpServiceArgumentResolver { public boolean resolve( @Nullable Object argument, MethodParameter parameter, HttpRequestValues.Builder requestValues) { - if (!parameter.getParameterType().equals(URI.class)) { + parameter = parameter.nestedIfOptional(); + + if (!parameter.getNestedParameterType().equals(URI.class)) { return false; } + if (argument instanceof Optional optionalValue) { + argument = optionalValue.orElse(null); + } + if (argument != null) { requestValues.setUri((URI) argument); + return true; } + Assert.isTrue(parameter.isOptional(), "URI is required"); + 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 d060b550af7b..e2f87f01cca4 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -16,6 +16,8 @@ package org.springframework.web.service.invoker; +import java.util.Optional; + import org.junit.jupiter.api.Test; import org.springframework.http.HttpMethod; @@ -68,12 +70,38 @@ void nullHttpMethod() { assertThatIllegalArgumentException().isThrownBy(() -> this.service.execute(null)); } + @Test + void nullHttpMethodWithNullable() { + this.service.executeNullableHttpMethod(null); + assertThat(getActualMethod()).isEqualTo(HttpMethod.GET); + } + + @Test + void nullHttpMethodWithOptional() { + this.service.executeOptionalHttpMethod(null); + assertThat(getActualMethod()).isEqualTo(HttpMethod.GET); + } + + @Test + void emptyOptionalHttpMethod() { + this.service.executeOptionalHttpMethod(Optional.empty()); + assertThat(getActualMethod()).isEqualTo(HttpMethod.GET); + } + + @Test + void optionalHttpMethod() { + this.service.executeOptionalHttpMethod(Optional.of(HttpMethod.POST)); + assertThat(getActualMethod()).isEqualTo(HttpMethod.POST); + } + + @Nullable private HttpMethod getActualMethod() { return this.client.getRequestValues().getHttpMethod(); } + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") private interface Service { @HttpExchange @@ -85,6 +113,12 @@ private interface Service { @GetExchange void executeNotHttpMethod(String test); + @GetExchange + void executeNullableHttpMethod(@Nullable HttpMethod method); + + @GetExchange + void executeOptionalHttpMethod(Optional method); + } } diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/NamedValueArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/NamedValueArgumentResolverTests.java index 399b5c3f4f5d..cbac555179ab 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/NamedValueArgumentResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/NamedValueArgumentResolverTests.java @@ -48,6 +48,7 @@ * {@link TestValue @TestValue} annotation and {@link TestNamedValueArgumentResolver}. * * @author Rossen Stoyanchev + * @author Olga Maciaszek-Sharma */ class NamedValueArgumentResolverTests { @@ -134,7 +135,7 @@ void optionalEmpty() { } @Test - void optionalEmpthyWithDefaultValue() { + void optionalEmptyWithDefaultValue() { this.service.executeOptionalWithDefaultValue(Optional.empty()); assertTestValue("value", "default"); } @@ -157,6 +158,12 @@ void mapOfTestValuesHasOptionalValue() { assertTestValue("value", "test"); } + @Test + void nullTestValueWithNullable() { + this.service.executeNullable(null); + assertTestValue("value"); + } + private void assertTestValue(String key, String... values) { List actualValues = this.argumentResolver.getTestValues().get(key); if (ObjectUtils.isEmpty(values)) { @@ -207,6 +214,9 @@ private interface Service { @GetExchange void executeMapWithOptionalValue(@TestValue Map> values); + @GetExchange + void executeNullable(@Nullable @TestValue String value); + } 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 index 12b8b81575d9..3d730897886b 100644 --- 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 @@ -16,6 +16,8 @@ package org.springframework.web.service.invoker; +import java.util.Optional; + import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Single; import org.junit.jupiter.api.Test; @@ -35,7 +37,9 @@ * Tests for {@link RequestBodyArgumentResolver}. * * @author Rossen Stoyanchev + * @author Olga Maciaszek-Sharma */ +@SuppressWarnings({"DataFlowIssue", "OptionalAssignedToNull"}) class RequestBodyArgumentResolverTests { private final TestReactorExchangeAdapter client = new TestReactorExchangeAdapter(); @@ -102,18 +106,88 @@ void notRequestBody() { } @Test - void ignoreNull() { - this.service.execute(null); + void nullRequestBody() { + assertThatIllegalArgumentException() + .isThrownBy(() -> this.service.execute(null)) + .withMessage("RequestBody is required"); + + assertThatIllegalArgumentException() + .isThrownBy(() -> this.service.executeMono(null)) + .withMessage("RequestBody is required"); + } + + @Test + void nullRequestBodyWithNullable() { + this.service.executeNullable(null); + + assertThat(getBodyValue()).isNull(); + assertThat(getPublisherBody()).isNull(); + + this.service.executeNullableMono(null); + + assertThat(getBodyValue()).isNull(); + assertThat(getPublisherBody()).isNull(); + } + + @Test + void nullRequestBodyWithNotRequired() { + this.service.executeNotRequired(null); + + assertThat(getBodyValue()).isNull(); + assertThat(getPublisherBody()).isNull(); + + this.service.executeNotRequiredMono(null); + + assertThat(getBodyValue()).isNull(); + assertThat(getPublisherBody()).isNull(); + } + + @Test + void nullRequestBodyWithOptional() { + this.service.executeOptional(null); + + assertThat(getBodyValue()).isNull(); + assertThat(getPublisherBody()).isNull(); + + this.service.executeOptionalMono(null); + + assertThat(getBodyValue()).isNull(); + assertThat(getPublisherBody()).isNull(); + } + + @Test + void emptyOptionalRequestBody() { + this.service.executeOptional(Optional.empty()); assertThat(getBodyValue()).isNull(); assertThat(getPublisherBody()).isNull(); - this.service.executeMono(null); + this.service.executeOptionalMono(Optional.empty()); assertThat(getBodyValue()).isNull(); assertThat(getPublisherBody()).isNull(); } + @Test + void optionalStringBody() { + String body = "bodyValue"; + this.service.executeOptional(Optional.of(body)); + + assertThat(getBodyValue()).isEqualTo(body); + assertThat(getPublisherBody()).isNull(); + } + + @Test + void optionalMonoBody() { + Mono bodyMono = Mono.just("bodyValue"); + this.service.executeOptionalMono(Optional.of(bodyMono)); + + assertThat(getBodyValue()).isNull(); + assertThat(getPublisherBody()).isSameAs(bodyMono); + assertThat(getBodyElementType()).isEqualTo(new ParameterizedTypeReference() { }); + } + + @Nullable private Object getBodyValue() { return getReactiveRequestValues().getBodyValue(); @@ -134,14 +208,33 @@ private ReactiveHttpRequestValues getReactiveRequestValues() { } + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") private interface Service { @GetExchange void execute(@RequestBody String body); + @GetExchange + void executeNullable(@Nullable @RequestBody String body); + + @GetExchange + void executeNotRequired(@RequestBody(required = false) String body); + + @GetExchange + void executeOptional(@RequestBody Optional body); + @GetExchange void executeMono(@RequestBody Mono body); + @GetExchange + void executeNullableMono(@Nullable @RequestBody Mono body); + + @GetExchange + void executeNotRequiredMono(@RequestBody(required = false) Mono body); + + @GetExchange + void executeOptionalMono(@RequestBody Optional> body); + @GetExchange void executeSingle(@RequestBody Single body); diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolverTests.java index 670d680929aa..36b8c9ec562c 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolverTests.java @@ -16,6 +16,8 @@ package org.springframework.web.service.invoker; +import java.util.Optional; + import org.junit.jupiter.api.Test; import org.springframework.lang.Nullable; @@ -24,12 +26,14 @@ import org.springframework.web.util.UriBuilderFactory; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; /** * Tests for {@link UriBuilderFactoryArgumentResolver}. * * @author Olga Maciaszek-Sharma */ +@SuppressWarnings({"DataFlowIssue", "OptionalAssignedToNull"}) class UriBuilderFactoryArgumentResolverTests { private final TestExchangeAdapter client = new TestExchangeAdapter(); @@ -49,24 +53,65 @@ void uriBuilderFactory(){ } @Test - void ignoreNullUriBuilderFactory(){ - this.service.execute(null); + void nullUriBuilderFactory() { + assertThatIllegalArgumentException() + .isThrownBy(() -> this.service.execute(null)) + .withMessage("UriBuilderFactory is required"); + } + + @Test + void nullUriBuilderFactoryWithNullable(){ + this.service.executeNullable(null); assertThat(getRequestValues().getUriBuilderFactory()).isEqualTo(null); assertThat(getRequestValues().getUriTemplate()).isEqualTo("/path"); assertThat(getRequestValues().getUri()).isNull(); } + @Test + void nullUriBuilderFactoryWithOptional(){ + this.service.executeOptional(null); + + assertThat(getRequestValues().getUriBuilderFactory()).isEqualTo(null); + assertThat(getRequestValues().getUriTemplate()).isEqualTo("/path"); + assertThat(getRequestValues().getUri()).isNull(); + } + + @Test + void emptyOptionalUriBuilderFactory(){ + this.service.executeOptional(Optional.empty()); + + assertThat(getRequestValues().getUriBuilderFactory()).isEqualTo(null); + assertThat(getRequestValues().getUriTemplate()).isEqualTo("/path"); + assertThat(getRequestValues().getUri()).isNull(); + } + + @Test + void optionalUriBuilderFactory(){ + UriBuilderFactory factory = new DefaultUriBuilderFactory("https://example.com"); + this.service.executeOptional(Optional.of(factory)); + + assertThat(getRequestValues().getUriBuilderFactory()).isEqualTo(factory); + assertThat(getRequestValues().getUriTemplate()).isEqualTo("/path"); + assertThat(getRequestValues().getUri()).isNull(); + } private HttpRequestValues getRequestValues() { return this.client.getRequestValues(); } + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") private interface Service { @GetExchange("/path") - void execute(@Nullable UriBuilderFactory uri); + void execute(UriBuilderFactory uri); + + @GetExchange("/path") + void executeNullable(@Nullable UriBuilderFactory uri); + + @GetExchange("/path") + void executeOptional(Optional uri); } } diff --git a/spring-web/src/test/java/org/springframework/web/service/invoker/UrlArgumentResolverTests.java b/spring-web/src/test/java/org/springframework/web/service/invoker/UrlArgumentResolverTests.java index 668373ccf133..2039e851a48c 100644 --- a/spring-web/src/test/java/org/springframework/web/service/invoker/UrlArgumentResolverTests.java +++ b/spring-web/src/test/java/org/springframework/web/service/invoker/UrlArgumentResolverTests.java @@ -17,6 +17,7 @@ package org.springframework.web.service.invoker; import java.net.URI; +import java.util.Optional; import org.junit.jupiter.api.Test; @@ -24,13 +25,16 @@ 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; /** * Tests for {@link UrlArgumentResolver}. * * @author Rossen Stoyanchev + * @author Olga Maciaszek-Sharma */ +@SuppressWarnings({"DataFlowIssue", "OptionalAssignedToNull"}) class UrlArgumentResolverTests { private final TestExchangeAdapter client = new TestExchangeAdapter(); @@ -59,22 +63,62 @@ void notUrl() { } @Test - void ignoreNull() { - this.service.execute(null); + void nullUrl() { + assertThatIllegalArgumentException() + .isThrownBy(() -> this.service.execute(null)) + .withMessage("URI is required"); + } + + @Test + void nullUrlWithNullable() { + this.service.executeNullable(null); + + assertThat(getRequestValues().getUri()).isNull(); + assertThat(getRequestValues().getUriTemplate()).isEqualTo("/path"); + } + + @Test + void nullUrlWithOptional() { + this.service.executeOptional(null); assertThat(getRequestValues().getUri()).isNull(); assertThat(getRequestValues().getUriTemplate()).isEqualTo("/path"); } + @Test + void emptyOptionalUrl() { + this.service.executeOptional(Optional.empty()); + + assertThat(getRequestValues().getUri()).isNull(); + assertThat(getRequestValues().getUriTemplate()).isEqualTo("/path"); + } + + @Test + void optionalUrl() { + URI dynamicUrl = URI.create("dynamic-path"); + this.service.executeOptional(Optional.of(dynamicUrl)); + + assertThat(getRequestValues().getUri()).isEqualTo(dynamicUrl); + assertThat(getRequestValues().getUriTemplate()).isEqualTo("/path"); + } + + private HttpRequestValues getRequestValues() { return this.client.getRequestValues(); } + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") private interface Service { @GetExchange("/path") - void execute(@Nullable URI uri); + void execute(URI uri); + + @GetExchange("/path") + void executeNullable(@Nullable URI uri); + + @GetExchange("/path") + void executeOptional(Optional uri); @GetExchange void executeNotUri(String other); From d957f35f173a534eb86ca946a5b103d643c54db2 Mon Sep 17 00:00:00 2001 From: Olga Maciaszek-Sharma Date: Mon, 12 Aug 2024 12:32:47 +0200 Subject: [PATCH 2/2] Fix after review. --- .../service/PayloadArgumentResolver.java | 43 ++++++--------- .../service/PayloadArgumentResolverTests.java | 43 +-------------- .../invoker/HttpMethodArgumentResolver.java | 15 ++--- .../invoker/RequestBodyArgumentResolver.java | 55 +++++++++---------- .../UriBuilderFactoryArgumentResolver.java | 7 +-- .../service/invoker/UrlArgumentResolver.java | 7 +-- .../RequestBodyArgumentResolverTests.java | 23 -------- 7 files changed, 58 insertions(+), 135 deletions(-) diff --git a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolver.java b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolver.java index 4998b109a953..e8081efcedea 100644 --- a/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolver.java +++ b/spring-messaging/src/main/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolver.java @@ -16,8 +16,6 @@ package org.springframework.messaging.rsocket.service; -import java.util.Optional; - import org.springframework.core.MethodParameter; import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.ReactiveAdapter; @@ -57,35 +55,28 @@ public boolean resolve( return false; } - parameter = parameter.nestedIfOptional(); - - if (argument instanceof Optional optionalValue) { - argument = optionalValue.orElse(null); + if (argument == null) { + boolean required = (annot == null || annot.required()) && !parameter.isOptional(); + Assert.isTrue(!required, () -> "Missing payload"); + return true; } - if (argument != null) { - ReactiveAdapter reactiveAdapter = this.reactiveAdapterRegistry - .getAdapter(parameter.getNestedParameterType()); - if (reactiveAdapter == null) { - requestValues.setPayloadValue(argument); - } - else { - MethodParameter nestedParameter = parameter.nested(); + ReactiveAdapter reactiveAdapter = this.reactiveAdapterRegistry + .getAdapter(parameter.getParameterType()); + if (reactiveAdapter == null) { + requestValues.setPayloadValue(argument); + } + else { + MethodParameter nestedParameter = parameter.nested(); - String message = "Async type for @Payload should produce value(s)"; - Assert.isTrue(nestedParameter.getNestedParameterType() != Void.class, message); - Assert.isTrue(!reactiveAdapter.isNoValue(), message); + String message = "Async type for @Payload should produce value(s)"; + Assert.isTrue(nestedParameter.getNestedParameterType() != Void.class, message); + Assert.isTrue(!reactiveAdapter.isNoValue(), message); - requestValues.setPayload( - reactiveAdapter.toPublisher(argument), - ParameterizedTypeReference.forType(nestedParameter.getNestedGenericParameterType())); - } - return true; + requestValues.setPayload( + reactiveAdapter.toPublisher(argument), + ParameterizedTypeReference.forType(nestedParameter.getNestedGenericParameterType())); } - boolean required = (annot == null || annot.required()) && !parameter.isOptional(); - Assert.isTrue(!required, () -> "Missing payload"); - return true; } - } diff --git a/spring-messaging/src/test/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolverTests.java b/spring-messaging/src/test/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolverTests.java index 16c90c42de3d..f1f5ac9527d1 100644 --- a/spring-messaging/src/test/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolverTests.java +++ b/spring-messaging/src/test/java/org/springframework/messaging/rsocket/service/PayloadArgumentResolverTests.java @@ -16,8 +16,6 @@ package org.springframework.messaging.rsocket.service; -import java.util.Optional; - import io.reactivex.rxjava3.core.Completable; import io.reactivex.rxjava3.core.Single; import org.junit.jupiter.api.Test; @@ -128,41 +126,6 @@ void nullPayloadWithNotRequired() { assertNullValues(resolvedMono); } - @Test - void nullPayloadWithOptional() { - boolean resolved = execute(null, initMethodParameter(Service.class, "executeOptional", 0)); - assertNullValues(resolved); - - boolean resolvedMono = execute(null, initMethodParameter(Service.class, "executeOptionalMono", 0)); - assertNullValues(resolvedMono); - } - - @Test - void emptyPayloadWithOptional() { - boolean resolved = execute(Optional.empty(), initMethodParameter(Service.class, "executeOptional", 0)); - assertNullValues(resolved); - - boolean resolvedMono = execute(Optional.empty(), initMethodParameter(Service.class, "executeOptionalMono", 0)); - assertNullValues(resolvedMono); - } - - @Test - void optionalStringPayload() { - String payload = "payloadValue"; - boolean resolved = execute(Optional.of(payload), initMethodParameter(Service.class, "executeOptional", 0)); - - assertPayload(resolved, payload); - } - - @Test - void optionalMonoPayload() { - Mono payloadMono = Mono.just("payloadValue"); - boolean resolved = execute(Optional.of(payloadMono), initMethodParameter(Service.class, "executeOptionalMono", 0)); - - assertPayloadMono(resolved, payloadMono); - } - - private void assertPayload(boolean resolved, String payload) { assertThat(resolved).isTrue(); assertThat(getRequestValues().getPayloadValue()).isEqualTo(payload); @@ -183,7 +146,7 @@ private void assertNullValues(boolean resolved) { assertThat(getRequestValues().getPayloadElementType()).isNull(); } - @SuppressWarnings({"unused", "OptionalUsedAsFieldOrParameterType"}) + @SuppressWarnings({"unused"}) private interface Service { void execute(@Payload String body); @@ -194,14 +157,10 @@ private interface Service { void executeMono(@Payload Mono body); - void executeOptional(@Payload Optional body); - void executeNullableMono(@Nullable @Payload Mono body); void executeNotRequiredMono(@Payload(required = false) Mono body); - void executeOptionalMono(@Payload Optional> body); - void executeSingle(@Payload Single body); void executeMonoVoid(@Payload Mono body); 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 daaaf7c1fa44..4a8cdd161e85 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 @@ -53,15 +53,16 @@ public boolean resolve( argument = optionalValue.orElse(null); } - if(argument != null) { - HttpMethod httpMethod = (HttpMethod) argument; - requestValues.setHttpMethod(httpMethod); - if (logger.isTraceEnabled()) { - logger.trace("Resolved HTTP method to: " + httpMethod.name()); - } + if (argument == null) { + Assert.isTrue(parameter.isOptional(), "HttpMethod is required"); return true; } - Assert.isTrue(parameter.isOptional(), "HttpMethod is required"); + + HttpMethod httpMethod = (HttpMethod) argument; + requestValues.setHttpMethod(httpMethod); + if (logger.isTraceEnabled()) { + logger.trace("Resolved HTTP method to: " + httpMethod.name()); + } return true; } 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 index 2126853a81bc..e35edb9171b2 100644 --- 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 @@ -71,42 +71,39 @@ public boolean resolve( return false; } - parameter = parameter.nestedIfOptional(); - if (argument instanceof Optional optionalValue) { argument = optionalValue.orElse(null); } - if (argument != null) { - if (this.reactiveAdapterRegistry != null) { - ReactiveAdapter adapter = this.reactiveAdapterRegistry - .getAdapter(parameter.getNestedParameterType()); - if (adapter != null) { - MethodParameter nestedParameter = parameter.nested(); - - String message = "Async type for @RequestBody should produce value(s)"; - Assert.isTrue(!adapter.isNoValue(), message); - Assert.isTrue(nestedParameter.getNestedParameterType() != Void.class, message); - - if (requestValues instanceof ReactiveHttpRequestValues.Builder reactiveRequestValues) { - reactiveRequestValues.setBodyPublisher( - adapter.toPublisher(argument), asParameterizedTypeRef(nestedParameter)); - } - else { - throw new IllegalStateException( - "RequestBody with a reactive type is only supported with reactive client"); - } - - return true; - } - } - - // Not a reactive type - requestValues.setBodyValue(argument); + if (argument == null) { + Assert.isTrue(!annot.required() || parameter.isOptional(), "RequestBody is required"); return true; } - Assert.isTrue(!annot.required() || parameter.isOptional(), "RequestBody is required"); + if (this.reactiveAdapterRegistry != null) { + ReactiveAdapter adapter = this.reactiveAdapterRegistry + .getAdapter(parameter.getParameterType()); + if (adapter != null) { + MethodParameter nestedParameter = parameter.nested(); + + String message = "Async type for @RequestBody should produce value(s)"; + Assert.isTrue(!adapter.isNoValue(), message); + Assert.isTrue(nestedParameter.getNestedParameterType() != Void.class, message); + + if (requestValues instanceof ReactiveHttpRequestValues.Builder reactiveRequestValues) { + reactiveRequestValues.setBodyPublisher( + adapter.toPublisher(argument), asParameterizedTypeRef(nestedParameter)); + } + else { + throw new IllegalStateException( + "RequestBody with a reactive type is only supported with reactive client"); + } + + return true; + } + } + // Not a reactive type + requestValues.setBodyValue(argument); return true; } diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolver.java index ffefc07c24a1..0c89f905e593 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/UriBuilderFactoryArgumentResolver.java @@ -54,13 +54,12 @@ public boolean resolve( argument = optionalValue.orElse(null); } - if (argument != null) { - requestValues.setUriBuilderFactory((UriBuilderFactory) argument); + if (argument == null) { + Assert.isTrue(parameter.isOptional(), "UriBuilderFactory is required"); return true; } - Assert.isTrue(parameter.isOptional(), "UriBuilderFactory is required"); - + requestValues.setUriBuilderFactory((UriBuilderFactory) argument); return true; } } diff --git a/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java index 1a7090bd161d..e3840a55347c 100644 --- a/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java +++ b/spring-web/src/main/java/org/springframework/web/service/invoker/UrlArgumentResolver.java @@ -47,13 +47,12 @@ public boolean resolve( argument = optionalValue.orElse(null); } - if (argument != null) { - requestValues.setUri((URI) argument); + if (argument == null) { + Assert.isTrue(parameter.isOptional(), "URI is required"); return true; } - Assert.isTrue(parameter.isOptional(), "URI is required"); - + requestValues.setUri((URI) argument); return true; } 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 index 3d730897886b..3b82d00164a1 100644 --- 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 @@ -148,11 +148,6 @@ void nullRequestBodyWithOptional() { assertThat(getBodyValue()).isNull(); assertThat(getPublisherBody()).isNull(); - - this.service.executeOptionalMono(null); - - assertThat(getBodyValue()).isNull(); - assertThat(getPublisherBody()).isNull(); } @Test @@ -161,11 +156,6 @@ void emptyOptionalRequestBody() { assertThat(getBodyValue()).isNull(); assertThat(getPublisherBody()).isNull(); - - this.service.executeOptionalMono(Optional.empty()); - - assertThat(getBodyValue()).isNull(); - assertThat(getPublisherBody()).isNull(); } @Test @@ -177,16 +167,6 @@ void optionalStringBody() { assertThat(getPublisherBody()).isNull(); } - @Test - void optionalMonoBody() { - Mono bodyMono = Mono.just("bodyValue"); - this.service.executeOptionalMono(Optional.of(bodyMono)); - - assertThat(getBodyValue()).isNull(); - assertThat(getPublisherBody()).isSameAs(bodyMono); - assertThat(getBodyElementType()).isEqualTo(new ParameterizedTypeReference() { }); - } - @Nullable private Object getBodyValue() { @@ -232,9 +212,6 @@ private interface Service { @GetExchange void executeNotRequiredMono(@RequestBody(required = false) Mono body); - @GetExchange - void executeOptionalMono(@RequestBody Optional> body); - @GetExchange void executeSingle(@RequestBody Single body);