Consistent handling of null header values in HttpHeaders

Issue: SPR-17588

Author: jhoeller

spring-web/src/main/java/org/springframework/http/HttpHeaders.java

@@ -75,11 +75,7 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
 
 	private static final long serialVersionUID = -8578554704772377436L;
 
-	/**
-	 * The empty {@code HttpHeaders} instance (immutable).
-	 */
-	public static final HttpHeaders EMPTY =
-			new ReadOnlyHttpHeaders(new HttpHeaders(new LinkedMultiValueMap<>(0)));
+
 	/**
 	 * The HTTP {@code Accept} header field name.
 	 * @see <a href="http://tools.ietf.org/html/rfc7231#section-5.3.2">Section 5.3.2 of RFC 7231</a>
@@ -381,6 +377,12 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
 	 */
 	public static final String WWW_AUTHENTICATE = "WWW-Authenticate";
 
+
+	/**
+	 * The empty {@code HttpHeaders} instance (immutable).
+	 */
+	public static final HttpHeaders EMPTY = new ReadOnlyHttpHeaders(new HttpHeaders(new LinkedMultiValueMap<>(0)));
+
 	/**
 	 * Pattern matching ETag multiple field values in headers such as "If-Match", "If-None-Match".
 	 * @see <a href="https://tools.ietf.org/html/rfc7232#section-2.3">Section 2.3 of RFC 7232</a>
@@ -409,15 +411,15 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
 	 * Construct a new, empty instance of the {@code HttpHeaders} object.
 	 */
 	public HttpHeaders() {
-		this(CollectionUtils.toMultiValueMap(
-				new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH)));
+		this(CollectionUtils.toMultiValueMap(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH)));
 	}
 
 	/**
 	 * Construct a new {@code HttpHeaders} instance backed by an existing map.
+	 * @since 5.1
 	 */
 	public HttpHeaders(MultiValueMap<String, String> headers) {
-		Assert.notNull(headers, "headers must not be null");
+		Assert.notNull(headers, "MultiValueMap must not be null");
 		this.headers = headers;
 	}
 
@@ -445,7 +447,7 @@ public List<MediaType> getAccept() {
 	 * @since 5.0
 	 */
 	public void setAcceptLanguage(List<Locale.LanguageRange> languages) {
-		Assert.notNull(languages, "'languages' must not be null");
+		Assert.notNull(languages, "LanguageRange List must not be null");
 		DecimalFormat decimal = new DecimalFormat("0.0", DECIMAL_FORMAT_SYMBOLS);
 		List<String> values = languages.stream()
 				.map(range ->
@@ -555,7 +557,7 @@ public List<HttpMethod> getAccessControlAllowMethods() {
 	 * Set the (new) value of the {@code Access-Control-Allow-Origin} response header.
 	 */
 	public void setAccessControlAllowOrigin(@Nullable String allowedOrigin) {
-		set(ACCESS_CONTROL_ALLOW_ORIGIN, allowedOrigin);
+		setOrRemove(ACCESS_CONTROL_ALLOW_ORIGIN, allowedOrigin);
 	}
 
 	/**
@@ -614,7 +616,7 @@ public List<String> getAccessControlRequestHeaders() {
 	 * Set the (new) value of the {@code Access-Control-Request-Method} request header.
 	 */
 	public void setAccessControlRequestMethod(@Nullable HttpMethod requestMethod) {
-		set(ACCESS_CONTROL_REQUEST_METHOD, (requestMethod != null ? requestMethod.name() : null));
+		setOrRemove(ACCESS_CONTROL_REQUEST_METHOD, (requestMethod != null ? requestMethod.name() : null));
 	}
 
 	/**
@@ -766,14 +768,14 @@ public void setBearerAuth(String token) {
 	 * @since 5.0.5
 	 */
 	public void setCacheControl(CacheControl cacheControl) {
-		set(CACHE_CONTROL, cacheControl.getHeaderValue());
+		setOrRemove(CACHE_CONTROL, cacheControl.getHeaderValue());
 	}
 
 	/**
 	 * Set the (new) value of the {@code Cache-Control} header.
 	 */
 	public void setCacheControl(@Nullable String cacheControl) {
-		set(CACHE_CONTROL, cacheControl);
+		setOrRemove(CACHE_CONTROL, cacheControl);
 	}
 
 	/**
@@ -817,7 +819,7 @@ public List<String> getConnection() {
 	 * @see #getContentDisposition()
 	 */
 	public void setContentDispositionFormData(String name, @Nullable String filename) {
-		Assert.notNull(name, "'name' must not be null");
+		Assert.notNull(name, "Name must not be null");
 		ContentDisposition.Builder disposition = ContentDisposition.builder("form-data").name(name);
 		if (filename != null) {
 			disposition.filename(filename);
@@ -860,7 +862,7 @@ public ContentDisposition getContentDisposition() {
 	 * @since 5.0
 	 */
 	public void setContentLanguage(@Nullable Locale locale) {
-		set(CONTENT_LANGUAGE, (locale != null ? locale.toLanguageTag() : null));
+		setOrRemove(CONTENT_LANGUAGE, (locale != null ? locale.toLanguageTag() : null));
 	}
 
 	/**
@@ -904,12 +906,12 @@ public long getContentLength() {
 	 */
 	public void setContentType(@Nullable MediaType mediaType) {
 		if (mediaType != null) {
-			Assert.isTrue(!mediaType.isWildcardType(), "'Content-Type' cannot contain wildcard type '*'");
-			Assert.isTrue(!mediaType.isWildcardSubtype(), "'Content-Type' cannot contain wildcard subtype '*'");
+			Assert.isTrue(!mediaType.isWildcardType(), "Content-Type cannot contain wildcard type '*'");
+			Assert.isTrue(!mediaType.isWildcardSubtype(), "Content-Type cannot contain wildcard subtype '*'");
 			set(CONTENT_TYPE, mediaType.toString());
 		}
 		else {
-			set(CONTENT_TYPE, null);
+			remove(CONTENT_TYPE);
 		}
 	}
 
@@ -953,8 +955,11 @@ public void setETag(@Nullable String etag) {
 			Assert.isTrue(etag.startsWith("\"") || etag.startsWith("W/"),
 					"Invalid ETag: does not start with W/ or \"");
 			Assert.isTrue(etag.endsWith("\""), "Invalid ETag: does not end with \"");
+			set(ETAG, etag);
+		}
+		else {
+			remove(ETAG);
 		}
-		set(ETAG, etag);
 	}
 
 	/**
@@ -1012,7 +1017,7 @@ public void setHost(@Nullable InetSocketAddress host) {
 			set(HOST, value);
 		}
 		else {
-			set(HOST, null);
+			remove(HOST, null);
 		}
 	}
 
@@ -1160,7 +1165,7 @@ public void setLastModified(Instant lastModified) {
 	 * @since 5.1.4
 	 */
 	public void setLastModified(ZonedDateTime lastModified) {
-		setZonedDateTime(LAST_MODIFIED, lastModified.withZoneSameInstant(ZoneId.of("GMT")));
+		setZonedDateTime(LAST_MODIFIED, lastModified.withZoneSameInstant(GMT));
 	}
 
 	/**
@@ -1179,7 +1184,7 @@ public long getLastModified() {
 	 * as specified by the {@code Location} header.
 	 */
 	public void setLocation(@Nullable URI location) {
-		set(LOCATION, (location != null ? location.toASCIIString() : null));
+		setOrRemove(LOCATION, (location != null ? location.toASCIIString() : null));
 	}
 
 	/**
@@ -1197,7 +1202,7 @@ public URI getLocation() {
 	 * Set the (new) value of the {@code Origin} header.
 	 */
 	public void setOrigin(@Nullable String origin) {
-		set(ORIGIN, origin);
+		setOrRemove(ORIGIN, origin);
 	}
 
 	/**
@@ -1212,7 +1217,7 @@ public String getOrigin() {
 	 * Set the (new) value of the {@code Pragma} header.
 	 */
 	public void setPragma(@Nullable String pragma) {
-		set(PRAGMA, pragma);
+		setOrRemove(PRAGMA, pragma);
 	}
 
 	/**
@@ -1244,7 +1249,7 @@ public List<HttpRange> getRange() {
 	 * Set the (new) value of the {@code Upgrade} header.
 	 */
 	public void setUpgrade(@Nullable String upgrade) {
-		set(UPGRADE, upgrade);
+		setOrRemove(UPGRADE, upgrade);
 	}
 
 	/**
@@ -1406,10 +1411,7 @@ public List<String> getValuesAsList(String headerName) {
 			List<String> result = new ArrayList<>();
 			for (String value : values) {
 				if (value != null) {
-					String[] tokens = StringUtils.tokenizeToStringArray(value, ",");
-					for (String token : tokens) {
-						result.add(token);
-					}
+					Collections.addAll(result, StringUtils.tokenizeToStringArray(value, ","));
 				}
 			}
 			return result;
@@ -1468,16 +1470,32 @@ protected String getFieldValues(String headerName) {
 	 */
 	protected String toCommaDelimitedString(List<String> headerValues) {
 		StringBuilder builder = new StringBuilder();
-		for (Iterator<String> it = headerValues.iterator(); it.hasNext(); ) {
+		for (Iterator<String> it = headerValues.iterator(); it.hasNext();) {
 			String val = it.next();
-			builder.append(val);
-			if (it.hasNext()) {
-				builder.append(", ");
+			if (val != null) {
+				builder.append(val);
+				if (it.hasNext()) {
+					builder.append(", ");
+				}
 			}
 		}
 		return builder.toString();
 	}
 
+	/**
+	 * Set the given header value, or remove the header if {@code null}.
+	 * @param headerName the header name
+	 * @param headerValue the header value, or {@code null} for none
+	 */
+	private void setOrRemove(String headerName, @Nullable String headerValue) {
+		if (headerValue != null) {
+			set(headerName, headerValue);
+		}
+		else {
+			remove(headerName);
+		}
+	}
+
 
 	// MultiValueMap implementation
 

spring-web/src/main/java/org/springframework/http/ResponseEntity.java

@@ -537,10 +537,7 @@ public BodyBuilder location(URI location) {
 
 		@Override
 		public BodyBuilder cacheControl(CacheControl cacheControl) {
-			String ccValue = cacheControl.getHeaderValue();
-			if (ccValue != null) {
-				this.headers.setCacheControl(cacheControl.getHeaderValue());
-			}
+			this.headers.setCacheControl(cacheControl);
 			return this;
 		}
 

spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java

@@ -38,8 +38,8 @@
 import org.hamcrest.Matchers;
 import org.junit.Test;
 
-import static java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME;
-import static org.hamcrest.Matchers.is;
+import static java.time.format.DateTimeFormatter.*;
+import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.*;
 
 /**
@@ -355,11 +355,18 @@ public void cacheControlBuilder() {
 		assertEquals("Invalid Cache-Control header", "no-cache", headers.getFirst("cache-control"));
 	}
 
+	@Test
+	public void cacheControlEmpty() {
+		headers.setCacheControl(CacheControl.empty());
+		assertNull("Invalid Cache-Control header", headers.getCacheControl());
+		assertNull("Invalid Cache-Control header", headers.getFirst("cache-control"));
+	}
+
 	@Test
 	public void cacheControlAllValues() {
 		headers.add(HttpHeaders.CACHE_CONTROL, "max-age=1000, public");
 		headers.add(HttpHeaders.CACHE_CONTROL, "s-maxage=1000");
-		assertThat(headers.getCacheControl(), is("max-age=1000, public, s-maxage=1000"));
+		assertEquals("max-age=1000, public, s-maxage=1000", headers.getCacheControl());
 	}
 
 	@Test
@@ -375,7 +382,7 @@ public void contentDisposition() {
 
 	@Test  // SPR-11917
 	public void getAllowEmptySet() {
-		headers.setAllow(Collections.<HttpMethod> emptySet());
+		headers.setAllow(Collections.emptySet());
 		assertThat(headers.getAllow(), Matchers.emptyCollectionOf(HttpMethod.class));
 	}
 

spring-web/src/test/java/org/springframework/http/client/SimpleClientHttpRequestFactoryTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2015 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.
@@ -29,7 +29,7 @@
  */
 public class SimpleClientHttpRequestFactoryTests {
 
-	@Test // SPR-13225
+	@Test  // SPR-13225
 	public void headerWithNullValue() {
 		HttpURLConnection urlConnection = mock(HttpURLConnection.class);
 		HttpHeaders headers = new HttpHeaders();

spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerResponseBuilder.java

@@ -177,10 +177,7 @@ public ServerResponse.BodyBuilder location(URI location) {
 
 	@Override
 	public ServerResponse.BodyBuilder cacheControl(CacheControl cacheControl) {
-		String ccValue = cacheControl.getHeaderValue();
-		if (ccValue != null) {
-			this.headers.setCacheControl(cacheControl.getHeaderValue());
-		}
+		this.headers.setCacheControl(cacheControl);
 		return this;
 	}
 

spring-webflux/src/main/java/org/springframework/web/reactive/resource/ResourceWebHandler.java

@@ -345,11 +345,9 @@ public Mono<Void> handle(ServerWebExchange exchange) {
 						}
 
 						// Apply cache settings, if any
-						if (getCacheControl() != null) {
-							String value = getCacheControl().getHeaderValue();
-							if (value != null) {
-								exchange.getResponse().getHeaders().setCacheControl(value);
-							}
+						CacheControl cacheControl = getCacheControl();
+						if (cacheControl != null) {
+							exchange.getResponse().getHeaders().setCacheControl(cacheControl);
 						}
 
 						// Check the media type for the resource