Avoid parse cookies when mutating request or response

When mutating a ServerHttpRequest or ClientResponse, the respective
builders no longer access cookies automatically which causes them to
be parsed and does so only if necessary. Likewise re-applying the
read-only HttpHeaders wrapper is avoided.

See gh-24680

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 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.
@@ -20,7 +20,6 @@ import java.net.InetSocketAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.Arrays;
-import java.util.LinkedList;
 import java.util.function.Consumer;
 
 import reactor.core.publisher.Flux;
@@ -31,7 +30,6 @@ import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpMethod;
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
-import org.springframework.util.LinkedMultiValueMap;
 import org.springframework.util.MultiValueMap;
 import org.springframework.util.StringUtils;
 
@@ -46,12 +44,10 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder {
 
 	private URI uri;
 
-	private HttpHeaders httpHeaders;
+	private HttpHeaders headers;
 
 	private String httpMethodValue;
 
-	private final MultiValueMap<String, HttpCookie> cookies;
-
 	@Nullable
 	private String uriPath;
 
@@ -70,21 +66,12 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder {
 		Assert.notNull(original, "ServerHttpRequest is required");
 
 		this.uri = original.getURI();
+		this.headers = HttpHeaders.writableHttpHeaders(original.getHeaders());
 		this.httpMethodValue = original.getMethodValue();
 		this.body = original.getBody();
-
-		this.httpHeaders = HttpHeaders.writableHttpHeaders(original.getHeaders());
-
-		this.cookies = new LinkedMultiValueMap<>(original.getCookies().size());
-		copyMultiValueMap(original.getCookies(), this.cookies);
-
 		this.originalRequest = original;
 	}
 
-	private static <K, V> void copyMultiValueMap(MultiValueMap<K,V> source, MultiValueMap<K,V> target) {
-		source.forEach((key, value) -> target.put(key, new LinkedList<>(value)));
-	}
-
 
 	@Override
 	public ServerHttpRequest.Builder method(HttpMethod httpMethod) {
@@ -113,14 +100,14 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder {
 
 	@Override
 	public ServerHttpRequest.Builder header(String headerName, String... headerValues) {
-		this.httpHeaders.put(headerName, Arrays.asList(headerValues));
+		this.headers.put(headerName, Arrays.asList(headerValues));
 		return this;
 	}
 
 	@Override
 	public ServerHttpRequest.Builder headers(Consumer<HttpHeaders> headersConsumer) {
 		Assert.notNull(headersConsumer, "'headersConsumer' must not be null");
-		headersConsumer.accept(this.httpHeaders);
+		headersConsumer.accept(this.headers);
 		return this;
 	}
 
@@ -132,8 +119,8 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder {
 
 	@Override
 	public ServerHttpRequest build() {
-		return new MutatedServerHttpRequest(getUriToUse(), this.contextPath, this.httpHeaders,
-				this.httpMethodValue, this.cookies, this.sslInfo, this.body, this.originalRequest);
+		return new MutatedServerHttpRequest(getUriToUse(), this.contextPath,
+				this.httpMethodValue, this.sslInfo, this.body, this.originalRequest);
 	}
 
 	private URI getUriToUse() {
@@ -179,8 +166,6 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder {
 
 		private final String methodValue;
 
-		private final MultiValueMap<String, HttpCookie> cookies;
-
 		@Nullable
 		private final SslInfo sslInfo;
 
@@ -190,12 +175,11 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder {
 
 
 		public MutatedServerHttpRequest(URI uri, @Nullable String contextPath,
-				HttpHeaders headers, String methodValue, MultiValueMap<String, HttpCookie> cookies,
-				@Nullable SslInfo sslInfo, Flux<DataBuffer> body, ServerHttpRequest originalRequest) {
+				String methodValue, @Nullable SslInfo sslInfo,
+				Flux<DataBuffer> body, ServerHttpRequest originalRequest) {
 
-			super(uri, contextPath, headers);
+			super(uri, contextPath, originalRequest.getHeaders());
 			this.methodValue = methodValue;
-			this.cookies = cookies;
 			this.sslInfo = sslInfo != null ? sslInfo : originalRequest.getSslInfo();
 			this.body = body;
 			this.originalRequest = originalRequest;
@@ -208,7 +192,7 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder {
 
 		@Override
 		protected MultiValueMap<String, HttpCookie> initCookies() {
-			return this.cookies;
+			return this.originalRequest.getCookies();
 		}
 
 		@Nullable

spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpRequestTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2019 the original author or authors.
+ * Copyright 2002-2020 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.
@@ -49,13 +49,13 @@ public class ServerHttpRequestTests {
 
 	@Test
 	public void queryParamsNone() throws Exception {
-		MultiValueMap<String, String> params = createHttpRequest("/path").getQueryParams();
+		MultiValueMap<String, String> params = createRequest("/path").getQueryParams();
 		assertThat(params.size()).isEqualTo(0);
 	}
 
 	@Test
 	public void queryParams() throws Exception {
-		MultiValueMap<String, String> params = createHttpRequest("/path?a=A&b=B").getQueryParams();
+		MultiValueMap<String, String> params = createRequest("/path?a=A&b=B").getQueryParams();
 		assertThat(params.size()).isEqualTo(2);
 		assertThat(params.get("a")).isEqualTo(Collections.singletonList("A"));
 		assertThat(params.get("b")).isEqualTo(Collections.singletonList("B"));
@@ -63,84 +63,87 @@ public class ServerHttpRequestTests {
 
 	@Test
 	public void queryParamsWithMultipleValues() throws Exception {
-		MultiValueMap<String, String> params = createHttpRequest("/path?a=1&a=2").getQueryParams();
+		MultiValueMap<String, String> params = createRequest("/path?a=1&a=2").getQueryParams();
 		assertThat(params.size()).isEqualTo(1);
 		assertThat(params.get("a")).isEqualTo(Arrays.asList("1", "2"));
 	}
 
 	@Test  // SPR-15140
 	public void queryParamsWithEncodedValue() throws Exception {
-		MultiValueMap<String, String> params = createHttpRequest("/path?a=%20%2B+%C3%A0").getQueryParams();
+		MultiValueMap<String, String> params = createRequest("/path?a=%20%2B+%C3%A0").getQueryParams();
 		assertThat(params.size()).isEqualTo(1);
 		assertThat(params.get("a")).isEqualTo(Collections.singletonList(" + \u00e0"));
 	}
 
 	@Test
 	public void queryParamsWithEmptyValue() throws Exception {
-		MultiValueMap<String, String> params = createHttpRequest("/path?a=").getQueryParams();
+		MultiValueMap<String, String> params = createRequest("/path?a=").getQueryParams();
 		assertThat(params.size()).isEqualTo(1);
 		assertThat(params.get("a")).isEqualTo(Collections.singletonList(""));
 	}
 
 	@Test
 	public void queryParamsWithNoValue() throws Exception {
-		MultiValueMap<String, String> params = createHttpRequest("/path?a").getQueryParams();
+		MultiValueMap<String, String> params = createRequest("/path?a").getQueryParams();
 		assertThat(params.size()).isEqualTo(1);
 		assertThat(params.get("a")).isEqualTo(Collections.singletonList(null));
 	}
 
 	@Test
-	public void mutateRequest() throws Exception {
+	public void mutateRequestMethod() throws Exception {
+		ServerHttpRequest request = createRequest("/").mutate().method(HttpMethod.DELETE).build();
+		assertThat(request.getMethod()).isEqualTo(HttpMethod.DELETE);
+	}
+
+	@Test
+	public void mutateSslInfo() throws Exception {
 		SslInfo sslInfo = mock(SslInfo.class);
-		ServerHttpRequest request = createHttpRequest("/").mutate().sslInfo(sslInfo).build();
+		ServerHttpRequest request = createRequest("/").mutate().sslInfo(sslInfo).build();
 		assertThat(request.getSslInfo()).isSameAs(sslInfo);
+	}
 
-		request = createHttpRequest("/").mutate().method(HttpMethod.DELETE).build();
-		assertThat(request.getMethod()).isEqualTo(HttpMethod.DELETE);
-
+	@Test
+	public void mutateUriAndPath() throws Exception {
 		String baseUri = "https://aaa.org:8080/a";
 
-		request = createHttpRequest(baseUri).mutate().uri(URI.create("https://bbb.org:9090/b")).build();
+		ServerHttpRequest request = createRequest(baseUri).mutate().uri(URI.create("https://bbb.org:9090/b")).build();
 		assertThat(request.getURI().toString()).isEqualTo("https://bbb.org:9090/b");
 
-		request = createHttpRequest(baseUri).mutate().path("/b/c/d").build();
+		request = createRequest(baseUri).mutate().path("/b/c/d").build();
 		assertThat(request.getURI().toString()).isEqualTo("https://aaa.org:8080/b/c/d");
 
-		request = createHttpRequest(baseUri).mutate().path("/app/b/c/d").contextPath("/app").build();
+		request = createRequest(baseUri).mutate().path("/app/b/c/d").contextPath("/app").build();
 		assertThat(request.getURI().toString()).isEqualTo("https://aaa.org:8080/app/b/c/d");
 		assertThat(request.getPath().contextPath().value()).isEqualTo("/app");
 	}
 
-	@Test
-	public void mutateWithInvalidPath() throws Exception {
-		assertThatIllegalArgumentException().isThrownBy(() ->
-				createHttpRequest("/").mutate().path("foo-bar"));
-	}
-
 	@Test  // SPR-16434
 	public void mutatePathWithEncodedQueryParams() throws Exception {
-		ServerHttpRequest request = createHttpRequest("/path?name=%E6%89%8E%E6%A0%B9");
+		ServerHttpRequest request = createRequest("/path?name=%E6%89%8E%E6%A0%B9");
 		request = request.mutate().path("/mutatedPath").build();
 
 		assertThat(request.getURI().getRawPath()).isEqualTo("/mutatedPath");
 		assertThat(request.getURI().getRawQuery()).isEqualTo("name=%E6%89%8E%E6%A0%B9");
 	}
 
+	@Test
+	public void mutateWithInvalidPath() {
+		assertThatIllegalArgumentException().isThrownBy(() -> createRequest("/").mutate().path("foo-bar"));
+	}
+
 	@Test
 	public void mutateHeadersViaConsumer() throws Exception {
 		String headerName = "key";
 		String headerValue1 = "value1";
 		String headerValue2 = "value2";
 
-		ServerHttpRequest request = createHttpRequest("/path");
+		ServerHttpRequest request = createRequest("/path");
 		assertThat(request.getHeaders().get(headerName)).isNull();
 
 		request = request.mutate().headers(headers -> headers.add(headerName, headerValue1)).build();
-
 		assertThat(request.getHeaders().get(headerName)).containsExactly(headerValue1);
 
 		request = request.mutate().headers(headers -> headers.add(headerName, headerValue2)).build();
-
 		assertThat(request.getHeaders().get(headerName)).containsExactly(headerValue1, headerValue2);
 	}
 
@@ -151,19 +154,17 @@ public class ServerHttpRequestTests {
 		String headerValue2 = "value2";
 		String headerValue3 = "value3";
 
-		ServerHttpRequest request = createHttpRequest("/path");
+		ServerHttpRequest request = createRequest("/path");
 		assertThat(request.getHeaders().get(headerName)).isNull();
 
 		request = request.mutate().header(headerName, headerValue1, headerValue2).build();
-
 		assertThat(request.getHeaders().get(headerName)).containsExactly(headerValue1, headerValue2);
 
 		request = request.mutate().header(headerName, headerValue3).build();
-
 		assertThat(request.getHeaders().get(headerName)).containsExactly(headerValue3);
 	}
 
-	private ServerHttpRequest createHttpRequest(String uriString) throws Exception {
+	private ServerHttpRequest createRequest(String uriString) throws Exception {
 		URI uri = URI.create(uriString);
 		MockHttpServletRequest request = new TestHttpServletRequest(uri);
 		AsyncContext asyncContext = new MockAsyncContext(request, new MockHttpServletResponse());

spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java

@@ -31,6 +31,7 @@ import org.springframework.http.HttpRequest;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseCookie;
 import org.springframework.http.client.reactive.ClientHttpResponse;
+import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.LinkedMultiValueMap;
@@ -69,12 +70,17 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder {
 
 	private int statusCode = 200;
 
-	private final HttpHeaders headers;
+	@Nullable
+	private HttpHeaders headers;
 
-	private final MultiValueMap<String, ResponseCookie> cookies;
+	@Nullable
+	private MultiValueMap<String, ResponseCookie> cookies;
 
 	private Flux<DataBuffer> body = Flux.empty();
 
+	@Nullable
+	private ClientResponse originalResponse;
+
 	private HttpRequest request;
 
 
@@ -91,14 +97,13 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder {
 		this.strategies = other.strategies();
 		this.statusCode = other.rawStatusCode();
 		if (mutate) {
-			this.headers = HttpHeaders.writableHttpHeaders(other.headers().asHttpHeaders());
 			this.body = other.bodyToFlux(DataBuffer.class);
 		}
 		else {
 			this.headers = new HttpHeaders();
-			headers(headers -> headers.addAll(other.headers().asHttpHeaders()));
+			this.headers.addAll(other.headers().asHttpHeaders());
 		}
-		this.cookies = new LinkedMultiValueMap<>(other.cookies());
+		this.originalResponse = other;
 		this.request = (other instanceof DefaultClientResponse ?
 				((DefaultClientResponse) other).request() : EMPTY_REQUEST);
 	}
@@ -119,31 +124,47 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder {
 	@Override
 	public ClientResponse.Builder header(String headerName, String... headerValues) {
 		for (String headerValue : headerValues) {
-			this.headers.add(headerName, headerValue);
+			getHeaders().add(headerName, headerValue);
 		}
 		return this;
 	}
 
 	@Override
 	public ClientResponse.Builder headers(Consumer<HttpHeaders> headersConsumer) {
-		headersConsumer.accept(this.headers);
+		headersConsumer.accept(getHeaders());
 		return this;
 	}
 
+	@SuppressWarnings("ConstantConditions")
+	private HttpHeaders getHeaders() {
+		if (this.headers == null) {
+			this.headers = HttpHeaders.writableHttpHeaders(this.originalResponse.headers().asHttpHeaders());
+		}
+		return this.headers;
+	}
+
 	@Override
 	public DefaultClientResponseBuilder cookie(String name, String... values) {
 		for (String value : values) {
-			this.cookies.add(name, ResponseCookie.from(name, value).build());
+			getCookies().add(name, ResponseCookie.from(name, value).build());
 		}
 		return this;
 	}
 
 	@Override
 	public ClientResponse.Builder cookies(Consumer<MultiValueMap<String, ResponseCookie>> cookiesConsumer) {
-		cookiesConsumer.accept(this.cookies);
+		cookiesConsumer.accept(getCookies());
 		return this;
 	}
 
+	@SuppressWarnings("ConstantConditions")
+	private MultiValueMap<String, ResponseCookie> getCookies() {
+		if (this.cookies == null) {
+			this.cookies = new LinkedMultiValueMap<>(this.originalResponse.cookies());
+		}
+		return this.cookies;
+	}
+
 	@Override
 	public ClientResponse.Builder body(Function<Flux<DataBuffer>, Flux<DataBuffer>> transformer) {
 		this.body = transformer.apply(this.body);
@@ -184,8 +205,8 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder {
 	@Override
 	public ClientResponse build() {
 
-		ClientHttpResponse httpResponse =
-				new BuiltClientHttpResponse(this.statusCode, this.headers, this.cookies, this.body);
+		ClientHttpResponse httpResponse = new BuiltClientHttpResponse(
+				this.statusCode, this.headers, this.cookies, this.body, this.originalResponse);
 
 		return new DefaultClientResponse(
 				httpResponse, this.strategies, "", "", () -> this.request);
@@ -196,19 +217,33 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder {
 
 		private final int statusCode;
 
+		@Nullable
 		private final HttpHeaders headers;
 
+		@Nullable
 		private final MultiValueMap<String, ResponseCookie> cookies;
 
 		private final Flux<DataBuffer> body;
 
-		public BuiltClientHttpResponse(int statusCode, HttpHeaders headers,
-				MultiValueMap<String, ResponseCookie> cookies, Flux<DataBuffer> body) {
+		@Nullable
+		private final ClientResponse originalResponse;
+
+
+		public BuiltClientHttpResponse(int statusCode, @Nullable HttpHeaders headers,
+				@Nullable MultiValueMap<String, ResponseCookie> cookies, Flux<DataBuffer> body,
+				@Nullable ClientResponse originalResponse) {
+
+			Assert.isTrue(headers != null || originalResponse != null,
+					"Expected either headers or an original response with headers.");
+
+			Assert.isTrue(cookies != null || originalResponse != null,
+					"Expected either cookies or an original response with cookies.");
 
 			this.statusCode = statusCode;
-			this.headers = HttpHeaders.readOnlyHttpHeaders(headers);
-			this.cookies = CollectionUtils.unmodifiableMultiValueMap(cookies);
+			this.headers = (headers != null ? HttpHeaders.readOnlyHttpHeaders(headers) : null);
+			this.cookies = (cookies != null ? CollectionUtils.unmodifiableMultiValueMap(cookies) : null);
 			this.body = body;
+			this.originalResponse = originalResponse;
 		}
 
 		@Override
@@ -222,13 +257,15 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder {
 		}
 
 		@Override
+		@SuppressWarnings("ConstantConditions")
 		public HttpHeaders getHeaders() {
-			return this.headers;
+			return (this.headers != null ? this.headers : this.originalResponse.headers().asHttpHeaders());
 		}
 
 		@Override
+		@SuppressWarnings("ConstantConditions")
 		public MultiValueMap<String, ResponseCookie> getCookies() {
-			return this.cookies;
+			return (this.cookies != null ? this.cookies : this.originalResponse.cookies());
 		}
 
 		@Override