From 73e5aa38ecea90c286c3739429efb49a704455e6 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Wed, 23 Oct 2024 10:42:09 +0100 Subject: [PATCH] Polishing contribution See gh-33697 --- .../web/client/DefaultRestClient.java | 47 +++++++------ .../web/client/DefaultRestClientBuilder.java | 42 ++++++------ .../web/client/RestClientBuilderTests.java | 2 +- .../client/RestClientIntegrationTests.java | 66 ++++++++++--------- .../client/DefaultWebClientBuilder.java | 32 ++++----- 5 files changed, 95 insertions(+), 94 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java b/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java index a9ab2c1364c..a2cf1eee6da 100644 --- a/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java +++ b/spring-web/src/main/java/org/springframework/web/client/DefaultRestClient.java @@ -40,7 +40,6 @@ import org.apache.commons.logging.LogFactory; import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.ResolvableType; -import org.springframework.http.HttpCookie; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; import org.springframework.http.HttpRequest; @@ -301,8 +300,6 @@ final class DefaultRestClient implements RestClient { private class DefaultRequestBodyUriSpec implements RequestBodyUriSpec { - private static final String COOKIE_DELIMITER = "; "; - private final HttpMethod httpMethod; @Nullable @@ -557,10 +554,9 @@ final class DefaultRestClient implements RestClient { try { uri = initUri(); HttpHeaders headers = initHeaders(); - MultiValueMap cookies = initCookies(); if (!CollectionUtils.isEmpty(cookies)) { - headers.put(HttpHeaders.COOKIE, List.of(cookiesToHeaderValue(cookies))); + headers.set(HttpHeaders.COOKIE, serializeCookies(cookies)); } ClientHttpRequest clientRequest = createRequest(uri); @@ -638,25 +634,36 @@ final class DefaultRestClient implements RestClient { } private MultiValueMap initCookies() { - MultiValueMap mergedCookies = new LinkedMultiValueMap<>(); - - if(!CollectionUtils.isEmpty(defaultCookies)) { - mergedCookies.putAll(defaultCookies); + MultiValueMap defaultCookies = DefaultRestClient.this.defaultCookies; + if (CollectionUtils.isEmpty(this.cookies)) { + return (defaultCookies != null ? defaultCookies : new LinkedMultiValueMap<>()); } - - if(!CollectionUtils.isEmpty(this.cookies)) { - mergedCookies.putAll(this.cookies); + else if (CollectionUtils.isEmpty(defaultCookies)) { + return this.cookies; + } + else { + MultiValueMap map = new LinkedMultiValueMap<>(); + map.putAll(DefaultRestClient.this.defaultCookies); + map.putAll(this.cookies); + return map; } - - return mergedCookies; } - private String cookiesToHeaderValue(MultiValueMap cookies) { - List flatCookies = new ArrayList<>(); - cookies.forEach((name, cookieValues) -> cookieValues.forEach(value -> - flatCookies.add(new HttpCookie(name, value).toString()) - )); - return String.join(COOKIE_DELIMITER, flatCookies); + private String serializeCookies(MultiValueMap cookies) { + boolean first = true; + StringBuilder sb = new StringBuilder(); + for (Map.Entry> entry : cookies.entrySet()) { + for (String value : entry.getValue()) { + if (!first) { + sb.append("; "); + } + else { + first = false; + } + sb.append(entry.getKey()).append("=").append(value); + } + } + return sb.toString(); } private ClientHttpRequest createRequest(URI uri) throws IOException { diff --git a/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java b/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java index 9522cd24962..975b1f183f9 100644 --- a/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java +++ b/spring-web/src/main/java/org/springframework/web/client/DefaultRestClientBuilder.java @@ -468,21 +468,21 @@ final class DefaultRestClientBuilder implements RestClient.Builder { public RestClient build() { ClientHttpRequestFactory requestFactory = initRequestFactory(); UriBuilderFactory uriBuilderFactory = initUriBuilderFactory(); + HttpHeaders defaultHeaders = copyDefaultHeaders(); MultiValueMap defaultCookies = copyDefaultCookies(); - List> messageConverters = (this.messageConverters != null ? - this.messageConverters : initMessageConverters()); - return new DefaultRestClient(requestFactory, - this.interceptors, this.initializers, uriBuilderFactory, - defaultHeaders, - defaultCookies, + + List> converters = + (this.messageConverters != null ? this.messageConverters : initMessageConverters()); + + return new DefaultRestClient( + requestFactory, this.interceptors, this.initializers, + uriBuilderFactory, defaultHeaders, defaultCookies, this.defaultRequest, this.statusHandlers, - messageConverters, - this.observationRegistry, - this.observationConvention, - new DefaultRestClientBuilder(this) - ); + converters, + this.observationRegistry, this.observationConvention, + new DefaultRestClientBuilder(this)); } private ClientHttpRequestFactory initRequestFactory() { @@ -519,26 +519,22 @@ final class DefaultRestClientBuilder implements RestClient.Builder { @Nullable private HttpHeaders copyDefaultHeaders() { - if (this.defaultHeaders != null) { - HttpHeaders copy = new HttpHeaders(); - this.defaultHeaders.forEach((key, values) -> copy.put(key, new ArrayList<>(values))); - return HttpHeaders.readOnlyHttpHeaders(copy); - } - else { + if (this.defaultHeaders == null) { return null; } + HttpHeaders copy = new HttpHeaders(); + this.defaultHeaders.forEach((key, values) -> copy.put(key, new ArrayList<>(values))); + return HttpHeaders.readOnlyHttpHeaders(copy); } @Nullable private MultiValueMap copyDefaultCookies() { - if (this.defaultCookies != null) { - MultiValueMap copy = new LinkedMultiValueMap<>(this.defaultCookies.size()); - this.defaultCookies.forEach((key, values) -> copy.put(key, new ArrayList<>(values))); - return CollectionUtils.unmodifiableMultiValueMap(copy); - } - else { + if (this.defaultCookies == null) { return null; } + MultiValueMap copy = new LinkedMultiValueMap<>(this.defaultCookies.size()); + this.defaultCookies.forEach((key, values) -> copy.put(key, new ArrayList<>(values))); + return CollectionUtils.unmodifiableMultiValueMap(copy); } } diff --git a/spring-web/src/test/java/org/springframework/web/client/RestClientBuilderTests.java b/spring-web/src/test/java/org/springframework/web/client/RestClientBuilderTests.java index 918d718730d..e328bcfb849 100644 --- a/spring-web/src/test/java/org/springframework/web/client/RestClientBuilderTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/RestClientBuilderTests.java @@ -151,7 +151,7 @@ public class RestClientBuilderTests { } @Test - void defaultCookieWithMultipleValuesAddsCookieToDefaultCookiesMapWithAllValues() { + void defaultCookieWithMultipleValuesAddsCookieToDefaultCookiesMap() { RestClient.Builder builder = RestClient.builder(); builder.defaultCookie("myCookie", "testValue1", "testValue2"); diff --git a/spring-web/src/test/java/org/springframework/web/client/RestClientIntegrationTests.java b/spring-web/src/test/java/org/springframework/web/client/RestClientIntegrationTests.java index 43be4226af3..6f6df924de1 100644 --- a/spring-web/src/test/java/org/springframework/web/client/RestClientIntegrationTests.java +++ b/spring-web/src/test/java/org/springframework/web/client/RestClientIntegrationTests.java @@ -23,7 +23,6 @@ import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; import java.net.URI; import java.net.URISyntaxException; -import java.nio.charset.StandardCharsets; import java.util.List; import java.util.Map; import java.util.function.Consumer; @@ -56,6 +55,7 @@ import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.web.testfixture.xml.Pojo; +import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.junit.jupiter.api.Assumptions.assumeFalse; @@ -660,8 +660,8 @@ class RestClientIntegrationTests { startServer(requestFactory); String content = "Internal Server error"; - prepareResponse(response -> response.setResponseCode(500) - .setHeader("Content-Type", "text/plain").setBody(content)); + prepareResponse(response -> + response.setResponseCode(500).setHeader("Content-Type", "text/plain").setBody(content)); ResponseEntity result = this.restClient.get() .uri("/").accept(MediaType.APPLICATION_JSON) @@ -689,7 +689,7 @@ class RestClientIntegrationTests { String result = this.restClient.get() .uri("/greeting") .header("X-Test-Header", "testvalue") - .exchange((request, response) -> new String(RestClientUtils.getBody(response), StandardCharsets.UTF_8)); + .exchange((request, response) -> new String(RestClientUtils.getBody(response), UTF_8)); assertThat(result).isEqualTo("Hello Spring!"); @@ -753,12 +753,12 @@ class RestClientIntegrationTests { void exchangeFor404(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setResponseCode(404) - .setHeader("Content-Type", "text/plain").setBody("Not Found")); + prepareResponse(response -> + response.setResponseCode(404).setHeader("Content-Type", "text/plain").setBody("Not Found")); String result = this.restClient.get() .uri("/greeting") - .exchange((request, response) -> new String(RestClientUtils.getBody(response), StandardCharsets.UTF_8)); + .exchange((request, response) -> new String(RestClientUtils.getBody(response), UTF_8)); assertThat(result).isEqualTo("Not Found"); @@ -770,8 +770,8 @@ class RestClientIntegrationTests { void requestInitializer(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); RestClient initializedClient = this.restClient.mutate() .requestInitializer(request -> request.getHeaders().add("foo", "bar")) @@ -792,9 +792,8 @@ class RestClientIntegrationTests { void requestInterceptor(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); - + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); RestClient interceptedClient = this.restClient.mutate() .requestInterceptor((request, body, execution) -> { @@ -819,6 +818,7 @@ class RestClientIntegrationTests { startServer(requestFactory); prepareResponse(response -> response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); + RestClient restClientWithCookies = this.restClient.mutate() .defaultCookie("testCookie", "firstValue", "secondValue") .build(); @@ -852,8 +852,8 @@ class RestClientIntegrationTests { RestClient interceptedClient = this.restClient.mutate().requestInterceptor(interceptor).build(); // header not present - prepareResponse(response -> response - .setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); assertThatExceptionOfType(MyException.class).isThrownBy(() -> interceptedClient.get() @@ -881,8 +881,8 @@ class RestClientIntegrationTests { void defaultHeaders(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); RestClient headersClient = this.restClient.mutate() .defaultHeaders(headers -> headers.add("foo", "bar")) @@ -903,8 +903,8 @@ class RestClientIntegrationTests { void defaultRequest(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); RestClient headersClient = this.restClient.mutate() .defaultRequest(request -> request.header("foo", "bar")) @@ -925,8 +925,8 @@ class RestClientIntegrationTests { void defaultRequestOverride(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); RestClient headersClient = this.restClient.mutate() .defaultRequest(request -> request.accept(MediaType.APPLICATION_JSON)) @@ -948,8 +948,8 @@ class RestClientIntegrationTests { void relativeUri(ClientHttpRequestFactory requestFactory) throws URISyntaxException { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); URI uri = new URI(null, null, "/foo bar", null); @@ -969,23 +969,28 @@ class RestClientIntegrationTests { @ParameterizedRestClientTest void cookieAddsCookie(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); + + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); this.restClient.get() .uri("/greeting") - .cookie("foo", "bar") + .cookie("c1", "v1a") + .cookie("c1", "v1b") + .cookie("c2", "v2a") .retrieve() .body(String.class); - expectRequest(request -> assertThat(request.getHeader("Cookie")).isEqualTo("foo=bar")); + expectRequest(request -> assertThat(request.getHeader("Cookie")).isEqualTo("c1=v1a; c1=v1b; c2=v2a")); } @ParameterizedRestClientTest void cookieOverridesDefaultCookie(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); + + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); + RestClient restClientWithCookies = this.restClient.mutate() .defaultCookie("testCookie", "firstValue", "secondValue") .build(); @@ -1002,8 +1007,9 @@ class RestClientIntegrationTests { @ParameterizedRestClientTest void cookiesCanRemoveCookie(ClientHttpRequestFactory requestFactory) { startServer(requestFactory); - prepareResponse(response -> response.setHeader("Content-Type", "text/plain") - .setBody("Hello Spring!")); + + prepareResponse(response -> + response.setHeader("Content-Type", "text/plain").setBody("Hello Spring!")); this.restClient.get() .uri("/greeting") diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClientBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClientBuilder.java index 59da4e80d02..b67ace1c82f 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClientBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClientBuilder.java @@ -319,18 +319,14 @@ final class DefaultWebClientBuilder implements WebClient.Builder { .orElse(null) : null); HttpHeaders defaultHeaders = copyDefaultHeaders(); - MultiValueMap defaultCookies = copyDefaultCookies(); - return new DefaultWebClient(exchange, - filterFunctions, - initUriBuilderFactory(), - defaultHeaders, - defaultCookies, + return new DefaultWebClient( + exchange, filterFunctions, + initUriBuilderFactory(), defaultHeaders, defaultCookies, this.defaultRequest, this.statusHandlers, - this.observationRegistry, - this.observationConvention, + this.observationRegistry, this.observationConvention, new DefaultWebClientBuilder(this)); } @@ -374,26 +370,22 @@ final class DefaultWebClientBuilder implements WebClient.Builder { @Nullable private HttpHeaders copyDefaultHeaders() { - if (this.defaultHeaders != null) { - HttpHeaders copy = new HttpHeaders(); - this.defaultHeaders.forEach((key, values) -> copy.put(key, new ArrayList<>(values))); - return HttpHeaders.readOnlyHttpHeaders(copy); - } - else { + if (this.defaultHeaders == null) { return null; } + HttpHeaders headers = new HttpHeaders(); + this.defaultHeaders.forEach((key, values) -> headers.put(key, new ArrayList<>(values))); + return HttpHeaders.readOnlyHttpHeaders(headers); } @Nullable private MultiValueMap copyDefaultCookies() { - if (this.defaultCookies != null) { - MultiValueMap copy = new LinkedMultiValueMap<>(this.defaultCookies.size()); - this.defaultCookies.forEach((key, values) -> copy.put(key, new ArrayList<>(values))); - return CollectionUtils.unmodifiableMultiValueMap(copy); - } - else { + if (this.defaultCookies == null) { return null; } + MultiValueMap map = new LinkedMultiValueMap<>(this.defaultCookies.size()); + this.defaultCookies.forEach((key, values) -> map.put(key, new ArrayList<>(values))); + return CollectionUtils.unmodifiableMultiValueMap(map); } }