Polish default headers/attributes in WebClient

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<String, String> result = new LinkedMultiValueMap<>();
+				result.putAll(defaultCookies);
 				result.putAll(this.cookies);
-				defaultCookies.forEach(result::putIfAbsent);
 				return result;
 			}
 		}

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 <K, V> MultiValueMap<K, V> unmodifiableCopy(@Nullable MultiValueMap<K, V> 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 <K, V> MultiValueMap<K, V> unmodifiableCopy(MultiValueMap<K, V> map) {
+		return CollectionUtils.unmodifiableMultiValueMap(new LinkedMultiValueMap<>(map));
 	}
 
 	@Override

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<HttpHeaders> 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<MultiValueMap<String, String>> 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<MultiValueMap<String, String>> 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<HttpHeaders> 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<Map<String, Object>> attributesConsumer);

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<Void> 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<String, Object> 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);