From 653f28727ca9cbdfdaaca646f8b5b8d93ddd86c5 Mon Sep 17 00:00:00 2001
From: Oliver Gierke <ogierke@gopivotal.com>
Date: Thu, 2 Jan 2014 12:15:47 +0100
Subject: [PATCH] #122 - Fixed parameter binding for optional request
 parameters.

If the method arguments handed into a dummy controller method invocation are required but null we now throw an exception as we cannot build a URI that hits the very same method in the very same way. Based on that, we now inspect @RequestParam annotated parameters for null values, allow them but don't bind a parameter for them anymore.
---
 .../AnnotatedParametersParameterAccessor.java | 31 +++++++++++++++-
 .../mvc/ControllerLinkBuilderFactory.java     | 27 ++++++++++++--
 .../mvc/ControllerLinkBuilderUnitTest.java    | 36 +++++++++++++++++++
 3 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/src/main/java/org/springframework/hateoas/mvc/AnnotatedParametersParameterAccessor.java b/src/main/java/org/springframework/hateoas/mvc/AnnotatedParametersParameterAccessor.java
index b00925c4..06b744db 100644
--- a/src/main/java/org/springframework/hateoas/mvc/AnnotatedParametersParameterAccessor.java
+++ b/src/main/java/org/springframework/hateoas/mvc/AnnotatedParametersParameterAccessor.java
@@ -65,12 +65,41 @@ class AnnotatedParametersParameterAccessor {
 		List<BoundMethodParameter> result = new ArrayList<BoundMethodParameter>();
 
 		for (MethodParameter parameter : parameters.getParametersWith(attribute.getAnnotationType())) {
-			result.add(new BoundMethodParameter(parameter, arguments[parameter.getParameterIndex()], attribute));
+
+			Object value = arguments[parameter.getParameterIndex()];
+			Object verifiedValue = verifyParameterValue(parameter, value);
+
+			if (verifiedValue != null) {
+				result.add(new BoundMethodParameter(parameter, value, attribute));
+			}
 		}
 
 		return result;
 	}
 
+	/**
+	 * Callback to verifiy the parameter values given for a dummy invocation. Default implementation rejects
+	 * {@literal null} values as they indicate an invalid dummy call.
+	 * 
+	 * @param parameter will never be {@literal null}.
+	 * @param value could be {@literal null}.
+	 * @return the verified value.
+	 */
+	protected Object verifyParameterValue(MethodParameter parameter, Object value) {
+
+		if (value == null) {
+
+			Object indexOrName = StringUtils.hasText(parameter.getParameterName()) ? parameter.getParameterName() : parameter
+					.getParameterIndex();
+
+			throw new IllegalArgumentException(String.format(
+					"Required controller parameter %s of method %s found but null value given!", indexOrName,
+					parameter.getMethod()));
+		}
+
+		return value;
+	}
+
 	/**
 	 * Represents a {@link MethodParameter} alongside the value it has been bound to.
 	 * 
diff --git a/src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilderFactory.java b/src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilderFactory.java
index 882b4c68..8cc0d7e9 100644
--- a/src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilderFactory.java
+++ b/src/main/java/org/springframework/hateoas/mvc/ControllerLinkBuilderFactory.java
@@ -56,8 +56,7 @@ public class ControllerLinkBuilderFactory implements MethodLinkBuilderFactory<Co
 	private static final MappingDiscoverer DISCOVERER = new AnnotationMappingDiscoverer(RequestMapping.class);
 	private static final AnnotatedParametersParameterAccessor PATH_VARIABLE_ACCESSOR = new AnnotatedParametersParameterAccessor(
 			new AnnotationAttribute(PathVariable.class));
-	private static final AnnotatedParametersParameterAccessor REQUEST_PARAM_ACCESSOR = new AnnotatedParametersParameterAccessor(
-			new AnnotationAttribute(RequestParam.class));
+	private static final AnnotatedParametersParameterAccessor REQUEST_PARAM_ACCESSOR = new RequestParamParameterAccessor();
 
 	private List<UriComponentsContributor> uriComponentsContributors = new ArrayList<UriComponentsContributor>();
 
@@ -169,4 +168,28 @@ public class ControllerLinkBuilderFactory implements MethodLinkBuilderFactory<Co
 
 		return builder;
 	}
+
+	/**
+	 * Custom extension of {@link AnnotatedParametersParameterAccessor} for {@link RequestParam} to allow {@literal null}
+	 * values handed in for optional request parameters.
+	 * 
+	 * @author Oliver Gierke
+	 */
+	private static class RequestParamParameterAccessor extends AnnotatedParametersParameterAccessor {
+
+		public RequestParamParameterAccessor() {
+			super(new AnnotationAttribute(RequestParam.class));
+		}
+
+		/* 
+		 * (non-Javadoc)
+		 * @see org.springframework.hateoas.mvc.AnnotatedParametersParameterAccessor#verifyParameterValue(org.springframework.core.MethodParameter, java.lang.Object)
+		 */
+		@Override
+		protected Object verifyParameterValue(MethodParameter parameter, Object value) {
+
+			RequestParam annotation = parameter.getParameterAnnotation(RequestParam.class);
+			return annotation.required() ? super.verifyParameterValue(parameter, value) : value;
+		}
+	}
 }
diff --git a/src/test/java/org/springframework/hateoas/mvc/ControllerLinkBuilderUnitTest.java b/src/test/java/org/springframework/hateoas/mvc/ControllerLinkBuilderUnitTest.java
index 4ad27d30..40bcb411 100644
--- a/src/test/java/org/springframework/hateoas/mvc/ControllerLinkBuilderUnitTest.java
+++ b/src/test/java/org/springframework/hateoas/mvc/ControllerLinkBuilderUnitTest.java
@@ -269,6 +269,32 @@ public class ControllerLinkBuilderUnitTest extends TestUtils {
 		assertThat(link.getHref(), startsWith("http://barfoo:8888"));
 	}
 
+	/**
+	 * @see #122
+	 */
+	@Test
+	public void doesNotAppendParameterForNullRequestParameters() {
+
+		Link link = linkTo(methodOn(ControllerWithMethods.class).methodForOptionalNextPage(null)).withSelfRel();
+		assertThat(link.getHref(), endsWith("/foo"));
+	}
+
+	/**
+	 * @see #122
+	 */
+	@Test(expected = IllegalArgumentException.class)
+	public void rejectsMissingPathVariable() {
+		linkTo(methodOn(ControllerWithMethods.class).methodWithPathVariable(null));
+	}
+
+	/**
+	 * @see #122
+	 */
+	@Test(expected = IllegalArgumentException.class)
+	public void rejectsMissingRequiredRequestParam() {
+		linkTo(methodOn(ControllerWithMethods.class).methodWithRequestParam(null));
+	}
+
 	private static UriComponents toComponents(Link link) {
 		return UriComponentsBuilder.fromUriString(link.getHref()).build();
 	}
@@ -323,6 +349,11 @@ public class ControllerLinkBuilderUnitTest extends TestUtils {
 			return null;
 		}
 
+		@RequestMapping("/foo")
+		HttpEntity<Void> methodWithRequestParam(@RequestParam String id) {
+			return null;
+		}
+
 		@RequestMapping(value = "/{id}/foo")
 		HttpEntity<Void> methodForNextPage(@PathVariable String id, @RequestParam Integer offset,
 				@RequestParam Integer limit) {
@@ -334,5 +365,10 @@ public class ControllerLinkBuilderUnitTest extends TestUtils {
 				@RequestParam Integer limit) {
 			return null;
 		}
+
+		@RequestMapping(value = "/foo")
+		HttpEntity<Void> methodForOptionalNextPage(@RequestParam(required = false) Integer offset) {
+			return null;
+		}
 	}
 }