Fix conditional requests support for HttpEntity

Prior to this commit, `HttpEntityMethodProcessor` would rely on
`ServletWebRequest` to process conditional requests and with incoming
`"If-Modified-Since"` / `"If-None-Match"` request headers.

This approach is problematic since in that class:

* response is wrapped in a `ServletServerHttpResponse`
* this wrapped response does not write response headers right away
* `ServletWebRequest.checkNotModified` methods can't apply their
logic with incomplete response headers

This solution adds some minimal code duplication and applies
the conditional request logic within the Processor.

A possible alternative would be to improve the
`ServletServerHttpResponse$ServletResponseHttpHeaders` implementation
with write methods - but this solution would only work for Servlet 3.x
applications.

Issue: SPR-13090

spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java

@@ -181,31 +181,19 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ
 	public boolean checkNotModified(long lastModifiedTimestamp) {
 		HttpServletResponse response = getResponse();
 		if (lastModifiedTimestamp >= 0 && !this.notModified) {
-			if (response == null || isResponseCompatibleWithConditional(response, HEADER_LAST_MODIFIED)) {
+			if (response == null || HttpStatus.valueOf(response.getStatus()).is2xxSuccessful()) {
 				this.notModified = isTimeStampNotModified(lastModifiedTimestamp);
 				if (response != null) {
 					if (this.notModified && supportsNotModifiedStatus()) {
 						response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
 					}
-					response.setHeader(HEADER_LAST_MODIFIED, formatDate(lastModifiedTimestamp));
-				}
-			}
-		}
-		return this.notModified;
-	}
-
-	private boolean isResponseCompatibleWithConditional(HttpServletResponse response, String... headers) {
-		if (response != null) {
-			if (HttpStatus.valueOf(response.getStatus()).is2xxSuccessful()) {
-				for (String header : headers) {
-					if (response.containsHeader(header)) {
-						return false;
+					if(response.getHeader(HEADER_LAST_MODIFIED) == null) {
+						response.setHeader(HEADER_LAST_MODIFIED, formatDate(lastModifiedTimestamp));
 					}
 				}
-				return true;
 			}
 		}
-		return false;
+		return this.notModified;
 	}
 
 	@SuppressWarnings("deprecation")
@@ -235,14 +223,16 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ
 	public boolean checkNotModified(String etag) {
 		HttpServletResponse response = getResponse();
 		if (StringUtils.hasLength(etag) && !this.notModified) {
-			if (response == null || isResponseCompatibleWithConditional(response, HEADER_ETAG)) {
+			if (response == null || HttpStatus.valueOf(response.getStatus()).is2xxSuccessful()) {
 				etag = addEtagPadding(etag);
 				this.notModified = isETagNotModified(etag);
 				if (response != null) {
 					if (this.notModified && supportsNotModifiedStatus()) {
 						response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
 					}
-					response.setHeader(HEADER_ETAG, etag);
+					if(response.getHeader(HEADER_ETAG) == null) {
+						response.setHeader(HEADER_ETAG, etag);
+					}
 				}
 			}
 		}
@@ -283,15 +273,19 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ
 	public boolean checkNotModified(String etag, long lastModifiedTimestamp) {
 		HttpServletResponse response = getResponse();
 		if (StringUtils.hasLength(etag) && !this.notModified) {
-			if (response == null || isResponseCompatibleWithConditional(response, HEADER_ETAG, HEADER_LAST_MODIFIED)) {
+			if (response == null || HttpStatus.valueOf(response.getStatus()).is2xxSuccessful()) {
 				etag = addEtagPadding(etag);
 				this.notModified = isETagNotModified(etag) && isTimeStampNotModified(lastModifiedTimestamp);
 				if (response != null) {
 					if (this.notModified && supportsNotModifiedStatus()) {
 						response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
 					}
-					response.setHeader(HEADER_ETAG, etag);
-					response.setHeader(HEADER_LAST_MODIFIED, formatDate(lastModifiedTimestamp));
+					if(response.getHeader(HEADER_ETAG) == null) {
+						response.setHeader(HEADER_ETAG, etag);
+					}
+					if(response.getHeader(HEADER_LAST_MODIFIED) == null) {
+						response.setHeader(HEADER_LAST_MODIFIED, formatDate(lastModifiedTimestamp));
+					}
 				}
 			}
 		}

spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java

@@ -93,8 +93,8 @@ public class ServletWebRequestHttpMethodsTests {
 		servletRequest.addHeader("If-Modified-Since", epochTime);
 		servletResponse.addHeader("Last-Modified", CURRENT_TIME);
 
-		assertFalse(request.checkNotModified(epochTime));
-		assertEquals(200, servletResponse.getStatus());
+		assertTrue(request.checkNotModified(epochTime));
+		assertEquals(304, servletResponse.getStatus());
 		assertEquals(1, servletResponse.getHeaders("Last-Modified").size());
 		assertEquals(CURRENT_TIME, servletResponse.getHeader("Last-Modified"));
 	}

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

@@ -27,6 +27,7 @@ import org.springframework.core.ResolvableType;
 import org.springframework.http.HttpEntity;
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
+import org.springframework.http.HttpStatus;
 import org.springframework.http.RequestEntity;
 import org.springframework.http.ResponseEntity;
 import org.springframework.http.converter.HttpMessageConverter;
@@ -163,32 +164,22 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro
 
 		Assert.isInstanceOf(HttpEntity.class, returnValue);
 		HttpEntity<?> responseEntity = (HttpEntity<?>) returnValue;
-		if (responseEntity instanceof ResponseEntity) {
-			outputMessage.setStatusCode(((ResponseEntity<?>) responseEntity).getStatusCode());
-		}
 
 		HttpHeaders entityHeaders = responseEntity.getHeaders();
-
+		if (!entityHeaders.isEmpty()) {
+			outputMessage.getHeaders().putAll(entityHeaders);
+		}
 		Object body = responseEntity.getBody();
 		if (responseEntity instanceof ResponseEntity) {
-			for (String headerName : entityHeaders.keySet()) {
-				if(!HttpHeaders.LAST_MODIFIED.equals(headerName)
-					&& !HttpHeaders.ETAG.equals(headerName)) {
-					outputMessage.getHeaders().put(headerName, entityHeaders.get(headerName));
-				}
-			}
-			if (isResourceNotModified(webRequest, (ResponseEntity<?>) responseEntity)) {
+			outputMessage.setStatusCode(((ResponseEntity<?>) responseEntity).getStatusCode());
+			if (isResourceNotModified(inputMessage, outputMessage)) {
+				outputMessage.setStatusCode(HttpStatus.NOT_MODIFIED);
 				// Ensure headers are flushed, no body should be written.
 				outputMessage.flush();
 				// Skip call to converters, as they may update the body.
 				return;
 			}
 		}
-		else {
-			if (!entityHeaders.isEmpty()) {
-				outputMessage.getHeaders().putAll(entityHeaders);
-			}
-		}
 
 		// Try even with null body. ResponseBodyAdvice could get involved.
 		writeWithMessageConverters(body, returnType, inputMessage, outputMessage);
@@ -197,22 +188,52 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro
 		outputMessage.flush();
 	}
 
-	private boolean isResourceNotModified(NativeWebRequest webRequest, ResponseEntity<?> responseEntity) {
-		String eTag = responseEntity.getHeaders().getETag();
-		long lastModified = responseEntity.getHeaders().getLastModified();
+	private boolean isResourceNotModified(ServletServerHttpRequest inputMessage, ServletServerHttpResponse outputMessage) {
+
+		List<String> ifNoneMatch = inputMessage.getHeaders().getIfNoneMatch();
+		long ifModifiedSince = inputMessage.getHeaders().getIfModifiedSince();
+		String eTag = addEtagPadding(outputMessage.getHeaders().getETag());
+		long lastModified = outputMessage.getHeaders().getLastModified();
 		boolean notModified = false;
+
 		if (lastModified != -1 && StringUtils.hasLength(eTag)) {
-			notModified = webRequest.checkNotModified(eTag, lastModified);
+			notModified = isETagNotModified(ifNoneMatch, eTag) && isTimeStampNotModified(ifModifiedSince, lastModified);
 		}
 		else if (lastModified != -1) {
-			notModified = webRequest.checkNotModified(lastModified);
+			notModified = isTimeStampNotModified(ifModifiedSince, lastModified);
 		}
 		else if (StringUtils.hasLength(eTag)) {
-			notModified = webRequest.checkNotModified(eTag);
+			notModified = isETagNotModified(ifNoneMatch, eTag);
 		}
 		return notModified;
 	}
 
+	private boolean isETagNotModified(List<String> ifNoneMatch, String etag) {
+		if (StringUtils.hasLength(etag)) {
+			for (String clientETag : ifNoneMatch) {
+				// compare weak/strong ETags as per https://tools.ietf.org/html/rfc7232#section-2.3
+				if (StringUtils.hasLength(clientETag) &&
+						(clientETag.replaceFirst("^W/", "").equals(etag.replaceFirst("^W/", ""))
+								|| clientETag.equals("*"))) {
+					return true;
+				}
+			}
+		}
+		return false;
+	}
+
+	private boolean isTimeStampNotModified(long ifModifiedSince, long lastModifiedTimestamp) {
+		return (ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000));
+	}
+
+	private String addEtagPadding(String etag) {
+		if (StringUtils.hasLength(etag) &&
+				(!(etag.startsWith("\"") || etag.startsWith("W/\"")) || !etag.endsWith("\"")) ) {
+			etag = "\"" + etag + "\"";
+		}
+		return etag;
+	}
+
 	@Override
 	protected Class<?> getReturnValueType(Object returnValue, MethodParameter returnType) {
 		if (returnValue != null) {

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

@@ -336,7 +336,7 @@ public class HttpEntityMethodProcessorMockTests {
 	public void handleReturnTypeLastModified() throws Exception {
 		long currentTime = new Date().getTime();
 		long oneMinuteAgo  = currentTime - (1000 * 60);
-		servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, currentTime);
+		servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, dateFormat.format(currentTime));
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setDate(HttpHeaders.LAST_MODIFIED, oneMinuteAgo);
 		ResponseEntity<String> returnValue = new ResponseEntity<String>("body", responseHeaders, HttpStatus.OK);
@@ -380,7 +380,7 @@ public class HttpEntityMethodProcessorMockTests {
 		long currentTime = new Date().getTime();
 		long oneMinuteAgo  = currentTime - (1000 * 60);
 		String etagValue = "\"deadb33f8badf00d\"";
-		servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, currentTime);
+		servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, dateFormat.format(currentTime));
 		servletRequest.addHeader(HttpHeaders.IF_NONE_MATCH, etagValue);
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setDate(HttpHeaders.LAST_MODIFIED, oneMinuteAgo);
@@ -402,13 +402,38 @@ public class HttpEntityMethodProcessorMockTests {
 		assertEquals(0, servletResponse.getContentAsByteArray().length);
 	}
 
+	@Test
+	public void handleReturnTypeNotModified() throws Exception {
+		long currentTime = new Date().getTime();
+		long oneMinuteAgo  = currentTime - (1000 * 60);
+		String etagValue = "\"deadb33f8badf00d\"";
+		HttpHeaders responseHeaders = new HttpHeaders();
+		responseHeaders.setDate(HttpHeaders.LAST_MODIFIED, oneMinuteAgo);
+		responseHeaders.set(HttpHeaders.ETAG, etagValue);
+		ResponseEntity<String> returnValue = new ResponseEntity<String>("body", responseHeaders, HttpStatus.NOT_MODIFIED);
+
+		given(messageConverter.canWrite(String.class, null)).willReturn(true);
+		given(messageConverter.getSupportedMediaTypes()).willReturn(Collections.singletonList(MediaType.TEXT_PLAIN));
+		given(messageConverter.canWrite(String.class, MediaType.TEXT_PLAIN)).willReturn(true);
+
+		processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest);
+
+		assertTrue(mavContainer.isRequestHandled());
+		assertEquals(HttpStatus.NOT_MODIFIED.value(), servletResponse.getStatus());
+		assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.LAST_MODIFIED).size());
+		assertEquals(dateFormat.format(oneMinuteAgo), servletResponse.getHeader(HttpHeaders.LAST_MODIFIED));
+		assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.ETAG).size());
+		assertEquals(etagValue, servletResponse.getHeader(HttpHeaders.ETAG));
+		assertEquals(0, servletResponse.getContentAsByteArray().length);
+	}
+
 	@Test
 	public void handleReturnTypeChangedETagAndLastModified() throws Exception {
 		long currentTime = new Date().getTime();
 		long oneMinuteAgo  = currentTime - (1000 * 60);
 		String etagValue = "\"deadb33f8badf00d\"";
 		String changedEtagValue = "\"changed-etag-value\"";
-		servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, currentTime);
+		servletRequest.addHeader(HttpHeaders.IF_MODIFIED_SINCE, dateFormat.format(currentTime));
 		servletRequest.addHeader(HttpHeaders.IF_NONE_MATCH, etagValue);
 		HttpHeaders responseHeaders = new HttpHeaders();
 		responseHeaders.setDate(HttpHeaders.LAST_MODIFIED, oneMinuteAgo);