From d3da7a50ec524739b4267e45cc137955e143ff59 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 4 Mar 2020 18:59:18 +0000 Subject: [PATCH 1/3] Polishing and minor refactoring See gh-24635 --- .../web/filter/ShallowEtagHeaderFilter.java | 78 ++++++++++--------- .../filter/ShallowEtagHeaderFilterTests.java | 43 +++++----- .../annotation/HttpEntityMethodProcessor.java | 11 +-- .../ServletInvocableHandlerMethodTests.java | 23 ++++-- 4 files changed, 81 insertions(+), 74 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java b/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java index 8de782ac98..3007318e94 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java +++ b/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 @@ public class ShallowEtagHeaderFilter extends OncePerRequestFilter { 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 @@ public class ShallowEtagHeaderFilter extends OncePerRequestFilter { } private void updateResponse(HttpServletRequest request, HttpServletResponse response) throws IOException { - ContentCachingResponseWrapper responseWrapper = - 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); + ContentCachingResponseWrapper wrapper = + WebUtils.getNativeResponse(response, ContentCachingResponseWrapper.class); + + 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. - *

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: *

* @param request the HTTP request * @param response the HTTP response @@ -151,11 +147,14 @@ public class ShallowEtagHeaderFilter extends OncePerRequestFilter { 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 @@ public class ShallowEtagHeaderFilter extends OncePerRequestFilter { /** - * 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. + *

Note: 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 @@ public class ShallowEtagHeaderFilter extends OncePerRequestFilter { } - 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()); } } diff --git a/spring-web/src/test/java/org/springframework/web/filter/ShallowEtagHeaderFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/ShallowEtagHeaderFilterTests.java index 804a3c5b0f..1dccc8e02a 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/ShallowEtagHeaderFilterTests.java +++ b/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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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 class ShallowEtagHeaderFilterTests { 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); } 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 38d437db22..8624c2e7ba 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 @@ -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.ParameterizedType; 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 @@ import org.springframework.web.servlet.support.RequestContextUtils; */ public class HttpEntityMethodProcessor extends AbstractMessageConverterMethodProcessor { - private static final Set 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 class HttpEntityMethodProcessor extends AbstractMessageConverterMethodPro 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; } } 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 584289bbad..88e3deb28b 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 @@ -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. @@ -90,8 +90,10 @@ public class ServletInvocableHandlerMethodTests { ServletInvocableHandlerMethod handlerMethod = getHandlerMethod(new Handler(), "responseStatus"); handlerMethod.invokeAndHandle(this.webRequest, this.mavContainer); - assertThat(this.mavContainer.isRequestHandled()).as("Null return value + @ResponseStatus should result in 'request handled'").isTrue(); assertThat(this.response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + assertThat(this.mavContainer.isRequestHandled()) + .as("Null return value + @ResponseStatus should result in 'request handled'") + .isTrue(); } @Test @@ -99,8 +101,10 @@ public class ServletInvocableHandlerMethodTests { ServletInvocableHandlerMethod handlerMethod = getHandlerMethod(new Handler(), "composedResponseStatus"); handlerMethod.invokeAndHandle(this.webRequest, this.mavContainer); - assertThat(this.mavContainer.isRequestHandled()).as("Null return value + @ComposedResponseStatus should result in 'request handled'").isTrue(); assertThat(this.response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + assertThat(this.mavContainer.isRequestHandled()) + .as("Null return value + @ComposedResponseStatus should result in 'request handled'") + .isTrue(); } @Test @@ -120,7 +124,9 @@ public class ServletInvocableHandlerMethodTests { getHandlerMethod(new Handler(), "httpServletResponse", HttpServletResponse.class); handlerMethod.invokeAndHandle(this.webRequest, this.mavContainer); - assertThat(this.mavContainer.isRequestHandled()).as("Null return value + HttpServletResponse arg should result in 'request handled'").isTrue(); + assertThat(this.mavContainer.isRequestHandled()) + .as("Null return value + HttpServletResponse arg should result in 'request handled'") + .isTrue(); } @Test @@ -159,9 +165,10 @@ public class ServletInvocableHandlerMethodTests { ServletInvocableHandlerMethod handlerMethod = getHandlerMethod(new Handler(), "responseStatusWithReason"); handlerMethod.invokeAndHandle(this.webRequest, this.mavContainer); - assertThat(this.mavContainer.isRequestHandled()).as("When a status reason w/ used, the request is handled").isTrue(); assertThat(this.response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); assertThat(this.response.getErrorMessage()).isEqualTo("400 Bad Request"); + assertThat(this.mavContainer.isRequestHandled()) + .as("When a status reason w/ used, the request is handled").isTrue(); } @Test @@ -180,15 +187,15 @@ public class ServletInvocableHandlerMethodTests { this.returnValueHandlers.addHandler(new ViewNameMethodReturnValueHandler()); // Invoke without a request parameter (String return value) - ServletInvocableHandlerMethod handlerMethod = getHandlerMethod(new Handler(), "dynamicReturnValue", String.class); - handlerMethod.invokeAndHandle(this.webRequest, this.mavContainer); + ServletInvocableHandlerMethod hm = getHandlerMethod(new Handler(), "dynamicReturnValue", String.class); + hm.invokeAndHandle(this.webRequest, this.mavContainer); assertThat(this.mavContainer.getView()).isNotNull(); assertThat(this.mavContainer.getView().getClass()).isEqualTo(RedirectView.class); // Invoke with a request parameter (RedirectView return value) this.request.setParameter("param", "value"); - handlerMethod.invokeAndHandle(this.webRequest, this.mavContainer); + hm.invokeAndHandle(this.webRequest, this.mavContainer); assertThat(this.mavContainer.getViewName()).isEqualTo("view"); } From c7e037da39d2c0d57e0b895504c7a296fe77e43a Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 4 Mar 2020 17:20:13 +0000 Subject: [PATCH 2/3] 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 8624c2e7ba..6b9f0af707 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 9d574bef38..12ab262310 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 dde74a7ceb..574536c781 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 88e3deb28b..c20cedc9a2 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); - assertThat(this.request.getAttribute(ShallowEtagHeaderFilter.class.getName() + ".STREAMING")) - .isEqualTo(true); + 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(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()); From a98bf30ee6044ef05cd2d4aec3790b7c259eaa70 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 4 Mar 2020 18:55:14 +0000 Subject: [PATCH 3/3] ShallowEtagHeaderFilter checks for pre-existing eTag The filter now checks for an explicitly set eTag and uses it instead of generating one, and also suppresses caching. Closes gh-24635 --- .../web/filter/ShallowEtagHeaderFilter.java | 21 ++++++--- .../HttpEntityMethodProcessorTests.java | 44 ++++++++++++++++++- 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java b/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java index 3007318e94..fb7abfeb8e 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/ShallowEtagHeaderFilter.java @@ -31,6 +31,8 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; import org.springframework.util.Assert; import org.springframework.util.DigestUtils; +import org.springframework.util.StringUtils; +import org.springframework.web.context.request.ServletWebRequest; import org.springframework.web.util.ContentCachingResponseWrapper; import org.springframework.web.util.WebUtils; @@ -117,11 +119,12 @@ public class ShallowEtagHeaderFilter extends OncePerRequestFilter { 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); + String eTag = wrapper.getHeader(HttpHeaders.ETAG); + if (!StringUtils.hasText(eTag)) { + eTag = generateETagHeaderValue(wrapper.getContentInputStream(), this.writeWeakETag); + rawResponse.setHeader(HttpHeaders.ETAG, eTag); + } + if (new ServletWebRequest(request, rawResponse).checkNotModified(eTag)) { return; } } @@ -224,15 +227,19 @@ public class ShallowEtagHeaderFilter extends OncePerRequestFilter { @Override public ServletOutputStream getOutputStream() throws IOException { - return (isContentCachingDisabled(this.request) ? + return (isContentCachingDisabled(this.request) || hasETag() ? getResponse().getOutputStream() : super.getOutputStream()); } @Override public PrintWriter getWriter() throws IOException { - return (isContentCachingDisabled(this.request) ? + return (isContentCachingDisabled(this.request) || hasETag()? getResponse().getWriter() : super.getWriter()); } + + private boolean hasETag() { + return StringUtils.hasText(getHeader(HttpHeaders.ETAG)); + } } } diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java index 10b3188654..1b809cf281 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/HttpEntityMethodProcessorTests.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. @@ -19,8 +19,13 @@ package org.springframework.web.servlet.mvc.method.annotation; import java.io.Serializable; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import javax.servlet.FilterChain; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + import com.fasterxml.jackson.annotation.JsonTypeInfo; import com.fasterxml.jackson.annotation.JsonTypeName; import org.junit.jupiter.api.BeforeEach; @@ -28,6 +33,7 @@ import org.junit.jupiter.api.Test; import org.springframework.core.MethodParameter; import org.springframework.http.HttpEntity; +import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; import org.springframework.http.converter.ByteArrayHttpMessageConverter; @@ -42,6 +48,7 @@ import org.springframework.web.bind.annotation.ResponseBody; 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.HandlerMethod; import org.springframework.web.method.support.ModelAndViewContainer; import org.springframework.web.testfixture.servlet.MockHttpServletRequest; @@ -200,6 +207,41 @@ public class HttpEntityMethodProcessorTests { assertThat(servletResponse.getContentAsString()).isEqualTo("Foo"); } + @Test // SPR-13423 + public void handleReturnValueWithETagAndETagFilter() throws Exception { + + String eTagValue = "\"deadb33f8badf00d\""; + String content = "body"; + + Method method = getClass().getDeclaredMethod("handle"); + MethodParameter returnType = new MethodParameter(method, -1); + + FilterChain chain = (req, res) -> { + ResponseEntity returnValue = ResponseEntity.ok().eTag(eTagValue).body(content); + try { + ServletWebRequest requestToUse = + new ServletWebRequest((HttpServletRequest) req, (HttpServletResponse) res); + + new HttpEntityMethodProcessor(Collections.singletonList(new StringHttpMessageConverter())) + .handleReturnValue(returnValue, returnType, mavContainer, requestToUse); + + assertThat(this.servletResponse.getContentAsString()) + .as("Response body was cached? It should be written directly to the raw response") + .isEqualTo(content); + } + catch (Exception ex) { + throw new IllegalStateException(ex); + } + }; + + this.servletRequest.setMethod("GET"); + new ShallowEtagHeaderFilter().doFilter(this.servletRequest, this.servletResponse, chain); + + assertThat(this.servletResponse.getStatus()).isEqualTo(200); + assertThat(this.servletResponse.getHeader(HttpHeaders.ETAG)).isEqualTo(eTagValue); + assertThat(this.servletResponse.getContentAsString()).isEqualTo(content); + } + @SuppressWarnings("unused") private void handle(HttpEntity> arg1, HttpEntity arg2) {