From 942dba2ca94420054897ddb8eeadf23f5b9b609b Mon Sep 17 00:00:00 2001 From: "Bryce J. Fisher" Date: Wed, 29 Jan 2025 17:07:56 -0700 Subject: [PATCH] impl: HttpHeaders.backedBy and HttpHeaders.copyOf to reduce ambiguity on the constructor. Fixes #34340 chore: Update constructor references with backedBy method. Signed-off-by: Bryce J. Fisher --- .../org/springframework/http/HttpHeaders.java | 47 ++++++++++++++ .../http/ReadOnlyHttpHeaders.java | 3 +- .../multipart/PartHttpMessageWriter.java | 2 +- .../reactive/JettyCoreServerHttpRequest.java | 2 +- .../reactive/JettyCoreServerHttpResponse.java | 2 +- .../ReactorNetty2ServerHttpRequest.java | 2 +- .../ReactorNetty2ServerHttpResponse.java | 2 +- .../reactive/ReactorServerHttpRequest.java | 2 +- .../reactive/ReactorServerHttpResponse.java | 2 +- .../reactive/ServletServerHttpRequest.java | 4 +- .../reactive/TomcatHttpHandlerAdapter.java | 4 +- .../reactive/UndertowServerHttpRequest.java | 2 +- .../reactive/UndertowServerHttpResponse.java | 2 +- .../http/HttpHeadersTests.java | 64 ++++++++++++++++--- 14 files changed, 118 insertions(+), 22 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java index f0432d50502a..def39963ea08 100644 --- a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java @@ -452,7 +452,9 @@ public HttpHeaders() { * headers map structures, primarily for internal use within the framework. * @param headers the headers map (expected to operate with case-insensitive keys) * @since 5.1 + * @deprecated Will be made default visibility in favor of {@link #backedBy(MultiValueMap)} in a future release. */ + @Deprecated public HttpHeaders(MultiValueMap headers) { Assert.notNull(headers, "MultiValueMap must not be null"); if (headers == EMPTY) { @@ -477,7 +479,9 @@ else if (headers instanceof HttpHeaders httpHeaders) { * likely to be out of sync and should be discarded. * @param httpHeaders the headers to expose * @since 7.0 + * @deprecated Will be made private in favor of {@link #backedBy(HttpHeaders)} in a future release. */ + @Deprecated public HttpHeaders(HttpHeaders httpHeaders) { Assert.notNull(httpHeaders, "HttpHeaders must not be null"); if (httpHeaders == EMPTY) { @@ -491,6 +495,49 @@ public HttpHeaders(HttpHeaders httpHeaders) { } } + /** + * Construct a new {@code HttpHeaders} instance backed by an existing map. + *

This constructor is available as an optimization for adapting to existing + * headers map structures, primarily for internal use within the framework. + * @param headers the headers map (expected to operate with case-insensitive keys) + */ + @SuppressWarnings("deprecation") // @Deprecated wll be removed when visibility is changed. + public static HttpHeaders backedBy(MultiValueMap headers) { + return new HttpHeaders(headers); + } + + /** + * Construct a new {@code HttpHeaders} instance by removing any read-only + * wrapper that may have been previously applied around the given + * {@code HttpHeaders} via {@link #readOnlyHttpHeaders(HttpHeaders)}. + *

Once the writable instance is mutated, the read-only instance is + * likely to be out of sync and should be discarded. + * @param httpHeaders the headers to expose + */ + @SuppressWarnings("deprecation") // @Deprecated wll be removed when visibility is changed. + public static HttpHeaders backedBy(HttpHeaders httpHeaders) { + return new HttpHeaders(httpHeaders); + } + + /** + * Constructs a new {@code HttpHeaders} instance with the given headers. + * Changes made to this new instance will NOT be reflected in the original headers. + * @param headers The headers to copy + */ + public static HttpHeaders copyOf(MultiValueMap headers) { + HttpHeaders httpHeadersCopy = new HttpHeaders(); + headers.forEach((key, values) -> httpHeadersCopy.put(key, new ArrayList<>(values))); + return httpHeadersCopy; + } + + /** + * Constructs a new {@code HttpHeaders} instance with the given headers. + * Changes made to this new instance will NOT be reflected in the original headers. + * @param httpHeaders The headers to copy + */ + public static HttpHeaders copyOf(HttpHeaders httpHeaders) { + return copyOf(httpHeaders.headers); + } /** * Get the list of header values for the given header name, if any. diff --git a/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java b/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java index eb5fafdc41d2..27f5a6c43853 100644 --- a/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java @@ -36,6 +36,7 @@ *

This caches the parsed representations of the "Accept" and "Content-Type" headers * and will get out of sync with the backing map it is mutated at runtime. * + * @implNote This does NOT make the underlying MultiValueMap readonly. * @author Brian Clozel * @author Sam Brannen * @since 5.1.1 @@ -49,7 +50,7 @@ class ReadOnlyHttpHeaders extends HttpHeaders { @SuppressWarnings("serial") private @Nullable List cachedAccept; - + @SuppressWarnings("deprecation") // @Deprecated wll be removed when visibility is changed. ReadOnlyHttpHeaders(MultiValueMap headers) { super(headers); } diff --git a/spring-web/src/main/java/org/springframework/http/codec/multipart/PartHttpMessageWriter.java b/spring-web/src/main/java/org/springframework/http/codec/multipart/PartHttpMessageWriter.java index fe33e3b2114c..103838ca4cab 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/multipart/PartHttpMessageWriter.java +++ b/spring-web/src/main/java/org/springframework/http/codec/multipart/PartHttpMessageWriter.java @@ -91,7 +91,7 @@ public Mono write(Publisher parts, } private Flux encodePart(byte[] boundary, Part part, DataBufferFactory bufferFactory) { - HttpHeaders headers = new HttpHeaders(part.headers()); + HttpHeaders headers = HttpHeaders.backedBy(part.headers()); String name = part.name(); if (!headers.containsHeader(HttpHeaders.CONTENT_DISPOSITION)) { diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java index a43730b8b445..8ddfe704e3af 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java @@ -56,7 +56,7 @@ public JettyCoreServerHttpRequest(Request request, JettyDataBufferFactory dataBu super(HttpMethod.valueOf(request.getMethod()), request.getHttpURI().toURI(), request.getContext().getContextPath(), - new HttpHeaders(new JettyHeadersAdapter(request.getHeaders()))); + HttpHeaders.backedBy(new JettyHeadersAdapter(request.getHeaders()))); this.dataBufferFactory = dataBufferFactory; this.request = request; } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpResponse.java index dc319c186201..33974018300a 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpResponse.java @@ -56,7 +56,7 @@ class JettyCoreServerHttpResponse extends AbstractServerHttpResponse implements public JettyCoreServerHttpResponse(Response response, JettyDataBufferFactory dataBufferFactory) { - super(dataBufferFactory, new HttpHeaders(new JettyHeadersAdapter(response.getHeaders()))); + super(dataBufferFactory, HttpHeaders.backedBy(new JettyHeadersAdapter(response.getHeaders()))); this.response = response; // remove all existing cookies from the response and add them to the cookie map, to be added back later diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpRequest.java index 833e67aaa0e4..5e4d4af696cb 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpRequest.java @@ -71,7 +71,7 @@ public ReactorNetty2ServerHttpRequest(HttpServerRequest request, Netty5DataBuffe throws URISyntaxException { super(HttpMethod.valueOf(request.method().name()), initUri(request), "", - new HttpHeaders(new Netty5HeadersAdapter(request.requestHeaders()))); + HttpHeaders.backedBy(new Netty5HeadersAdapter(request.requestHeaders()))); Assert.notNull(bufferFactory, "DataBufferFactory must not be null"); this.request = request; this.bufferFactory = bufferFactory; diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpResponse.java index 3d89eaad7a44..45789db8e8b8 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpResponse.java @@ -58,7 +58,7 @@ class ReactorNetty2ServerHttpResponse extends AbstractServerHttpResponse impleme public ReactorNetty2ServerHttpResponse(HttpServerResponse response, DataBufferFactory bufferFactory) { - super(bufferFactory, new HttpHeaders(new Netty5HeadersAdapter(response.responseHeaders()))); + super(bufferFactory, HttpHeaders.backedBy(new Netty5HeadersAdapter(response.responseHeaders()))); Assert.notNull(response, "HttpServerResponse must not be null"); this.response = response; } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java index 53715fb2489d..2a42c6f4cefe 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java @@ -68,7 +68,7 @@ public ReactorServerHttpRequest(HttpServerRequest request, NettyDataBufferFactor super(HttpMethod.valueOf(request.method().name()), ReactorUriHelper.createUri(request), request.forwardedPrefix(), - new HttpHeaders(new Netty4HeadersAdapter(request.requestHeaders()))); + HttpHeaders.backedBy(new Netty4HeadersAdapter(request.requestHeaders()))); Assert.notNull(bufferFactory, "DataBufferFactory must not be null"); this.request = request; this.bufferFactory = bufferFactory; diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java index d0f8cea29ef9..f7aed1d21148 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java @@ -57,7 +57,7 @@ class ReactorServerHttpResponse extends AbstractServerHttpResponse implements Ze public ReactorServerHttpResponse(HttpServerResponse response, DataBufferFactory bufferFactory) { - super(bufferFactory, new HttpHeaders(new Netty4HeadersAdapter(Objects.requireNonNull(response, + super(bufferFactory, HttpHeaders.backedBy(new Netty4HeadersAdapter(Objects.requireNonNull(response, "HttpServerResponse must not be null").responseHeaders()))); this.response = response; } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java index b4d59361895f..011430fd8ab3 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java @@ -168,7 +168,7 @@ private static HttpHeaders initHeaders(HttpHeaders headerValues, HttpServletRequ String requestContentType = request.getContentType(); if (StringUtils.hasLength(requestContentType)) { contentType = MediaType.parseMediaType(requestContentType); - headers = new HttpHeaders(headerValues); + headers = HttpHeaders.backedBy(headerValues); headers.setContentType(contentType); } } @@ -184,7 +184,7 @@ private static HttpHeaders initHeaders(HttpHeaders headerValues, HttpServletRequ if (headerValues.getFirst(HttpHeaders.CONTENT_TYPE) == null) { int contentLength = request.getContentLength(); if (contentLength != -1) { - headers = (headers != null ? headers : new HttpHeaders(headerValues)); + headers = (headers != null ? headers : HttpHeaders.backedBy(headerValues)); headers.setContentLength(contentLength); } } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java index 6fd3af0148d5..c5c018e11667 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java @@ -96,7 +96,7 @@ private static HttpHeaders createTomcatHttpHeaders(HttpServletRequest request) { ReflectionUtils.getField(COYOTE_REQUEST_FIELD, requestFacade); Assert.state(connectorRequest != null, "No Tomcat connector request"); Request tomcatRequest = connectorRequest.getCoyoteRequest(); - return new HttpHeaders(new TomcatHeadersAdapter(tomcatRequest.getMimeHeaders())); + return HttpHeaders.backedBy(new TomcatHeadersAdapter(tomcatRequest.getMimeHeaders())); } private static RequestFacade getRequestFacade(HttpServletRequest request) { @@ -140,7 +140,7 @@ private static HttpHeaders createTomcatHttpHeaders(HttpServletResponse response) Assert.state(connectorResponse != null, "No Tomcat connector response"); Response tomcatResponse = connectorResponse.getCoyoteResponse(); TomcatHeadersAdapter headers = new TomcatHeadersAdapter(tomcatResponse.getMimeHeaders()); - return new HttpHeaders(headers); + return HttpHeaders.backedBy(headers); } private static ResponseFacade getResponseFacade(HttpServletResponse response) { diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java index e1c6494fe670..b3f96a533d86 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java @@ -66,7 +66,7 @@ public UndertowServerHttpRequest(HttpServerExchange exchange, DataBufferFactory throws URISyntaxException { super(HttpMethod.valueOf(exchange.getRequestMethod().toString()), initUri(exchange), "", - new HttpHeaders(new UndertowHeadersAdapter(exchange.getRequestHeaders()))); + HttpHeaders.backedBy(new UndertowHeadersAdapter(exchange.getRequestHeaders()))); this.exchange = exchange; this.body = new RequestBodyPublisher(exchange, bufferFactory); this.body.registerListeners(exchange); diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java index 1a3f0f9ca448..1db025f339ee 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java @@ -70,7 +70,7 @@ class UndertowServerHttpResponse extends AbstractListenerServerHttpResponse impl private static HttpHeaders createHeaders(HttpServerExchange exchange) { Assert.notNull(exchange, "HttpServerExchange must not be null"); UndertowHeadersAdapter headersMap = new UndertowHeadersAdapter(exchange.getResponseHeaders()); - return new HttpHeaders(headersMap); + return HttpHeaders.backedBy(headersMap); } diff --git a/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java b/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java index 9045bcdfefbc..64ff07e948ce 100644 --- a/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java +++ b/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java @@ -58,23 +58,71 @@ class HttpHeadersTests { final HttpHeaders headers = new HttpHeaders(); @Test - void constructorUnwrapsReadonly() { + void backedByFactoryUnwrapsReadonly() { headers.setContentType(MediaType.APPLICATION_JSON); HttpHeaders readOnly = HttpHeaders.readOnlyHttpHeaders(headers); assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON); - HttpHeaders writable = new HttpHeaders(readOnly); + HttpHeaders writable = HttpHeaders.backedBy(readOnly); writable.setContentType(MediaType.TEXT_PLAIN); + // content-type value is cached by ReadOnlyHttpHeaders + assertThat(readOnly.getContentType()) + .describedAs("readOnly HttpHeaders should NOT have updated content-type value") + .isEqualTo(MediaType.APPLICATION_JSON); + assertThat(writable.getContentType()) + .describedAs("writable HttpHeaders should have updated content-type value") + .isEqualTo(MediaType.TEXT_PLAIN); + assertThat(headers.getContentType()) + .describedAs("initial HttpHeaders should have updated content-type value") + .isEqualTo(MediaType.TEXT_PLAIN); + } + + @Test + void backedByFactoryUnwrapsMultipleHeaders() { + headers.setContentType(MediaType.APPLICATION_JSON); + HttpHeaders readonlyOriginalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(headers); + HttpHeaders firewallHeaders = HttpHeaders.backedBy(readonlyOriginalExchangeHeaders); + HttpHeaders writeable = HttpHeaders.backedBy(firewallHeaders); + writeable.setContentType(MediaType.TEXT_PLAIN); + + // If readonly headers are unwrapped multiple times, the content-type value should be updated + assertThat(writeable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); + assertThat(firewallHeaders.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); + assertThat(readonlyOriginalExchangeHeaders.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); + assertThat(headers.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); + } + + @Test + void copyOfFactoryUnwrapsReadonly() { + headers.setContentType(MediaType.APPLICATION_JSON); + HttpHeaders readOnly = HttpHeaders.readOnlyHttpHeaders(headers); assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON); - assertThat(writable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); + HttpHeaders writable = HttpHeaders.copyOf(readOnly); + writable.setContentType(MediaType.TEXT_PLAIN); + + assertThat(readOnly.getContentType()) + .describedAs("readOnly HttpHeaders should NOT have updated content-type value") + .isEqualTo(MediaType.APPLICATION_JSON); + assertThat(writable.getContentType()) + .describedAs("writable HttpHeaders should have updated content-type value") + .isEqualTo(MediaType.TEXT_PLAIN); + assertThat(headers.getContentType()) + .describedAs("initial HttpHeaders should have updated content-type value") + .isEqualTo(MediaType.APPLICATION_JSON); } @Test - void writableHttpHeadersUnwrapsMultiple() { - HttpHeaders originalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(new HttpHeaders()); - HttpHeaders firewallHeaders = new HttpHeaders(originalExchangeHeaders); - HttpHeaders writeable = new HttpHeaders(firewallHeaders); - writeable.setContentType(MediaType.APPLICATION_JSON); + void copyOfFactoryUnwrapsMultipleHeaders() { + headers.setContentType(MediaType.APPLICATION_JSON); + HttpHeaders readonlyOriginalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(headers); + HttpHeaders firewallHeaders = HttpHeaders.copyOf(readonlyOriginalExchangeHeaders); + HttpHeaders writeable = HttpHeaders.copyOf(firewallHeaders); + writeable.setContentType(MediaType.TEXT_PLAIN); + + assertThat(writeable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); + assertThat(firewallHeaders.getContentType()).isEqualTo(MediaType.APPLICATION_JSON); + assertThat(readonlyOriginalExchangeHeaders.getContentType()).isEqualTo(MediaType.APPLICATION_JSON); + assertThat(headers.getContentType()).isEqualTo(MediaType.APPLICATION_JSON); } @Test