Support null query param values in HtmlUnitRequestBuilder

Prior to this commit, HtmlUnitRequestBuilder would incorrectly attempt
to decode null values for query parameters (i.e., query parameters such
as 'error' in 'http://example.com/login?error') which resulted in a
NullPointerException since URLDecoder.decode() does not support null
values.

This commit fixes this issue by ensuring that HtmlUnitRequestBuilder
only attempts to decode non-null query parameter values.

Issue: SPR-13524

spring-test/src/main/java/org/springframework/test/web/servlet/htmlunit/HtmlUnitRequestBuilder.java

@@ -362,7 +362,10 @@ final class HtmlUnitRequestBuilder implements RequestBuilder, Mergeable {
 			String name = values.getKey();
 			for (String value : values.getValue()) {
 				try {
-					request.addParameter(name, URLDecoder.decode(value, "UTF-8"));
+					if (value != null) {
+						value = URLDecoder.decode(value, "UTF-8");
+					}
+					request.addParameter(name, value);
 				}
 				catch (UnsupportedEncodingException e) {
 					throw new RuntimeException(e);

spring-test/src/test/java/org/springframework/test/web/servlet/htmlunit/HtmlUnitRequestBuilderTests.java

@@ -18,8 +18,6 @@ package org.springframework.test.web.servlet.htmlunit;
 
 import java.net.MalformedURLException;
 import java.net.URL;
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
@@ -47,6 +45,7 @@ import com.gargoylesoftware.htmlunit.WebClient;
 import com.gargoylesoftware.htmlunit.WebRequest;
 import com.gargoylesoftware.htmlunit.util.NameValuePair;
 
+import static java.util.Arrays.asList;
 import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.assertThat;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
@@ -75,7 +74,6 @@ public class HtmlUnitRequestBuilderTests {
 	public void setUp() throws Exception {
 		webRequest = new WebRequest(new URL("http://example.com:80/test/this/here"));
 		webRequest.setHttpMethod(HttpMethod.GET);
-		webRequest.setRequestParameters(new ArrayList<>());
 		requestBuilder = new HtmlUnitRequestBuilder(sessions, webClient, webRequest);
 	}
 
@@ -314,9 +312,9 @@ public class HtmlUnitRequestBuilderTests {
 
 		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
 
-		// FIXME Locale.ENGLISH is due to fact cannot remove it from MockHttpServletRequest
+		// Append Locale.ENGLISH since MockHttpServletRequest automatically sets it as the
-		List<Locale> expected = Arrays.asList(new Locale("da"), new Locale("en", "gb", "0.8"), new Locale("en", "",
+		// preferred locale.
-				"0.7"), Locale.ENGLISH);
+		List<Locale> expected = asList(new Locale("da"), new Locale("en", "gb", "0.8"), new Locale("en", "", "0.7"), Locale.ENGLISH);
 		assertThat(Collections.list(actualRequest.getLocales()), equalTo(expected));
 	}
 
@@ -344,9 +342,7 @@ public class HtmlUnitRequestBuilderTests {
 
 	@Test
 	public void buildRequestMethods() {
-		HttpMethod[] methods = HttpMethod.values();
+		for (HttpMethod expectedMethod : HttpMethod.values()) {
-
-		for (HttpMethod expectedMethod : methods) {
 			webRequest.setHttpMethod(expectedMethod);
 			String actualMethod = requestBuilder.buildRequest(servletContext).getMethod();
 			assertThat(actualMethod, equalTo(expectedMethod.name()));
@@ -354,8 +350,8 @@ public class HtmlUnitRequestBuilderTests {
 	}
 
 	@Test
-	public void buildRequestParameterMap() throws Exception {
+	public void buildRequestParameterMapViaWebRequestDotSetRequestParametersWithSingleRequestParam() {
-		setParameter("name", "value");
+		webRequest.setRequestParameters(asList(new NameValuePair("name", "value")));
 
 		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
 
@@ -363,6 +359,47 @@ public class HtmlUnitRequestBuilderTests {
 		assertThat(actualRequest.getParameter("name"), equalTo("value"));
 	}
 
+	@Test
+	public void buildRequestParameterMapViaWebRequestDotSetRequestParametersWithSingleRequestParamWithNullValue() {
+		webRequest.setRequestParameters(asList(new NameValuePair("name", null)));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getParameterMap().size(), equalTo(1));
+		assertThat(actualRequest.getParameter("name"), nullValue());
+	}
+
+	@Test
+	public void buildRequestParameterMapViaWebRequestDotSetRequestParametersWithSingleRequestParamWithEmptyValue() {
+		webRequest.setRequestParameters(asList(new NameValuePair("name", "")));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getParameterMap().size(), equalTo(1));
+		assertThat(actualRequest.getParameter("name"), equalTo(""));
+	}
+
+	@Test
+	public void buildRequestParameterMapViaWebRequestDotSetRequestParametersWithSingleRequestParamWithValueSetToSpace() {
+		webRequest.setRequestParameters(asList(new NameValuePair("name", " ")));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getParameterMap().size(), equalTo(1));
+		assertThat(actualRequest.getParameter("name"), equalTo(" "));
+	}
+
+	@Test
+	public void buildRequestParameterMapViaWebRequestDotSetRequestParametersWithMultipleRequestParams() {
+		webRequest.setRequestParameters(asList(new NameValuePair("name1", "value1"), new NameValuePair("name2", "value2")));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getParameterMap().size(), equalTo(2));
+		assertThat(actualRequest.getParameter("name1"), equalTo("value1"));
+		assertThat(actualRequest.getParameter("name2"), equalTo("value2"));
+	}
+
 	@Test
 	public void buildRequestParameterMapFromSingleQueryParam() throws Exception {
 		webRequest.setUrl(new URL("http://example.com/example/?name=value"));
@@ -373,6 +410,36 @@ public class HtmlUnitRequestBuilderTests {
 		assertThat(actualRequest.getParameter("name"), equalTo("value"));
 	}
 
+	@Test
+	public void buildRequestParameterMapFromSingleQueryParamWithoutValueAndWithoutEqualsSign() throws Exception {
+		webRequest.setUrl(new URL("http://example.com/example/?name"));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getParameterMap().size(), equalTo(1));
+		assertThat(actualRequest.getParameter("name"), nullValue());
+	}
+
+	@Test
+	public void buildRequestParameterMapFromSingleQueryParamWithoutValueButWithEqualsSign() throws Exception {
+		webRequest.setUrl(new URL("http://example.com/example/?name="));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getParameterMap().size(), equalTo(1));
+		assertThat(actualRequest.getParameter("name"), equalTo(""));
+	}
+
+	@Test
+	public void buildRequestParameterMapFromSingleQueryParamWithValueSetToEncodedSpace() throws Exception {
+		webRequest.setUrl(new URL("http://example.com/example/?name=%20"));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getParameterMap().size(), equalTo(1));
+		assertThat(actualRequest.getParameter("name"), equalTo(" "));
+	}
+
 	@Test
 	public void buildRequestParameterMapFromMultipleQueryParams() throws Exception {
 		webRequest.setUrl(new URL("http://example.com/example/?name=value&param2=value+2"));
@@ -428,6 +495,36 @@ public class HtmlUnitRequestBuilderTests {
 		assertThat(actualRequest.getQueryString(), equalTo(expectedQuery));
 	}
 
+	@Test
+	public void buildRequestQueryWithSingleQueryParamWithoutValueAndWithoutEqualsSign() throws Exception {
+		String expectedQuery = "param";
+		webRequest.setUrl(new URL("http://example.com/example?" + expectedQuery));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getQueryString(), equalTo(expectedQuery));
+	}
+
+	@Test
+	public void buildRequestQueryWithSingleQueryParamWithoutValueButWithEqualsSign() throws Exception {
+		String expectedQuery = "param=";
+		webRequest.setUrl(new URL("http://example.com/example?" + expectedQuery));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getQueryString(), equalTo(expectedQuery));
+	}
+
+	@Test
+	public void buildRequestQueryWithSingleQueryParamWithValueSetToEncodedSpace() throws Exception {
+		String expectedQuery = "param=%20";
+		webRequest.setUrl(new URL("http://example.com/example?" + expectedQuery));
+
+		MockHttpServletRequest actualRequest = requestBuilder.buildRequest(servletContext);
+
+		assertThat(actualRequest.getQueryString(), equalTo(expectedQuery));
+	}
+
 	@Test
 	public void buildRequestQueryWithMultipleQueryParams() throws Exception {
 		String expectedQuery = "param1=value1&param2=value2";
@@ -744,7 +841,8 @@ public class HtmlUnitRequestBuilderTests {
 				.defaultRequest(get("/").param(paramName, paramValue, paramValue2))
 				.build();
 
-		assertThat(Arrays.asList(mockMvc.perform(requestBuilder).andReturn().getRequest().getParameterValues(paramName)), contains(paramValue, paramValue2));
+		MockHttpServletRequest performedRequest = mockMvc.perform(requestBuilder).andReturn().getRequest();
+		assertThat(asList(performedRequest.getParameterValues(paramName)), contains(paramValue, paramValue2));
 	}
 
 	@Test
@@ -785,10 +883,6 @@ public class HtmlUnitRequestBuilderTests {
 		assertThat("JSESSIONID=" + actual + "; Path=/test; Domain=example.com", equalTo(expected));
 	}
 
-	private void setParameter(String name, String value) {
-		webRequest.getRequestParameters().add(new NameValuePair(name, value));
-	}
-
 	private String getContextPath() {
 		return (String) ReflectionTestUtils.getField(requestBuilder, "contextPath");
 	}