Improve multi-valued HTTP headers support

Prior to this change, getting header values with `HttpHeaders` when
headers are multi-valued would cause issues.
For example, for a given HTTP message with headers:
    Cache-Control: public, s-maxage=50
    Cache-Control: max-age=42

Getting a `List` of all values would return <"public", "s-maxage=50">
and getting the header value would return "public, s-maxage=50".

This commit takes now into account multi-valued HTTP headers and adds
new getters/setters for "If-Match" and "If-Unmodified-Since" headers.

Note that for ETag-related headers such as "If-Match" and
"If-None-Match", a special parser has been implemented since ETag values
can contain separator characters.

Issue: SPR-14223, SPR-14228

Author: bclozel

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

@@ -34,6 +34,8 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.TimeZone;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 
 import org.springframework.util.Assert;
 import org.springframework.util.LinkedCaseInsensitiveMap;
@@ -55,6 +57,7 @@
  *
  * @author Arjen Poutsma
  * @author Sebastien Deleuze
+ * @author Brian Clozel
  * @since 3.0
  */
 public class HttpHeaders implements MultiValueMap<String, String>, Serializable {
@@ -372,6 +375,12 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
 			"EEE MMM dd HH:mm:ss yyyy"
 	};
 
+	/**
+	 * 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>
+	 */
+	private static final Pattern ETAG_HEADER_VALUE_PATTERN = Pattern.compile("\\*|\\s*((W\\/)?(\"[^\"]*\"))\\s*,?");
+
 	private static TimeZone GMT = TimeZone.getTimeZone("GMT");
 
 
@@ -459,7 +468,7 @@ public void setAccessControlAllowHeaders(List<String> allowedHeaders) {
 	 * Returns the value of the {@code Access-Control-Allow-Headers} response header.
 	 */
 	public List<String> getAccessControlAllowHeaders() {
-		return getFirstValueAsList(ACCESS_CONTROL_ALLOW_HEADERS);
+		return getValuesAsList(ACCESS_CONTROL_ALLOW_HEADERS);
 	}
 
 	/**
@@ -476,7 +485,7 @@ public List<HttpMethod> getAccessControlAllowMethods() {
 		List<HttpMethod> result = new ArrayList<HttpMethod>();
 		String value = getFirst(ACCESS_CONTROL_ALLOW_METHODS);
 		if (value != null) {
-			String[] tokens = value.split(",\\s*");
+			String[] tokens = StringUtils.tokenizeToStringArray(value, ",", true, true);
 			for (String token : tokens) {
 				HttpMethod resolved = HttpMethod.resolve(token);
 				if (resolved != null) {
@@ -498,7 +507,23 @@ public void setAccessControlAllowOrigin(String allowedOrigin) {
 	 * Return the value of the {@code Access-Control-Allow-Origin} response header.
 	 */
 	public String getAccessControlAllowOrigin() {
-		return getFirst(ACCESS_CONTROL_ALLOW_ORIGIN);
+		return getFieldValues(ACCESS_CONTROL_ALLOW_ORIGIN);
+	}
+
+	protected String getFieldValues(String headerName) {
+		List<String> headerValues = this.headers.get(headerName);
+		if (headerValues != null) {
+			StringBuilder builder = new StringBuilder();
+			for (Iterator<String> iterator = headerValues.iterator(); iterator.hasNext(); ) {
+				String ifNoneMatch = iterator.next();
+				builder.append(ifNoneMatch);
+				if (iterator.hasNext()) {
+					builder.append(", ");
+				}
+			}
+			return builder.toString();
+		}
+		return null;
 	}
 
 	/**
@@ -512,7 +537,7 @@ public void setAccessControlExposeHeaders(List<String> exposedHeaders) {
 	 * Returns the value of the {@code Access-Control-Expose-Headers} response header.
 	 */
 	public List<String> getAccessControlExposeHeaders() {
-		return getFirstValueAsList(ACCESS_CONTROL_EXPOSE_HEADERS);
+		return getValuesAsList(ACCESS_CONTROL_EXPOSE_HEADERS);
 	}
 
 	/**
@@ -542,7 +567,7 @@ public void setAccessControlRequestHeaders(List<String> requestHeaders) {
 	 * Returns the value of the {@code Access-Control-Request-Headers} request header.
 	 */
 	public List<String> getAccessControlRequestHeaders() {
-		return getFirstValueAsList(ACCESS_CONTROL_REQUEST_HEADERS);
+		return getValuesAsList(ACCESS_CONTROL_REQUEST_HEADERS);
 	}
 
 	/**
@@ -643,7 +668,7 @@ public void setCacheControl(String cacheControl) {
 	 * Return the value of the {@code Cache-Control} header.
 	 */
 	public String getCacheControl() {
-		return getFirst(CACHE_CONTROL);
+		return getFieldValues(CACHE_CONTROL);
 	}
 
 	/**
@@ -664,7 +689,7 @@ public void setConnection(List<String> connection) {
 	 * Return the value of the {@code Connection} header.
 	 */
 	public List<String> getConnection() {
-		return getFirstValueAsList(CONNECTION);
+		return getValuesAsList(CONNECTION);
 	}
 
 	/**
@@ -782,6 +807,64 @@ public long getExpires() {
 		return getFirstDate(EXPIRES, false);
 	}
 
+	/**
+	 * Set the (new) value of the {@code If-Match} header.
+	 */
+	public void setIfMatch(String ifMatch) {
+		set(IF_MATCH, ifMatch);
+	}
+
+	/**
+	 * Set the (new) value of the {@code If-Match} header.
+	 */
+	public void setIfMatch(List<String> ifMatchList) {
+		set(IF_MATCH, toCommaDelimitedString(ifMatchList));
+	}
+
+	protected String toCommaDelimitedString(List<String> list) {
+		StringBuilder builder = new StringBuilder();
+		for (Iterator<String> iterator = list.iterator(); iterator.hasNext(); ) {
+			String ifNoneMatch = iterator.next();
+			builder.append(ifNoneMatch);
+			if (iterator.hasNext()) {
+				builder.append(", ");
+			}
+		}
+		return builder.toString();
+	}
+
+	/**
+	 * Return the value of the {@code If-Match} header.
+	 */
+	public List<String> getIfMatch() {
+		return getETagValuesAsList(IF_MATCH);
+	}
+
+	protected List<String> getETagValuesAsList(String headerName) {
+		List<String> values = get(headerName);
+		if (values != null) {
+			List<String> result = new ArrayList<String>();
+			for (String value : values) {
+				if (value != null) {
+					Matcher matcher = ETAG_HEADER_VALUE_PATTERN.matcher(value);
+					while (matcher.find()) {
+						if ("*".equals(matcher.group())) {
+							result.add(matcher.group());
+						}
+						else {
+							result.add(matcher.group(1));
+						}
+					}
+					if(result.size() == 0) {
+						throw new IllegalArgumentException("Could not parse '" + headerName + "' value=" + value);
+					}
+				}
+			}
+			return result;
+		}
+		return Collections.emptyList();
+	}
+
 	/**
 	 * Set the (new) value of the {@code If-Modified-Since} header.
 	 * <p>The date should be specified as the number of milliseconds since
@@ -814,35 +897,51 @@ public void setIfNoneMatch(List<String> ifNoneMatchList) {
 		set(IF_NONE_MATCH, toCommaDelimitedString(ifNoneMatchList));
 	}
 
-	protected String toCommaDelimitedString(List<String> list) {
-		StringBuilder builder = new StringBuilder();
-		for (Iterator<String> iterator = list.iterator(); iterator.hasNext();) {
-			String ifNoneMatch = iterator.next();
-			builder.append(ifNoneMatch);
-			if (iterator.hasNext()) {
-				builder.append(", ");
-			}
-		}
-		return builder.toString();
-	}
-
 	/**
 	 * Return the value of the {@code If-None-Match} header.
 	 */
 	public List<String> getIfNoneMatch() {
-		return getFirstValueAsList(IF_NONE_MATCH);
+		return getETagValuesAsList(IF_NONE_MATCH);
 	}
 
-	protected List<String> getFirstValueAsList(String header) {
-		List<String> result = new ArrayList<String>();
-		String value = getFirst(header);
-		if (value != null) {
-			String[] tokens = value.split(",\\s*");
-			for (String token : tokens) {
-				result.add(token);
+	/**
+	 * Return all values of a given header name,
+	 * even if this header is set multiple times.
+	 * @since 4.3.0
+	 */
+	public List<String> getValuesAsList(String headerName) {
+		List<String> values = get(headerName);
+		if (values != null) {
+			List<String> result = new ArrayList<String>();
+			for (String value : values) {
+				if (value != null) {
+					String[] tokens = StringUtils.tokenizeToStringArray(value, ",");
+					for (String token : tokens) {
+						result.add(token);
+					}
+				}
 			}
+			return result;
 		}
-		return result;
+		return Collections.emptyList();
+	}
+
+	/**
+	 * Set the (new) value of the {@code If-Unmodified-Since} header.
+	 * <p>The date should be specified as the number of milliseconds since
+	 * January 1, 1970 GMT.
+	 */
+	public void setIfUnmodifiedSince(long ifUnmodifiedSince) {
+		setDate(IF_UNMODIFIED_SINCE, ifUnmodifiedSince);
+	}
+
+	/**
+	 * Return the value of the {@code If-Unmodified-Since} header.
+	 * <p>The date is returned as the number of milliseconds since
+	 * January 1, 1970 GMT. Returns -1 when the date is unknown.
+	 */
+	public long getIfUnmodifiedSince() {
+		return getFirstDate(IF_UNMODIFIED_SINCE, false);
 	}
 
 	/**
@@ -957,7 +1056,7 @@ public void setVary(List<String> requestHeaders) {
 	 * Return the request header names subject to content negotiation.
 	 */
 	public List<String> getVary() {
-		return getFirstValueAsList(VARY);
+		return getValuesAsList(VARY);
 	}
 
 	/**

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

@@ -32,19 +32,28 @@
 import org.hamcrest.Matchers;
 import org.junit.Test;
 
+import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.*;
 
 /**
  * Unit tests for {@link org.springframework.http.HttpHeaders}.
  *
  * @author Arjen Poutsma
  * @author Sebastien Deleuze
+ * @author Brian Clozel
  */
 public class HttpHeadersTests {
 
 	private final HttpHeaders headers = new HttpHeaders();
 
 
+	@Test
+	public void getFirst() {
+		headers.add(HttpHeaders.CACHE_CONTROL, "max-age=1000, public");
+		headers.add(HttpHeaders.CACHE_CONTROL, "s-maxage=1000");
+		assertThat(headers.getFirst(HttpHeaders.CACHE_CONTROL), is("max-age=1000, public"));
+	}
+
 	@Test
 	public void accept() {
 		MediaType mediaType1 = new MediaType("text", "html");
@@ -132,6 +141,29 @@ public void illegalETag() {
 		assertEquals("Invalid ETag header", "\"v2.6\"", headers.getFirst("ETag"));
 	}
 
+	@Test
+	public void ifMatch() {
+		String ifMatch = "\"v2.6\"";
+		headers.setIfMatch(ifMatch);
+		assertEquals("Invalid If-Match header", ifMatch, headers.getIfMatch().get(0));
+		assertEquals("Invalid If-Match header", "\"v2.6\"", headers.getFirst("If-Match"));
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void ifMatchIllegalHeader() {
+		headers.setIfMatch("Illegal");
+		headers.getIfMatch();
+	}
+
+	@Test
+	public void ifMatchMultipleHeaders() {
+		headers.add(HttpHeaders.IF_MATCH, "\"v2,0\"");
+		headers.add(HttpHeaders.IF_MATCH, "W/\"v2,1\", \"v2,2\"");
+		assertEquals("Invalid If-Match header", "\"v2,0\"", headers.get(HttpHeaders.IF_MATCH).get(0));
+		assertEquals("Invalid If-Match header", "W/\"v2,1\", \"v2,2\"", headers.get(HttpHeaders.IF_MATCH).get(1));
+		assertThat(headers.getIfMatch(), Matchers.contains("\"v2,0\"", "W/\"v2,1\"", "\"v2,2\""));
+	}
+
 	@Test
 	public void ifNoneMatch() {
 		String ifNoneMatch = "\"v2.6\"";
@@ -140,16 +172,24 @@ public void ifNoneMatch() {
 		assertEquals("Invalid If-None-Match header", "\"v2.6\"", headers.getFirst("If-None-Match"));
 	}
 
+	@Test
+	public void ifNoneMatchWildCard() {
+		String ifNoneMatch = "*";
+		headers.setIfNoneMatch(ifNoneMatch);
+		assertEquals("Invalid If-None-Match header", ifNoneMatch, headers.getIfNoneMatch().get(0));
+		assertEquals("Invalid If-None-Match header", "*", headers.getFirst("If-None-Match"));
+	}
+
 	@Test
 	public void ifNoneMatchList() {
 		String ifNoneMatch1 = "\"v2.6\"";
-		String ifNoneMatch2 = "\"v2.7\"";
+		String ifNoneMatch2 = "\"v2.7\", \"v2.8\"";
 		List<String> ifNoneMatchList = new ArrayList<String>(2);
 		ifNoneMatchList.add(ifNoneMatch1);
 		ifNoneMatchList.add(ifNoneMatch2);
 		headers.setIfNoneMatch(ifNoneMatchList);
-		assertEquals("Invalid If-None-Match header", ifNoneMatchList, headers.getIfNoneMatch());
-		assertEquals("Invalid If-None-Match header", "\"v2.6\", \"v2.7\"", headers.getFirst("If-None-Match"));
+		assertThat(headers.getIfNoneMatch(), Matchers.contains("\"v2.6\"", "\"v2.7\"", "\"v2.8\""));
+		assertEquals("Invalid If-None-Match header", "\"v2.6\", \"v2.7\", \"v2.8\"", headers.getFirst("If-None-Match"));
 	}
 
 	@Test
@@ -255,6 +295,13 @@ public void cacheControl() {
 		assertEquals("Invalid Cache-Control header", "no-cache", 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"));
+	}
+
 	@Test
 	public void contentDisposition() {
 		headers.setContentDispositionFormData("name", null);
@@ -290,6 +337,16 @@ public void accessControlAllowHeaders() {
 		assertEquals(allowedHeaders, Arrays.asList("header1", "header2"));
 	}
 
+	@Test
+	public void accessControlAllowHeadersMultipleValues() {
+		List<String> allowedHeaders = headers.getAccessControlAllowHeaders();
+		assertThat(allowedHeaders, Matchers.emptyCollectionOf(String.class));
+		headers.add(HttpHeaders.ACCESS_CONTROL_ALLOW_HEADERS, "header1, header2");
+		headers.add(HttpHeaders.ACCESS_CONTROL_ALLOW_HEADERS, "header3");
+		allowedHeaders = headers.getAccessControlAllowHeaders();
+		assertEquals(Arrays.asList("header1", "header2", "header3"), allowedHeaders);
+	}
+
 	@Test
 	public void accessControlAllowMethods() {
 		List<HttpMethod> allowedMethods = headers.getAccessControlAllowMethods();

spring-websocket/src/main/java/org/springframework/web/socket/WebSocketHttpHeaders.java

@@ -172,7 +172,7 @@ public List<String> getSecWebSocketProtocol() {
 			return Collections.emptyList();
 		}
 		else if (values.size() == 1) {
-			return getFirstValueAsList(SEC_WEBSOCKET_PROTOCOL);
+			return getValuesAsList(SEC_WEBSOCKET_PROTOCOL);
 		}
 		else {
 			return values;