From 39d689da0c938fa242f1c26cfe507a84a4256711 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Fri, 26 Jun 2015 15:28:59 +0200 Subject: [PATCH] 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 --- .../context/request/ServletWebRequest.java | 36 +++++----- .../ServletWebRequestHttpMethodsTests.java | 4 +- .../annotation/HttpEntityMethodProcessor.java | 65 ++++++++++++------- .../HttpEntityMethodProcessorMockTests.java | 31 ++++++++- 4 files changed, 88 insertions(+), 48 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java b/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java index 4f787a6cdfa..37bc6121d35 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/ServletWebRequest.java +++ b/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)); + } } } } diff --git a/spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java b/spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java index 68b700c91ec..e0e1dcdd284 100644 --- a/spring-web/src/test/java/org/springframework/web/context/request/ServletWebRequestHttpMethodsTests.java +++ b/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")); } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java index e563bcd48c2..22ebdbc3a23 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessor.java +++ b/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 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 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) { diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java index b169426cb3c..30b54e06adf 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorMockTests.java +++ b/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 returnValue = new ResponseEntity("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 returnValue = new ResponseEntity("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);