From c7e037da39d2c0d57e0b895504c7a296fe77e43a Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 4 Mar 2020 17:20:13 +0000 Subject: [PATCH] Remove unnecessary calls to disableContentCaching These calls were added in error when trying to fix #22797 and #23775. They are not needed in 304 scenarios. Those have no response content and are skipped by ShallowETagHeaderFilter based on the status. This leaves disableContentCaching invoked only in streaming scenarios, which was the original intent and should be the only reason for that method. See gh-24635 --- .../annotation/HttpEntityMethodProcessor.java | 2 - .../ServletInvocableHandlerMethod.java | 2 - .../HttpEntityMethodProcessorMockTests.java | 25 +++++++++ .../ServletInvocableHandlerMethodTests.java | 56 +++++++++++++++---- 4 files changed, 70 insertions(+), 15 deletions(-) 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 8624c2e7ba8..6b9f0af7072 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 @@ -47,7 +47,6 @@ 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.filter.ShallowEtagHeaderFilter; import org.springframework.web.method.support.ModelAndViewContainer; import org.springframework.web.servlet.mvc.support.RedirectAttributes; import org.springframework.web.servlet.support.RequestContextUtils; @@ -205,7 +204,6 @@ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro if ((HttpMethod.GET.equals(method) || HttpMethod.HEAD.equals(method)) && isResourceNotModified(inputMessage, outputMessage)) { outputMessage.flush(); - ShallowEtagHeaderFilter.disableContentCaching(inputMessage.getServletRequest()); return; } } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletInvocableHandlerMethod.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletInvocableHandlerMethod.java index 9d574bef384..12ab2623104 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletInvocableHandlerMethod.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ServletInvocableHandlerMethod.java @@ -36,7 +36,6 @@ import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.context.request.ServletWebRequest; -import org.springframework.web.filter.ShallowEtagHeaderFilter; import org.springframework.web.method.HandlerMethod; import org.springframework.web.method.support.HandlerMethodReturnValueHandler; import org.springframework.web.method.support.HandlerMethodReturnValueHandlerComposite; @@ -172,7 +171,6 @@ public class ServletInvocableHandlerMethod extends InvocableHandlerMethod { if (StringUtils.hasText(response.getHeader(HttpHeaders.ETAG))) { HttpServletRequest request = webRequest.getNativeRequest(HttpServletRequest.class); Assert.notNull(request, "Expected HttpServletRequest"); - ShallowEtagHeaderFilter.disableContentCaching(request); } } } 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 dde74a7ceb3..574536c781f 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 @@ -30,6 +30,8 @@ import java.util.Collections; import java.util.Date; import java.util.Set; +import javax.servlet.FilterChain; + import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; @@ -53,6 +55,7 @@ import org.springframework.web.HttpMediaTypeNotAcceptableException; import org.springframework.web.HttpMediaTypeNotSupportedException; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.context.request.ServletWebRequest; +import org.springframework.web.filter.ShallowEtagHeaderFilter; import org.springframework.web.method.support.ModelAndViewContainer; import org.springframework.web.testfixture.servlet.MockHttpServletRequest; import org.springframework.web.testfixture.servlet.MockHttpServletResponse; @@ -429,6 +432,28 @@ public class HttpEntityMethodProcessorMockTests { assertConditionalResponse(HttpStatus.NOT_MODIFIED, null, etagValue, -1); } + @Test + public void handleEtagWithHttp304AndEtagFilterHasNoImpact() throws Exception { + + String eTagValue = "\"deadb33f8badf00d\""; + + FilterChain chain = (req, res) -> { + servletRequest.addHeader(HttpHeaders.IF_NONE_MATCH, eTagValue); + ResponseEntity returnValue = ResponseEntity.ok().eTag(eTagValue).body("body"); + initStringMessageConversion(TEXT_PLAIN); + try { + processor.handleReturnValue(returnValue, returnTypeResponseEntity, mavContainer, webRequest); + } + catch (Exception ex) { + throw new IllegalStateException(ex); + } + }; + + new ShallowEtagHeaderFilter().doFilter(this.servletRequest, this.servletResponse, chain); + + assertConditionalResponse(HttpStatus.NOT_MODIFIED, null, eTagValue, -1); + } + @Test // SPR-14559 public void shouldHandleInvalidIfNoneMatchWithHttp200() throws Exception { String etagValue = "\"deadb33f8badf00d\""; diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletInvocableHandlerMethodTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletInvocableHandlerMethodTests.java index 88e3deb28bf..c20cedc9a2a 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletInvocableHandlerMethodTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletInvocableHandlerMethodTests.java @@ -23,6 +23,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import javax.servlet.FilterChain; import javax.servlet.http.HttpServletResponse; import org.junit.jupiter.api.Test; @@ -143,21 +144,29 @@ public class ServletInvocableHandlerMethodTests { .isTrue(); } - @Test // gh-23775 + @Test public void invokeAndHandle_VoidNotModifiedWithEtag() throws Exception { - String etag = "\"deadb33f8badf00d\""; - this.request.addHeader(HttpHeaders.IF_NONE_MATCH, etag); - this.webRequest.checkNotModified(etag); - ServletInvocableHandlerMethod handlerMethod = getHandlerMethod(new Handler(), "notModified"); - handlerMethod.invokeAndHandle(this.webRequest, this.mavContainer); + String eTagValue = "\"deadb33f8badf00d\""; - assertThat(this.mavContainer.isRequestHandled()) - .as("Null return value + 'not modified' request should result in 'request handled'") - .isTrue(); + FilterChain chain = (req, res) -> { + request.addHeader(HttpHeaders.IF_NONE_MATCH, eTagValue); + webRequest.checkNotModified(eTagValue); + + try { + ServletInvocableHandlerMethod handlerMethod = getHandlerMethod(new Handler(), "notModified"); + handlerMethod.invokeAndHandle(webRequest, mavContainer); + } + catch (Exception ex) { + throw new IllegalStateException(ex); + } + }; + + new ShallowEtagHeaderFilter().doFilter(this.request, this.response, chain); - assertThat(this.request.getAttribute(ShallowEtagHeaderFilter.class.getName() + ".STREAMING")) - .isEqualTo(true); + assertThat(response.getStatus()).isEqualTo(304); + assertThat(response.getHeader(HttpHeaders.ETAG)).isEqualTo(eTagValue); + assertThat(response.getContentAsString()).isEmpty(); } @Test // SPR-9159 @@ -171,6 +180,31 @@ public class ServletInvocableHandlerMethodTests { .as("When a status reason w/ used, the request is handled").isTrue(); } + @Test // gh-23775, gh-24635 + public void invokeAndHandle_ETagFilterHasNoImpactWhenETagPresent() throws Exception { + + String eTagValue = "\"deadb33f8badf00d\""; + + FilterChain chain = (req, res) -> { + request.addHeader(HttpHeaders.IF_NONE_MATCH, eTagValue); + webRequest.checkNotModified(eTagValue); + + try { + ServletInvocableHandlerMethod handlerMethod = getHandlerMethod(new Handler(), "notModified"); + handlerMethod.invokeAndHandle(webRequest, mavContainer); + } + catch (Exception ex) { + throw new IllegalStateException(ex); + } + }; + + new ShallowEtagHeaderFilter().doFilter(this.request, this.response, chain); + + assertThat(this.response.getStatus()).isEqualTo(304); + assertThat(this.response.getHeader(HttpHeaders.ETAG)).isEqualTo(eTagValue); + assertThat(this.response.getContentAsString()).isEmpty(); + } + @Test public void invokeAndHandle_Exception() throws Exception { this.returnValueHandlers.addHandler(new ExceptionRaisingReturnValueHandler());