HttpEntityMethodProcessor lets entity headers override existing headers (again)

Issue: SPR-15952

(cherry picked from commit 5bdcb89)

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 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.
@@ -172,15 +172,15 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro
 
 		HttpHeaders outputHeaders = outputMessage.getHeaders();
 		HttpHeaders entityHeaders = responseEntity.getHeaders();
-		if (outputHeaders.containsKey(HttpHeaders.VARY) && entityHeaders.containsKey(HttpHeaders.VARY)) {
-			List<String> values = getVaryRequestHeadersToAdd(outputHeaders, entityHeaders);
-			if (!values.isEmpty()) {
-				outputHeaders.setVary(values);
-			}
-		}
 		if (!entityHeaders.isEmpty()) {
 			for (Map.Entry<String, List<String>> entry : entityHeaders.entrySet()) {
-				if (!outputHeaders.containsKey(entry.getKey())) {
+				if (HttpHeaders.VARY.equals(entry.getKey()) && outputHeaders.containsKey(HttpHeaders.VARY)) {
+					List<String> values = getVaryRequestHeadersToAdd(outputHeaders, entityHeaders);
+					if (!values.isEmpty()) {
+						outputHeaders.setVary(values);
+					}
+				}
+				else {
 					outputHeaders.put(entry.getKey(), entry.getValue());
 				}
 			}
@@ -207,24 +207,25 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro
 	}
 
 	private List<String> getVaryRequestHeadersToAdd(HttpHeaders responseHeaders, HttpHeaders entityHeaders) {
-		if (!responseHeaders.containsKey(HttpHeaders.VARY)) {
-			return entityHeaders.getVary();
-		}
 		List<String> entityHeadersVary = entityHeaders.getVary();
-		List<String> result = new ArrayList<String>(entityHeadersVary);
+		List<String> vary = responseHeaders.get(HttpHeaders.VARY);
-		for (String header : responseHeaders.get(HttpHeaders.VARY)) {
+		if (vary != null) {
-			for (String existing : StringUtils.tokenizeToStringArray(header, ",")) {
+			List<String> result = new ArrayList<String>(entityHeadersVary);
-				if ("*".equals(existing)) {
+			for (String header : vary) {
-					return Collections.emptyList();
+				for (String existing : StringUtils.tokenizeToStringArray(header, ",")) {
-				}
+					if ("*".equals(existing)) {
-				for (String value : entityHeadersVary) {
+						return Collections.emptyList();
-					if (value.equalsIgnoreCase(existing)) {
+					}
-						result.remove(value);
+					for (String value : entityHeadersVary) {
+						if (value.equalsIgnoreCase(existing)) {
+							result.remove(value);
+						}
 					}
 				}
 			}
+			return result;
 		}
-		return result;
+		return entityHeadersVary;
 	}
 
 	private boolean isResourceNotModified(ServletServerHttpRequest inputMessage, ServletServerHttpResponse outputMessage) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2017 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,18 +16,14 @@
 
 package org.springframework.web.servlet.mvc.method.annotation;
 
-import static org.junit.Assert.*;
-import static org.mockito.BDDMockito.*;
-import static org.springframework.web.servlet.HandlerMapping.*;
-
 import java.lang.reflect.Method;
 import java.net.URI;
 import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 import java.text.SimpleDateFormat;
-import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Date;
-import java.util.List;
 import java.util.Locale;
 import java.util.TimeZone;
 
@@ -58,6 +54,10 @@ import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.context.request.ServletWebRequest;
 import org.springframework.web.method.support.ModelAndViewContainer;
 
+import static org.junit.Assert.*;
+import static org.mockito.BDDMockito.*;
+import static org.springframework.web.servlet.HandlerMapping.*;
+
 /**
  * Test fixture for {@link HttpEntityMethodProcessor} delegating to a mock
  * {@link HttpMessageConverter}.
@@ -81,6 +81,8 @@ public class HttpEntityMethodProcessorMockTests {
 
 	private HttpMessageConverter<Resource> resourceMessageConverter;
 
+	private HttpMessageConverter<Object> resourceRegionMessageConverter;
+
 	private MethodParameter paramHttpEntity;
 
 	private MethodParameter paramRequestEntity;
@@ -110,9 +112,9 @@ public class HttpEntityMethodProcessorMockTests {
 	private ServletWebRequest webRequest;
 
 
-	@SuppressWarnings("unchecked")
 	@Before
-	public void setUp() throws Exception {
+	@SuppressWarnings("unchecked")
+	public void setup() throws Exception {
 		dateFormat = new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss z", Locale.US);
 		dateFormat.setTimeZone(TimeZone.getTimeZone("GMT"));
 
@@ -120,12 +122,11 @@ public class HttpEntityMethodProcessorMockTests {
 		given(stringHttpMessageConverter.getSupportedMediaTypes()).willReturn(Collections.singletonList(MediaType.TEXT_PLAIN));
 		resourceMessageConverter = mock(HttpMessageConverter.class);
 		given(resourceMessageConverter.getSupportedMediaTypes()).willReturn(Collections.singletonList(MediaType.ALL));
-		List<HttpMessageConverter<?>> converters = new ArrayList<>();
+		resourceRegionMessageConverter = mock(HttpMessageConverter.class);
-		converters.add(stringHttpMessageConverter);
+		given(resourceRegionMessageConverter.getSupportedMediaTypes()).willReturn(Collections.singletonList(MediaType.ALL));
-		converters.add(resourceMessageConverter);
+
-		processor = new HttpEntityMethodProcessor(converters);
+		processor = new HttpEntityMethodProcessor(
-		reset(stringHttpMessageConverter);
+				Arrays.asList(stringHttpMessageConverter, resourceMessageConverter, resourceRegionMessageConverter));
-		reset(resourceMessageConverter);
 
 		Method handle1 = getClass().getMethod("handle1", HttpEntity.class, ResponseEntity.class,
 				Integer.TYPE, RequestEntity.class);
@@ -172,7 +173,7 @@ public class HttpEntityMethodProcessorMockTests {
 
 		MediaType contentType = MediaType.TEXT_PLAIN;
 		servletRequest.addHeader("Content-Type", contentType.toString());
-		servletRequest.setContent(body.getBytes(Charset.forName("UTF-8")));
+		servletRequest.setContent(body.getBytes(StandardCharsets.UTF_8));
 
 		given(stringHttpMessageConverter.canRead(String.class, contentType)).willReturn(true);
 		given(stringHttpMessageConverter.read(eq(String.class), isA(HttpInputMessage.class))).willReturn(body);
@@ -259,8 +260,8 @@ public class HttpEntityMethodProcessorMockTests {
 		verify(stringHttpMessageConverter).write(eq(body), eq(MediaType.TEXT_HTML), isA(HttpOutputMessage.class));
 	}
 
-	@SuppressWarnings("unchecked")
 	@Test
+	@SuppressWarnings("unchecked")
 	public void shouldHandleReturnValueWithResponseBodyAdvice() throws Exception {
 		servletRequest.addHeader("Accept", "text/*");
 		servletRequest.setAttribute(PRODUCIBLE_MEDIA_TYPES_ATTRIBUTE, Collections.singleton(MediaType.TEXT_HTML));
@@ -310,7 +311,7 @@ public class HttpEntityMethodProcessorMockTests {
 		processor.handleReturnValue(returnValue, returnTypeResponseEntityProduces, mavContainer, webRequest);
 	}
 
-	@Test // SPR-9142
+	@Test  // SPR-9142
 	public void shouldFailHandlingWhenAcceptHeaderIllegal() throws Exception {
 		ResponseEntity<String> returnValue = new ResponseEntity<>("Body", HttpStatus.ACCEPTED);
 		servletRequest.addHeader("Accept", "01");
@@ -371,7 +372,7 @@ public class HttpEntityMethodProcessorMockTests {
 		assertConditionalResponse(HttpStatus.NOT_MODIFIED, null, etagValue, -1);
 	}
 
-	@Test // SPR-14559
+	@Test  // SPR-14559
 	public void shouldHandleInvalidIfNoneMatchWithHttp200() throws Exception {
 		String etagValue = "\"deadb33f8badf00d\"";
 		servletRequest.addHeader(HttpHeaders.IF_NONE_MATCH, "unquoted");
@@ -429,7 +430,7 @@ public class HttpEntityMethodProcessorMockTests {
 		assertConditionalResponse(HttpStatus.OK, null, changedEtagValue, oneMinuteAgo);
 	}
 
-	@Test // SPR-13496
+	@Test  // SPR-13496
 	public void shouldHandleConditionalRequestIfNoneMatchWildcard() throws Exception {
 		String wildcardValue = "*";
 		String etagValue = "\"some-etag\"";
@@ -443,7 +444,7 @@ public class HttpEntityMethodProcessorMockTests {
 		assertConditionalResponse(HttpStatus.OK, "body", etagValue, -1);
 	}
 
-	@Test // SPR-13626
+	@Test  // SPR-13626
 	public void shouldHandleGetIfNoneMatchWildcard() throws Exception {
 		String wildcardValue = "*";
 		String etagValue = "\"some-etag\"";
@@ -456,7 +457,7 @@ public class HttpEntityMethodProcessorMockTests {
 		assertConditionalResponse(HttpStatus.OK, "body", etagValue, -1);
 	}
 
-	@Test // SPR-13626
+	@Test  // SPR-13626
 	public void shouldHandleIfNoneMatchIfMatch() throws Exception {
 		String etagValue = "\"some-etag\"";
 		servletRequest.addHeader(HttpHeaders.IF_NONE_MATCH, etagValue);
@@ -469,7 +470,7 @@ public class HttpEntityMethodProcessorMockTests {
 		assertConditionalResponse(HttpStatus.NOT_MODIFIED, null, etagValue, -1);
 	}
 
-	@Test // SPR-13626
+	@Test  // SPR-13626
 	public void shouldHandleIfNoneMatchIfUnmodifiedSince() throws Exception {
 		String etagValue = "\"some-etag\"";
 		servletRequest.addHeader(HttpHeaders.IF_NONE_MATCH, etagValue);
@@ -485,7 +486,7 @@ public class HttpEntityMethodProcessorMockTests {
 	@Test
 	public void shouldHandleResource() throws Exception {
 		ResponseEntity<Resource> returnValue = ResponseEntity
-				.ok(new ByteArrayResource("Content".getBytes(Charset.forName("UTF-8"))));
+				.ok(new ByteArrayResource("Content".getBytes(StandardCharsets.UTF_8)));
 
 		given(resourceMessageConverter.canWrite(ByteArrayResource.class, null)).willReturn(true);
 		given(resourceMessageConverter.getSupportedMediaTypes()).willReturn(Collections.singletonList(MediaType.ALL));
@@ -498,7 +499,7 @@ public class HttpEntityMethodProcessorMockTests {
 		assertEquals(200, servletResponse.getStatus());
 	}
 
-	@Test //SPR-14767
+	@Test  //SPR-14767
 	public void shouldHandleValidatorHeadersInPutResponses() throws Exception {
 		servletRequest.setMethod("PUT");
 		String etagValue = "\"some-etag\"";
@@ -510,6 +511,60 @@ public class HttpEntityMethodProcessorMockTests {
 		assertConditionalResponse(HttpStatus.OK, "body", etagValue, -1);
 	}
 
+	@Test
+	public void varyHeader() throws Exception {
+		String[] entityValues = {"Accept-Language", "User-Agent"};
+		String[] existingValues = {};
+		String[] expected = {"Accept-Language, User-Agent"};
+		testVaryHeader(entityValues, existingValues, expected);
+	}
+
+	@Test
+	public void varyHeaderWithExistingWildcard() throws Exception {
+		String[] entityValues = {"Accept-Language"};
+		String[] existingValues = {"*"};
+		String[] expected = {"*"};
+		testVaryHeader(entityValues, existingValues, expected);
+	}
+
+	@Test
+	public void varyHeaderWithExistingCommaValues() throws Exception {
+		String[] entityValues = {"Accept-Language", "User-Agent"};
+		String[] existingValues = {"Accept-Encoding", "Accept-Language"};
+		String[] expected = {"Accept-Encoding", "Accept-Language", "User-Agent"};
+		testVaryHeader(entityValues, existingValues, expected);
+	}
+
+	@Test
+	public void varyHeaderWithExistingCommaSeparatedValues() throws Exception {
+		String[] entityValues = {"Accept-Language", "User-Agent"};
+		String[] existingValues = {"Accept-Encoding, Accept-Language"};
+		String[] expected = {"Accept-Encoding, Accept-Language", "User-Agent"};
+		testVaryHeader(entityValues, existingValues, expected);
+	}
+
+	@Test
+	public void handleReturnValueVaryHeader() throws Exception {
+		String[] entityValues = {"Accept-Language", "User-Agent"};
+		String[] existingValues = {"Accept-Encoding, Accept-Language"};
+		String[] expected = {"Accept-Encoding, Accept-Language", "User-Agent"};
+		testVaryHeader(entityValues, existingValues, expected);
+	}
+
+
+	private void testVaryHeader(String[] entityValues, String[] existingValues, String[] expected) throws Exception {
+		ResponseEntity<String> returnValue = ResponseEntity.ok().varyBy(entityValues).body("Foo");
+		for (String value : existingValues) {
+			servletResponse.addHeader("Vary", value);
+		}
+		initStringMessageConversion(MediaType.TEXT_PLAIN);
+		processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest);
+
+		assertTrue(mavContainer.isRequestHandled());
+		assertEquals(Arrays.asList(expected), servletResponse.getHeaders("Vary"));
+		verify(stringHttpMessageConverter).write(eq("Foo"), eq(MediaType.TEXT_PLAIN), isA(HttpOutputMessage.class));
+	}
+
 	private void initStringMessageConversion(MediaType accepted) {
 		given(stringHttpMessageConverter.canWrite(String.class, null)).willReturn(true);
 		given(stringHttpMessageConverter.getSupportedMediaTypes()).willReturn(Collections.singletonList(MediaType.TEXT_PLAIN));
@@ -521,8 +576,7 @@ public class HttpEntityMethodProcessorMockTests {
 		verify(stringHttpMessageConverter).write(eq(body), eq(MediaType.TEXT_PLAIN), outputMessage.capture());
 	}
 
-	private void assertConditionalResponse(HttpStatus status, String body,
+	private void assertConditionalResponse(HttpStatus status, String body, String etag, long lastModified) throws Exception {
-			String etag, long lastModified) throws Exception {
 		assertEquals(status.value(), servletResponse.getStatus());
 		assertTrue(mavContainer.isRequestHandled());
 		if (body != null) {
@@ -541,6 +595,7 @@ public class HttpEntityMethodProcessorMockTests {
 		}
 	}
 
+
 	@SuppressWarnings("unused")
 	public ResponseEntity<String> handle1(HttpEntity<String> httpEntity, ResponseEntity<String> entity,
 			int i, RequestEntity<String> requestEntity) {