Skip to content

Commit

Permalink
Reject null for non-optional arguments
Browse files Browse the repository at this point in the history
Closes gh-33339
  • Loading branch information
OlgaMaciaszek authored and rstoyanchev committed Aug 12, 2024
1 parent 4ac4c1b commit 51de84e
Show file tree
Hide file tree
Showing 14 changed files with 376 additions and 79 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -978,6 +978,9 @@ method parameters:
`MultiValueMap<String, ?>` 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`.

|===


Expand Down
3 changes: 2 additions & 1 deletion framework-docs/modules/ROOT/pages/rsocket.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -29,6 +29,7 @@
* annotated arguments.
*
* @author Rossen Stoyanchev
* @author Olga Maciaszek-Sharma
* @since 6.0
*/
public class PayloadArgumentResolver implements RSocketServiceArgumentResolver {
Expand All @@ -54,25 +55,28 @@ public boolean resolve(
return false;
}

if (argument != null) {
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);
if (argument == null) {
boolean required = (annot == null || annot.required()) && !parameter.isOptional();
Assert.isTrue(!required, () -> "Missing payload");
return true;
}

requestValues.setPayload(
reactiveAdapter.toPublisher(argument),
ParameterizedTypeReference.forType(nestedParameter.getNestedGenericParameterType()));
}
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);

requestValues.setPayload(
reactiveAdapter.toPublisher(argument),
ParameterizedTypeReference.forType(nestedParameter.getNestedGenericParameterType()));
}
return true;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,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;
Expand All @@ -34,7 +35,9 @@
* Tests for {@link PayloadArgumentResolver}.
*
* @author Rossen Stoyanchev
* @author Olga Maciaszek-Sharma
*/
@SuppressWarnings("DataFlowIssue")
class PayloadArgumentResolverTests extends RSocketServiceArgumentResolverTestSupport {

@Override
Expand All @@ -47,20 +50,15 @@ 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
void monoPayload() {
Mono<String> 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<String>() {});
assertPayloadMono(resolved, payloadMono);
}

@Test
Expand Down Expand Up @@ -92,31 +90,77 @@ void completable() {
}

@Test
void notRequestBody() {
void notPayload() {
MethodParameter parameter = initMethodParameter(Service.class, "executeNotAnnotated", 0);
boolean resolved = execute("value", parameter);

assertThat(resolved).isFalse();
}

@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);
}

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<String> payloadMono) {
assertThat(resolved).isTrue();
assertThat(getRequestValues().getPayloadValue()).isNull();
assertThat(getRequestValues().getPayload()).isSameAs(payloadMono);
assertThat(getRequestValues().getPayloadElementType()).isEqualTo(new ParameterizedTypeReference<String>() { });
}

private void assertNullValues(boolean resolved) {
assertThat(resolved).isTrue();
assertThat(getRequestValues().getPayloadValue()).isNull();
assertThat(getRequestValues().getPayload()).isNull();
assertThat(getRequestValues().getPayloadElementType()).isNull();
}


@SuppressWarnings("unused")
@SuppressWarnings({"unused"})
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<String> body);

void executeNullableMono(@Nullable @Payload Mono<String> body);

void executeNotRequiredMono(@Payload(required = false) Mono<String> body);

void executeSingle(@Payload Single<String> body);

void executeMonoVoid(@Payload Mono<Void> body);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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;

Expand All @@ -41,17 +43,26 @@ 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");
if (argument instanceof Optional<?> optionalValue) {
argument = optionalValue.orElse(null);
}

if (argument == null) {
Assert.isTrue(parameter.isOptional(), "HttpMethod is required");
return true;
}

HttpMethod httpMethod = (HttpMethod) argument;
requestValues.setHttpMethod(httpMethod);
if (logger.isTraceEnabled()) {
logger.trace("Resolved HTTP method to: " + httpMethod.name());
}

return true;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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;
Expand All @@ -30,6 +32,7 @@
* annotated arguments.
*
* @author Rossen Stoyanchev
* @author Olga Maciaszek-Sharma
* @since 6.0
*/
public class RequestBodyArgumentResolver implements HttpServiceArgumentResolver {
Expand Down Expand Up @@ -68,33 +71,39 @@ public boolean resolve(
return false;
}

if (argument != null) {
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;
}
}
if (argument instanceof Optional<?> optionalValue) {
argument = optionalValue.orElse(null);
}

// Not a reactive type
requestValues.setBodyValue(argument);
if (argument == null) {
Assert.isTrue(!annot.required() || parameter.isOptional(), "RequestBody is required");
return true;
}

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;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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;

Expand All @@ -42,14 +44,22 @@ 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 != null) {
requestValues.setUriBuilderFactory((UriBuilderFactory) argument);
if (argument instanceof Optional<?> optionalValue) {
argument = optionalValue.orElse(null);
}

if (argument == null) {
Assert.isTrue(parameter.isOptional(), "UriBuilderFactory is required");
return true;
}

requestValues.setUriBuilderFactory((UriBuilderFactory) argument);
return true;
}
}
Loading

0 comments on commit 51de84e

Please sign in to comment.