Polishing and minor refactoring

See spring-projectsgh-24635

spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 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.
@@ -98,7 +98,7 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
 
  HttpServletResponse responseToUse = response;
  if (!isAsyncDispatch(request) && !(response instanceof ContentCachingResponseWrapper)) {
- responseToUse = new HttpStreamingAwareContentCachingResponseWrapper(response, request);
+ responseToUse = new ConditionalContentCachingResponseWrapper(response, request);
  }
 
  filterChain.doFilter(request, responseToUse);
@@ -109,38 +109,34 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
  }
 
  private void updateResponse(HttpServletRequest request, HttpServletResponse response) throws IOException {
- ContentCachingResponseWrapper responseWrapper =
+
+ ContentCachingResponseWrapper wrapper =
  WebUtils.getNativeResponse(response, ContentCachingResponseWrapper.class);
- Assert.notNull(responseWrapper, "ContentCachingResponseWrapper not found");
- HttpServletResponse rawResponse = (HttpServletResponse) responseWrapper.getResponse();
- int statusCode = responseWrapper.getStatus();
 
- if (rawResponse.isCommitted()) {
-  responseWrapper.copyBodyToResponse();
- }
- else if (isEligibleForEtag(request, responseWrapper, statusCode, responseWrapper.getContentInputStream())) {
- String responseETag = generateETagHeaderValue(responseWrapper.getContentInputStream(), this.writeWeakETag);
+ Assert.notNull(wrapper, "ContentCachingResponseWrapper not found");
+ HttpServletResponse rawResponse = (HttpServletResponse) wrapper.getResponse();
+
+ if (isEligibleForEtag(request, wrapper, wrapper.getStatus(), wrapper.getContentInputStream())) {
+ String responseETag = generateETagHeaderValue(wrapper.getContentInputStream(), this.writeWeakETag);
  rawResponse.setHeader(HttpHeaders.ETAG, responseETag);
  String requestETag = request.getHeader(HttpHeaders.IF_NONE_MATCH);
  if (requestETag != null && ("*".equals(requestETag) || compareETagHeaderValue(requestETag, responseETag))) {
  rawResponse.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
- }
- else {
- responseWrapper.copyBodyToResponse();
+ return;
  }
  }
- else {
- responseWrapper.copyBodyToResponse();
- }
+
+ wrapper.copyBodyToResponse();
  }
 
  /**
- * Indicates whether the given request and response are eligible for ETag generation.
- * <p>The default implementation returns {@code true} if all conditions match:
+ * Whether an ETag should be calculated for the given request and response
+ * exchange. By default this is {@code true} if all of the following match:
  * <ul>
- * <li>response status codes in the {@code 2xx} series</li>
- * <li>request method is a GET</li>
- * <li>response Cache-Control header is not set or does not contain a "no-store" directive</li>
+ * <li>Response is not committed.</li>
+ * <li>Response status codes is in the {@code 2xx} series.</li>
+ * <li>Request method is a GET.</li>
+ * <li>Response Cache-Control header does not contain "no-store" (or is not present at all).</li>
  * </ul>
  * @param request the HTTP request
  * @param response the HTTP response
@@ -151,11 +147,14 @@ else if (isEligibleForEtag(request, responseWrapper, statusCode, responseWrapper
  protected boolean isEligibleForEtag(HttpServletRequest request, HttpServletResponse response,
  int responseStatusCode, InputStream inputStream) {
 
- String method = request.getMethod();
- if (responseStatusCode >= 200 && responseStatusCode < 300 && HttpMethod.GET.matches(method)) {
+ if (!response.isCommitted() &&
+ responseStatusCode >= 200 && responseStatusCode < 300 &&
+ HttpMethod.GET.matches(request.getMethod())) {
+
  String cacheControl = response.getHeader(HttpHeaders.CACHE_CONTROL);
  return (cacheControl == null || !cacheControl.contains(DIRECTIVE_NO_STORE));
  }
+
  return false;
  }
 
@@ -191,10 +190,12 @@ private boolean compareETagHeaderValue(String requestETag, String responseETag)
 
 
  /**
- * This method can be used to disable the content caching response wrapper
- * of the ShallowEtagHeaderFilter. This can be done before the start of HTTP
- * streaming for example where the response will be written to asynchronously
- * and not in the context of a Servlet container thread.
+ * This method can be used to suppress the content caching response wrapper
+ * of the ShallowEtagHeaderFilter. The main reason for this is streaming
+ * scenarios which are not to be cached and do not need an eTag.
+ * <p><strong>Note:</strong> This method must be called before the response
+ * is written to in order for the entire response content to be written
+ * without caching.
  * @since 4.2
  */
  public static void disableContentCaching(ServletRequest request) {
@@ -207,27 +208,30 @@ private static boolean isContentCachingDisabled(HttpServletRequest request) {
  }
 
 
- private static class HttpStreamingAwareContentCachingResponseWrapper extends ContentCachingResponseWrapper {
+ /**
+ * Returns the raw OutputStream, instead of the one that does caching,
+ * if {@link #isContentCachingDisabled}.
+ */
+ private static class ConditionalContentCachingResponseWrapper extends ContentCachingResponseWrapper {
 
  private final HttpServletRequest request;
 
- public HttpStreamingAwareContentCachingResponseWrapper(HttpServletResponse response, HttpServletRequest request) {
+
+ ConditionalContentCachingResponseWrapper(HttpServletResponse response, HttpServletRequest request) {
  super(response);
  this.request = request;
  }
 
  @Override
  public ServletOutputStream getOutputStream() throws IOException {
- return (useRawResponse() ? getResponse().getOutputStream() : super.getOutputStream());
+ return (isContentCachingDisabled(this.request) ?
+ getResponse().getOutputStream() : super.getOutputStream());
  }
 
  @Override
  public PrintWriter getWriter() throws IOException {
- return (useRawResponse() ? getResponse().getWriter() : super.getWriter());
- }
-
- private boolean useRawResponse() {
- return isContentCachingDisabled(this.request);
+ return (isContentCachingDisabled(this.request) ?
+ getResponse().getWriter() : super.getWriter());
  }
  }
 

spring-web/src/test/java/org/springframework/web/filter/ShallowEtagHeaderFilterTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 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,6 +16,8 @@
 
 package org.springframework.web.filter;
 
+import java.nio.charset.StandardCharsets;
+
 import javax.servlet.FilterChain;
 import javax.servlet.http.HttpServletResponse;
 
@@ -62,7 +64,7 @@ public void filterNoMatch() throws Exception {
  final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels");
  MockHttpServletResponse response = new MockHttpServletResponse();
 
- final byte[] responseBody = "Hello World".getBytes("UTF-8");
+ final byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
  ((HttpServletResponse) filterResponse).setStatus(HttpServletResponse.SC_OK);
@@ -71,7 +73,7 @@ public void filterNoMatch() throws Exception {
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(200);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
  assertThat(response.getContentLength() > 0).as("Invalid Content-Length header").isTrue();
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(responseBody);
  }
@@ -82,7 +84,7 @@ public void filterNoMatchWeakETag() throws Exception {
  final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels");
  MockHttpServletResponse response = new MockHttpServletResponse();
 
- final byte[] responseBody = "Hello World".getBytes("UTF-8");
+ final byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
  ((HttpServletResponse) filterResponse).setStatus(HttpServletResponse.SC_OK);
@@ -91,7 +93,7 @@ public void filterNoMatchWeakETag() throws Exception {
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(200);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isEqualTo("W/\"0b10a8db164e0754105b7a99be72e3fe5\"");
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isEqualTo("W/\"0b10a8db164e0754105b7a99be72e3fe5\"");
  assertThat(response.getContentLength() > 0).as("Invalid Content-Length header").isTrue();
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(responseBody);
  }
@@ -105,14 +107,14 @@ public void filterMatch() throws Exception {
 
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
- byte[] responseBody = "Hello World".getBytes("UTF-8");
+ byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FileCopyUtils.copy(responseBody, filterResponse.getOutputStream());
  filterResponse.setContentLength(responseBody.length);
  };
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(304);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
  assertThat(response.containsHeader("Content-Length")).as("Response has Content-Length header").isFalse();
  byte[] expecteds = new byte[0];
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(expecteds);
@@ -127,14 +129,14 @@ public void filterMatchWeakEtag() throws Exception {
 
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
- byte[] responseBody = "Hello World".getBytes("UTF-8");
+ byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FileCopyUtils.copy(responseBody, filterResponse.getOutputStream());
  filterResponse.setContentLength(responseBody.length);
  };
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(304);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
  assertThat(response.containsHeader("Content-Length")).as("Response has Content-Length header").isFalse();
  byte[] expecteds = new byte[0];
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(expecteds);
@@ -156,7 +158,7 @@ public void filterWriter() throws Exception {
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(304);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
  assertThat(response.containsHeader("Content-Length")).as("Response has Content-Length header").isFalse();
  byte[] expecteds = new byte[0];
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(expecteds);
@@ -167,7 +169,7 @@ public void filterWriterWithDisabledCaching() throws Exception {
  final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels");
  MockHttpServletResponse response = new MockHttpServletResponse();
 
- final byte[] responseBody = "Hello World".getBytes("UTF-8");
+ final byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
  ((HttpServletResponse) filterResponse).setStatus(HttpServletResponse.SC_OK);
@@ -187,7 +189,7 @@ public void filterSendError() throws Exception {
  final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels");
  MockHttpServletResponse response = new MockHttpServletResponse();
 
- final byte[] responseBody = "Hello World".getBytes("UTF-8");
+ final byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
  response.setContentLength(100);
@@ -197,7 +199,7 @@ public void filterSendError() throws Exception {
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(403);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isNull();
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isNull();
  assertThat(response.getContentLength()).as("Invalid Content-Length header").isEqualTo(100);
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(responseBody);
  }
@@ -207,7 +209,7 @@ public void filterSendErrorMessage() throws Exception {
  final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels");
  MockHttpServletResponse response = new MockHttpServletResponse();
 
- final byte[] responseBody = "Hello World".getBytes("UTF-8");
+ final byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
  response.setContentLength(100);
@@ -217,7 +219,7 @@ public void filterSendErrorMessage() throws Exception {
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(403);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isNull();
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isNull();
  assertThat(response.getContentLength()).as("Invalid Content-Length header").isEqualTo(100);
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(responseBody);
  assertThat(response.getErrorMessage()).as("Invalid error message").isEqualTo("ERROR");
@@ -228,7 +230,7 @@ public void filterSendRedirect() throws Exception {
  final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels");
  MockHttpServletResponse response = new MockHttpServletResponse();
 
- final byte[] responseBody = "Hello World".getBytes("UTF-8");
+ final byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
  response.setContentLength(100);
@@ -238,19 +240,18 @@ public void filterSendRedirect() throws Exception {
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(302);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isNull();
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isNull();
  assertThat(response.getContentLength()).as("Invalid Content-Length header").isEqualTo(100);
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(responseBody);
  assertThat(response.getRedirectedUrl()).as("Invalid redirect URL").isEqualTo("https://www.google.com");
  }
 
- // SPR-13717
- @Test
+ @Test // SPR-13717
  public void filterFlushResponse() throws Exception {
  final MockHttpServletRequest request = new MockHttpServletRequest("GET", "/hotels");
  MockHttpServletResponse response = new MockHttpServletResponse();
 
- final byte[] responseBody = "Hello World".getBytes("UTF-8");
+ final byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8);
  FilterChain filterChain = (filterRequest, filterResponse) -> {
  assertThat(filterRequest).as("Invalid request passed").isEqualTo(request);
  ((HttpServletResponse) filterResponse).setStatus(HttpServletResponse.SC_OK);
@@ -260,7 +261,7 @@ public void filterFlushResponse() throws Exception {
  filter.doFilter(request, response, filterChain);
 
  assertThat(response.getStatus()).as("Invalid status").isEqualTo(200);
- assertThat(response.getHeader("ETag")).as("Invalid ETag header").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
+ assertThat(response.getHeader("ETag")).as("Invalid ETag").isEqualTo("\"0b10a8db164e0754105b7a99be72e3fe5\"");
  assertThat(response.getContentLength() > 0).as("Invalid Content-Length header").isTrue();
  assertThat(response.getContentAsByteArray()).as("Invalid content").isEqualTo(responseBody);
  }

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 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.
@@ -21,10 +21,8 @@
 import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -70,8 +68,6 @@
  */
 public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodProcessor {
 
- private static final Set<HttpMethod> SAFE_METHODS = EnumSet.of(HttpMethod.GET, HttpMethod.HEAD);
-
  /**
  * Basic constructor with converters only. Suitable for resolving
  * {@code HttpEntity}. For handling {@code ResponseEntity} consider also
@@ -205,12 +201,11 @@ public void handleReturnValue(@Nullable Object returnValue, MethodParameter retu
  int returnStatus = ((ResponseEntity<?>) responseEntity).getStatusCodeValue();
  outputMessage.getServletResponse().setStatus(returnStatus);
  if (returnStatus == 200) {
- if (SAFE_METHODS.contains(inputMessage.getMethod())
+ HttpMethod method = inputMessage.getMethod();
+ if ((HttpMethod.GET.equals(method) || HttpMethod.HEAD.equals(method))
  && isResourceNotModified(inputMessage, outputMessage)) {
- // Ensure headers are flushed, no body should be written.
  outputMessage.flush();
  ShallowEtagHeaderFilter.disableContentCaching(inputMessage.getServletRequest());
- // Skip call to converters, as they may update the body.
  return;
  }
  }