From 87873818920a0081103c9e30bc7b6b3a248b0a16 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sat, 24 Feb 2024 13:34:48 +0100 Subject: [PATCH 1/2] Polish ContentCachingResponseWrapper[Tests] --- .../util/ContentCachingResponseWrapper.java | 19 ++++++++++-------- .../ContentCachingResponseWrapperTests.java | 20 +++++++++---------- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/util/ContentCachingResponseWrapper.java b/spring-web/src/main/java/org/springframework/web/util/ContentCachingResponseWrapper.java index 4223a67397..017c0f6356 100644 --- a/spring-web/src/main/java/org/springframework/web/util/ContentCachingResponseWrapper.java +++ b/spring-web/src/main/java/org/springframework/web/util/ContentCachingResponseWrapper.java @@ -38,7 +38,7 @@ import org.springframework.util.FastByteArrayOutputStream; /** * {@link jakarta.servlet.http.HttpServletResponse} wrapper that caches all content written to * the {@linkplain #getOutputStream() output stream} and {@linkplain #getWriter() writer}, - * and allows this content to be retrieved via a {@link #getContentAsByteArray() byte array}. + * and allows this content to be retrieved via a {@linkplain #getContentAsByteArray() byte array}. * *

Used e.g. by {@link org.springframework.web.filter.ShallowEtagHeaderFilter}. * @@ -120,9 +120,16 @@ public class ContentCachingResponseWrapper extends HttpServletResponseWrapper { return this.writer; } + /** + * This method neither flushes content to the client nor commits the underlying + * response, since the content has not yet been copied to the response. + *

Invoke {@link #copyBodyToResponse()} to copy the cached body content to + * the wrapped response object and flush its buffer. + * @see jakarta.servlet.ServletResponseWrapper#flushBuffer() + */ @Override public void flushBuffer() throws IOException { - // do not flush the underlying response as the content has not been copied to it yet + // no-op } @Override @@ -139,15 +146,11 @@ public class ContentCachingResponseWrapper extends HttpServletResponseWrapper { throw new IllegalArgumentException("Content-Length exceeds ContentCachingResponseWrapper's maximum (" + Integer.MAX_VALUE + "): " + len); } - int lenInt = (int) len; - if (lenInt > this.content.size()) { - this.content.resize(lenInt); - } - this.contentLength = lenInt; + setContentLength((int) len); } @Override - public void setContentType(String type) { + public void setContentType(@Nullable String type) { this.contentType = type; } diff --git a/spring-web/src/test/java/org/springframework/web/filter/ContentCachingResponseWrapperTests.java b/spring-web/src/test/java/org/springframework/web/filter/ContentCachingResponseWrapperTests.java index 691386f998..23912e9d77 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/ContentCachingResponseWrapperTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/ContentCachingResponseWrapperTests.java @@ -16,17 +16,17 @@ package org.springframework.web.filter; -import java.nio.charset.StandardCharsets; - import jakarta.servlet.http.HttpServletResponse; import org.junit.jupiter.api.Test; -import org.springframework.http.HttpHeaders; import org.springframework.util.FileCopyUtils; import org.springframework.web.testfixture.servlet.MockHttpServletResponse; import org.springframework.web.util.ContentCachingResponseWrapper; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; +import static org.springframework.http.HttpHeaders.CONTENT_LENGTH; +import static org.springframework.http.HttpHeaders.TRANSFER_ENCODING; /** * Tests for {@link ContentCachingResponseWrapper}. @@ -37,7 +37,7 @@ class ContentCachingResponseWrapperTests { @Test void copyBodyToResponse() throws Exception { - byte[] responseBody = "Hello World".getBytes(StandardCharsets.UTF_8); + byte[] responseBody = "Hello World".getBytes(UTF_8); MockHttpServletResponse response = new MockHttpServletResponse(); ContentCachingResponseWrapper responseWrapper = new ContentCachingResponseWrapper(response); @@ -45,25 +45,25 @@ class ContentCachingResponseWrapperTests { FileCopyUtils.copy(responseBody, responseWrapper.getOutputStream()); responseWrapper.copyBodyToResponse(); - assertThat(response.getStatus()).isEqualTo(200); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_OK); assertThat(response.getContentLength()).isGreaterThan(0); assertThat(response.getContentAsByteArray()).isEqualTo(responseBody); } @Test void copyBodyToResponseWithTransferEncoding() throws Exception { - byte[] responseBody = "6\r\nHello 5\r\nWorld0\r\n\r\n".getBytes(StandardCharsets.UTF_8); + byte[] responseBody = "6\r\nHello 5\r\nWorld0\r\n\r\n".getBytes(UTF_8); MockHttpServletResponse response = new MockHttpServletResponse(); ContentCachingResponseWrapper responseWrapper = new ContentCachingResponseWrapper(response); responseWrapper.setStatus(HttpServletResponse.SC_OK); - responseWrapper.setHeader(HttpHeaders.TRANSFER_ENCODING, "chunked"); + responseWrapper.setHeader(TRANSFER_ENCODING, "chunked"); FileCopyUtils.copy(responseBody, responseWrapper.getOutputStream()); responseWrapper.copyBodyToResponse(); - assertThat(response.getStatus()).isEqualTo(200); - assertThat(response.getHeader(HttpHeaders.TRANSFER_ENCODING)).isEqualTo("chunked"); - assertThat(response.getHeader(HttpHeaders.CONTENT_LENGTH)).isNull(); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + assertThat(response.getHeader(TRANSFER_ENCODING)).isEqualTo("chunked"); + assertThat(response.getHeader(CONTENT_LENGTH)).isNull(); assertThat(response.getContentAsByteArray()).isEqualTo(responseBody); } From 5680d2063744380583bdc58fca0a17ac382beaa4 Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sat, 24 Feb 2024 13:41:14 +0100 Subject: [PATCH 2/2] Honor Content-[Type|Length] headers from wrapped response again Commit 375e0e6827 introduced a regression in ContentCachingResponseWrapper (CCRW). Specifically, CCRW no longer honors Content-Type and Content-Length headers that have been set on the wrapped response and returns null for those header values if they have not been set directly on the CCRW. This commit fixes this regression as follows. - The Content-Type and Content-Length headers set in the wrapped response are honored in getContentType(), containsHeader(), getHeader(), and getHeaders() unless those headers have been set directly on the CCRW. - In copyBodyToResponse(), the Content-Type in the wrapped response is only overridden if the Content-Type has been set directly on the CCRW. See gh-32039 Closes gh-32317 --- .../util/ContentCachingResponseWrapper.java | 31 ++++---- .../ContentCachingResponseWrapperTests.java | 76 +++++++++++++++++++ 2 files changed, 93 insertions(+), 14 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/util/ContentCachingResponseWrapper.java b/spring-web/src/main/java/org/springframework/web/util/ContentCachingResponseWrapper.java index 017c0f6356..1487809d94 100644 --- a/spring-web/src/main/java/org/springframework/web/util/ContentCachingResponseWrapper.java +++ b/spring-web/src/main/java/org/springframework/web/util/ContentCachingResponseWrapper.java @@ -43,6 +43,7 @@ import org.springframework.util.FastByteArrayOutputStream; *

Used e.g. by {@link org.springframework.web.filter.ShallowEtagHeaderFilter}. * * @author Juergen Hoeller + * @author Sam Brannen * @since 4.1.3 * @see ContentCachingRequestWrapper */ @@ -157,16 +158,19 @@ public class ContentCachingResponseWrapper extends HttpServletResponseWrapper { @Override @Nullable public String getContentType() { - return this.contentType; + if (this.contentType != null) { + return this.contentType; + } + return super.getContentType(); } @Override public boolean containsHeader(String name) { - if (HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(name)) { - return this.contentLength != null; + if (this.contentLength != null && HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(name)) { + return true; } - else if (HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(name)) { - return this.contentType != null; + else if (this.contentType != null && HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(name)) { + return true; } else { return super.containsHeader(name); @@ -222,10 +226,10 @@ public class ContentCachingResponseWrapper extends HttpServletResponseWrapper { @Override @Nullable public String getHeader(String name) { - if (HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(name)) { - return (this.contentLength != null) ? this.contentLength.toString() : null; + if (this.contentLength != null && HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(name)) { + return this.contentLength.toString(); } - else if (HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(name)) { + else if (this.contentType != null && HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(name)) { return this.contentType; } else { @@ -235,12 +239,11 @@ public class ContentCachingResponseWrapper extends HttpServletResponseWrapper { @Override public Collection getHeaders(String name) { - if (HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(name)) { - return this.contentLength != null ? Collections.singleton(this.contentLength.toString()) : - Collections.emptySet(); + if (this.contentLength != null && HttpHeaders.CONTENT_LENGTH.equalsIgnoreCase(name)) { + return Collections.singleton(this.contentLength.toString()); } - else if (HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(name)) { - return this.contentType != null ? Collections.singleton(this.contentType) : Collections.emptySet(); + else if (this.contentType != null && HttpHeaders.CONTENT_TYPE.equalsIgnoreCase(name)) { + return Collections.singleton(this.contentType); } else { return super.getHeaders(name); @@ -330,7 +333,7 @@ public class ContentCachingResponseWrapper extends HttpServletResponseWrapper { } this.contentLength = null; } - if (complete || this.contentType != null) { + if (this.contentType != null) { rawResponse.setContentType(this.contentType); this.contentType = null; } diff --git a/spring-web/src/test/java/org/springframework/web/filter/ContentCachingResponseWrapperTests.java b/spring-web/src/test/java/org/springframework/web/filter/ContentCachingResponseWrapperTests.java index 23912e9d77..e63f206f95 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/ContentCachingResponseWrapperTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/ContentCachingResponseWrapperTests.java @@ -19,6 +19,7 @@ package org.springframework.web.filter; import jakarta.servlet.http.HttpServletResponse; import org.junit.jupiter.api.Test; +import org.springframework.http.MediaType; import org.springframework.util.FileCopyUtils; import org.springframework.web.testfixture.servlet.MockHttpServletResponse; import org.springframework.web.util.ContentCachingResponseWrapper; @@ -26,12 +27,14 @@ import org.springframework.web.util.ContentCachingResponseWrapper; import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; import static org.springframework.http.HttpHeaders.CONTENT_LENGTH; +import static org.springframework.http.HttpHeaders.CONTENT_TYPE; import static org.springframework.http.HttpHeaders.TRANSFER_ENCODING; /** * Tests for {@link ContentCachingResponseWrapper}. * * @author Rossen Stoyanchev + * @author Sam Brannen */ class ContentCachingResponseWrapperTests { @@ -50,6 +53,79 @@ class ContentCachingResponseWrapperTests { assertThat(response.getContentAsByteArray()).isEqualTo(responseBody); } + @Test + void copyBodyToResponseWithPresetHeaders() throws Exception { + String PUZZLE = "puzzle"; + String ENIGMA = "enigma"; + String NUMBER = "number"; + String MAGIC = "42"; + + byte[] responseBody = "Hello World".getBytes(UTF_8); + String responseLength = Integer.toString(responseBody.length); + String contentType = MediaType.APPLICATION_JSON_VALUE; + + MockHttpServletResponse response = new MockHttpServletResponse(); + response.setContentType(contentType); + response.setContentLength(999); + response.setHeader(PUZZLE, ENIGMA); + response.setIntHeader(NUMBER, 42); + + ContentCachingResponseWrapper responseWrapper = new ContentCachingResponseWrapper(response); + responseWrapper.setStatus(HttpServletResponse.SC_OK); + + assertThat(responseWrapper.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + assertThat(responseWrapper.getContentSize()).isZero(); + assertThat(responseWrapper.getHeaderNames()) + .containsExactlyInAnyOrder(PUZZLE, NUMBER, CONTENT_TYPE, CONTENT_LENGTH); + + assertThat(responseWrapper.containsHeader(PUZZLE)).as(PUZZLE).isTrue(); + assertThat(responseWrapper.getHeader(PUZZLE)).as(PUZZLE).isEqualTo(ENIGMA); + assertThat(responseWrapper.getHeaders(PUZZLE)).as(PUZZLE).containsExactly(ENIGMA); + + assertThat(responseWrapper.containsHeader(NUMBER)).as(NUMBER).isTrue(); + assertThat(responseWrapper.getHeader(NUMBER)).as(NUMBER).isEqualTo(MAGIC); + assertThat(responseWrapper.getHeaders(NUMBER)).as(NUMBER).containsExactly(MAGIC); + + assertThat(responseWrapper.containsHeader(CONTENT_TYPE)).as(CONTENT_TYPE).isTrue(); + assertThat(responseWrapper.getHeader(CONTENT_TYPE)).as(CONTENT_TYPE).isEqualTo(contentType); + assertThat(responseWrapper.getHeaders(CONTENT_TYPE)).as(CONTENT_TYPE).containsExactly(contentType); + assertThat(responseWrapper.getContentType()).as(CONTENT_TYPE).isEqualTo(contentType); + + assertThat(responseWrapper.containsHeader(CONTENT_LENGTH)).as(CONTENT_LENGTH).isTrue(); + assertThat(responseWrapper.getHeader(CONTENT_LENGTH)).as(CONTENT_LENGTH).isEqualTo("999"); + assertThat(responseWrapper.getHeaders(CONTENT_LENGTH)).as(CONTENT_LENGTH).containsExactly("999"); + + FileCopyUtils.copy(responseBody, responseWrapper.getOutputStream()); + responseWrapper.copyBodyToResponse(); + + assertThat(responseWrapper.getHeaderNames()) + .containsExactlyInAnyOrder(PUZZLE, NUMBER, CONTENT_TYPE, CONTENT_LENGTH); + + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + assertThat(response.getContentType()).isEqualTo(contentType); + assertThat(response.getContentLength()).isEqualTo(responseBody.length); + assertThat(response.getContentAsByteArray()).isEqualTo(responseBody); + assertThat(response.getHeaderNames()) + .containsExactlyInAnyOrder(PUZZLE, NUMBER, CONTENT_TYPE, CONTENT_LENGTH); + + assertThat(response.containsHeader(PUZZLE)).as(PUZZLE).isTrue(); + assertThat(response.getHeader(PUZZLE)).as(PUZZLE).isEqualTo(ENIGMA); + assertThat(response.getHeaders(PUZZLE)).as(PUZZLE).containsExactly(ENIGMA); + + assertThat(response.containsHeader(NUMBER)).as(NUMBER).isTrue(); + assertThat(response.getHeader(NUMBER)).as(NUMBER).isEqualTo(MAGIC); + assertThat(response.getHeaders(NUMBER)).as(NUMBER).containsExactly(MAGIC); + + assertThat(response.containsHeader(CONTENT_TYPE)).as(CONTENT_TYPE).isTrue(); + assertThat(response.getHeader(CONTENT_TYPE)).as(CONTENT_TYPE).isEqualTo(contentType); + assertThat(response.getHeaders(CONTENT_TYPE)).as(CONTENT_TYPE).containsExactly(contentType); + assertThat(response.getContentType()).as(CONTENT_TYPE).isEqualTo(contentType); + + assertThat(response.containsHeader(CONTENT_LENGTH)).as(CONTENT_LENGTH).isTrue(); + assertThat(response.getHeader(CONTENT_LENGTH)).as(CONTENT_LENGTH).isEqualTo(responseLength); + assertThat(response.getHeaders(CONTENT_LENGTH)).as(CONTENT_LENGTH).containsExactly(responseLength); + } + @Test void copyBodyToResponseWithTransferEncoding() throws Exception { byte[] responseBody = "6\r\nHello 5\r\nWorld0\r\n\r\n".getBytes(UTF_8);