Skip to content

Commit f0861cd

Browse files
impl: HttpHeaders.backedBy and HttpHeaders.copyOf to reduce ambiguity on the constructor.
chore: Update constructor references with backedBy method. chore: Remove `@Deprecated` annotations. Fixes spring-projects#34340 Signed-off-by: Bryce J. Fisher <bryce.fisher@gmail.com>
1 parent d4030a8 commit f0861cd

14 files changed

+118
-22
lines changed

spring-web/src/main/java/org/springframework/http/HttpHeaders.java

+41
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,47 @@ public HttpHeaders(HttpHeaders httpHeaders) {
491491
}
492492
}
493493

494+
/**
495+
* Construct a new {@code HttpHeaders} instance backed by an existing map.
496+
* <p>This factory method is available as an optimization for adapting to existing
497+
* headers map structures, primarily for internal use within the framework.
498+
* @param headers the headers map (expected to operate with case-insensitive keys)
499+
*/
500+
public static HttpHeaders backedBy(MultiValueMap<String, String> headers) {
501+
return new HttpHeaders(headers);
502+
}
503+
504+
/**
505+
* Construct a new {@code HttpHeaders} instance by removing any read-only
506+
* wrapper that may have been previously applied around the given
507+
* {@code HttpHeaders} via {@link #readOnlyHttpHeaders(HttpHeaders)}.
508+
* <p>Once the writable instance is mutated, the read-only instance is
509+
* likely to be out of sync and should be discarded.
510+
* @param httpHeaders the headers to expose
511+
*/
512+
public static HttpHeaders backedBy(HttpHeaders httpHeaders) {
513+
return new HttpHeaders(httpHeaders);
514+
}
515+
516+
/**
517+
* Constructs a new {@code HttpHeaders} instance with the given headers with a new underlying map.
518+
* Changes made to this new instance will NOT be reflected in the original headers.
519+
* @param headers The headers to copy
520+
*/
521+
public static HttpHeaders copyOf(MultiValueMap<String, String> headers) {
522+
HttpHeaders httpHeadersCopy = new HttpHeaders();
523+
headers.forEach((key, values) -> httpHeadersCopy.put(key, new ArrayList<>(values)));
524+
return httpHeadersCopy;
525+
}
526+
527+
/**
528+
* Constructs a new {@code HttpHeaders} instance with the given headers with a new underlying map.
529+
* Changes made to this new instance will NOT be reflected in the original headers.
530+
* @param httpHeaders The headers to copy
531+
*/
532+
public static HttpHeaders copyOf(HttpHeaders httpHeaders) {
533+
return copyOf(httpHeaders.headers);
534+
}
494535

495536
/**
496537
* Get the list of header values for the given header name, if any.

spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
* <p>This caches the parsed representations of the "Accept" and "Content-Type" headers
3737
* and will get out of sync with the backing map it is mutated at runtime.
3838
*
39+
* @implNote This does NOT make the underlying MultiValueMap readonly.
3940
* @author Brian Clozel
4041
* @author Sam Brannen
4142
* @since 5.1.1
@@ -49,7 +50,6 @@ class ReadOnlyHttpHeaders extends HttpHeaders {
4950
@SuppressWarnings("serial")
5051
private @Nullable List<MediaType> cachedAccept;
5152

52-
5353
ReadOnlyHttpHeaders(MultiValueMap<String, String> headers) {
5454
super(headers);
5555
}

spring-web/src/main/java/org/springframework/http/codec/multipart/PartHttpMessageWriter.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ public Mono<Void> write(Publisher<? extends Part> parts,
9191
}
9292

9393
private <T> Flux<DataBuffer> encodePart(byte[] boundary, Part part, DataBufferFactory bufferFactory) {
94-
HttpHeaders headers = new HttpHeaders(part.headers());
94+
HttpHeaders headers = HttpHeaders.backedBy(part.headers());
9595

9696
String name = part.name();
9797
if (!headers.containsHeader(HttpHeaders.CONTENT_DISPOSITION)) {

spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpRequest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ public JettyCoreServerHttpRequest(Request request, JettyDataBufferFactory dataBu
5656
super(HttpMethod.valueOf(request.getMethod()),
5757
request.getHttpURI().toURI(),
5858
request.getContext().getContextPath(),
59-
new HttpHeaders(new JettyHeadersAdapter(request.getHeaders())));
59+
HttpHeaders.backedBy(new JettyHeadersAdapter(request.getHeaders())));
6060
this.dataBufferFactory = dataBufferFactory;
6161
this.request = request;
6262
}

spring-web/src/main/java/org/springframework/http/server/reactive/JettyCoreServerHttpResponse.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class JettyCoreServerHttpResponse extends AbstractServerHttpResponse implements
5656

5757

5858
public JettyCoreServerHttpResponse(Response response, JettyDataBufferFactory dataBufferFactory) {
59-
super(dataBufferFactory, new HttpHeaders(new JettyHeadersAdapter(response.getHeaders())));
59+
super(dataBufferFactory, HttpHeaders.backedBy(new JettyHeadersAdapter(response.getHeaders())));
6060
this.response = response;
6161

6262
// remove all existing cookies from the response and add them to the cookie map, to be added back later

spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpRequest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public ReactorNetty2ServerHttpRequest(HttpServerRequest request, Netty5DataBuffe
7171
throws URISyntaxException {
7272

7373
super(HttpMethod.valueOf(request.method().name()), initUri(request), "",
74-
new HttpHeaders(new Netty5HeadersAdapter(request.requestHeaders())));
74+
HttpHeaders.backedBy(new Netty5HeadersAdapter(request.requestHeaders())));
7575
Assert.notNull(bufferFactory, "DataBufferFactory must not be null");
7676
this.request = request;
7777
this.bufferFactory = bufferFactory;

spring-web/src/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpResponse.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ class ReactorNetty2ServerHttpResponse extends AbstractServerHttpResponse impleme
5858

5959

6060
public ReactorNetty2ServerHttpResponse(HttpServerResponse response, DataBufferFactory bufferFactory) {
61-
super(bufferFactory, new HttpHeaders(new Netty5HeadersAdapter(response.responseHeaders())));
61+
super(bufferFactory, HttpHeaders.backedBy(new Netty5HeadersAdapter(response.responseHeaders())));
6262
Assert.notNull(response, "HttpServerResponse must not be null");
6363
this.response = response;
6464
}

spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ public ReactorServerHttpRequest(HttpServerRequest request, NettyDataBufferFactor
6868

6969
super(HttpMethod.valueOf(request.method().name()),
7070
ReactorUriHelper.createUri(request), request.forwardedPrefix(),
71-
new HttpHeaders(new Netty4HeadersAdapter(request.requestHeaders())));
71+
HttpHeaders.backedBy(new Netty4HeadersAdapter(request.requestHeaders())));
7272
Assert.notNull(bufferFactory, "DataBufferFactory must not be null");
7373
this.request = request;
7474
this.bufferFactory = bufferFactory;

spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ class ReactorServerHttpResponse extends AbstractServerHttpResponse implements Ze
5757

5858

5959
public ReactorServerHttpResponse(HttpServerResponse response, DataBufferFactory bufferFactory) {
60-
super(bufferFactory, new HttpHeaders(new Netty4HeadersAdapter(Objects.requireNonNull(response,
60+
super(bufferFactory, HttpHeaders.backedBy(new Netty4HeadersAdapter(Objects.requireNonNull(response,
6161
"HttpServerResponse must not be null").responseHeaders())));
6262
this.response = response;
6363
}

spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ private static HttpHeaders initHeaders(HttpHeaders headerValues, HttpServletRequ
168168
String requestContentType = request.getContentType();
169169
if (StringUtils.hasLength(requestContentType)) {
170170
contentType = MediaType.parseMediaType(requestContentType);
171-
headers = new HttpHeaders(headerValues);
171+
headers = HttpHeaders.backedBy(headerValues);
172172
headers.setContentType(contentType);
173173
}
174174
}
@@ -184,7 +184,7 @@ private static HttpHeaders initHeaders(HttpHeaders headerValues, HttpServletRequ
184184
if (headerValues.getFirst(HttpHeaders.CONTENT_TYPE) == null) {
185185
int contentLength = request.getContentLength();
186186
if (contentLength != -1) {
187-
headers = (headers != null ? headers : new HttpHeaders(headerValues));
187+
headers = (headers != null ? headers : HttpHeaders.backedBy(headerValues));
188188
headers.setContentLength(contentLength);
189189
}
190190
}

spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ private static HttpHeaders createTomcatHttpHeaders(HttpServletRequest request) {
9696
ReflectionUtils.getField(COYOTE_REQUEST_FIELD, requestFacade);
9797
Assert.state(connectorRequest != null, "No Tomcat connector request");
9898
Request tomcatRequest = connectorRequest.getCoyoteRequest();
99-
return new HttpHeaders(new TomcatHeadersAdapter(tomcatRequest.getMimeHeaders()));
99+
return HttpHeaders.backedBy(new TomcatHeadersAdapter(tomcatRequest.getMimeHeaders()));
100100
}
101101

102102
private static RequestFacade getRequestFacade(HttpServletRequest request) {
@@ -140,7 +140,7 @@ private static HttpHeaders createTomcatHttpHeaders(HttpServletResponse response)
140140
Assert.state(connectorResponse != null, "No Tomcat connector response");
141141
Response tomcatResponse = connectorResponse.getCoyoteResponse();
142142
TomcatHeadersAdapter headers = new TomcatHeadersAdapter(tomcatResponse.getMimeHeaders());
143-
return new HttpHeaders(headers);
143+
return HttpHeaders.backedBy(headers);
144144
}
145145

146146
private static ResponseFacade getResponseFacade(HttpServletResponse response) {

spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public UndertowServerHttpRequest(HttpServerExchange exchange, DataBufferFactory
6666
throws URISyntaxException {
6767

6868
super(HttpMethod.valueOf(exchange.getRequestMethod().toString()), initUri(exchange), "",
69-
new HttpHeaders(new UndertowHeadersAdapter(exchange.getRequestHeaders())));
69+
HttpHeaders.backedBy(new UndertowHeadersAdapter(exchange.getRequestHeaders())));
7070
this.exchange = exchange;
7171
this.body = new RequestBodyPublisher(exchange, bufferFactory);
7272
this.body.registerListeners(exchange);

spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpResponse.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ class UndertowServerHttpResponse extends AbstractListenerServerHttpResponse impl
7070
private static HttpHeaders createHeaders(HttpServerExchange exchange) {
7171
Assert.notNull(exchange, "HttpServerExchange must not be null");
7272
UndertowHeadersAdapter headersMap = new UndertowHeadersAdapter(exchange.getResponseHeaders());
73-
return new HttpHeaders(headersMap);
73+
return HttpHeaders.backedBy(headersMap);
7474
}
7575

7676

spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java

+63-8
Original file line numberDiff line numberDiff line change
@@ -57,24 +57,79 @@ class HttpHeadersTests {
5757

5858
final HttpHeaders headers = new HttpHeaders();
5959

60+
/**
61+
* Not the different behaviors found in {@link HttpHeaders#backedBy(HttpHeaders)) {@link HttpHeaders#HttpHeaders(HttpHeaders)}
62+
* versus the {@link HttpHeaders#copyOf(HttpHeaders)} methods.
63+
*/
6064
@Test
61-
void constructorUnwrapsReadonly() {
65+
void backedByFactoryUnwrapsReadonly() {
6266
headers.setContentType(MediaType.APPLICATION_JSON);
6367
HttpHeaders readOnly = HttpHeaders.readOnlyHttpHeaders(headers);
68+
6469
assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
65-
HttpHeaders writable = new HttpHeaders(readOnly);
70+
HttpHeaders writable = HttpHeaders.backedBy(readOnly);
6671
writable.setContentType(MediaType.TEXT_PLAIN);
72+
6773
// content-type value is cached by ReadOnlyHttpHeaders
74+
assertThat(readOnly.getContentType())
75+
.describedAs("readOnly HttpHeaders should NOT have updated content-type value")
76+
.isEqualTo(MediaType.APPLICATION_JSON); // Note that the content type was cached by the previous assertion
77+
assertThat(writable.getContentType())
78+
.describedAs("writable HttpHeaders should have updated content-type value")
79+
.isEqualTo(MediaType.TEXT_PLAIN);
80+
assertThat(headers.getContentType())
81+
.describedAs("initial HttpHeaders should have updated content-type value")
82+
.isEqualTo(MediaType.TEXT_PLAIN); // Note that both writable and the original changed because the backing map is shared
83+
}
84+
85+
@Test
86+
void backedByFactoryUnwrapsMultipleHeaders() {
87+
headers.setContentType(MediaType.APPLICATION_JSON);
88+
HttpHeaders readonlyOriginalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(headers);
89+
HttpHeaders firewallHeaders = HttpHeaders.backedBy(readonlyOriginalExchangeHeaders);
90+
HttpHeaders writeable = HttpHeaders.backedBy(firewallHeaders);
91+
writeable.setContentType(MediaType.TEXT_PLAIN);
92+
93+
// If readonly headers are unwrapped multiple times, the content-type value should be updated
94+
assertThat(writeable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); // Note that all changed because the backing map is shared
95+
assertThat(firewallHeaders.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
96+
assertThat(readonlyOriginalExchangeHeaders.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); // Note that the content type was not cached because there was no previous call to getContentType
97+
assertThat(headers.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
98+
}
99+
100+
@Test
101+
void copyOfFactoryUnwrapsReadonly() {
102+
headers.setContentType(MediaType.APPLICATION_JSON);
103+
HttpHeaders readOnly = HttpHeaders.readOnlyHttpHeaders(headers);
104+
68105
assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
69-
assertThat(writable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
106+
HttpHeaders writable = HttpHeaders.copyOf(readOnly);
107+
writable.setContentType(MediaType.TEXT_PLAIN);
108+
109+
// content-type value is cached by ReadOnlyHttpHeaders
110+
assertThat(readOnly.getContentType())
111+
.describedAs("readOnly HttpHeaders should NOT have updated content-type value")
112+
.isEqualTo(MediaType.APPLICATION_JSON); // Note that the content type was cached by the previous assertion
113+
assertThat(writable.getContentType())
114+
.describedAs("writable HttpHeaders should have updated content-type value")
115+
.isEqualTo(MediaType.TEXT_PLAIN); // Note that only the copy is changed, because the backing map is a new instance
116+
assertThat(headers.getContentType())
117+
.describedAs("initial HttpHeaders should have updated content-type value")
118+
.isEqualTo(MediaType.APPLICATION_JSON);
70119
}
71120

72121
@Test
73-
void writableHttpHeadersUnwrapsMultiple() {
74-
HttpHeaders originalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(new HttpHeaders());
75-
HttpHeaders firewallHeaders = new HttpHeaders(originalExchangeHeaders);
76-
HttpHeaders writeable = new HttpHeaders(firewallHeaders);
77-
writeable.setContentType(MediaType.APPLICATION_JSON);
122+
void copyOfFactoryUnwrapsMultipleHeaders() {
123+
headers.setContentType(MediaType.APPLICATION_JSON);
124+
HttpHeaders readonlyOriginalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(headers);
125+
HttpHeaders firewallHeaders = HttpHeaders.copyOf(readonlyOriginalExchangeHeaders);
126+
HttpHeaders writeable = HttpHeaders.copyOf(firewallHeaders);
127+
writeable.setContentType(MediaType.TEXT_PLAIN);
128+
129+
assertThat(writeable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); // Note that only the copy is changed, because the backing map is a new instance
130+
assertThat(firewallHeaders.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
131+
assertThat(readonlyOriginalExchangeHeaders.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
132+
assertThat(headers.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
78133
}
79134

80135
@Test

0 commit comments

Comments
 (0)