From da5b705328c6e62f6a492cb672f3b89b356ddc18 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Tue, 3 Jul 2018 04:43:54 -0400 Subject: [PATCH] Polish default headers/attributes in WebClient --- .../function/client/DefaultWebClient.java | 8 +- .../client/DefaultWebClientBuilder.java | 62 ++++---- .../reactive/function/client/WebClient.java | 51 +++---- .../client/DefaultWebClientTests.java | 137 +++++++++++------- 4 files changed, 134 insertions(+), 124 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java index 213e7e52afc..2d41cb02e0a 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClient.java @@ -333,12 +333,8 @@ class DefaultWebClient implements WebClient { } else { HttpHeaders result = new HttpHeaders(); + result.putAll(defaultHeaders); result.putAll(this.headers); - defaultHeaders.forEach((name, values) -> { - if (!this.headers.containsKey(name)) { - values.forEach(value -> result.add(name, value)); - } - }); return result; } } @@ -352,8 +348,8 @@ class DefaultWebClient implements WebClient { } else { MultiValueMap result = new LinkedMultiValueMap<>(); + result.putAll(defaultCookies); result.putAll(this.cookies); - defaultCookies.forEach(result::putIfAbsent); return result; } } 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 23207a38541..4e1dc131bc9 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 @@ -77,8 +77,8 @@ final class DefaultWebClientBuilder implements WebClient.Builder { Assert.notNull(other, "DefaultWebClientBuilder must not be null"); this.baseUrl = other.baseUrl; - this.defaultUriVariables = (other.defaultUriVariables != null ? - new LinkedHashMap<>(other.defaultUriVariables) : null); + this.defaultUriVariables = + other.defaultUriVariables != null ? new LinkedHashMap<>(other.defaultUriVariables) : null; this.uriBuilderFactory = other.uriBuilderFactory; if (other.defaultHeaders != null) { this.defaultHeaders = new HttpHeaders(); @@ -87,9 +87,9 @@ final class DefaultWebClientBuilder implements WebClient.Builder { else { this.defaultHeaders = null; } - this.defaultCookies = (other.defaultCookies != null ? - new LinkedMultiValueMap<>(other.defaultCookies) : null); - this.filters = (other.filters != null ? new ArrayList<>(other.filters) : null); + this.defaultCookies = + other.defaultCookies != null ? new LinkedMultiValueMap<>(other.defaultCookies) : null; + this.filters = other.filters != null ? new ArrayList<>(other.filters) : null; this.connector = other.connector; this.exchangeFunction = other.exchangeFunction; this.exchangeStrategies = other.exchangeStrategies; @@ -115,11 +115,8 @@ final class DefaultWebClientBuilder implements WebClient.Builder { } @Override - public WebClient.Builder defaultHeader(String headerName, String... headerValues) { - HttpHeaders headers = initHeaders(); - for (String headerValue : headerValues) { - headers.add(headerName, headerValue); - } + public WebClient.Builder defaultHeader(String header, String... values) { + initHeaders().put(header, Arrays.asList(values)); return this; } @@ -137,8 +134,8 @@ final class DefaultWebClientBuilder implements WebClient.Builder { } @Override - public WebClient.Builder defaultCookie(String cookieName, String... cookieValues) { - initCookies().addAll(cookieName, Arrays.asList(cookieValues)); + public WebClient.Builder defaultCookie(String cookie, String... values) { + initCookies().addAll(cookie, Arrays.asList(values)); return this; } @@ -202,25 +199,20 @@ final class DefaultWebClientBuilder implements WebClient.Builder { .map(filter -> filter.apply(exchange)) .orElse(exchange) : exchange); return new DefaultWebClient(filteredExchange, initUriBuilderFactory(), - unmodifiableCopy(this.defaultHeaders), unmodifiableCopy(this.defaultCookies), + this.defaultHeaders != null ? unmodifiableCopy(this.defaultHeaders) : null, + this.defaultCookies != null ? unmodifiableCopy(this.defaultCookies) : null, new DefaultWebClientBuilder(this)); } - private static @Nullable HttpHeaders unmodifiableCopy(@Nullable HttpHeaders original) { - if (original != null) { - return HttpHeaders.readOnlyHttpHeaders(original); - } - else { - return null; + private ExchangeFunction initExchangeFunction() { + if (this.exchangeFunction != null) { + return this.exchangeFunction; } - } - - private static @Nullable MultiValueMap unmodifiableCopy(@Nullable MultiValueMap original) { - if (original != null) { - return CollectionUtils.unmodifiableMultiValueMap(new LinkedMultiValueMap<>(original)); + else if (this.connector != null) { + return ExchangeFunctions.create(this.connector, this.exchangeStrategies); } else { - return null; + return ExchangeFunctions.create(new ReactorClientHttpConnector(), this.exchangeStrategies); } } @@ -228,22 +220,18 @@ final class DefaultWebClientBuilder implements WebClient.Builder { if (this.uriBuilderFactory != null) { return this.uriBuilderFactory; } - DefaultUriBuilderFactory factory = (this.baseUrl != null ? - new DefaultUriBuilderFactory(this.baseUrl) : new DefaultUriBuilderFactory()); + DefaultUriBuilderFactory factory = this.baseUrl != null ? + new DefaultUriBuilderFactory(this.baseUrl) : new DefaultUriBuilderFactory(); factory.setDefaultUriVariables(this.defaultUriVariables); return factory; } - private ExchangeFunction initExchangeFunction() { - if (this.exchangeFunction != null) { - return this.exchangeFunction; - } - else if (this.connector != null) { - return ExchangeFunctions.create(this.connector, this.exchangeStrategies); - } - else { - return ExchangeFunctions.create(new ReactorClientHttpConnector(), this.exchangeStrategies); - } + private static HttpHeaders unmodifiableCopy(HttpHeaders headers) { + return HttpHeaders.readOnlyHttpHeaders(headers); + } + + private static MultiValueMap unmodifiableCopy(MultiValueMap map) { + return CollectionUtils.unmodifiableMultiValueMap(new LinkedMultiValueMap<>(map)); } @Override diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java index df5e85c2e5e..0878ad2fe4c 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/WebClient.java @@ -226,31 +226,31 @@ public interface WebClient { Builder uriBuilderFactory(UriBuilderFactory uriBuilderFactory); /** - * Add the given header to all requests that have not added it. - * @param headerName the header name - * @param headerValues the header values + * A global option to specify a header to be added to every request, + * if the request does not already contain such a header. + * @param header the header name + * @param values the header values */ - Builder defaultHeader(String headerName, String... headerValues); + Builder defaultHeader(String header, String... values); /** - * Manipulate the default headers with the given consumer. The - * headers provided to the consumer are "live", so that the consumer - * can be used to overwrite or remove existing values. - * @param headersConsumer the headers consumer + * Provides access to every {@link #defaultHeader(String, String...)} + * declared so far with the possibility to add, replace, or remove. + * @param headersConsumer the consumer */ Builder defaultHeaders(Consumer headersConsumer); /** - * Add the given header to all requests that haven't added it. - * @param cookieName the cookie name - * @param cookieValues the cookie values + * A global option to specify a cookie to be added to every request, + * if the request does not already contain such a cookie. + * @param cookie the cookie name + * @param values the cookie values */ - Builder defaultCookie(String cookieName, String... cookieValues); + Builder defaultCookie(String cookie, String... values); /** - * Manipulate the default cookies with the given consumer. The - * cookies provided to the consumer are "live", so that the consumer - * can be used to overwrite or remove existing values. + * Provides access to every {@link #defaultCookie(String, String...)} + * declared so far with the possibility to add, replace, or remove. * @param cookiesConsumer a function that consumes the cookies map */ Builder defaultCookies(Consumer> cookiesConsumer); @@ -383,10 +383,9 @@ public interface WebClient { S cookie(String name, String value); /** - * Manipulate the default cookies with the given consumer. The - * cookies provided to the consumer are "live", so that the consumer - * can be used to overwrite or remove existing values. - * @param cookiesConsumer a function that consumes the cookies map + * Provides access to every cookie declared so far with the possibility + * to add, replace, or remove values. + * @param cookiesConsumer the consumer to provide access to * @return this builder */ S cookies(Consumer> cookiesConsumer); @@ -416,10 +415,9 @@ public interface WebClient { S header(String headerName, String... headerValues); /** - * Manipulate the default headers with the given consumer. The - * headers provided to the consumer are "live", so that the consumer - * can be used to overwrite or remove existing values. - * @param headersConsumer a function that consumes the {@code HttpHeaders} + * Provides access to every header declared so far with the possibility + * to add, replace, or remove values. + * @param headersConsumer the consumer to provide access to * @return this builder */ S headers(Consumer headersConsumer); @@ -433,10 +431,9 @@ public interface WebClient { S attribute(String name, Object value); /** - * Manipulate the request attributes with the given consumer. The attributes provided to - * the consumer are "live", so that the consumer can be used to inspect attributes, - * remove attributes, or use any of the other map-provided methods. - * @param attributesConsumer a function that consumes the attributes + * Provides access to every attribute declared so far with the + * possibility to add, replace, or remove values. + * @param attributesConsumer the consumer to provide access to * @return this builder */ S attributes(Consumer> attributesConsumer); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultWebClientTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultWebClientTests.java index 99d0e0857a8..a4e0f72c388 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultWebClientTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultWebClientTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2018 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. @@ -18,6 +18,8 @@ package org.springframework.web.reactive.function.client; import java.time.Duration; import java.util.Collections; +import java.util.HashMap; +import java.util.Map; import org.junit.Before; import org.junit.Test; @@ -40,6 +42,8 @@ import static org.mockito.Mockito.*; */ public class DefaultWebClientTests { + private WebClient.Builder builder; + private ExchangeFunction exchangeFunction; @Captor @@ -50,16 +54,17 @@ public class DefaultWebClientTests { public void setup() { MockitoAnnotations.initMocks(this); this.exchangeFunction = mock(ExchangeFunction.class); - when(this.exchangeFunction.exchange(captor.capture())).thenReturn(Mono.empty()); + when(this.exchangeFunction.exchange(this.captor.capture())).thenReturn(Mono.empty()); + this.builder = WebClient.builder().baseUrl("/base").exchangeFunction(this.exchangeFunction); } @Test public void basic() { - WebClient client = builder().build(); - client.get().uri("/path").exchange(); - ClientRequest request = verifyExchange(); + this.builder.build().get().uri("/path").exchange(); + + ClientRequest request = verifyAndGetRequest(); assertEquals("/base/path", request.url().toString()); assertEquals(new HttpHeaders(), request.headers()); assertEquals(Collections.emptyMap(), request.cookies()); @@ -67,32 +72,36 @@ public class DefaultWebClientTests { @Test public void uriBuilder() { - WebClient client = builder().build(); - client.get().uri(builder -> builder.path("/path").queryParam("q", "12").build()).exchange(); - ClientRequest request = verifyExchange(); + this.builder.build().get() + .uri(builder -> builder.path("/path").queryParam("q", "12").build()) + .exchange(); + + ClientRequest request = verifyAndGetRequest(); assertEquals("/base/path?q=12", request.url().toString()); verifyNoMoreInteractions(this.exchangeFunction); } @Test public void uriBuilderWithPathOverride() { - WebClient client = builder().build(); - client.get().uri(builder -> builder.replacePath("/path").build()).exchange(); - ClientRequest request = verifyExchange(); + this.builder.build().get() + .uri(builder -> builder.replacePath("/path").build()) + .exchange(); + + ClientRequest request = verifyAndGetRequest(); assertEquals("/path", request.url().toString()); verifyNoMoreInteractions(this.exchangeFunction); } @Test public void requestHeaderAndCookie() { - WebClient client = builder().build(); - client.get().uri("/path").accept(MediaType.APPLICATION_JSON) + + this.builder.build().get().uri("/path").accept(MediaType.APPLICATION_JSON) .cookies(cookies -> cookies.add("id", "123")) // SPR-16178 .exchange(); - ClientRequest request = verifyExchange(); + ClientRequest request = verifyAndGetRequest(); assertEquals("application/json", request.headers().getFirst("Accept")); assertEquals("123", request.cookies().getFirst("id")); verifyNoMoreInteractions(this.exchangeFunction); @@ -100,10 +109,14 @@ public class DefaultWebClientTests { @Test public void defaultHeaderAndCookie() { - WebClient client = builder().defaultHeader("Accept", "application/json").defaultCookie("id", "123").build(); + + WebClient client = this.builder + .defaultHeader("Accept", "application/json").defaultCookie("id", "123") + .build(); + client.get().uri("/path").exchange(); - ClientRequest request = verifyExchange(); + ClientRequest request = verifyAndGetRequest(); assertEquals("application/json", request.headers().getFirst("Accept")); assertEquals("123", request.cookies().getFirst("id")); verifyNoMoreInteractions(this.exchangeFunction); @@ -111,10 +124,15 @@ public class DefaultWebClientTests { @Test public void defaultHeaderAndCookieOverrides() { - WebClient client = builder().defaultHeader("Accept", "application/json").defaultCookie("id", "123").build(); + + WebClient client = this.builder + .defaultHeader("Accept", "application/json") + .defaultCookie("id", "123") + .build(); + client.get().uri("/path").header("Accept", "application/xml").cookie("id", "456").exchange(); - ClientRequest request = verifyExchange(); + ClientRequest request = verifyAndGetRequest(); assertEquals("application/xml", request.headers().getFirst("Accept")); assertEquals("456", request.cookies().getFirst("id")); verifyNoMoreInteractions(this.exchangeFunction); @@ -123,7 +141,7 @@ public class DefaultWebClientTests { @Test(expected = IllegalArgumentException.class) public void bodyObjectPublisher() { Mono mono = Mono.empty(); - WebClient client = builder().build(); + WebClient client = this.builder.build(); client.post().uri("http://example.com").syncBody(mono); } @@ -131,57 +149,73 @@ public class DefaultWebClientTests { @Test public void mutateDoesCopy() { - WebClient.Builder builder = WebClient.builder(); - builder.filter((request, next) -> next.exchange(request)); - builder.defaultHeader("foo", "bar"); - builder.defaultCookie("foo", "bar"); + // First, build the clients + + WebClient.Builder builder = WebClient.builder() + .filter((request, next) -> next.exchange(request)) + .defaultHeader("foo", "bar") + .defaultCookie("foo", "bar"); + WebClient client1 = builder.build(); - builder.filter((request, next) -> next.exchange(request)); - builder.defaultHeader("baz", "qux"); - builder.defaultCookie("baz", "qux"); - WebClient client2 = builder.build(); + WebClient client2 = builder.filter((request, next) -> next.exchange(request)) + .defaultHeader("baz", "qux") + .defaultCookie("baz", "qux") + .build(); - WebClient.Builder mutatedBuilder = client1.mutate(); + WebClient client1a = client1.mutate() + .filter((request, next) -> next.exchange(request)) + .defaultHeader("baz", "qux") + .defaultCookie("baz", "qux") + .build(); - mutatedBuilder.filter((request, next) -> next.exchange(request)); - mutatedBuilder.defaultHeader("baz", "qux"); - mutatedBuilder.defaultCookie("baz", "qux"); - WebClient clientFromMutatedBuilder = mutatedBuilder.build(); + // Now, verify what each client has.. - client1.mutate().filters(filters -> assertEquals(1, filters.size())); - client1.mutate().defaultHeaders(headers -> assertEquals(1, headers.size())); - client1.mutate().defaultCookies(cookies -> assertEquals(1, cookies.size())); + WebClient.Builder builder1 = client1.mutate(); + builder1.filters(filters -> assertEquals(1, filters.size())); + builder1.defaultHeaders(headers -> assertEquals(1, headers.size())); + builder1.defaultCookies(cookies -> assertEquals(1, cookies.size())); - client2.mutate().filters(filters -> assertEquals(2, filters.size())); - client2.mutate().defaultHeaders(headers -> assertEquals(2, headers.size())); - client2.mutate().defaultCookies(cookies -> assertEquals(2, cookies.size())); + WebClient.Builder builder2 = client2.mutate(); + builder2.filters(filters -> assertEquals(2, filters.size())); + builder2.defaultHeaders(headers -> assertEquals(2, headers.size())); + builder2.defaultCookies(cookies -> assertEquals(2, cookies.size())); - clientFromMutatedBuilder.mutate().filters(filters -> assertEquals(2, filters.size())); - clientFromMutatedBuilder.mutate().defaultHeaders(headers -> assertEquals(2, headers.size())); - clientFromMutatedBuilder.mutate().defaultCookies(cookies -> assertEquals(2, cookies.size())); + WebClient.Builder builder1a = client1a.mutate(); + builder1a.filters(filters -> assertEquals(2, filters.size())); + builder1a.defaultHeaders(headers -> assertEquals(2, headers.size())); + builder1a.defaultCookies(cookies -> assertEquals(2, cookies.size())); } @Test public void attributes() { + + Map actual = new HashMap<>(); ExchangeFilterFunction filter = (request, next) -> { - assertEquals("bar", request.attributes().get("foo")); + actual.putAll(request.attributes()); return next.exchange(request); }; - WebClient client = builder().filter(filter).build(); + this.builder.filter(filter).build() + .get().uri("/path") + .attribute("foo", "bar") + .exchange(); - client.get().uri("/path").attribute("foo", "bar").exchange(); + assertEquals("bar", actual.get("foo")); } @Test public void apply() { - WebClient client = builder() - .apply(builder -> builder.defaultHeader("Accept", "application/json").defaultCookie("id", "123")) + + WebClient client = this.builder + .apply(builder -> builder + .defaultHeader("Accept", "application/json") + .defaultCookie("id", "123")) .build(); + client.get().uri("/path").exchange(); - ClientRequest request = verifyExchange(); + ClientRequest request = verifyAndGetRequest(); assertEquals("application/json", request.headers().getFirst("Accept")); assertEquals("123", request.cookies().getFirst("id")); verifyNoMoreInteractions(this.exchangeFunction); @@ -189,17 +223,12 @@ public class DefaultWebClientTests { @Test public void switchToErrorOnEmptyClientResponseMono() { - StepVerifier.create(builder().build().get().uri("/path").exchange()) + StepVerifier.create(builder.build().get().uri("/path").exchange()) .expectErrorMessage("The underlying HTTP client completed without emitting a response.") .verify(Duration.ofSeconds(5)); } - - private WebClient.Builder builder() { - return WebClient.builder().baseUrl("/base").exchangeFunction(this.exchangeFunction); - } - - private ClientRequest verifyExchange() { + private ClientRequest verifyAndGetRequest() { ClientRequest request = this.captor.getValue(); Mockito.verify(this.exchangeFunction).exchange(request); verifyNoMoreInteractions(this.exchangeFunction);