From cc5300c4d558e3f86d592ef615104ab9092a34f4 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Tue, 6 Sep 2016 14:32:14 +0200 Subject: [PATCH] Align MVC checkNotModified with reactive support Since SPR-14522, the web reactive framework supports checkNotModified features. This commit aligns the existing MVC infrastructure with web reactive's behavior. Because of the new Servlet 3.0 baseline, some constraints aren't relevant anymore and duplicate code has been removed in `HttpEntityMethodProcessor`. Issue: SPR-14659 --- .../context/request/ServletWebRequest.java | 280 +++++++++--------- .../ServletWebRequestHttpMethodsTests.java | 10 +- .../annotation/HttpEntityMethodProcessor.java | 79 ++--- .../HttpEntityMethodProcessorMockTests.java | 7 +- 4 files changed, 162 insertions(+), 214 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 19b1256effe..ebc46b34913 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 @@ -17,12 +17,18 @@ package org.springframework.web.context.request; import java.security.Principal; -import java.util.Date; +import java.text.ParseException; +import java.text.SimpleDateFormat; +import java.util.Arrays; +import java.util.Enumeration; import java.util.Iterator; +import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.TimeZone; import java.util.regex.Matcher; import java.util.regex.Pattern; + import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; @@ -44,25 +50,17 @@ import org.springframework.web.util.WebUtils; */ public class ServletWebRequest extends ServletRequestAttributes implements NativeWebRequest { - private static final String HEADER_ETAG = "ETag"; - - private static final String HEADER_IF_MODIFIED_SINCE = "If-Modified-Since"; - - private static final String HEADER_IF_UNMODIFIED_SINCE = "If-Unmodified-Since"; - - private static final String HEADER_IF_NONE_MATCH = "If-None-Match"; - - private static final String HEADER_LAST_MODIFIED = "Last-Modified"; + private static final String ETAG = "ETag"; - private static final String METHOD_GET = "GET"; + private static final String IF_MODIFIED_SINCE = "If-Modified-Since"; - private static final String METHOD_HEAD = "HEAD"; + private static final String IF_UNMODIFIED_SINCE = "If-Unmodified-Since"; - private static final String METHOD_POST = "POST"; + private static final String IF_NONE_MATCH = "If-None-Match"; - private static final String METHOD_PUT = "PUT"; + private static final String LAST_MODIFIED = "Last-Modified"; - private static final String METHOD_DELETE = "DELETE"; + private static final List SAFE_METHODS = Arrays.asList("GET", "HEAD"); /** * Pattern matching ETag multiple field values in headers such as "If-Match", "If-None-Match" @@ -70,6 +68,17 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ */ private static final Pattern ETAG_HEADER_VALUE_PATTERN = Pattern.compile("\\*|\\s*((W\\/)?(\"[^\"]*\"))\\s*,?"); + /** + * Date formats as specified in the HTTP RFC + * @see Section 7.1.1.1 of RFC 7231 + */ + private static final String[] DATE_FORMATS = new String[] { + "EEE, dd MMM yyyy HH:mm:ss zzz", + "EEE, dd-MMM-yy HH:mm:ss zzz", + "EEE MMM dd HH:mm:ss yyyy" + }; + + private static TimeZone GMT = TimeZone.getTimeZone("GMT"); private boolean notModified = false; @@ -189,141 +198,128 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ @Override public boolean checkNotModified(long lastModifiedTimestamp) { - HttpServletResponse response = getResponse(); - if (lastModifiedTimestamp >= 0 && !this.notModified) { - if (isCompatibleWithConditionalRequests(response)) { - this.notModified = isTimestampNotModified(lastModifiedTimestamp); - if (response != null) { - if (supportsNotModifiedStatus()) { - if (this.notModified) { - response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); - } - if (isHeaderAbsent(response, HEADER_LAST_MODIFIED)) { - response.setDateHeader(HEADER_LAST_MODIFIED, lastModifiedTimestamp); - } - } - else if (supportsConditionalUpdate()) { - if (this.notModified) { - response.setStatus(HttpServletResponse.SC_PRECONDITION_FAILED); - } - } - } - } - } - return this.notModified; + return checkNotModified(null, lastModifiedTimestamp); } @Override public boolean checkNotModified(String etag) { - HttpServletResponse response = getResponse(); - if (StringUtils.hasLength(etag) && !this.notModified) { - if (isCompatibleWithConditionalRequests(response)) { - etag = addEtagPadding(etag); - if (hasRequestHeader(HEADER_IF_NONE_MATCH)) { - this.notModified = isEtagNotModified(etag); - } - if (response != null) { - if (this.notModified && supportsNotModifiedStatus()) { - response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); - } - if (isHeaderAbsent(response, HEADER_ETAG)) { - response.setHeader(HEADER_ETAG, etag); - } - } - } - } - return this.notModified; + return checkNotModified(etag, -1); } @Override public boolean checkNotModified(String etag, long lastModifiedTimestamp) { HttpServletResponse response = getResponse(); - if (StringUtils.hasLength(etag) && !this.notModified) { - if (isCompatibleWithConditionalRequests(response)) { - etag = addEtagPadding(etag); - if (hasRequestHeader(HEADER_IF_NONE_MATCH)) { - this.notModified = isEtagNotModified(etag); - } - else if (hasRequestHeader(HEADER_IF_MODIFIED_SINCE)) { - this.notModified = isTimestampNotModified(lastModifiedTimestamp); - } - if (response != null) { - if (supportsNotModifiedStatus()) { - if (this.notModified) { - response.setStatus(HttpServletResponse.SC_NOT_MODIFIED); - } - if (isHeaderAbsent(response, HEADER_ETAG)) { - response.setHeader(HEADER_ETAG, etag); - } - if (isHeaderAbsent(response, HEADER_LAST_MODIFIED)) { - response.setDateHeader(HEADER_LAST_MODIFIED, lastModifiedTimestamp); - } - } - else if (supportsConditionalUpdate()) { - if (this.notModified) { - response.setStatus(HttpServletResponse.SC_PRECONDITION_FAILED); - } - } - } - } + if (this.notModified || HttpStatus.OK.value() != response.getStatus()) { + return this.notModified; } - return this.notModified; - } - public boolean isNotModified() { - return this.notModified; - } + // Evaluate conditions in order of precedence. + // See https://tools.ietf.org/html/rfc7232#section-6 - - private boolean isCompatibleWithConditionalRequests(HttpServletResponse response) { - try { - if (response == null) { - // Can't check response.getStatus() - let's assume we're good - return true; + if (validateIfUnmodifiedSince(lastModifiedTimestamp)) { + if (this.notModified) { + response.setStatus(HttpStatus.PRECONDITION_FAILED.value()); } - return HttpStatus.valueOf(response.getStatus()).is2xxSuccessful(); + return this.notModified; } - catch (IllegalArgumentException ex) { - return true; + + boolean validated = validateIfNoneMatch(etag); + + if (!validated) { + validateIfModifiedSince(lastModifiedTimestamp); } - } - private boolean isHeaderAbsent(HttpServletResponse response, String header) { - if (response == null) { - // Can't check response.getHeader(header) - let's assume it's not set - return true; + // Update response + + boolean isHttpGetOrHead = SAFE_METHODS.contains(getRequest().getMethod()); + if (this.notModified) { + response.setStatus(isHttpGetOrHead ? + HttpStatus.NOT_MODIFIED.value() : HttpStatus.PRECONDITION_FAILED.value()); } - return (response.getHeader(header) == null); + if (isHttpGetOrHead) { + if(lastModifiedTimestamp > 0 && parseDateValue(response.getHeader(LAST_MODIFIED)) == -1) { + response.setDateHeader(LAST_MODIFIED, lastModifiedTimestamp); + } + if (StringUtils.hasLength(etag) && response.getHeader(ETAG) == null) { + response.setHeader(ETAG, padEtagIfNecessary(etag)); + } + } + + return this.notModified; } - private boolean hasRequestHeader(String headerName) { - return StringUtils.hasLength(getHeader(headerName)); + private boolean validateIfUnmodifiedSince(long lastModifiedTimestamp) { + if (lastModifiedTimestamp < 0) { + return false; + } + long ifUnmodifiedSince = parseDateHeader(IF_UNMODIFIED_SINCE); + if (ifUnmodifiedSince == -1) { + return false; + } + // We will perform this validation... + this.notModified = (ifUnmodifiedSince < (lastModifiedTimestamp / 1000 * 1000)); + return true; } - private boolean supportsNotModifiedStatus() { - String method = getRequest().getMethod(); - return (METHOD_GET.equals(method) || METHOD_HEAD.equals(method)); + private boolean validateIfNoneMatch(String etag) { + if (!StringUtils.hasLength(etag)) { + return false; + } + Enumeration ifNoneMatch; + try { + ifNoneMatch = getRequest().getHeaders(IF_NONE_MATCH); + } + catch (IllegalArgumentException ex) { + return false; + } + if (!ifNoneMatch.hasMoreElements()) { + return false; + } + // We will perform this validation... + etag = padEtagIfNecessary(etag); + while (ifNoneMatch.hasMoreElements()) { + String clientETags = ifNoneMatch.nextElement(); + + Matcher eTagMatcher = ETAG_HEADER_VALUE_PATTERN.matcher(clientETags); + // Compare weak/strong ETags as per https://tools.ietf.org/html/rfc7232#section-2.3 + while (eTagMatcher.find()) { + if (StringUtils.hasLength(eTagMatcher.group()) + && etag.replaceFirst("^W/", "").equals(eTagMatcher.group(3))) { + this.notModified = true; + break; + } + } + } + return true; } - private boolean supportsConditionalUpdate() { - String method = getRequest().getMethod(); - return (METHOD_POST.equals(method) || METHOD_PUT.equals(method) || METHOD_DELETE.equals(method)) - && hasRequestHeader(HEADER_IF_UNMODIFIED_SINCE); + private String padEtagIfNecessary(String etag) { + if (!StringUtils.hasLength(etag)) { + return etag; + } + if ((etag.startsWith("\"") || etag.startsWith("W/\"")) && etag.endsWith("\"")) { + return etag; + } + return "\"" + etag + "\""; } - private boolean isTimestampNotModified(long lastModifiedTimestamp) { - long ifModifiedSince = parseDateHeader(HEADER_IF_MODIFIED_SINCE); - if (ifModifiedSince != -1) { - return (ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000)); + private boolean validateIfModifiedSince(long lastModifiedTimestamp) { + if (lastModifiedTimestamp < 0) { + return false; } - long ifUnmodifiedSince = parseDateHeader(HEADER_IF_UNMODIFIED_SINCE); - if (ifUnmodifiedSince != -1) { - return (ifUnmodifiedSince < (lastModifiedTimestamp / 1000 * 1000)); + long ifModifiedSince = parseDateHeader(IF_MODIFIED_SINCE); + if (ifModifiedSince == -1) { + return false; } - return false; + // We will perform this validation... + this.notModified = ifModifiedSince >= (lastModifiedTimestamp / 1000 * 1000); + return true; + } + + public boolean isNotModified() { + return this.notModified; } - @SuppressWarnings("deprecation") private long parseDateHeader(String headerName) { long dateValue = -1; try { @@ -335,36 +331,32 @@ public class ServletWebRequest extends ServletRequestAttributes implements Nativ int separatorIndex = headerValue.indexOf(';'); if (separatorIndex != -1) { String datePart = headerValue.substring(0, separatorIndex); - try { - dateValue = Date.parse(datePart); - } - catch (IllegalArgumentException ex2) { - // Giving up - } + dateValue = parseDateValue(datePart); } } return dateValue; } - private boolean isEtagNotModified(String etag) { - String ifNoneMatch = getHeader(HEADER_IF_NONE_MATCH); - // compare weak/strong ETag as per https://tools.ietf.org/html/rfc7232#section-2.3 - String serverETag = etag.replaceFirst("^W/", ""); - Matcher eTagMatcher = ETAG_HEADER_VALUE_PATTERN.matcher(ifNoneMatch); - while (eTagMatcher.find()) { - if ("*".equals(eTagMatcher.group()) - || serverETag.equals(eTagMatcher.group(3))) { - return true; - } + private long parseDateValue(String headerValue) { + if (headerValue == null) { + // No header value sent at all + return -1; } - return false; - } - - private String addEtagPadding(String etag) { - if (!(etag.startsWith("\"") || etag.startsWith("W/\"")) || !etag.endsWith("\"")) { - etag = "\"" + etag + "\""; + if (headerValue.length() >= 3) { + // Short "0" or "-1" like values are never valid HTTP date headers... + // Let's only bother with SimpleDateFormat parsing for long enough values. + for (String dateFormat : DATE_FORMATS) { + SimpleDateFormat simpleDateFormat = new SimpleDateFormat(dateFormat, Locale.US); + simpleDateFormat.setTimeZone(GMT); + try { + return simpleDateFormat.parse(headerValue).getTime(); + } + catch (ParseException ex) { + // ignore + } + } } - return etag; + return -1; } @Override 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 e5ff3c2c858..de9e3eed17a 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 @@ -94,9 +94,7 @@ public class ServletWebRequestHttpMethodsTests { servletRequest.addHeader("If-Modified-Since", epochTime); servletResponse.setStatus(0); - assertTrue(request.checkNotModified(epochTime)); - assertEquals(304, servletResponse.getStatus()); - assertEquals(dateFormat.format(epochTime), servletResponse.getHeader("Last-Modified")); + assertFalse(request.checkNotModified(epochTime)); } @Test // SPR-14559 @@ -202,13 +200,13 @@ public class ServletWebRequestHttpMethodsTests { } @Test - public void checkNotModifiedWildcardETag() { + public void checkNotModifiedWildcardIsIgnored() { String eTag = "\"Foo\""; servletRequest.addHeader("If-None-Match", "*"); - assertTrue(request.checkNotModified(eTag)); + assertFalse(request.checkNotModified(eTag)); - assertEquals(304, servletResponse.getStatus()); + assertEquals(200, servletResponse.getStatus()); assertEquals(eTag, servletResponse.getHeader("ETag")); } 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 ea4f7de7910..3cc72280664 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 @@ -28,8 +28,6 @@ import org.springframework.core.MethodParameter; 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; @@ -41,6 +39,7 @@ import org.springframework.web.HttpMediaTypeNotSupportedException; import org.springframework.web.accept.ContentNegotiationManager; import org.springframework.web.bind.support.WebDataBinderFactory; import org.springframework.web.context.request.NativeWebRequest; +import org.springframework.web.context.request.ServletWebRequest; import org.springframework.web.method.support.ModelAndViewContainer; /** @@ -182,15 +181,15 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro } if (responseEntity instanceof ResponseEntity) { - outputMessage.getServletResponse().setStatus(((ResponseEntity) responseEntity).getStatusCodeValue()); - HttpMethod method = inputMessage.getMethod(); - boolean isGetOrHead = (HttpMethod.GET == method || HttpMethod.HEAD == method); - if (isGetOrHead && 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; + int returnStatus = ((ResponseEntity) responseEntity).getStatusCodeValue(); + outputMessage.getServletResponse().setStatus(returnStatus); + if(returnStatus == 200) { + if (isResourceNotModified(inputMessage, outputMessage)) { + // Ensure headers are flushed, no body should be written. + outputMessage.flush(); + // Skip call to converters, as they may update the body. + return; + } } } @@ -223,55 +222,15 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro } private boolean isResourceNotModified(ServletServerHttpRequest inputMessage, ServletServerHttpResponse outputMessage) { - boolean notModified = false; - try { - long ifModifiedSince = inputMessage.getHeaders().getIfModifiedSince(); - String eTag = addEtagPadding(outputMessage.getHeaders().getETag()); - long lastModified = outputMessage.getHeaders().getLastModified(); - List ifNoneMatch = inputMessage.getHeaders().getIfNoneMatch(); - if (!ifNoneMatch.isEmpty() && (inputMessage.getHeaders().containsKey(HttpHeaders.IF_UNMODIFIED_SINCE) - || inputMessage.getHeaders().containsKey(HttpHeaders.IF_MATCH))) { - // invalid conditional request, do not process - } - else if (lastModified != -1 && StringUtils.hasLength(eTag)) { - notModified = isETagNotModified(ifNoneMatch, eTag) && isTimeStampNotModified(ifModifiedSince, lastModified); - } - else if (lastModified != -1) { - notModified = isTimeStampNotModified(ifModifiedSince, lastModified); - } - else if (StringUtils.hasLength(eTag)) { - notModified = isETagNotModified(ifNoneMatch, eTag); - } - } - catch (IllegalArgumentException exc) { - // invalid conditional request, do not process - } - 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/", "")))) { - 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; + ServletWebRequest servletWebRequest = + new ServletWebRequest(inputMessage.getServletRequest(), outputMessage.getServletResponse()); + HttpHeaders responseHeaders = outputMessage.getHeaders(); + String etag = responseHeaders.getETag(); + long lastModifiedTimestamp = responseHeaders.getLastModified(); + responseHeaders.remove(HttpHeaders.ETAG); + responseHeaders.remove(HttpHeaders.LAST_MODIFIED); + + return servletWebRequest.checkNotModified(etag, lastModifiedTimestamp); } @Override 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 9b3f1e4071c..021dffaf602 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 @@ -476,8 +476,7 @@ public class HttpEntityMethodProcessorMockTests { processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest); assertResponseOkWithBody("body"); - assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.ETAG).size()); - assertEquals(etagValue, servletResponse.getHeader(HttpHeaders.ETAG)); + assertEquals(0, servletResponse.getHeaderValues(HttpHeaders.ETAG).size()); } // SPR-13626 @@ -511,7 +510,7 @@ public class HttpEntityMethodProcessorMockTests { initStringMessageConversion(MediaType.TEXT_PLAIN); processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest); - assertResponseOkWithBody("body"); + assertResponseNotModified(); assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.ETAG).size()); assertEquals(etagValue, servletResponse.getHeader(HttpHeaders.ETAG)); } @@ -529,7 +528,7 @@ public class HttpEntityMethodProcessorMockTests { initStringMessageConversion(MediaType.TEXT_PLAIN); processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest); - assertResponseOkWithBody("body"); + assertResponseNotModified(); assertEquals(1, servletResponse.getHeaderValues(HttpHeaders.ETAG).size()); assertEquals(etagValue, servletResponse.getHeader(HttpHeaders.ETAG)); }