Skip to content

Commit

Permalink
Resolve property-dependent parameter names for exception messages
Browse files Browse the repository at this point in the history 
Prior to this commit when a required parameter defined as a property or
expression placeholder was missing, the exception thrown would refer to
the placeholder instead of the resolved name.

This change covers messaging handlers and web controllers, both blocking
and reactive. It also fixes the error message when handling null values
for non-required parameters, as well as in cases that need conversion.

See gh-32323
Closes gh-32462
  • Loading branch information
@andrea-mauro @simonbasle
andrea-mauro authored and simonbasle committed Mar 20, 2024
1 parent 5698191 commit 458c30c
Show file tree
Hide file tree
Showing 9 changed files with 234 additions and 19 deletions.
6 changes: 3 additions & 3 deletions ...ework/messaging/handler/annotation/reactive/AbstractNamedValueMethodArgumentResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@ public Object resolveArgumentValue(MethodParameter parameter, Message<?> message
arg = resolveEmbeddedValuesAndExpressions(namedValueInfo.defaultValue);
}
else if (namedValueInfo.required && !nestedParameter.isOptional()) {
handleMissingValue(namedValueInfo.name, nestedParameter, message);
handleMissingValue(resolvedName.toString(), nestedParameter, message);
}
arg = handleNullValue(namedValueInfo.name, arg, nestedParameter.getNestedParameterType());
arg = handleNullValue(resolvedName.toString(), arg, nestedParameter.getNestedParameterType());
}
else if ("".equals(arg) && namedValueInfo.defaultValue != null) {
arg = resolveEmbeddedValuesAndExpressions(namedValueInfo.defaultValue);
Expand All @@ -113,7 +113,7 @@ else if ("".equals(arg) && namedValueInfo.defaultValue != null) {
arg = resolveEmbeddedValuesAndExpressions(namedValueInfo.defaultValue);
}
else if (namedValueInfo.required && !nestedParameter.isOptional()) {
handleMissingValue(namedValueInfo.name, nestedParameter, message);
handleMissingValue(resolvedName.toString(), nestedParameter, message);
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions ...mework/messaging/handler/annotation/support/AbstractNamedValueMethodArgumentResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ public Object resolveArgument(MethodParameter parameter, Message<?> message) thr
arg = resolveEmbeddedValuesAndExpressions(namedValueInfo.defaultValue);
}
else if (namedValueInfo.required && !nestedParameter.isOptional()) {
handleMissingValue(namedValueInfo.name, nestedParameter, message);
handleMissingValue(resolvedName.toString(), nestedParameter, message);
}
arg = handleNullValue(namedValueInfo.name, arg, nestedParameter.getNestedParameterType());
arg = handleNullValue(resolvedName.toString(), arg, nestedParameter.getNestedParameterType());
}
else if ("".equals(arg) && namedValueInfo.defaultValue != null) {
arg = resolveEmbeddedValuesAndExpressions(namedValueInfo.defaultValue);
Expand All @@ -121,7 +121,7 @@ else if ("".equals(arg) && namedValueInfo.defaultValue != null) {
arg = resolveEmbeddedValuesAndExpressions(namedValueInfo.defaultValue);
}
else if (namedValueInfo.required && !nestedParameter.isOptional()) {
handleMissingValue(namedValueInfo.name, nestedParameter, message);
handleMissingValue(resolvedName.toString(), nestedParameter, message);
}
}
}
Expand Down
35 changes: 34 additions & 1 deletion ...ingframework/messaging/handler/annotation/reactive/HeaderMethodArgumentResolverTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.springframework.messaging.handler.annotation.MessagingPredicates.header;
import static org.springframework.messaging.handler.annotation.MessagingPredicates.headerPlain;

Expand Down Expand Up @@ -151,6 +152,37 @@ void resolveOptionalHeaderAsEmpty() {
assertThat(result).isEqualTo(Optional.empty());
}

@Test
void missingParameterFromSystemPropertyThroughPlaceholder() {
String expected = "sysbar";
System.setProperty("systemProperty", expected);
Message<byte[]> message = MessageBuilder.withPayload(new byte[0]).build();
MethodParameter param = this.resolvable.annot(header("#{systemProperties.systemProperty}")).arg();

assertThatThrownBy(() ->
resolveArgument(param, message))
.isInstanceOf(MessageHandlingException.class)
.hasMessageContaining(expected);

System.clearProperty("systemProperty");
}

@Test
void notNullablePrimitiveParameterFromSystemPropertyThroughPlaceholder() {
String expected = "sysbar";
System.setProperty("systemProperty", expected);
Message<byte[]> message = MessageBuilder.withPayload(new byte[0]).build();
MethodParameter param = this.resolvable.annot(header("${systemProperty}").required(false)).arg();

assertThatThrownBy(() ->
resolver.resolveArgument(param, message))
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining(expected);

System.clearProperty("systemProperty");
}


@SuppressWarnings({"unchecked", "ConstantConditions"})
private <T> T resolveArgument(MethodParameter param, Message<?> message) {
return (T) this.resolver.resolveArgument(param, message).block(Duration.ofSeconds(5));
Expand All @@ -165,7 +197,8 @@ public void handleMessage(
@Header(name = "#{systemProperties.systemProperty}") String param4,
String param5,
@Header("foo") Optional<String> param6,
@Header("nativeHeaders.param1") String nativeHeaderParam1) {
@Header("nativeHeaders.param1") String nativeHeaderParam1,
@Header(name = "${systemProperty}", required = false) int primitivePlaceholderParam) {
}


Expand Down
34 changes: 33 additions & 1 deletion ...ringframework/messaging/handler/annotation/support/HeaderMethodArgumentResolverTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.springframework.messaging.handler.annotation.MessagingPredicates.header;
import static org.springframework.messaging.handler.annotation.MessagingPredicates.headerPlain;

Expand Down Expand Up @@ -137,6 +138,36 @@ void resolveNameFromSystemProperty() throws Exception {
}
}

@Test
void missingParameterFromSystemPropertyThroughPlaceholder() {
String expected = "sysbar";
System.setProperty("systemProperty", expected);
Message<byte[]> message = MessageBuilder.withPayload(new byte[0]).build();
MethodParameter param = this.resolvable.annot(header("#{systemProperties.systemProperty}")).arg();

assertThatThrownBy(() ->
resolver.resolveArgument(param, message))
.isInstanceOf(MessageHandlingException.class)
.hasMessageContaining(expected);

System.clearProperty("systemProperty");
}

@Test
void notNullablePrimitiveParameterFromSystemPropertyThroughPlaceholder() {
String expected = "sysbar";
System.setProperty("systemProperty", expected);
Message<byte[]> message = MessageBuilder.withPayload(new byte[0]).build();
MethodParameter param = this.resolvable.annot(header("${systemProperty}").required(false)).arg();

assertThatThrownBy(() ->
resolver.resolveArgument(param, message))
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining(expected);

System.clearProperty("systemProperty");
}

@Test
void resolveOptionalHeaderWithValue() throws Exception {
Message<String> message = MessageBuilder.withPayload("foo").setHeader("foo", "bar").build();
Expand All @@ -162,7 +193,8 @@ public void handleMessage(
@Header(name = "#{systemProperties.systemProperty}") String param4,
String param5,
@Header("foo") Optional<String> param6,
@Header("nativeHeaders.param1") String nativeHeaderParam1) {
@Header("nativeHeaders.param1") String nativeHeaderParam1,
@Header(name = "${systemProperty}", required = false) int primitivePlaceholderParam) {
}


Expand Down
6 changes: 3 additions & 3 deletions ...a/org/springframework/web/method/annotation/AbstractNamedValueMethodArgumentResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,10 +123,10 @@ public final Object resolveArgument(MethodParameter parameter, @Nullable ModelAn
arg = resolveEmbeddedValuesAndExpressions(namedValueInfo.defaultValue);
}
else if (namedValueInfo.required && !nestedParameter.isOptional()) {
handleMissingValue(namedValueInfo.name, nestedParameter, webRequest);
handleMissingValue(resolvedName.toString(), nestedParameter, webRequest);
}
if (!hasDefaultValue) {
arg = handleNullValue(namedValueInfo.name, arg, nestedParameter.getNestedParameterType());
arg = handleNullValue(resolvedName.toString(), arg, nestedParameter.getNestedParameterType());
}
}
else if ("".equals(arg) && namedValueInfo.defaultValue != null) {
Expand All @@ -142,7 +142,7 @@ else if ("".equals(arg) && namedValueInfo.defaultValue != null) {
arg = convertIfNecessary(parameter, webRequest, binderFactory, namedValueInfo, arg);
}
else if (namedValueInfo.required && !nestedParameter.isOptional()) {
handleMissingValueAfterConversion(namedValueInfo.name, nestedParameter, webRequest);
handleMissingValueAfterConversion(resolvedName.toString(), nestedParameter, webRequest);
}
}
}
Expand Down
37 changes: 36 additions & 1 deletion ...a/org/springframework/web/method/annotation/RequestHeaderMethodArgumentResolverTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ class RequestHeaderMethodArgumentResolverTests {
private MethodParameter paramInstant;
private MethodParameter paramUuid;
private MethodParameter paramUuidOptional;
private MethodParameter paramUuidPlaceholder;

private MockHttpServletRequest servletRequest;

Expand All @@ -93,6 +94,7 @@ void setup() throws Exception {
paramInstant = new SynthesizingMethodParameter(method, 8);
paramUuid = new SynthesizingMethodParameter(method, 9);
paramUuidOptional = new SynthesizingMethodParameter(method, 10);
paramUuidPlaceholder = new SynthesizingMethodParameter(method, 11);

servletRequest = new MockHttpServletRequest();
webRequest = new ServletWebRequest(servletRequest, new MockHttpServletResponse());
Expand Down Expand Up @@ -186,6 +188,20 @@ void resolveNameFromSystemPropertyThroughPlaceholder() throws Exception {
}
}

@Test
void missingParameterFromSystemPropertyThroughPlaceholder() {
String expected = "bar";

System.setProperty("systemProperty", expected);

assertThatThrownBy(() ->
resolver.resolveArgument(paramResolvedNameWithPlaceholder, null, webRequest, null))
.isInstanceOf(MissingRequestHeaderException.class)
.extracting("headerName").isEqualTo(expected);

System.clearProperty("systemProperty");
}

@Test
void resolveDefaultValueFromRequest() throws Exception {
servletRequest.setContextPath("/bar");
Expand Down Expand Up @@ -296,6 +312,24 @@ private void uuidConversionWithEmptyOrBlankValueOptional(String uuid) throws Exc
assertThat(result).isNull();
}

@Test
public void uuidPlaceholderConversionWithEmptyValue() {
String expected = "name";
servletRequest.addHeader(expected, "");

System.setProperty("systemProperty", expected);

ConfigurableWebBindingInitializer bindingInitializer = new ConfigurableWebBindingInitializer();
bindingInitializer.setConversionService(new DefaultFormattingConversionService());

assertThatThrownBy(() ->
resolver.resolveArgument(paramUuidPlaceholder, null, webRequest,
new DefaultDataBinderFactory(bindingInitializer)))
.isInstanceOf(MissingRequestHeaderException.class)
.extracting("headerName").isEqualTo(expected);

System.clearProperty("systemProperty");
}

void params(
@RequestHeader(name = "name", defaultValue = "bar") String param1,
Expand All @@ -308,7 +342,8 @@ void params(
@RequestHeader("name") Date dateParam,
@RequestHeader("name") Instant instantParam,
@RequestHeader("name") UUID uuid,
@RequestHeader(name = "name", required = false) UUID uuidOptional) {
@RequestHeader(name = "name", required = false) UUID uuidOptional,
@RequestHeader(name = "${systemProperty}") UUID uuidPlaceholder) {
}

}
74 changes: 72 additions & 2 deletions ...va/org/springframework/web/method/annotation/RequestParamMethodArgumentResolverTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import jakarta.servlet.http.Part;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import org.springframework.beans.propertyeditors.StringTrimmerEditor;
Expand All @@ -37,7 +38,9 @@
import org.springframework.web.bind.support.WebDataBinderFactory;
import org.springframework.web.bind.support.WebRequestDataBinder;
import org.springframework.web.context.request.NativeWebRequest;
import org.springframework.web.context.request.RequestContextHolder;
import org.springframework.web.context.request.ServletWebRequest;
import org.springframework.web.context.support.GenericWebApplicationContext;
import org.springframework.web.multipart.MultipartException;
import org.springframework.web.multipart.MultipartFile;
import org.springframework.web.multipart.support.MissingServletRequestPartException;
Expand All @@ -50,6 +53,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.BDDMockito.given;
import static org.mockito.Mockito.mock;
import static org.springframework.web.testfixture.method.MvcAnnotationPredicates.requestParam;
Expand All @@ -64,14 +68,23 @@
*/
class RequestParamMethodArgumentResolverTests {

private RequestParamMethodArgumentResolver resolver = new RequestParamMethodArgumentResolver(null, true);
private RequestParamMethodArgumentResolver resolver;

private MockHttpServletRequest request = new MockHttpServletRequest();

private NativeWebRequest webRequest = new ServletWebRequest(request, new MockHttpServletResponse());

private ResolvableMethod testMethod = ResolvableMethod.on(getClass()).named("handle").build();

@BeforeEach
void setup() {
GenericWebApplicationContext context = new GenericWebApplicationContext();
context.refresh();
resolver = new RequestParamMethodArgumentResolver(context.getBeanFactory(), true);

// Expose request to the current thread (for SpEL expressions)
RequestContextHolder.setRequestAttributes(webRequest);
}

@Test
void supportsParameter() {
Expand Down Expand Up @@ -141,6 +154,12 @@ void supportsParameter() {

param = this.testMethod.annotPresent(RequestPart.class).arg(MultipartFile.class);
assertThat(resolver.supportsParameter(param)).isFalse();

param = this.testMethod.annotPresent(RequestParam.class).arg(Integer.class);
assertThat(resolver.supportsParameter(param)).isTrue();

param = this.testMethod.annotPresent(RequestParam.class).arg(int.class);
assertThat(resolver.supportsParameter(param)).isTrue();
}

@Test
Expand Down Expand Up @@ -678,6 +697,55 @@ void optionalMultipartFileWithoutMultipartRequest() throws Exception {
assertThat(actual).isEqualTo(Optional.empty());
}

@Test
void resolveNameFromSystemPropertyThroughPlaceholder() throws Exception {
ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
initializer.setConversionService(new DefaultConversionService());
WebDataBinderFactory binderFactory = new DefaultDataBinderFactory(initializer);

Integer expected = 100;
request.addParameter("name", expected.toString());

System.setProperty("systemProperty", "name");

try {
MethodParameter param = this.testMethod.annot(requestParam().name("${systemProperty}")).arg(Integer.class);
Object result = resolver.resolveArgument(param, null, webRequest, binderFactory);
boolean condition = result instanceof Integer;
assertThat(condition).isTrue();
}
finally {
System.clearProperty("systemProperty");
}
}

@Test
void missingParameterFromSystemPropertyThroughPlaceholder() {
String expected = "name";
System.setProperty("systemProperty", expected);

MethodParameter param = this.testMethod.annot(requestParam().name("${systemProperty}")).arg(Integer.class);
assertThatThrownBy(() ->
resolver.resolveArgument(param, null, webRequest, null))
.isInstanceOf(MissingServletRequestParameterException.class)
.extracting("parameterName").isEqualTo(expected);

System.clearProperty("systemProperty");
}

@Test
void notNullablePrimitiveParameterFromSystemPropertyThroughPlaceholder() {
String expected = "sysbar";
System.setProperty("systemProperty", expected);

MethodParameter param = this.testMethod.annot(requestParam().name("${systemProperty}").notRequired()).arg(int.class);
assertThatThrownBy(() ->
resolver.resolveArgument(param, null, webRequest, null))
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining(expected);

System.clearProperty("systemProperty");
}

@SuppressWarnings({"unused", "OptionalUsedAsFieldOrParameterType"})
public void handle(
Expand All @@ -702,7 +770,9 @@ public void handle(
@RequestParam("name") Optional<Integer[]> paramOptionalArray,
@RequestParam("name") Optional<List<?>> paramOptionalList,
@RequestParam("mfile") Optional<MultipartFile> multipartFileOptional,
@RequestParam(defaultValue = "false") Boolean booleanParam) {
@RequestParam(defaultValue = "false") Boolean booleanParam,
@RequestParam("${systemProperty}") Integer placeholderParam,
@RequestParam(name = "${systemProperty}", required = false) int primitivePlaceholderParam) {
}

}
Loading

0 comments on commit 458c30c

Please sign in to comment.