@PathVariable supports 'required' attribute (for model attribute methods)

Issue: SPR-14646

spring-web-reactive/src/main/java/org/springframework/web/reactive/result/method/annotation/PathVariableMethodArgumentResolver.java

@@ -48,12 +48,12 @@ import org.springframework.web.server.ServerWebExchange;
  * {@link Converter}.
  *
  * @author Rossen Stoyanchev
+ * @author Juergen Hoeller
  * @since 5.0
  * @see PathVariableMapMethodArgumentResolver
  */
 public class PathVariableMethodArgumentResolver extends AbstractNamedValueMethodArgumentResolver {
 
-
 	public PathVariableMethodArgumentResolver(ConversionService conversionService,
 			ConfigurableBeanFactory beanFactory) {
 
@@ -108,7 +108,7 @@ public class PathVariableMethodArgumentResolver extends AbstractNamedValueMethod
 	private static class PathVariableNamedValueInfo extends NamedValueInfo {
 
 		public PathVariableNamedValueInfo(PathVariable annotation) {
-			super(annotation.value(), true, ValueConstants.DEFAULT_NONE);
+			super(annotation.name(), annotation.required(), ValueConstants.DEFAULT_NONE);
 		}
 	}
 

spring-web-reactive/src/test/java/org/springframework/web/reactive/result/method/annotation/PathVariableMethodArgumentResolverTests.java

@@ -20,12 +20,14 @@ import java.lang.reflect.Method;
 import java.net.URI;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 
 import org.junit.Before;
 import org.junit.Test;
 import reactor.core.publisher.Mono;
 
 import org.springframework.core.MethodParameter;
+import org.springframework.core.annotation.SynthesizingMethodParameter;
 import org.springframework.core.convert.ConversionService;
 import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.http.HttpMethod;
@@ -34,6 +36,7 @@ import org.springframework.http.server.reactive.MockServerHttpResponse;
 import org.springframework.http.server.reactive.ServerHttpRequest;
 import org.springframework.tests.TestSubscriber;
 import org.springframework.ui.ModelMap;
+import org.springframework.util.ReflectionUtils;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.reactive.HandlerMapping;
 import org.springframework.web.server.ServerErrorException;
@@ -42,14 +45,13 @@ import org.springframework.web.server.adapter.DefaultServerWebExchange;
 import org.springframework.web.server.session.MockWebSessionManager;
 import org.springframework.web.server.session.WebSessionManager;
 
-import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.*;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 
 /**
  * Unit tests for {@link PathVariableMethodArgumentResolver}.
  *
  * @author Rossen Stoyanchev
+ * @author Juergen Hoeller
  */
 public class PathVariableMethodArgumentResolverTests {
 
@@ -58,8 +60,13 @@ public class PathVariableMethodArgumentResolverTests {
 	private ServerWebExchange exchange;
 
 	private MethodParameter paramNamedString;
+
 	private MethodParameter paramString;
 
+	private MethodParameter paramNotRequired;
+
+	private MethodParameter paramOptional;
+
 
 	@Before
 	public void setUp() throws Exception {
@@ -70,9 +77,11 @@ public class PathVariableMethodArgumentResolverTests {
 		WebSessionManager sessionManager = new MockWebSessionManager();
 		this.exchange = new DefaultServerWebExchange(request, new MockServerHttpResponse(), sessionManager);
 
-		Method method = getClass().getMethod("handle", String.class, String.class);
+		Method method = ReflectionUtils.findMethod(getClass(), "handle", (Class<?>[]) null);
-		this.paramNamedString = new MethodParameter(method, 0);
+		paramNamedString = new SynthesizingMethodParameter(method, 0);
-		this.paramString = new MethodParameter(method, 1);
+		paramString = new SynthesizingMethodParameter(method, 1);
+		paramNotRequired = new SynthesizingMethodParameter(method, 2);
+		paramOptional = new SynthesizingMethodParameter(method, 3);
 	}
 
 
@@ -90,11 +99,31 @@ public class PathVariableMethodArgumentResolverTests {
 
 		Mono<Object> mono = this.resolver.resolveArgument(this.paramNamedString, new ModelMap(), this.exchange);
 		Object result = mono.block();
-
-		assertTrue(result instanceof String);
 		assertEquals("value", result);
 	}
 
+	@Test
+	public void resolveArgumentNotRequired() throws Exception {
+		Map<String, String> uriTemplateVars = new HashMap<>();
+		uriTemplateVars.put("name", "value");
+		this.exchange.getAttributes().put(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVars);
+
+		Mono<Object> mono = this.resolver.resolveArgument(this.paramNotRequired, new ModelMap(), this.exchange);
+		Object result = mono.block();
+		assertEquals("value", result);
+	}
+
+	@Test
+	public void resolveArgumentWrappedAsOptional() throws Exception {
+		Map<String, String> uriTemplateVars = new HashMap<>();
+		uriTemplateVars.put("name", "value");
+		this.exchange.getAttributes().put(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVars);
+
+		Mono<Object> mono = this.resolver.resolveArgument(this.paramOptional, new ModelMap(), this.exchange);
+		Object result = mono.block();
+		assertEquals(Optional.of("value"), result);
+	}
+
 	@Test
 	public void handleMissingValue() throws Exception {
 		Mono<Object> mono = this.resolver.resolveArgument(this.paramNamedString, new ModelMap(), this.exchange);
@@ -103,8 +132,29 @@ public class PathVariableMethodArgumentResolverTests {
 				.assertError(ServerErrorException.class);
 	}
 
-	@SuppressWarnings("unused")
+	@Test
-	public void handle(@PathVariable(value = "name") String param1, String param2) {
+	public void nullIfNotRequired() throws Exception {
+		Mono<Object> mono = this.resolver.resolveArgument(this.paramNotRequired, new ModelMap(), this.exchange);
+		TestSubscriber
+				.subscribe(mono)
+				.assertComplete()
+				.assertNoValues();
 	}
 
-}
+	@Test
+	public void wrapEmptyWithOptional() throws Exception {
+		Mono<Object> mono = this.resolver.resolveArgument(this.paramOptional, new ModelMap(), this.exchange);
+		Object result = mono.block();
+		TestSubscriber
+				.subscribe(mono)
+				.assertValues(Optional.empty());
+	}
+
+
+	@SuppressWarnings("unused")
+	public void handle(@PathVariable(value = "name") String param1, String param2,
+			@PathVariable(name="name", required = false) String param3,
+			@PathVariable("name") Optional<String> param4) {
+	}
+
+}

spring-web/src/main/java/org/springframework/web/bind/annotation/PathVariable.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2016 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.
@@ -22,6 +22,8 @@ import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;
 
+import org.springframework.core.annotation.AliasFor;
+
 /**
  * Annotation which indicates that a method parameter should be bound to a URI template
  * variable. Supported for {@link RequestMapping} annotated handler methods in Servlet
@@ -32,6 +34,7 @@ import java.lang.annotation.Target;
  * then the map is populated with all path variable names and values.
  *
  * @author Arjen Poutsma
+ * @author Juergen Hoeller
  * @since 3.0
  * @see RequestMapping
  * @see org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter
@@ -42,8 +45,26 @@ import java.lang.annotation.Target;
 public @interface PathVariable {
 
 	/**
-	 * The URI template variable to bind to.
+	 * Alias for {@link #name}.
 	 */
+	@AliasFor("name")
 	String value() default "";
 
+	/**
+	 * The name of the path variable to bind to.
+	 * @since 4.3.3
+	 */
+	@AliasFor("value")
+	String name() default "";
+
+	/**
+	 * Whether the path variable is required.
+	 * <p>Defaults to {@code true}, leading to an exception being thrown if the path
+	 * variable is missing in the incoming request. Switch this to {@code false} if
+	 * you prefer a {@code null} or Java 8 {@code java.util.Optional} in this case.
+	 * e.g. on a {@code ModelAttribute} method which serves for different requests.
+	 * @since 4.3.3
+	 */
+	boolean required() default true;
+
 }

spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/PathVariableMethodArgumentResolver.java

@@ -57,6 +57,7 @@ import org.springframework.web.util.UriComponentsBuilder;
  *
  * @author Rossen Stoyanchev
  * @author Arjen Poutsma
+ * @author Juergen Hoeller
  * @since 3.1
  */
 public class PathVariableMethodArgumentResolver extends AbstractNamedValueMethodArgumentResolver
@@ -65,10 +66,6 @@ public class PathVariableMethodArgumentResolver extends AbstractNamedValueMethod
 	private static final TypeDescriptor STRING_TYPE_DESCRIPTOR = TypeDescriptor.valueOf(String.class);
 
 
-	public PathVariableMethodArgumentResolver() {
-	}
-
-
 	@Override
 	public boolean supportsParameter(MethodParameter parameter) {
 		if (!parameter.hasParameterAnnotation(PathVariable.class)) {
@@ -96,9 +93,7 @@ public class PathVariableMethodArgumentResolver extends AbstractNamedValueMethod
 	}
 
 	@Override
-	protected void handleMissingValue(String name, MethodParameter parameter)
+	protected void handleMissingValue(String name, MethodParameter parameter) throws ServletRequestBindingException {
-			throws ServletRequestBindingException {
-
 		throw new MissingPathVariableException(name, parameter);
 	}
 
@@ -121,13 +116,13 @@ public class PathVariableMethodArgumentResolver extends AbstractNamedValueMethod
 	public void contributeMethodArgument(MethodParameter parameter, Object value,
 			UriComponentsBuilder builder, Map<String, Object> uriVariables, ConversionService conversionService) {
 
-		if (Map.class.isAssignableFrom(parameter.getNestedParameterType())) {
+		if (Map.class.isAssignableFrom(parameter.nestedIfOptional().getNestedParameterType())) {
 			return;
 		}
 
 		PathVariable ann = parameter.getParameterAnnotation(PathVariable.class);
-		String name = (ann == null || StringUtils.isEmpty(ann.value()) ? parameter.getParameterName() : ann.value());
+		String name = (ann != null && !StringUtils.isEmpty(ann.value()) ? ann.value() : parameter.getParameterName());
-		value = formatUriValue(conversionService, new TypeDescriptor(parameter), value);
+		value = formatUriValue(conversionService, new TypeDescriptor(parameter.nestedIfOptional()), value);
 		uriVariables.put(name, value);
 	}
 
@@ -150,7 +145,7 @@ public class PathVariableMethodArgumentResolver extends AbstractNamedValueMethod
 	private static class PathVariableNamedValueInfo extends NamedValueInfo {
 
 		public PathVariableNamedValueInfo(PathVariable annotation) {
-			super(annotation.value(), true, ValueConstants.DEFAULT_NONE);
+			super(annotation.name(), annotation.required(), ValueConstants.DEFAULT_NONE);
 		}
 	}
 

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/PathVariableMethodArgumentResolverTests.java

@@ -16,29 +16,37 @@
 
 package org.springframework.web.servlet.mvc.method.annotation;
 
-import static org.junit.Assert.*;
-
 import java.lang.reflect.Method;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 
 import org.junit.Before;
 import org.junit.Test;
 
 import org.springframework.core.MethodParameter;
+import org.springframework.core.annotation.SynthesizingMethodParameter;
+import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.mock.web.test.MockHttpServletRequest;
 import org.springframework.mock.web.test.MockHttpServletResponse;
+import org.springframework.util.ReflectionUtils;
 import org.springframework.web.bind.MissingPathVariableException;
 import org.springframework.web.bind.annotation.PathVariable;
+import org.springframework.web.bind.support.ConfigurableWebBindingInitializer;
+import org.springframework.web.bind.support.DefaultDataBinderFactory;
+import org.springframework.web.bind.support.WebDataBinderFactory;
 import org.springframework.web.context.request.ServletWebRequest;
 import org.springframework.web.method.support.ModelAndViewContainer;
 import org.springframework.web.servlet.HandlerMapping;
 import org.springframework.web.servlet.View;
 
+import static org.junit.Assert.*;
+
 /**
  * Test fixture with {@link PathVariableMethodArgumentResolver}.
  *
  * @author Rossen Stoyanchev
+ * @author Juergen Hoeller
  */
 public class PathVariableMethodArgumentResolverTests {
 
@@ -48,25 +56,33 @@ public class PathVariableMethodArgumentResolverTests {
 
 	private MethodParameter paramString;
 
+	private MethodParameter paramNotRequired;
+
+	private MethodParameter paramOptional;
+
 	private ModelAndViewContainer mavContainer;
 
 	private ServletWebRequest webRequest;
 
 	private MockHttpServletRequest request;
 
+
 	@Before
 	public void setUp() throws Exception {
 		resolver = new PathVariableMethodArgumentResolver();
 
-		Method method = getClass().getMethod("handle", String.class, String.class);
+		Method method = ReflectionUtils.findMethod(getClass(), "handle", (Class<?>[]) null);
-		paramNamedString = new MethodParameter(method, 0);
+		paramNamedString = new SynthesizingMethodParameter(method, 0);
-		paramString = new MethodParameter(method, 1);
+		paramString = new SynthesizingMethodParameter(method, 1);
+		paramNotRequired = new SynthesizingMethodParameter(method, 2);
+		paramOptional = new SynthesizingMethodParameter(method, 3);
 
 		mavContainer = new ModelAndViewContainer();
 		request = new MockHttpServletRequest();
 		webRequest = new ServletWebRequest(request, new MockHttpServletResponse());
 	}
 
+
 	@Test
 	public void supportsParameter() {
 		assertTrue("Parameter with @PathVariable annotation", resolver.supportsParameter(paramNamedString));
@@ -89,21 +105,58 @@ public class PathVariableMethodArgumentResolverTests {
 		assertEquals("value", pathVars.get("name"));
 	}
 
-	@SuppressWarnings("unchecked")
+	@Test
+	public void resolveArgumentNotRequired() throws Exception {
+		Map<String, String> uriTemplateVars = new HashMap<>();
+		uriTemplateVars.put("name", "value");
+		request.setAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVars);
+
+		String result = (String) resolver.resolveArgument(paramNotRequired, mavContainer, webRequest, null);
+		assertEquals("PathVariable not resolved correctly", "value", result);
+
+		@SuppressWarnings("unchecked")
+		Map<String, Object> pathVars = (Map<String, Object>) request.getAttribute(View.PATH_VARIABLES);
+		assertNotNull(pathVars);
+		assertEquals(1, pathVars.size());
+		assertEquals("value", pathVars.get("name"));
+	}
+
+	@Test
+	public void resolveArgumentWrappedAsOptional() throws Exception {
+		Map<String, String> uriTemplateVars = new HashMap<>();
+		uriTemplateVars.put("name", "value");
+		request.setAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVars);
+
+		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
+		initializer.setConversionService(new DefaultConversionService());
+		WebDataBinderFactory binderFactory = new DefaultDataBinderFactory(initializer);
+
+		@SuppressWarnings("unchecked")
+		Optional<String> result = (Optional<String>)
+				resolver.resolveArgument(paramOptional, mavContainer, webRequest, binderFactory);
+		assertEquals("PathVariable not resolved correctly", "value", result.get());
+
+		@SuppressWarnings("unchecked")
+		Map<String, Object> pathVars = (Map<String, Object>) request.getAttribute(View.PATH_VARIABLES);
+		assertNotNull(pathVars);
+		assertEquals(1, pathVars.size());
+		assertEquals(Optional.of("value"), pathVars.get("name"));
+	}
+
 	@Test
 	public void resolveArgumentWithExistingPathVars() throws Exception {
 		Map<String, String> uriTemplateVars = new HashMap<>();
 		uriTemplateVars.put("name", "value");
 		request.setAttribute(HandlerMapping.URI_TEMPLATE_VARIABLES_ATTRIBUTE, uriTemplateVars);
 
-		Map<String, Object> pathVars;
 		uriTemplateVars.put("oldName", "oldValue");
 		request.setAttribute(View.PATH_VARIABLES, uriTemplateVars);
 
 		String result = (String) resolver.resolveArgument(paramNamedString, mavContainer, webRequest, null);
 		assertEquals("PathVariable not resolved correctly", "value", result);
 
-		pathVars = (Map<String, Object>) request.getAttribute(View.PATH_VARIABLES);
+		@SuppressWarnings("unchecked")
+		Map<String, Object> pathVars = (Map<String, Object>) request.getAttribute(View.PATH_VARIABLES);
 		assertNotNull(pathVars);
 		assertEquals(2, pathVars.size());
 		assertEquals("value", pathVars.get("name"));
@@ -113,11 +166,28 @@ public class PathVariableMethodArgumentResolverTests {
 	@Test(expected = MissingPathVariableException.class)
 	public void handleMissingValue() throws Exception {
 		resolver.resolveArgument(paramNamedString, mavContainer, webRequest, null);
-		fail("Unresolved path variable should lead to exception.");
+		fail("Unresolved path variable should lead to exception");
 	}
 
+	@Test
+	public void nullIfNotRequired() throws Exception {
+		assertNull(resolver.resolveArgument(paramNotRequired, mavContainer, webRequest, null));
+	}
+
+	@Test
+	public void wrapEmptyWithOptional() throws Exception {
+		ConfigurableWebBindingInitializer initializer = new ConfigurableWebBindingInitializer();
+		initializer.setConversionService(new DefaultConversionService());
+		WebDataBinderFactory binderFactory = new DefaultDataBinderFactory(initializer);
+
+		assertEquals(Optional.empty(), resolver.resolveArgument(paramOptional, mavContainer, webRequest, binderFactory));
+	}
+
+
 	@SuppressWarnings("unused")
-	public void handle(@PathVariable(value = "name") String param1, String param2) {
+	public void handle(@PathVariable("name") String param1, String param2,
+			@PathVariable(name="name", required = false) String param3,
+			@PathVariable("name") Optional<String> param4) {
 	}
 
-}
+}