Proper exception for controller method return types that do not work with MvcUriComponentsBuilder (e.g. final classes)

Issue: SPR-16710

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 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.
@@ -252,10 +252,8 @@ public class MvcUriComponentsBuilder {
 	 * controller.getAddressesForCountry("US")
 	 * builder = MvcUriComponentsBuilder.fromMethodCall(controller);
 	 * </pre>
-	 *
 	 * <p><strong>Note:</strong> This method extracts values from "Forwarded"
 	 * and "X-Forwarded-*" headers if found. See class-level docs.
-	 *
 	 * @param info either the value returned from a "mock" controller
 	 * invocation or the "mock" controller itself after an invocation
 	 * @return a UriComponents instance
@@ -725,6 +723,16 @@ public class MvcUriComponentsBuilder {
 	}
 
 
+	public interface MethodInvocationInfo {
+
+		Class<?> getControllerType();
+
+		Method getControllerMethod();
+
+		Object[] getArgumentValues();
+	}
+
+
 	private static class ControllerMethodInvocationInterceptor
 			implements org.springframework.cglib.proxy.MethodInterceptor, MethodInterceptor {
 
@@ -737,27 +745,27 @@ public class MvcUriComponentsBuilder {
 		private static final Method getControllerType =
 				ReflectionUtils.findMethod(MethodInvocationInfo.class, "getControllerType");
 
+		private final Class<?> controllerType;
+
 		private Method controllerMethod;
 
 		private Object[] argumentValues;
 
-		private Class<?> controllerType;
-
 		ControllerMethodInvocationInterceptor(Class<?> controllerType) {
 			this.controllerType = controllerType;
 		}
 
 		@Override
 		public Object intercept(Object obj, Method method, Object[] args, MethodProxy proxy) {
-			if (getControllerMethod.equals(method)) {
+			if (getControllerType.equals(method)) {
+				return this.controllerType;
+			}
+			else if (getControllerMethod.equals(method)) {
 				return this.controllerMethod;
 			}
 			else if (getArgumentValues.equals(method)) {
 				return this.argumentValues;
 			}
-			else if (getControllerType.equals(method)) {
-				return this.controllerType;
-			}
 			else if (ReflectionUtils.isObjectMethod(method)) {
 				return ReflectionUtils.invokeMethod(method, obj, args);
 			}
@@ -765,7 +773,13 @@ public class MvcUriComponentsBuilder {
 				this.controllerMethod = method;
 				this.argumentValues = args;
 				Class<?> returnType = method.getReturnType();
-				return (void.class == returnType ? null : returnType.cast(initProxy(returnType, this)));
+				try {
+					return (returnType == void.class ? null : returnType.cast(initProxy(returnType, this)));
+				}
+				catch (Throwable ex) {
+					throw new IllegalStateException(
+							"Failed to create proxy for controller method return type: " + method, ex);
+				}
 			}
 		}
 
@@ -776,16 +790,6 @@ public class MvcUriComponentsBuilder {
 	}
 
 
-	public interface MethodInvocationInfo {
-
-		Method getControllerMethod();
-
-		Object[] getArgumentValues();
-
-		Class<?> getControllerType();
-	}
-
-
 	public static class MethodArgumentBuilder {
 
 		private final Class<?> controllerType;

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2012-2016 the original author or authors.
+ * Copyright 2012-2018 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,10 +16,6 @@
 
 package org.springframework.web.servlet.mvc.method.annotation;
 
-import static org.hamcrest.Matchers.*;
-import static org.junit.Assert.*;
-import static org.springframework.web.servlet.mvc.method.annotation.MvcUriComponentsBuilder.*;
-
 import java.lang.annotation.Documented;
 import java.lang.annotation.ElementType;
 import java.lang.annotation.Retention;
@@ -44,6 +40,7 @@ import org.springframework.mock.web.test.MockHttpServletRequest;
 import org.springframework.mock.web.test.MockServletContext;
 import org.springframework.stereotype.Controller;
 import org.springframework.util.MultiValueMap;
+import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
 import org.springframework.web.bind.annotation.RequestBody;
 import org.springframework.web.bind.annotation.RequestMapping;
@@ -58,6 +55,10 @@ import org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter
 import org.springframework.web.util.UriComponents;
 import org.springframework.web.util.UriComponentsBuilder;
 
+import static org.hamcrest.Matchers.*;
+import static org.junit.Assert.*;
+import static org.springframework.web.servlet.mvc.method.annotation.MvcUriComponentsBuilder.*;
+
 /**
  * Unit tests for {@link org.springframework.web.servlet.mvc.method.annotation.MvcUriComponentsBuilder}.
  *
@@ -73,12 +74,12 @@ public class MvcUriComponentsBuilderTests {
 
 
 	@Before
-	public void setUp() {
+	public void setup() {
 		RequestContextHolder.setRequestAttributes(new ServletRequestAttributes(this.request));
 	}
 
 	@After
-	public void tearDown() {
+	public void reset() {
 		RequestContextHolder.resetRequestAttributes();
 	}
 
@@ -134,15 +135,15 @@ public class MvcUriComponentsBuilderTests {
 	}
 
 	@Test
-	public void testFromMethodNamePathVariable() throws Exception {
-		UriComponents uriComponents = fromMethodName(
-				ControllerWithMethods.class, "methodWithPathVariable", new Object[]{"1"}).build();
+	public void testFromMethodNamePathVariable() {
+		UriComponents uriComponents = fromMethodName(ControllerWithMethods.class,
+				"methodWithPathVariable", "1").build();
 
 		assertThat(uriComponents.toUriString(), is("http://localhost/something/1/foo"));
 	}
 
 	@Test
-	public void testFromMethodNameTypeLevelPathVariable() throws Exception {
+	public void testFromMethodNameTypeLevelPathVariable() {
 		this.request.setContextPath("/myapp");
 		UriComponents uriComponents = fromMethodName(
 				PersonsAddressesController.class, "getAddressesForCountry", "DE").buildAndExpand("1");
@@ -151,7 +152,7 @@ public class MvcUriComponentsBuilderTests {
 	}
 
 	@Test
-	public void testFromMethodNameTwoPathVariables() throws Exception {
+	public void testFromMethodNameTwoPathVariables() {
 		DateTime now = DateTime.now();
 		UriComponents uriComponents = fromMethodName(
 				ControllerWithMethods.class, "methodWithTwoPathVariables", 1, now).build();
@@ -160,7 +161,7 @@ public class MvcUriComponentsBuilderTests {
 	}
 
 	@Test
-	public void testFromMethodNameWithPathVarAndRequestParam() throws Exception {
+	public void testFromMethodNameWithPathVarAndRequestParam() {
 		UriComponents uriComponents = fromMethodName(
 				ControllerWithMethods.class, "methodForNextPage", "1", 10, 5).build();
 
@@ -170,59 +171,56 @@ public class MvcUriComponentsBuilderTests {
 		assertThat(queryParams.get("offset"), contains("10"));
 	}
 
-	// SPR-12977
-
-	@Test
-	public void fromMethodNameWithBridgedMethod() throws Exception {
+	@Test  // SPR-12977
+	public void fromMethodNameWithBridgedMethod() {
 		UriComponents uriComponents = fromMethodName(PersonCrudController.class, "get", (long) 42).build();
+
 		assertThat(uriComponents.toUriString(), is("http://localhost/42"));
 	}
 
-	// SPR-11391
-
-	@Test
-	public void testFromMethodNameTypeLevelPathVariableWithoutArgumentValue() throws Exception {
+	@Test  // SPR-11391
+	public void testFromMethodNameTypeLevelPathVariableWithoutArgumentValue() {
 		UriComponents uriComponents = fromMethodName(UserContactController.class, "showCreate", 123).build();
 
 		assertThat(uriComponents.getPath(), is("/user/123/contacts/create"));
 	}
 
 	@Test
-	public void testFromMethodNameNotMapped() throws Exception {
+	public void testFromMethodNameNotMapped() {
 		UriComponents uriComponents = fromMethodName(UnmappedController.class, "unmappedMethod").build();
 
 		assertThat(uriComponents.toUriString(), is("http://localhost/"));
 	}
 
 	@Test
-	public void testFromMethodNameWithCustomBaseUrlViaStaticCall() throws Exception {
+	public void testFromMethodNameWithCustomBaseUrlViaStaticCall() {
 		UriComponentsBuilder builder = UriComponentsBuilder.fromUriString("http://example.org:9090/base");
 		UriComponents uriComponents = fromMethodName(builder, ControllerWithMethods.class,
-				"methodWithPathVariable", new Object[] {"1"}).build();
+				"methodWithPathVariable", "1").build();
 
 		assertEquals("http://example.org:9090/base/something/1/foo", uriComponents.toString());
 		assertEquals("http://example.org:9090/base", builder.toUriString());
 	}
 
 	@Test
-	public void testFromMethodNameWithCustomBaseUrlViaInstance() throws Exception {
+	public void testFromMethodNameWithCustomBaseUrlViaInstance() {
 		UriComponentsBuilder builder = UriComponentsBuilder.fromUriString("http://example.org:9090/base");
 		MvcUriComponentsBuilder mvcBuilder = MvcUriComponentsBuilder.relativeTo(builder);
 		UriComponents uriComponents = mvcBuilder.withMethodName(ControllerWithMethods.class,
-				"methodWithPathVariable", new Object[] {"1"}).build();
+				"methodWithPathVariable", "1").build();
 
 		assertEquals("http://example.org:9090/base/something/1/foo", uriComponents.toString());
 		assertEquals("http://example.org:9090/base", builder.toUriString());
 	}
 
 	@Test
-	public void testFromMethodNameWithMetaAnnotation() throws Exception {
+	public void testFromMethodNameWithMetaAnnotation() {
 		UriComponents uriComponents = fromMethodName(MetaAnnotationController.class, "handleInput").build();
 		assertThat(uriComponents.toUriString(), is("http://localhost/input"));
 	}
 
-	@Test // SPR-14405
-	public void testFromMappingNameWithOptionalParam() throws Exception {
+	@Test  // SPR-14405
+	public void testFromMappingNameWithOptionalParam() {
 		UriComponents uriComponents = fromMethodName(ControllerWithMethods.class,
 				"methodWithOptionalParam", new Object[] {null}).build();
 
@@ -247,8 +245,8 @@ public class MvcUriComponentsBuilderTests {
 
 	@Test
 	public void testFromMethodCallWithTypeLevelUriVars() {
-		UriComponents uriComponents = fromMethodCall(on(
-				PersonsAddressesController.class).getAddressesForCountry("DE")).buildAndExpand(15);
+		UriComponents uriComponents = fromMethodCall(
+				on(PersonsAddressesController.class).getAddressesForCountry("DE")).buildAndExpand(15);
 
 		assertThat(uriComponents.toUriString(), endsWith("/people/15/addresses/DE"));
 	}
@@ -256,8 +254,8 @@ public class MvcUriComponentsBuilderTests {
 
 	@Test
 	public void testFromMethodCallWithPathVar() {
-		UriComponents uriComponents = fromMethodCall(on(
-				ControllerWithMethods.class).methodWithPathVariable("1")).build();
+		UriComponents uriComponents = fromMethodCall(
+				on(ControllerWithMethods.class).methodWithPathVariable("1")).build();
 
 		assertThat(uriComponents.toUriString(), startsWith("http://localhost"));
 		assertThat(uriComponents.toUriString(), endsWith("/something/1/foo"));
@@ -265,8 +263,8 @@ public class MvcUriComponentsBuilderTests {
 
 	@Test
 	public void testFromMethodCallWithPathVarAndRequestParams() {
-		UriComponents uriComponents = fromMethodCall(on(
-				ControllerWithMethods.class).methodForNextPage("1", 10, 5)).build();
+		UriComponents uriComponents = fromMethodCall(
+				on(ControllerWithMethods.class).methodForNextPage("1", 10, 5)).build();
 
 		assertThat(uriComponents.getPath(), is("/something/1/foo"));
 
@@ -277,8 +275,8 @@ public class MvcUriComponentsBuilderTests {
 
 	@Test
 	public void testFromMethodCallWithPathVarAndMultiValueRequestParams() {
-		UriComponents uriComponents = fromMethodCall(on(
-				ControllerWithMethods.class).methodWithMultiValueRequestParams("1", Arrays.asList(3, 7), 5)).build();
+		UriComponents uriComponents = fromMethodCall(
+				on(ControllerWithMethods.class).methodWithMultiValueRequestParams("1", Arrays.asList(3, 7), 5)).build();
 
 		assertThat(uriComponents.getPath(), is("/something/1/foo"));
 
@@ -307,7 +305,7 @@ public class MvcUriComponentsBuilderTests {
 	}
 
 	@Test
-	public void testFromMappingName() throws Exception {
+	public void testFromMappingName() {
 		AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext();
 		context.setServletContext(new MockServletContext());
 		context.register(WebConfig.class);
@@ -324,7 +322,7 @@ public class MvcUriComponentsBuilderTests {
 	}
 
 	@Test
-	public void testFromMappingNameWithCustomBaseUrl() throws Exception {
+	public void testFromMappingNameWithCustomBaseUrl() {
 		AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext();
 		context.setServletContext(new MockServletContext());
 		context.register(WebConfig.class);
@@ -362,6 +360,13 @@ public class MvcUriComponentsBuilderTests {
 		assertThat(uriComponents.toUriString(), startsWith("http://barfoo:8888"));
 	}
 
+	@Test  // SPR-16710
+	public void withStringReturnType() {
+		UriComponents uriComponents = MvcUriComponentsBuilder.fromMethodCall(
+				on(BookingController.class).getBooking(21L)).buildAndExpand(42);
+		assertEquals("http://localhost/hotels/42/bookings/21", uriComponents.encode().toUri().toString());
+	}
+
 
 	static class Person {
 
@@ -372,15 +377,16 @@ public class MvcUriComponentsBuilderTests {
 		}
 	}
 
+
 	@RequestMapping("/people")
 	interface PersonController {
-
 	}
 
-	private class PersonControllerImpl implements PersonController {
 
+	private class PersonControllerImpl implements PersonController {
 	}
 
+
 	@RequestMapping("/people/{id}/addresses")
 	private static class PersonsAddressesController {
 
@@ -390,11 +396,12 @@ public class MvcUriComponentsBuilderTests {
 		}
 	}
 
-	@RequestMapping({ "/persons", "/people" })
-	private class InvalidController {
 
+	@RequestMapping({"/persons", "/people"})
+	private class InvalidController {
 	}
 
+
 	private class UnmappedController {
 
 		@RequestMapping
@@ -402,6 +409,7 @@ public class MvcUriComponentsBuilderTests {
 		}
 	}
 
+
 	@RequestMapping("/something")
 	static class ControllerWithMethods {
 
@@ -439,11 +447,13 @@ public class MvcUriComponentsBuilderTests {
 		}
 	}
 
-	@RequestMapping("/extended") @SuppressWarnings("WeakerAccess")
-	static class ExtendedController extends ControllerWithMethods {
 
+	@RequestMapping("/extended")
+	@SuppressWarnings("WeakerAccess")
+	static class ExtendedController extends ControllerWithMethods {
 	}
 
+
 	@RequestMapping("/user/{userId}/contacts")
 	private static class UserContactController {
 
@@ -453,11 +463,13 @@ public class MvcUriComponentsBuilderTests {
 		}
 	}
 
+
 	static abstract class AbstractCrudController<T, ID> {
 
 		abstract T get(ID id);
 	}
 
+
 	private static class PersonCrudController extends AbstractCrudController<Person, Long> {
 
 		@RequestMapping(path = "/{id}", method = RequestMethod.GET)
@@ -466,6 +478,7 @@ public class MvcUriComponentsBuilderTests {
 		}
 	}
 
+
 	@Controller
 	private static class MetaAnnotationController {
 
@@ -473,12 +486,12 @@ public class MvcUriComponentsBuilderTests {
 		public void handle() {
 		}
 
-		@PostJson(path="/input")
+		@PostJson(path = "/input")
 		public void handleInput() {
 		}
-
 	}
 
+
 	@RequestMapping(method = RequestMethod.POST,
 			produces = MediaType.APPLICATION_JSON_VALUE,
 			consumes = MediaType.APPLICATION_JSON_VALUE)
@@ -486,9 +499,11 @@ public class MvcUriComponentsBuilderTests {
 	@Retention(RetentionPolicy.RUNTIME)
 	@Documented
 	private @interface PostJson {
+
 		String[] path() default {};
 	}
 
+
 	@EnableWebMvc
 	static class WebConfig extends WebMvcConfigurerAdapter {
 
@@ -498,4 +513,15 @@ public class MvcUriComponentsBuilderTests {
 		}
 	}
 
+
+	@Controller
+	@RequestMapping("/hotels/{hotel}")
+	public class BookingController {
+
+		@GetMapping("/bookings/{booking}")
+		public Object getBooking(@PathVariable Long booking) {
+			return "url";
+		}
+	}
+
 }