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 80a2d84b5e69..e39b88458138 100644 --- a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java @@ -441,7 +441,15 @@ public HttpHeaders() { */ public HttpHeaders(MultiValueMap headers) { Assert.notNull(headers, "MultiValueMap must not be null"); - this.headers = headers; + if (headers == EMPTY) { + this.headers = CollectionUtils.toMultiValueMap(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH)); + } + else if (headers instanceof ReadOnlyHttpHeaders readOnlyHttpHeaders) { + this.headers = readOnlyHttpHeaders.headers; + } + else { + this.headers = headers; + } } @@ -1869,7 +1877,7 @@ public static HttpHeaders readOnlyHttpHeaders(MultiValueMap head * Apply a read-only {@code HttpHeaders} wrapper around the given headers, if necessary. *

Also caches the parsed representations of the "Accept" and "Content-Type" headers. * @param headers the headers to expose - * @return a read-only variant of the headers, or the original headers as-is + * @return a read-only variant of the headers, or the original headers as-is if already read-only */ public static HttpHeaders readOnlyHttpHeaders(HttpHeaders headers) { Assert.notNull(headers, "HttpHeaders must not be null"); @@ -1879,16 +1887,16 @@ public static HttpHeaders readOnlyHttpHeaders(HttpHeaders headers) { /** * Remove any read-only wrapper that may have been previously applied around * the given headers 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 headers the headers to expose * @return a writable variant of the headers, or the original headers as-is * @since 5.1.1 + * @deprecated as of 6.2 in favor of {@link #HttpHeaders(MultiValueMap)}. */ + @Deprecated(since = "6.2", forRemoval = true) public static HttpHeaders writableHttpHeaders(HttpHeaders headers) { - Assert.notNull(headers, "HttpHeaders must not be null"); - if (headers == EMPTY) { - return new HttpHeaders(); - } - return (headers instanceof ReadOnlyHttpHeaders ? new HttpHeaders(headers.headers) : headers); + return new HttpHeaders(headers); } /** 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 7c16b19d6799..87eac6a9ae79 100644 --- a/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 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. @@ -31,6 +31,8 @@ /** * {@code HttpHeaders} object that can only be read, not written to. + *

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. * * @author Brian Clozel * @author Sam Brannen diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java b/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java index 54e2b5892649..a94e378e3c3c 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java @@ -70,7 +70,7 @@ public DefaultServerHttpRequestBuilder(ServerHttpRequest original) { Assert.notNull(original, "ServerHttpRequest is required"); this.uri = original.getURI(); - this.headers = HttpHeaders.writableHttpHeaders(original.getHeaders()); + this.headers = new HttpHeaders(original.getHeaders()); this.httpMethod = original.getMethod(); this.contextPath = original.getPath().contextPath().value(); this.remoteAddress = original.getRemoteAddress(); 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 059e3a98162d..de8ab6ac0cb6 100644 --- a/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java +++ b/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java @@ -35,6 +35,7 @@ import java.util.Set; import java.util.TimeZone; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import static java.util.stream.Collectors.toList; @@ -54,8 +55,19 @@ */ class HttpHeadersTests { - private final HttpHeaders headers = new HttpHeaders(); + final HttpHeaders headers = new HttpHeaders(); + @Test + void constructorUnwrapsReadonly() { + headers.setContentType(MediaType.APPLICATION_JSON); + HttpHeaders readOnly = HttpHeaders.readOnlyHttpHeaders(headers); + assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON); + HttpHeaders writable = new HttpHeaders(readOnly); + writable.setContentType(MediaType.TEXT_PLAIN); + // content-type value is cached by ReadOnlyHttpHeaders + assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON); + assertThat(writable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN); + } @Test void getOrEmpty() { @@ -578,182 +590,188 @@ void bearerAuth() { assertThat(authorization).isEqualTo("Bearer foo"); } - @Test - void keySetOperations() { - headers.add("Alpha", "apple"); - headers.add("Bravo", "banana"); - Set keySet = headers.keySet(); - - // Please DO NOT simplify the following with AssertJ's fluent API. - // - // We explicitly invoke methods directly on HttpHeaders#keySet() - // here to check the behavior of the entire contract. - - // isEmpty() and size() - assertThat(keySet).isNotEmpty(); - assertThat(keySet).hasSize(2); - - // contains() - assertThat(keySet.contains("Alpha")).as("Alpha should be present").isTrue(); - assertThat(keySet.contains("alpha")).as("alpha should be present").isTrue(); - assertThat(keySet.contains("Bravo")).as("Bravo should be present").isTrue(); - assertThat(keySet.contains("BRAVO")).as("BRAVO should be present").isTrue(); - assertThat(keySet.contains("Charlie")).as("Charlie should not be present").isFalse(); - - // toArray() - assertThat(keySet.toArray()).isEqualTo(new String[] {"Alpha", "Bravo"}); - - // spliterator() via stream() - assertThat(keySet.stream().collect(toList())).isEqualTo(Arrays.asList("Alpha", "Bravo")); - // iterator() - List results = new ArrayList<>(); - keySet.iterator().forEachRemaining(results::add); - assertThat(results).isEqualTo(Arrays.asList("Alpha", "Bravo")); - - // remove() - assertThat(keySet.remove("Alpha")).isTrue(); - assertThat(keySet).hasSize(1); - assertThat(headers).hasSize(1); - assertThat(keySet.remove("Alpha")).isFalse(); - assertThat(keySet).hasSize(1); - assertThat(headers).hasSize(1); - - // clear() - keySet.clear(); - assertThat(keySet).isEmpty(); - assertThat(keySet).isEmpty(); - assertThat(headers).isEmpty(); - assertThat(headers).isEmpty(); - - // Unsupported operations - assertThatExceptionOfType(UnsupportedOperationException.class) - .isThrownBy(() -> keySet.add("x")); - assertThatExceptionOfType(UnsupportedOperationException.class) - .isThrownBy(() -> keySet.addAll(Collections.singleton("enigma"))); - } - - /** - * This method intentionally checks a wider/different range of functionality - * than {@link #removalFromKeySetRemovesEntryFromUnderlyingMap()}. - */ - @Test // https://github.com/spring-projects/spring-framework/issues/23633 - void keySetRemovalChecks() { - // --- Given --- - headers.add("Alpha", "apple"); - headers.add("Bravo", "banana"); - assertThat(headers).containsOnlyKeys("Alpha", "Bravo"); - - // --- When --- - boolean removed = headers.keySet().remove("Alpha"); - - // --- Then --- - - // Please DO NOT simplify the following with AssertJ's fluent API. - // - // We explicitly invoke methods directly on HttpHeaders here to check - // the behavior of the entire contract. - - assertThat(removed).isTrue(); - assertThat(headers.keySet().remove("Alpha")).isFalse(); - assertThat(headers).hasSize(1); - assertThat(headers.containsKey("Alpha")).as("Alpha should have been removed").isFalse(); - assertThat(headers.containsKey("Bravo")).as("Bravo should be present").isTrue(); - assertThat(headers.keySet()).containsOnly("Bravo"); - assertThat(headers.entrySet()).containsOnly(entry("Bravo", List.of("banana"))); - } - - @Test - void removalFromKeySetRemovesEntryFromUnderlyingMap() { - String headerName = "MyHeader"; - String headerValue = "value"; + @Nested + class MapEntriesTests { + + @Test + void keySetOperations() { + headers.add("Alpha", "apple"); + headers.add("Bravo", "banana"); + Set keySet = headers.keySet(); + + // Please DO NOT simplify the following with AssertJ's fluent API. + // + // We explicitly invoke methods directly on HttpHeaders#keySet() + // here to check the behavior of the entire contract. + + // isEmpty() and size() + assertThat(keySet).isNotEmpty(); + assertThat(keySet).hasSize(2); + + // contains() + assertThat(keySet.contains("Alpha")).as("Alpha should be present").isTrue(); + assertThat(keySet.contains("alpha")).as("alpha should be present").isTrue(); + assertThat(keySet.contains("Bravo")).as("Bravo should be present").isTrue(); + assertThat(keySet.contains("BRAVO")).as("BRAVO should be present").isTrue(); + assertThat(keySet.contains("Charlie")).as("Charlie should not be present").isFalse(); + + // toArray() + assertThat(keySet.toArray()).isEqualTo(new String[] {"Alpha", "Bravo"}); + + // spliterator() via stream() + assertThat(keySet.stream().collect(toList())).isEqualTo(Arrays.asList("Alpha", "Bravo")); + + // iterator() + List results = new ArrayList<>(); + keySet.iterator().forEachRemaining(results::add); + assertThat(results).isEqualTo(Arrays.asList("Alpha", "Bravo")); + + // remove() + assertThat(keySet.remove("Alpha")).isTrue(); + assertThat(keySet).hasSize(1); + assertThat(headers).hasSize(1); + assertThat(keySet.remove("Alpha")).isFalse(); + assertThat(keySet).hasSize(1); + assertThat(headers).hasSize(1); + + // clear() + keySet.clear(); + assertThat(keySet).isEmpty(); + assertThat(keySet).isEmpty(); + assertThat(headers).isEmpty(); + assertThat(headers).isEmpty(); + + // Unsupported operations + assertThatExceptionOfType(UnsupportedOperationException.class) + .isThrownBy(() -> keySet.add("x")); + assertThatExceptionOfType(UnsupportedOperationException.class) + .isThrownBy(() -> keySet.addAll(Collections.singleton("enigma"))); + } - assertThat(headers).isEmpty(); - headers.add(headerName, headerValue); - assertThat(headers.containsKey(headerName)).isTrue(); - headers.keySet().removeIf(key -> key.equals(headerName)); - assertThat(headers).isEmpty(); - headers.add(headerName, headerValue); - assertThat(headers.get(headerName)).containsExactly(headerValue); - } + /** + * This method intentionally checks a wider/different range of functionality + * than {@link #removalFromKeySetRemovesEntryFromUnderlyingMap()}. + */ + @Test // https://github.com/spring-projects/spring-framework/issues/23633 + void keySetRemovalChecks() { + // --- Given --- + headers.add("Alpha", "apple"); + headers.add("Bravo", "banana"); + assertThat(headers).containsOnlyKeys("Alpha", "Bravo"); + + // --- When --- + boolean removed = headers.keySet().remove("Alpha"); + + // --- Then --- + + // Please DO NOT simplify the following with AssertJ's fluent API. + // + // We explicitly invoke methods directly on HttpHeaders here to check + // the behavior of the entire contract. + + assertThat(removed).isTrue(); + assertThat(headers.keySet().remove("Alpha")).isFalse(); + assertThat(headers).hasSize(1); + assertThat(headers.containsKey("Alpha")).as("Alpha should have been removed").isFalse(); + assertThat(headers.containsKey("Bravo")).as("Bravo should be present").isTrue(); + assertThat(headers.keySet()).containsOnly("Bravo"); + assertThat(headers.entrySet()).containsOnly(entry("Bravo", List.of("banana"))); + } - @Test - void removalFromEntrySetRemovesEntryFromUnderlyingMap() { - String headerName = "MyHeader"; - String headerValue = "value"; + @Test + void removalFromKeySetRemovesEntryFromUnderlyingMap() { + String headerName = "MyHeader"; + String headerValue = "value"; + + assertThat(headers).isEmpty(); + headers.add(headerName, headerValue); + assertThat(headers.containsKey(headerName)).isTrue(); + headers.keySet().removeIf(key -> key.equals(headerName)); + assertThat(headers).isEmpty(); + headers.add(headerName, headerValue); + assertThat(headers.get(headerName)).containsExactly(headerValue); + } - assertThat(headers).isEmpty(); - headers.add(headerName, headerValue); - assertThat(headers.containsKey(headerName)).isTrue(); - headers.entrySet().removeIf(entry -> entry.getKey().equals(headerName)); - assertThat(headers).isEmpty(); - headers.add(headerName, headerValue); - assertThat(headers.get(headerName)).containsExactly(headerValue); - } + @Test + void removalFromEntrySetRemovesEntryFromUnderlyingMap() { + String headerName = "MyHeader"; + String headerValue = "value"; + + assertThat(headers).isEmpty(); + headers.add(headerName, headerValue); + assertThat(headers.containsKey(headerName)).isTrue(); + headers.entrySet().removeIf(entry -> entry.getKey().equals(headerName)); + assertThat(headers).isEmpty(); + headers.add(headerName, headerValue); + assertThat(headers.get(headerName)).containsExactly(headerValue); + } - @Test - void readOnlyHttpHeadersRetainEntrySetOrder() { - headers.add("aardvark", "enigma"); - headers.add("beaver", "enigma"); - headers.add("cat", "enigma"); - headers.add("dog", "enigma"); - headers.add("elephant", "enigma"); + @Test + void readOnlyHttpHeadersRetainEntrySetOrder() { + headers.add("aardvark", "enigma"); + headers.add("beaver", "enigma"); + headers.add("cat", "enigma"); + headers.add("dog", "enigma"); + headers.add("elephant", "enigma"); - String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" }; + String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" }; - assertThat(headers.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); + assertThat(headers.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers); - assertThat(readOnlyHttpHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - } + HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers); + assertThat(readOnlyHttpHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); + } - @Test - void readOnlyHttpHeadersCopyOrderTest() { - headers.add("aardvark", "enigma"); - headers.add("beaver", "enigma"); - headers.add("cat", "enigma"); - headers.add("dog", "enigma"); - headers.add("elephant", "enigma"); + @Test + void readOnlyHttpHeadersCopyOrderTest() { + headers.add("aardvark", "enigma"); + headers.add("beaver", "enigma"); + headers.add("cat", "enigma"); + headers.add("dog", "enigma"); + headers.add("elephant", "enigma"); - String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" }; + String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" }; - HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers); + HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers); - HttpHeaders forEachHeaders = new HttpHeaders(); - readOnlyHttpHeaders.forEach(forEachHeaders::putIfAbsent); - assertThat(forEachHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); + HttpHeaders forEachHeaders = new HttpHeaders(); + readOnlyHttpHeaders.forEach(forEachHeaders::putIfAbsent); + assertThat(forEachHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - HttpHeaders putAllHeaders = new HttpHeaders(); - putAllHeaders.putAll(readOnlyHttpHeaders); - assertThat(putAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); + HttpHeaders putAllHeaders = new HttpHeaders(); + putAllHeaders.putAll(readOnlyHttpHeaders); + assertThat(putAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - HttpHeaders addAllHeaders = new HttpHeaders(); - addAllHeaders.addAll(readOnlyHttpHeaders); - assertThat(addAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - } + HttpHeaders addAllHeaders = new HttpHeaders(); + addAllHeaders.addAll(readOnlyHttpHeaders); + assertThat(addAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); + } - @Test // gh-25034 - void equalsUnwrapsHttpHeaders() { - HttpHeaders headers1 = new HttpHeaders(); - HttpHeaders headers2 = new HttpHeaders(new HttpHeaders(headers1)); + @Test // gh-25034 + void equalsUnwrapsHttpHeaders() { + HttpHeaders headers1 = new HttpHeaders(); + HttpHeaders headers2 = new HttpHeaders(new HttpHeaders(headers1)); - assertThat(headers1).isEqualTo(headers2); - assertThat(headers2).isEqualTo(headers1); - } + assertThat(headers1).isEqualTo(headers2); + assertThat(headers2).isEqualTo(headers1); + } - @Test - void getValuesAsList() { - HttpHeaders headers = new HttpHeaders(); - headers.add("Foo", "Bar"); - headers.add("Foo", "Baz, Qux"); - headers.add("Quux", "\t\"Corge\", \"Grault\""); - headers.add("Garply", " Waldo \"Fred\\!\", \"\tPlugh, Xyzzy! \""); - headers.add("Example-Dates", "\"Sat, 04 May 1996\", \"Wed, 14 Sep 2005\""); + @Test + void getValuesAsList() { + HttpHeaders headers = new HttpHeaders(); + headers.add("Foo", "Bar"); + headers.add("Foo", "Baz, Qux"); + headers.add("Quux", "\t\"Corge\", \"Grault\""); + headers.add("Garply", " Waldo \"Fred\\!\", \"\tPlugh, Xyzzy! \""); + headers.add("Example-Dates", "\"Sat, 04 May 1996\", \"Wed, 14 Sep 2005\""); + + assertThat(headers.getValuesAsList("Foo")).containsExactly("Bar", "Baz", "Qux"); + assertThat(headers.getValuesAsList("Quux")).containsExactly("Corge", "Grault"); + assertThat(headers.getValuesAsList("Garply")).containsExactly("Waldo \"Fred\\!\"", "\tPlugh, Xyzzy! "); + assertThat(headers.getValuesAsList("Example-Dates")).containsExactly("Sat, 04 May 1996", "Wed, 14 Sep 2005"); + } - assertThat(headers.getValuesAsList("Foo")).containsExactly("Bar", "Baz", "Qux"); - assertThat(headers.getValuesAsList("Quux")).containsExactly("Corge", "Grault"); - assertThat(headers.getValuesAsList("Garply")).containsExactly("Waldo \"Fred\\!\"", "\tPlugh, Xyzzy! "); - assertThat(headers.getValuesAsList("Example-Dates")).containsExactly("Sat, 04 May 1996", "Wed, 14 Sep 2005"); } } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java index e089b1f3ccb2..29845f26f1cc 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java @@ -140,7 +140,7 @@ public ClientResponse.Builder headers(Consumer headersConsumer) { @SuppressWarnings("ConstantConditions") private HttpHeaders getHeaders() { if (this.headers == null) { - this.headers = HttpHeaders.writableHttpHeaders(this.originalResponse.headers().asHttpHeaders()); + this.headers = new HttpHeaders(this.originalResponse.headers().asHttpHeaders()); } return this.headers; }