From 3276f818519f9bde187c4ccb83653c21c2e0e7fb Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 8 May 2020 10:34:26 +0100 Subject: [PATCH 1/6] Avoid one layer of HttpHeaders wrapping See gh-24680 --- .../org/springframework/http/HttpEntity.java | 8 ++--- .../reactive/AbstractServerHttpRequest.java | 4 +-- .../reactive/JettyHttpHandlerAdapter.java | 7 ++-- .../reactive/ReactorServerHttpRequest.java | 8 +---- .../reactive/ServletServerHttpRequest.java | 35 +++++++++++-------- .../reactive/TomcatHttpHandlerAdapter.java | 6 ++-- .../reactive/UndertowServerHttpRequest.java | 7 +--- .../result/view/DefaultRendering.java | 2 +- 8 files changed, 34 insertions(+), 43 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/HttpEntity.java b/spring-web/src/main/java/org/springframework/http/HttpEntity.java index 35b2b97caf..7b7fe9af60 100644 --- a/spring-web/src/main/java/org/springframework/http/HttpEntity.java +++ b/spring-web/src/main/java/org/springframework/http/HttpEntity.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 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. @@ -99,11 +99,7 @@ public class HttpEntity { */ public HttpEntity(@Nullable T body, @Nullable MultiValueMap headers) { this.body = body; - HttpHeaders tempHeaders = new HttpHeaders(); - if (headers != null) { - tempHeaders.putAll(headers); - } - this.headers = HttpHeaders.readOnlyHttpHeaders(tempHeaders); + this.headers = HttpHeaders.readOnlyHttpHeaders(headers != null ? headers : new HttpHeaders()); } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpRequest.java index 4f4d628c68..f9d7b54a22 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpRequest.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. @@ -76,7 +76,7 @@ public abstract class AbstractServerHttpRequest implements ServerHttpRequest { * @param contextPath the context path for the request * @param headers the headers for the request */ - public AbstractServerHttpRequest(URI uri, @Nullable String contextPath, HttpHeaders headers) { + public AbstractServerHttpRequest(URI uri, @Nullable String contextPath, MultiValueMap headers) { this.uri = uri; this.path = RequestPath.parse(uri, contextPath); this.headers = HttpHeaders.readOnlyHttpHeaders(headers); diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHttpHandlerAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHttpHandlerAdapter.java index e0c538bc47..22842c3554 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHttpHandlerAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHttpHandlerAdapter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 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. @@ -36,6 +36,7 @@ import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; import org.springframework.util.Assert; +import org.springframework.util.MultiValueMap; /** * {@link ServletHttpHandlerAdapter} extension that uses Jetty APIs for writing @@ -79,9 +80,9 @@ public class JettyHttpHandlerAdapter extends ServletHttpHandlerAdapter { super(createHeaders(request), request, asyncContext, servletPath, bufferFactory, bufferSize); } - private static HttpHeaders createHeaders(HttpServletRequest request) { + private static MultiValueMap createHeaders(HttpServletRequest request) { HttpFields fields = ((Request) request).getMetaData().getFields(); - return new HttpHeaders(new JettyHeadersAdapter(fields)); + return new JettyHeadersAdapter(fields); } } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java index 9cd57c08c0..8935787753 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java @@ -33,7 +33,6 @@ import reactor.netty.http.server.HttpServerRequest; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.NettyDataBufferFactory; import org.springframework.http.HttpCookie; -import org.springframework.http.HttpHeaders; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.LinkedMultiValueMap; @@ -59,7 +58,7 @@ class ReactorServerHttpRequest extends AbstractServerHttpRequest { public ReactorServerHttpRequest(HttpServerRequest request, NettyDataBufferFactory bufferFactory) throws URISyntaxException { - super(initUri(request), "", initHeaders(request)); + super(initUri(request), "", new NettyHeadersAdapter(request.requestHeaders())); Assert.notNull(bufferFactory, "DataBufferFactory must not be null"); this.request = request; this.bufferFactory = bufferFactory; @@ -127,11 +126,6 @@ class ReactorServerHttpRequest extends AbstractServerHttpRequest { return uri; } - private static HttpHeaders initHeaders(HttpServerRequest channel) { - NettyHeadersAdapter headersMap = new NettyHeadersAdapter(channel.requestHeaders()); - return new HttpHeaders(headersMap); - } - @Override public String getMethodValue() { diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java index 6ea6e9404d..867ac93f3e 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.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. @@ -23,6 +23,7 @@ import java.net.URISyntaxException; import java.nio.charset.Charset; import java.security.cert.X509Certificate; import java.util.Enumeration; +import java.util.Locale; import java.util.Map; import javax.servlet.AsyncContext; @@ -44,6 +45,7 @@ import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import org.springframework.util.CollectionUtils; import org.springframework.util.LinkedCaseInsensitiveMap; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; @@ -77,8 +79,8 @@ class ServletServerHttpRequest extends AbstractServerHttpRequest { this(createDefaultHttpHeaders(request), request, asyncContext, servletPath, bufferFactory, bufferSize); } - public ServletServerHttpRequest(HttpHeaders headers, HttpServletRequest request, AsyncContext asyncContext, - String servletPath, DataBufferFactory bufferFactory, int bufferSize) + public ServletServerHttpRequest(MultiValueMap headers, HttpServletRequest request, + AsyncContext asyncContext, String servletPath, DataBufferFactory bufferFactory, int bufferSize) throws IOException, URISyntaxException { super(initUri(request), request.getContextPath() + servletPath, initHeaders(headers, request)); @@ -99,8 +101,9 @@ class ServletServerHttpRequest extends AbstractServerHttpRequest { } - private static HttpHeaders createDefaultHttpHeaders(HttpServletRequest request) { - HttpHeaders headers = new HttpHeaders(); + private static MultiValueMap createDefaultHttpHeaders(HttpServletRequest request) { + MultiValueMap headers = + CollectionUtils.toMultiValueMap(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH)); for (Enumeration names = request.getHeaderNames(); names.hasMoreElements(); ) { String name = (String) names.nextElement(); for (Enumeration values = request.getHeaders(name); values.hasMoreElements(); ) { @@ -120,34 +123,36 @@ class ServletServerHttpRequest extends AbstractServerHttpRequest { return new URI(url.toString()); } - private static HttpHeaders initHeaders(HttpHeaders headers, HttpServletRequest request) { - MediaType contentType = headers.getContentType(); - if (contentType == null) { + private static MultiValueMap initHeaders( + MultiValueMap headerValues, HttpServletRequest request) { + + HttpHeaders headers = null; + MediaType contentType = null; + if (!StringUtils.hasLength(headerValues.getFirst(HttpHeaders.CONTENT_TYPE))) { String requestContentType = request.getContentType(); if (StringUtils.hasLength(requestContentType)) { contentType = MediaType.parseMediaType(requestContentType); + headers = new HttpHeaders(headerValues); headers.setContentType(contentType); } } if (contentType != null && contentType.getCharset() == null) { String encoding = request.getCharacterEncoding(); if (StringUtils.hasLength(encoding)) { - Charset charset = Charset.forName(encoding); Map params = new LinkedCaseInsensitiveMap<>(); params.putAll(contentType.getParameters()); - params.put("charset", charset.toString()); - headers.setContentType( - new MediaType(contentType.getType(), contentType.getSubtype(), - params)); + params.put("charset", Charset.forName(encoding).toString()); + headers.setContentType(new MediaType(contentType, params)); } } - if (headers.getContentLength() == -1) { + if (headerValues.getFirst(HttpHeaders.CONTENT_TYPE) == null) { int contentLength = request.getContentLength(); if (contentLength != -1) { + headers = (headers != null ? headers : new HttpHeaders(headerValues)); headers.setContentLength(contentLength); } } - return headers; + return (headers != null ? headers : headerValues); } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java index 783be84a52..7920c7ffd8 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java @@ -44,6 +44,7 @@ import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.http.HttpHeaders; import org.springframework.http.MediaType; import org.springframework.util.Assert; +import org.springframework.util.MultiValueMap; import org.springframework.util.ReflectionUtils; /** @@ -105,14 +106,13 @@ public class TomcatHttpHandlerAdapter extends ServletHttpHandlerAdapter { this.bufferSize = bufferSize; } - private static HttpHeaders createTomcatHttpHeaders(HttpServletRequest request) { + private static MultiValueMap createTomcatHttpHeaders(HttpServletRequest request) { RequestFacade requestFacade = getRequestFacade(request); org.apache.catalina.connector.Request connectorRequest = (org.apache.catalina.connector.Request) ReflectionUtils.getField(COYOTE_REQUEST_FIELD, requestFacade); Assert.state(connectorRequest != null, "No Tomcat connector request"); Request tomcatRequest = connectorRequest.getCoyoteRequest(); - TomcatHeadersAdapter headers = new TomcatHeadersAdapter(tomcatRequest.getMimeHeaders()); - return new HttpHeaders(headers); + return new TomcatHeadersAdapter(tomcatRequest.getMimeHeaders()); } private static RequestFacade getRequestFacade(HttpServletRequest request) { diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java b/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java index 11b4418951..045670a344 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java @@ -38,7 +38,6 @@ import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.core.io.buffer.DataBufferWrapper; import org.springframework.core.io.buffer.PooledDataBuffer; import org.springframework.http.HttpCookie; -import org.springframework.http.HttpHeaders; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.LinkedMultiValueMap; @@ -63,7 +62,7 @@ class UndertowServerHttpRequest extends AbstractServerHttpRequest { public UndertowServerHttpRequest(HttpServerExchange exchange, DataBufferFactory bufferFactory) throws URISyntaxException { - super(initUri(exchange), "", initHeaders(exchange)); + super(initUri(exchange), "", new UndertowHeadersAdapter(exchange.getRequestHeaders())); this.exchange = exchange; this.body = new RequestBodyPublisher(exchange, bufferFactory); this.body.registerListeners(exchange); @@ -77,10 +76,6 @@ class UndertowServerHttpRequest extends AbstractServerHttpRequest { return new URI(requestUriAndQuery); } - private static HttpHeaders initHeaders(HttpServerExchange exchange) { - return new HttpHeaders(new UndertowHeadersAdapter(exchange.getRequestHeaders())); - } - @Override public String getMethodValue() { return this.exchange.getRequestMethod().toString(); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRendering.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRendering.java index 30a53013db..a05e653c49 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRendering.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRendering.java @@ -32,7 +32,7 @@ import org.springframework.ui.Model; */ class DefaultRendering implements Rendering { - private static final HttpHeaders EMPTY_HEADERS = HttpHeaders.readOnlyHttpHeaders(new HttpHeaders()); + private static final HttpHeaders EMPTY_HEADERS = HttpHeaders.EMPTY; private final Object view; From df99889aa6f45988a5879356d89e644f926c85af Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 8 May 2020 14:07:30 +0100 Subject: [PATCH 2/6] Avoided repeated creation of ReadOnlyHttpHeaders wrapper See gh-24680 --- .../org/springframework/http/HttpHeaders.java | 4 ++- .../client/AbstractClientHttpRequest.java | 17 ++++++++++-- .../reactive/AbstractClientHttpRequest.java | 17 +++++++++--- .../server/ServletServerHttpResponse.java | 16 +++++++++-- .../reactive/AbstractServerHttpResponse.java | 15 +++++++++-- .../client/DefaultClientResponse.java | 13 +++++---- .../client/DefaultWebClientBuilder.java | 8 ++---- .../function/server/DefaultServerRequest.java | 27 +++++++++---------- .../client/DefaultClientResponseTests.java | 18 +++++-------- .../function/DefaultServerRequest.java | 26 +++++++++--------- 10 files changed, 99 insertions(+), 62 deletions(-) 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 ff1e9b0cbd..8f2e0ba14c 100644 --- a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java @@ -1769,7 +1769,9 @@ public class HttpHeaders implements MultiValueMap, Serializable /** - * Apply a read-only {@code HttpHeaders} wrapper around the given headers. + * Apply a read-only {@code HttpHeaders} wrapper around the given headers + * that also caches the parsed representations of the "Accept" and + * "Content-Type" headers. */ public static HttpHeaders readOnlyHttpHeaders(MultiValueMap headers) { Assert.notNull(headers, "HttpHeaders must not be null"); diff --git a/spring-web/src/main/java/org/springframework/http/client/AbstractClientHttpRequest.java b/spring-web/src/main/java/org/springframework/http/client/AbstractClientHttpRequest.java index 2848d75f52..f964e66e80 100644 --- a/spring-web/src/main/java/org/springframework/http/client/AbstractClientHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/client/AbstractClientHttpRequest.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 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,6 +20,7 @@ import java.io.IOException; import java.io.OutputStream; import org.springframework.http.HttpHeaders; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; /** @@ -35,10 +36,22 @@ public abstract class AbstractClientHttpRequest implements ClientHttpRequest { private boolean executed = false; + @Nullable + private HttpHeaders readOnlyHeaders; + @Override public final HttpHeaders getHeaders() { - return (this.executed ? HttpHeaders.readOnlyHttpHeaders(this.headers) : this.headers); + if (this.readOnlyHeaders != null) { + return this.readOnlyHeaders; + } + else if (this.executed) { + this.readOnlyHeaders = HttpHeaders.readOnlyHttpHeaders(this.headers); + return this.readOnlyHeaders; + } + else { + return this.headers; + } } @Override diff --git a/spring-web/src/main/java/org/springframework/http/client/reactive/AbstractClientHttpRequest.java b/spring-web/src/main/java/org/springframework/http/client/reactive/AbstractClientHttpRequest.java index 5414dd7a62..8734521f04 100644 --- a/spring-web/src/main/java/org/springframework/http/client/reactive/AbstractClientHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/client/reactive/AbstractClientHttpRequest.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 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. @@ -60,6 +60,9 @@ public abstract class AbstractClientHttpRequest implements ClientHttpRequest { private final List>> commitActions = new ArrayList<>(4); + @Nullable + private HttpHeaders readOnlyHeaders; + public AbstractClientHttpRequest() { this(new HttpHeaders()); @@ -74,10 +77,16 @@ public abstract class AbstractClientHttpRequest implements ClientHttpRequest { @Override public HttpHeaders getHeaders() { - if (State.COMMITTED.equals(this.state.get())) { - return HttpHeaders.readOnlyHttpHeaders(this.headers); + if (this.readOnlyHeaders != null) { + return this.readOnlyHeaders; + } + else if (State.COMMITTED.equals(this.state.get())) { + this.readOnlyHeaders = HttpHeaders.readOnlyHttpHeaders(this.headers); + return this.readOnlyHeaders; + } + else { + return this.headers; } - return this.headers; } @Override diff --git a/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpResponse.java index d718971e68..45758ec0d9 100644 --- a/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpResponse.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 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. @@ -47,6 +47,9 @@ public class ServletServerHttpResponse implements ServerHttpResponse { private boolean bodyUsed = false; + @Nullable + private HttpHeaders readOnlyHeaders; + /** * Construct a new instance of the ServletServerHttpResponse based on the given {@link HttpServletResponse}. @@ -74,7 +77,16 @@ public class ServletServerHttpResponse implements ServerHttpResponse { @Override public HttpHeaders getHeaders() { - return (this.headersWritten ? HttpHeaders.readOnlyHttpHeaders(this.headers) : this.headers); + if (this.readOnlyHeaders != null) { + return this.readOnlyHeaders; + } + else if (this.headersWritten) { + this.readOnlyHeaders = HttpHeaders.readOnlyHttpHeaders(this.headers); + return this.readOnlyHeaders; + } + else { + return this.headers; + } } @Override diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java index b6c7f98037..517a2436ba 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java @@ -75,6 +75,9 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { private final List>> commitActions = new ArrayList<>(4); + @Nullable + private HttpHeaders readOnlyHeaders; + public AbstractServerHttpResponse(DataBufferFactory dataBufferFactory) { this(dataBufferFactory, new HttpHeaders()); @@ -155,8 +158,16 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { @Override public HttpHeaders getHeaders() { - return (this.state.get() == State.COMMITTED ? - HttpHeaders.readOnlyHttpHeaders(this.headers) : this.headers); + if (this.readOnlyHeaders != null) { + return this.readOnlyHeaders; + } + else if (this.state.get() == State.COMMITTED) { + this.readOnlyHeaders = HttpHeaders.readOnlyHttpHeaders(this.headers); + return this.readOnlyHeaders; + } + else { + return this.headers; + } } @Override diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java index f818e47568..53d6c7fc3c 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java @@ -234,29 +234,28 @@ class DefaultClientResponse implements ClientResponse { private class DefaultHeaders implements Headers { - private HttpHeaders delegate() { - return response.getHeaders(); - } + private final HttpHeaders httpHeaders = + HttpHeaders.readOnlyHttpHeaders(response.getHeaders()); @Override public OptionalLong contentLength() { - return toOptionalLong(delegate().getContentLength()); + return toOptionalLong(this.httpHeaders.getContentLength()); } @Override public Optional contentType() { - return Optional.ofNullable(delegate().getContentType()); + return Optional.ofNullable(this.httpHeaders.getContentType()); } @Override public List header(String headerName) { - List headerValues = delegate().get(headerName); + List headerValues = this.httpHeaders.get(headerName); return (headerValues != null ? headerValues : Collections.emptyList()); } @Override public HttpHeaders asHttpHeaders() { - return HttpHeaders.readOnlyHttpHeaders(delegate()); + return this.httpHeaders; } private OptionalLong toOptionalLong(long value) { 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 fed9093abd..0fa80f70dc 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 @@ -265,8 +265,8 @@ final class DefaultWebClientBuilder implements WebClient.Builder { .map(filter -> filter.apply(exchange)) .orElse(exchange) : exchange); return new DefaultWebClient(filteredExchange, initUriBuilderFactory(), - this.defaultHeaders != null ? unmodifiableCopy(this.defaultHeaders) : null, - this.defaultCookies != null ? unmodifiableCopy(this.defaultCookies) : null, + this.defaultHeaders != null ? HttpHeaders.readOnlyHttpHeaders(this.defaultHeaders) : null, + this.defaultCookies != null ? HttpHeaders.readOnlyHttpHeaders(this.defaultCookies) : null, this.defaultRequest, new DefaultWebClientBuilder(this)); } @@ -308,10 +308,6 @@ final class DefaultWebClientBuilder implements WebClient.Builder { return factory; } - private static HttpHeaders unmodifiableCopy(HttpHeaders headers) { - return HttpHeaders.readOnlyHttpHeaders(headers); - } - private static MultiValueMap unmodifiableCopy(MultiValueMap map) { return CollectionUtils.unmodifiableMultiValueMap(new LinkedMultiValueMap<>(map)); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java index e1b12d8858..8ea642a90d 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java @@ -260,61 +260,60 @@ class DefaultServerRequest implements ServerRequest { private class DefaultHeaders implements Headers { - - private HttpHeaders delegate() { - return request().getHeaders(); - } + + private final HttpHeaders httpHeaders = + HttpHeaders.readOnlyHttpHeaders(request().getHeaders()); @Override public List accept() { - return delegate().getAccept(); + return this.httpHeaders.getAccept(); } @Override public List acceptCharset() { - return delegate().getAcceptCharset(); + return this.httpHeaders.getAcceptCharset(); } @Override public List acceptLanguage() { - return delegate().getAcceptLanguage(); + return this.httpHeaders.getAcceptLanguage(); } @Override public OptionalLong contentLength() { - long value = delegate().getContentLength(); + long value = this.httpHeaders.getContentLength(); return (value != -1 ? OptionalLong.of(value) : OptionalLong.empty()); } @Override public Optional contentType() { - return Optional.ofNullable(delegate().getContentType()); + return Optional.ofNullable(this.httpHeaders.getContentType()); } @Override public InetSocketAddress host() { - return delegate().getHost(); + return this.httpHeaders.getHost(); } @Override public List range() { - return delegate().getRange(); + return this.httpHeaders.getRange(); } @Override public List header(String headerName) { - List headerValues = delegate().get(headerName); + List headerValues = this.httpHeaders.get(headerName); return (headerValues != null ? headerValues : Collections.emptyList()); } @Override public HttpHeaders asHttpHeaders() { - return HttpHeaders.readOnlyHttpHeaders(delegate()); + return this.httpHeaders; } @Override public String toString() { - return delegate().toString(); + return this.httpHeaders.toString(); } } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java index ed05d7b2d7..5fe2940400 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.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. @@ -60,6 +60,8 @@ public class DefaultClientResponseTests { private ClientHttpResponse mockResponse; + private final HttpHeaders httpHeaders = new HttpHeaders(); + private ExchangeStrategies mockExchangeStrategies; private DefaultClientResponse defaultClientResponse; @@ -68,6 +70,7 @@ public class DefaultClientResponseTests { @BeforeEach public void createMocks() { mockResponse = mock(ClientHttpResponse.class); + given(mockResponse.getHeaders()).willReturn(this.httpHeaders); mockExchangeStrategies = mock(ExchangeStrategies.class); defaultClientResponse = new DefaultClientResponse(mockResponse, mockExchangeStrategies, "", "", () -> null); } @@ -91,7 +94,6 @@ public class DefaultClientResponseTests { @Test public void header() { - HttpHeaders httpHeaders = new HttpHeaders(); long contentLength = 42L; httpHeaders.setContentLength(contentLength); MediaType contentType = MediaType.TEXT_PLAIN; @@ -233,7 +235,6 @@ public class DefaultClientResponseTests { = factory.wrap(ByteBuffer.wrap("foo".getBytes(StandardCharsets.UTF_8))); Flux body = Flux.just(dataBuffer); - HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.setContentType(MediaType.TEXT_PLAIN); given(mockResponse.getHeaders()).willReturn(httpHeaders); given(mockResponse.getStatusCode()).willThrow(new IllegalArgumentException("999")); @@ -293,13 +294,12 @@ public class DefaultClientResponseTests { } @Test - public void toEntityListWithUnknownStatusCode() throws Exception { + public void toEntityListWithUnknownStatusCode() { DefaultDataBufferFactory factory = new DefaultDataBufferFactory(); DefaultDataBuffer dataBuffer = factory.wrap(ByteBuffer.wrap("foo".getBytes(StandardCharsets.UTF_8))); Flux body = Flux.just(dataBuffer); - HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.setContentType(MediaType.TEXT_PLAIN); given(mockResponse.getHeaders()).willReturn(httpHeaders); given(mockResponse.getStatusCode()).willThrow(new IllegalArgumentException("999")); @@ -321,8 +321,7 @@ public class DefaultClientResponseTests { @Test public void toEntityListTypeReference() { DefaultDataBufferFactory factory = new DefaultDataBufferFactory(); - DefaultDataBuffer dataBuffer = - factory.wrap(ByteBuffer.wrap("foo".getBytes(StandardCharsets.UTF_8))); + DefaultDataBuffer dataBuffer = factory.wrap(ByteBuffer.wrap("foo".getBytes(StandardCharsets.UTF_8))); Flux body = Flux.just(dataBuffer); mockTextPlainResponse(body); @@ -332,8 +331,7 @@ public class DefaultClientResponseTests { given(mockExchangeStrategies.messageReaders()).willReturn(messageReaders); ResponseEntity> result = defaultClientResponse.toEntityList( - new ParameterizedTypeReference() { - }).block(); + new ParameterizedTypeReference() {}).block(); assertThat(result.getBody()).isEqualTo(Collections.singletonList("foo")); assertThat(result.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(result.getStatusCodeValue()).isEqualTo(HttpStatus.OK.value()); @@ -342,9 +340,7 @@ public class DefaultClientResponseTests { private void mockTextPlainResponse(Flux body) { - HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.setContentType(MediaType.TEXT_PLAIN); - given(mockResponse.getHeaders()).willReturn(httpHeaders); given(mockResponse.getStatusCode()).willReturn(HttpStatus.OK); given(mockResponse.getRawStatusCode()).willReturn(HttpStatus.OK.value()); given(mockResponse.getBody()).willReturn(body); diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java index a3f5fe8f4a..d5405ef9e8 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java @@ -305,62 +305,62 @@ class DefaultServerRequest implements ServerRequest { */ static class DefaultRequestHeaders implements Headers { - private final HttpHeaders delegate; + private final HttpHeaders httpHeaders; - public DefaultRequestHeaders(HttpHeaders delegate) { - this.delegate = delegate; + public DefaultRequestHeaders(HttpHeaders httpHeaders) { + this.httpHeaders = HttpHeaders.readOnlyHttpHeaders(httpHeaders); } @Override public List accept() { - return this.delegate.getAccept(); + return this.httpHeaders.getAccept(); } @Override public List acceptCharset() { - return this.delegate.getAcceptCharset(); + return this.httpHeaders.getAcceptCharset(); } @Override public List acceptLanguage() { - return this.delegate.getAcceptLanguage(); + return this.httpHeaders.getAcceptLanguage(); } @Override public OptionalLong contentLength() { - long value = this.delegate.getContentLength(); + long value = this.httpHeaders.getContentLength(); return (value != -1 ? OptionalLong.of(value) : OptionalLong.empty()); } @Override public Optional contentType() { - return Optional.ofNullable(this.delegate.getContentType()); + return Optional.ofNullable(this.httpHeaders.getContentType()); } @Override public InetSocketAddress host() { - return this.delegate.getHost(); + return this.httpHeaders.getHost(); } @Override public List range() { - return this.delegate.getRange(); + return this.httpHeaders.getRange(); } @Override public List header(String headerName) { - List headerValues = this.delegate.get(headerName); + List headerValues = this.httpHeaders.get(headerName); return (headerValues != null ? headerValues : Collections.emptyList()); } @Override public HttpHeaders asHttpHeaders() { - return HttpHeaders.readOnlyHttpHeaders(this.delegate); + return this.httpHeaders; } @Override public String toString() { - return this.delegate.toString(); + return this.httpHeaders.toString(); } } From c908ec19378f3c6e8aac30d877fbf230fe2bc3c4 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 8 May 2020 14:11:37 +0100 Subject: [PATCH 3/6] Wrappers for native headers on the client side Native server request headers have been wrapped rather than copied since 5.1. This commit applies the same change to the client side where, to make matters worse, headers were copied repeatedly on every access, see also previous commit ca897b95. For Netty and Jetty the wrappers are the same as on the server side but duplicated in order to remain package private. See gh-24680 --- .../HttpComponentsClientHttpResponse.java | 11 +- .../HttpComponentsHeadersAdapter.java | 240 ++++++++++++++++++ .../reactive/JettyClientHttpResponse.java | 13 +- .../client/reactive/JettyHeadersAdapter.java | 230 +++++++++++++++++ .../client/reactive/NettyHeadersAdapter.java | 229 +++++++++++++++++ .../reactive/ReactorClientHttpResponse.java | 13 +- .../server/reactive/JettyHeadersAdapter.java | 4 +- .../server/reactive/NettyHeadersAdapter.java | 4 +- 8 files changed, 727 insertions(+), 17 deletions(-) create mode 100644 spring-web/src/main/java/org/springframework/http/client/reactive/HttpComponentsHeadersAdapter.java create mode 100644 spring-web/src/main/java/org/springframework/http/client/reactive/JettyHeadersAdapter.java create mode 100644 spring-web/src/main/java/org/springframework/http/client/reactive/NettyHeadersAdapter.java diff --git a/spring-web/src/main/java/org/springframework/http/client/reactive/HttpComponentsClientHttpResponse.java b/spring-web/src/main/java/org/springframework/http/client/reactive/HttpComponentsClientHttpResponse.java index 2ae593121e..7be76e78bd 100644 --- a/spring-web/src/main/java/org/springframework/http/client/reactive/HttpComponentsClientHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/client/reactive/HttpComponentsClientHttpResponse.java @@ -17,7 +17,6 @@ package org.springframework.http.client.reactive; import java.nio.ByteBuffer; -import java.util.Arrays; import java.util.concurrent.atomic.AtomicBoolean; import org.apache.hc.client5.http.cookie.Cookie; @@ -49,6 +48,8 @@ class HttpComponentsClientHttpResponse implements ClientHttpResponse { private final Message> message; + private final HttpHeaders headers; + private final HttpClientContext context; private final AtomicBoolean rejectSubscribers = new AtomicBoolean(); @@ -61,6 +62,9 @@ class HttpComponentsClientHttpResponse implements ClientHttpResponse { this.dataBufferFactory = dataBufferFactory; this.message = message; this.context = context; + + MultiValueMap adapter = new HttpComponentsHeadersAdapter(message.getHead()); + this.headers = HttpHeaders.readOnlyHttpHeaders(adapter); } @@ -107,9 +111,6 @@ class HttpComponentsClientHttpResponse implements ClientHttpResponse { @Override public HttpHeaders getHeaders() { - return Arrays.stream(this.message.getHead().getHeaders()) - .collect(HttpHeaders::new, - (httpHeaders, header) -> httpHeaders.add(header.getName(), header.getValue()), - HttpHeaders::putAll); + return this.headers; } } diff --git a/spring-web/src/main/java/org/springframework/http/client/reactive/HttpComponentsHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/client/reactive/HttpComponentsHeadersAdapter.java new file mode 100644 index 0000000000..7880f6e0e4 --- /dev/null +++ b/spring-web/src/main/java/org/springframework/http/client/reactive/HttpComponentsHeadersAdapter.java @@ -0,0 +1,240 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.http.client.reactive; + +import java.util.AbstractSet; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HttpResponse; + +import org.springframework.http.HttpHeaders; +import org.springframework.lang.Nullable; +import org.springframework.util.MultiValueMap; + +/** + * {@code MultiValueMap} implementation for wrapping Apache HttpComponents + * HttpClient headers. + * + * @author Rossen Stoyanchev + * @since 5.3 + */ +class HttpComponentsHeadersAdapter implements MultiValueMap { + + private final HttpResponse response; + + + HttpComponentsHeadersAdapter(HttpResponse response) { + this.response = response; + } + + + @Override + public String getFirst(String key) { + Header header = this.response.getFirstHeader(key); + return (header != null ? header.getValue() : null); + } + + @Override + public void add(String key, @Nullable String value) { + this.response.addHeader(key, value); + } + + @Override + public void addAll(String key, List values) { + values.forEach(value -> add(key, value)); + } + + @Override + public void addAll(MultiValueMap values) { + values.forEach(this::addAll); + } + + @Override + public void set(String key, @Nullable String value) { + this.response.setHeader(key, value); + } + + @Override + public void setAll(Map values) { + values.forEach(this::set); + } + + @Override + public Map toSingleValueMap() { + Map map = new LinkedHashMap<>(size()); + this.response.headerIterator().forEachRemaining(h -> map.putIfAbsent(h.getName(), h.getValue())); + return map; + } + + @Override + public int size() { + return this.response.getHeaders().length; + } + + @Override + public boolean isEmpty() { + return (this.response.getHeaders().length == 0); + } + + @Override + public boolean containsKey(Object key) { + return (key instanceof String && this.response.containsHeader((String) key)); + } + + @Override + public boolean containsValue(Object value) { + return (value instanceof String && + Arrays.stream(this.response.getHeaders()).anyMatch(h -> h.getValue().equals(value))); + } + + @Nullable + @Override + public List get(Object key) { + List values = null; + if (containsKey(key)) { + Header[] headers = this.response.getHeaders((String) key); + values = new ArrayList<>(headers.length); + for (Header header : headers) { + values.add(header.getValue()); + } + } + return values; + } + + @Nullable + @Override + public List put(String key, List values) { + List oldValues = remove(key); + values.forEach(value -> add(key, value)); + return oldValues; + } + + @Nullable + @Override + public List remove(Object key) { + if (key instanceof String) { + List oldValues = get(key); + this.response.removeHeaders((String) key); + return oldValues; + } + return null; + } + + @Override + public void putAll(Map> map) { + map.forEach(this::put); + } + + @Override + public void clear() { + this.response.setHeaders(); + } + + @Override + public Set keySet() { + Set keys = new LinkedHashSet<>(size()); + for (Header header : this.response.getHeaders()) { + keys.add(header.getName()); + } + return keys; + } + + @Override + public Collection> values() { + Collection> values = new ArrayList<>(size()); + for (Header header : this.response.getHeaders()) { + values.add(get(header.getName())); + } + return values; + } + + @Override + public Set>> entrySet() { + return new AbstractSet>>() { + @Override + public Iterator>> iterator() { + return new EntryIterator(); + } + + @Override + public int size() { + return HttpComponentsHeadersAdapter.this.size(); + } + }; + } + + + @Override + public String toString() { + return HttpHeaders.formatHeaders(this); + } + + + private class EntryIterator implements Iterator>> { + + private Iterator
iterator = response.headerIterator(); + + @Override + public boolean hasNext() { + return this.iterator.hasNext(); + } + + @Override + public Entry> next() { + return new HeaderEntry(this.iterator.next().getName()); + } + } + + + private class HeaderEntry implements Entry> { + + private final String key; + + HeaderEntry(String key) { + this.key = key; + } + + @Override + public String getKey() { + return this.key; + } + + @Override + public List getValue() { + List values = HttpComponentsHeadersAdapter.this.get(this.key); + return values != null ? values : Collections.emptyList(); + } + + @Override + public List setValue(List value) { + List previousValues = getValue(); + HttpComponentsHeadersAdapter.this.put(this.key, value); + return previousValues; + } + } + +} diff --git a/spring-web/src/main/java/org/springframework/http/client/reactive/JettyClientHttpResponse.java b/spring-web/src/main/java/org/springframework/http/client/reactive/JettyClientHttpResponse.java index 67c6a7aced..29683498e8 100644 --- a/spring-web/src/main/java/org/springframework/http/client/reactive/JettyClientHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/client/reactive/JettyClientHttpResponse.java @@ -36,7 +36,8 @@ import org.springframework.util.MultiValueMap; * * @author Sebastien Deleuze * @since 5.1 - * @see Jetty ReactiveStreams HttpClient + * @see + * Jetty ReactiveStreams HttpClient */ class JettyClientHttpResponse implements ClientHttpResponse { @@ -44,10 +45,15 @@ class JettyClientHttpResponse implements ClientHttpResponse { private final Flux content; + private final HttpHeaders headers; + public JettyClientHttpResponse(ReactiveResponse reactiveResponse, Publisher content) { this.reactiveResponse = reactiveResponse; this.content = Flux.from(content); + + MultiValueMap adapter = new JettyHeadersAdapter(reactiveResponse.getHeaders()); + this.headers = HttpHeaders.readOnlyHttpHeaders(adapter); } @@ -86,10 +92,7 @@ class JettyClientHttpResponse implements ClientHttpResponse { @Override public HttpHeaders getHeaders() { - HttpHeaders headers = new HttpHeaders(); - this.reactiveResponse.getHeaders().stream() - .forEach(field -> headers.add(field.getName(), field.getValue())); - return headers; + return this.headers; } } diff --git a/spring-web/src/main/java/org/springframework/http/client/reactive/JettyHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/client/reactive/JettyHeadersAdapter.java new file mode 100644 index 0000000000..dd2544f585 --- /dev/null +++ b/spring-web/src/main/java/org/springframework/http/client/reactive/JettyHeadersAdapter.java @@ -0,0 +1,230 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.http.client.reactive; + +import java.util.AbstractSet; +import java.util.Collection; +import java.util.Enumeration; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import org.eclipse.jetty.http.HttpField; +import org.eclipse.jetty.http.HttpFields; + +import org.springframework.http.HttpHeaders; +import org.springframework.lang.Nullable; +import org.springframework.util.MultiValueMap; + +/** + * {@code MultiValueMap} implementation for wrapping Jetty HTTP headers. + * + *

There is a duplicate of this class in the server package! + * + * @author Rossen Stoyanchev + * @since 5.3 + */ +class JettyHeadersAdapter implements MultiValueMap { + + private final HttpFields headers; + + + JettyHeadersAdapter(HttpFields headers) { + this.headers = headers; + } + + + @Override + public String getFirst(String key) { + return this.headers.get(key); + } + + @Override + public void add(String key, @Nullable String value) { + this.headers.add(key, value); + } + + @Override + public void addAll(String key, List values) { + values.forEach(value -> add(key, value)); + } + + @Override + public void addAll(MultiValueMap values) { + values.forEach(this::addAll); + } + + @Override + public void set(String key, @Nullable String value) { + this.headers.put(key, value); + } + + @Override + public void setAll(Map values) { + values.forEach(this::set); + } + + @Override + public Map toSingleValueMap() { + Map singleValueMap = new LinkedHashMap<>(this.headers.size()); + Iterator iterator = this.headers.iterator(); + iterator.forEachRemaining(field -> { + if (!singleValueMap.containsKey(field.getName())) { + singleValueMap.put(field.getName(), field.getValue()); + } + }); + return singleValueMap; + } + + @Override + public int size() { + return this.headers.getFieldNamesCollection().size(); + } + + @Override + public boolean isEmpty() { + return (this.headers.size() == 0); + } + + @Override + public boolean containsKey(Object key) { + return (key instanceof String && this.headers.containsKey((String) key)); + } + + @Override + public boolean containsValue(Object value) { + return (value instanceof String && + this.headers.stream().anyMatch(field -> field.contains((String) value))); + } + + @Nullable + @Override + public List get(Object key) { + if (containsKey(key)) { + return this.headers.getValuesList((String) key); + } + return null; + } + + @Nullable + @Override + public List put(String key, List value) { + List oldValues = get(key); + this.headers.put(key, value); + return oldValues; + } + + @Nullable + @Override + public List remove(Object key) { + if (key instanceof String) { + List oldValues = get(key); + this.headers.remove((String) key); + return oldValues; + } + return null; + } + + @Override + public void putAll(Map> map) { + map.forEach(this::put); + } + + @Override + public void clear() { + this.headers.clear(); + } + + @Override + public Set keySet() { + return this.headers.getFieldNamesCollection(); + } + + @Override + public Collection> values() { + return this.headers.getFieldNamesCollection().stream() + .map(this.headers::getValuesList).collect(Collectors.toList()); + } + + @Override + public Set>> entrySet() { + return new AbstractSet>>() { + @Override + public Iterator>> iterator() { + return new EntryIterator(); + } + + @Override + public int size() { + return headers.size(); + } + }; + } + + + @Override + public String toString() { + return HttpHeaders.formatHeaders(this); + } + + + private class EntryIterator implements Iterator>> { + + private Enumeration names = headers.getFieldNames(); + + @Override + public boolean hasNext() { + return this.names.hasMoreElements(); + } + + @Override + public Entry> next() { + return new HeaderEntry(this.names.nextElement()); + } + } + + + private class HeaderEntry implements Entry> { + + private final String key; + + HeaderEntry(String key) { + this.key = key; + } + + @Override + public String getKey() { + return this.key; + } + + @Override + public List getValue() { + return headers.getValuesList(this.key); + } + + @Override + public List setValue(List value) { + List previousValues = headers.getValuesList(this.key); + headers.put(this.key, value); + return previousValues; + } + } + +} diff --git a/spring-web/src/main/java/org/springframework/http/client/reactive/NettyHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/client/reactive/NettyHeadersAdapter.java new file mode 100644 index 0000000000..811d72837a --- /dev/null +++ b/spring-web/src/main/java/org/springframework/http/client/reactive/NettyHeadersAdapter.java @@ -0,0 +1,229 @@ +/* + * 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.http.client.reactive; + +import java.util.AbstractSet; +import java.util.Collection; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; + +import io.netty.handler.codec.http.HttpHeaders; + +import org.springframework.lang.Nullable; +import org.springframework.util.MultiValueMap; + +/** + * {@code MultiValueMap} implementation for wrapping Netty HTTP headers. + * + *

There is a duplicate of this class in the server package! + * + * @author Rossen Stoyanchev + * @since 5.3 + */ +class NettyHeadersAdapter implements MultiValueMap { + + private final HttpHeaders headers; + + + NettyHeadersAdapter(HttpHeaders headers) { + this.headers = headers; + } + + + @Override + @Nullable + public String getFirst(String key) { + return this.headers.get(key); + } + + @Override + public void add(String key, @Nullable String value) { + this.headers.add(key, value); + } + + @Override + public void addAll(String key, List values) { + this.headers.add(key, values); + } + + @Override + public void addAll(MultiValueMap values) { + values.forEach(this.headers::add); + } + + @Override + public void set(String key, @Nullable String value) { + this.headers.set(key, value); + } + + @Override + public void setAll(Map values) { + values.forEach(this.headers::set); + } + + @Override + public Map toSingleValueMap() { + Map singleValueMap = new LinkedHashMap<>(this.headers.size()); + this.headers.entries() + .forEach(entry -> { + if (!singleValueMap.containsKey(entry.getKey())) { + singleValueMap.put(entry.getKey(), entry.getValue()); + } + }); + return singleValueMap; + } + + @Override + public int size() { + return this.headers.names().size(); + } + + @Override + public boolean isEmpty() { + return this.headers.isEmpty(); + } + + @Override + public boolean containsKey(Object key) { + return (key instanceof String && this.headers.contains((String) key)); + } + + @Override + public boolean containsValue(Object value) { + return (value instanceof String && + this.headers.entries().stream() + .anyMatch(entry -> value.equals(entry.getValue()))); + } + + @Override + @Nullable + public List get(Object key) { + if (containsKey(key)) { + return this.headers.getAll((String) key); + } + return null; + } + + @Nullable + @Override + public List put(String key, @Nullable List value) { + List previousValues = this.headers.getAll(key); + this.headers.set(key, value); + return previousValues; + } + + @Nullable + @Override + public List remove(Object key) { + if (key instanceof String) { + List previousValues = this.headers.getAll((String) key); + this.headers.remove((String) key); + return previousValues; + } + return null; + } + + @Override + public void putAll(Map> map) { + map.forEach(this.headers::add); + } + + @Override + public void clear() { + this.headers.clear(); + } + + @Override + public Set keySet() { + return this.headers.names(); + } + + @Override + public Collection> values() { + return this.headers.names().stream() + .map(this.headers::getAll).collect(Collectors.toList()); + } + + @Override + public Set>> entrySet() { + return new AbstractSet>>() { + @Override + public Iterator>> iterator() { + return new EntryIterator(); + } + + @Override + public int size() { + return headers.size(); + } + }; + } + + + @Override + public String toString() { + return org.springframework.http.HttpHeaders.formatHeaders(this); + } + + + private class EntryIterator implements Iterator>> { + + private Iterator names = headers.names().iterator(); + + @Override + public boolean hasNext() { + return this.names.hasNext(); + } + + @Override + public Entry> next() { + return new HeaderEntry(this.names.next()); + } + } + + + private class HeaderEntry implements Entry> { + + private final String key; + + HeaderEntry(String key) { + this.key = key; + } + + @Override + public String getKey() { + return this.key; + } + + @Override + public List getValue() { + return headers.getAll(this.key); + } + + @Override + public List setValue(List value) { + List previousValues = headers.getAll(this.key); + headers.set(this.key, value); + return previousValues; + } + } + +} diff --git a/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpResponse.java b/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpResponse.java index d8a1ef0398..00a6a0dc3d 100644 --- a/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/client/reactive/ReactorClientHttpResponse.java @@ -42,12 +42,14 @@ import org.springframework.util.MultiValueMap; */ class ReactorClientHttpResponse implements ClientHttpResponse { - private final NettyDataBufferFactory bufferFactory; - private final HttpClientResponse response; private final NettyInbound inbound; + private final NettyDataBufferFactory bufferFactory; + + private final HttpHeaders headers; + private final AtomicBoolean rejectSubscribers = new AtomicBoolean(); @@ -55,6 +57,9 @@ class ReactorClientHttpResponse implements ClientHttpResponse { this.response = response; this.inbound = inbound; this.bufferFactory = new NettyDataBufferFactory(alloc); + + MultiValueMap adapter = new NettyHeadersAdapter(response.responseHeaders()); + this.headers = HttpHeaders.readOnlyHttpHeaders(adapter); } @@ -81,9 +86,7 @@ class ReactorClientHttpResponse implements ClientHttpResponse { @Override public HttpHeaders getHeaders() { - HttpHeaders headers = new HttpHeaders(); - this.response.responseHeaders().entries().forEach(e -> headers.add(e.getKey(), e.getValue())); - return headers; + return this.headers; } @Override diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java index 198e549bb6..98fd9db212 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 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. @@ -36,6 +36,8 @@ import org.springframework.util.MultiValueMap; /** * {@code MultiValueMap} implementation for wrapping Jetty HTTP headers. * + *

There is a duplicate of this class in the client package! + * * @author Brian Clozel * @since 5.1.1 */ diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/NettyHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/NettyHeadersAdapter.java index c32cb5426f..d69795b086 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/NettyHeadersAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/NettyHeadersAdapter.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 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. @@ -33,6 +33,8 @@ import org.springframework.util.MultiValueMap; /** * {@code MultiValueMap} implementation for wrapping Netty HTTP headers. * + *

There is a duplicate of this class in the client package! + * * @author Brian Clozel * @since 5.1.1 */ From 67a06f5edcc8697af0941e238ef29bdb2a73245d Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 8 May 2020 19:05:43 +0100 Subject: [PATCH 4/6] Add mutate() to ClientResponse and deprecate from() from() has the flaw of ignoring the body and it can't be fixed because applications are guaranteed to be setting it already and if set twice the builder drains the first body. mutate() is a better fit in any case for what needs to be done in a filter chain. It can be done more efficiently and is consistent with similar options on the server side. See gh-24680 --- .../function/client/ClientResponse.java | 40 +++++++++++++++---- .../client/DefaultClientResponseBuilder.java | 40 ++++++++++++------- .../client/ExchangeFilterFunctions.java | 14 +++---- .../function/server/DefaultServerRequest.java | 2 +- .../DefaultClientResponseBuilderTests.java | 34 +++++++--------- 5 files changed, 78 insertions(+), 52 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java index 801de88faf..6116fad2ae 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ClientResponse.java @@ -21,6 +21,7 @@ import java.util.List; import java.util.Optional; import java.util.OptionalLong; import java.util.function.Consumer; +import java.util.function.Function; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; @@ -215,16 +216,32 @@ public interface ClientResponse { */ String logPrefix(); + /** + * Return a builder to mutate the this response, for example to change + * the status, headers, cookies, and replace or transform the body. + * @return a builder to mutate the request with + * @since 5.3 + */ + default Builder mutate() { + return new DefaultClientResponseBuilder(this, true); + } + // Static builder methods /** * Create a builder with the status, headers, and cookies of the given response. + *

Note: Note that the body in the returned builder is + * {@link Flux#empty()} by default. To carry over the one from the original + * response, use {@code otherResponse.bodyToFlux(DataBuffer.class)} or + * simply use the instance based {@link #mutate()} method. * @param other the response to copy the status, headers, and cookies from * @return the created builder + * @deprecated as of 5.3 in favor of the instance based {@link #mutate()}. */ + @Deprecated static Builder from(ClientResponse other) { - return new DefaultClientResponseBuilder(other); + return new DefaultClientResponseBuilder(other, false); } /** @@ -371,19 +388,26 @@ public interface ClientResponse { Builder cookies(Consumer> cookiesConsumer); /** - * Set the body of the response. Calling this methods will - * {@linkplain org.springframework.core.io.buffer.DataBufferUtils#release(DataBuffer) release} - * the existing body of the builder. - * @param body the new body. + * Transform the response body, if set in the builder. + * @param transformer the transformation function to use + * @return this builder + * @since 5.3 + */ + Builder body(Function, Flux> transformer); + + /** + * Set the body of the response. + *

Note: This methods will drain the existing body, + * if set in the builder. + * @param body the new body to use * @return this builder */ Builder body(Flux body); /** * Set the body of the response to the UTF-8 encoded bytes of the given string. - * Calling this methods will - * {@linkplain org.springframework.core.io.buffer.DataBufferUtils#release(DataBuffer) release} - * the existing body of the builder. + *

Note: This methods will drain the existing body, + * if set in the builder. * @param body the new body. * @return this builder */ 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 3c43175d04..45ad32585d 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 @@ -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. @@ -19,11 +19,11 @@ package org.springframework.web.reactive.function.client; import java.net.URI; import java.nio.charset.StandardCharsets; import java.util.function.Consumer; +import java.util.function.Function; import reactor.core.publisher.Flux; import org.springframework.core.io.buffer.DataBuffer; -import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.core.io.buffer.DefaultDataBufferFactory; import org.springframework.http.HttpHeaders; @@ -69,9 +69,9 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder { private int statusCode = 200; - private final HttpHeaders headers = new HttpHeaders(); + private final HttpHeaders headers; - private final MultiValueMap cookies = new LinkedMultiValueMap<>(); + private final MultiValueMap cookies; private Flux body = Flux.empty(); @@ -81,21 +81,26 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder { public DefaultClientResponseBuilder(ExchangeStrategies strategies) { Assert.notNull(strategies, "ExchangeStrategies must not be null"); this.strategies = strategies; + this.headers = new HttpHeaders(); + this.cookies = new LinkedMultiValueMap<>(); this.request = EMPTY_REQUEST; } - public DefaultClientResponseBuilder(ClientResponse other) { + public DefaultClientResponseBuilder(ClientResponse other, boolean mutate) { Assert.notNull(other, "ClientResponse must not be null"); this.strategies = other.strategies(); this.statusCode = other.rawStatusCode(); - headers(headers -> headers.addAll(other.headers().asHttpHeaders())); - cookies(cookies -> cookies.addAll(other.cookies())); - if (other instanceof DefaultClientResponse) { - this.request = ((DefaultClientResponse) other).request(); + if (mutate) { + this.headers = HttpHeaders.writableHttpHeaders(other.headers().asHttpHeaders()); + this.body = other.bodyToFlux(DataBuffer.class); } else { - this.request = EMPTY_REQUEST; + this.headers = new HttpHeaders(); + headers(headers -> headers.addAll(other.headers().asHttpHeaders())); } + this.cookies = new LinkedMultiValueMap<>(other.cookies()); + this.request = (other instanceof DefaultClientResponse ? + ((DefaultClientResponse) other).request() : EMPTY_REQUEST); } @@ -139,6 +144,12 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder { return this; } + @Override + public ClientResponse.Builder body(Function, Flux> transformer) { + this.body = transformer.apply(this.body); + return this; + } + @Override public ClientResponse.Builder body(Flux body) { Assert.notNull(body, "Body must not be null"); @@ -151,11 +162,10 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder { public ClientResponse.Builder body(String body) { Assert.notNull(body, "Body must not be null"); releaseBody(); - DataBufferFactory dataBufferFactory = new DefaultDataBufferFactory(); this.body = Flux.just(body). map(s -> { byte[] bytes = body.getBytes(StandardCharsets.UTF_8); - return dataBufferFactory.wrap(bytes); + return new DefaultDataBufferFactory().wrap(bytes); }); return this; } @@ -173,12 +183,12 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder { @Override public ClientResponse build() { + ClientHttpResponse httpResponse = new BuiltClientHttpResponse(this.statusCode, this.headers, this.cookies, this.body); - // When building ClientResponse manually, the ClientRequest.logPrefix() has to be passed, - // e.g. via ClientResponse.Builder, but this (builder) is not used currently. - return new DefaultClientResponse(httpResponse, this.strategies, "", "", () -> this.request); + return new DefaultClientResponse( + httpResponse, this.strategies, "", "", () -> this.request); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ExchangeFilterFunctions.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ExchangeFilterFunctions.java index baeb77a9c8..b7d6c3d720 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ExchangeFilterFunctions.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ExchangeFilterFunctions.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. @@ -22,16 +22,13 @@ import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; -import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; -import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.lang.Nullable; import org.springframework.util.Assert; -import org.springframework.web.reactive.function.BodyExtractors; /** * Static factory methods providing access to built-in implementations of @@ -64,11 +61,10 @@ public abstract class ExchangeFilterFunctions { */ public static ExchangeFilterFunction limitResponseSize(long maxByteCount) { return (request, next) -> - next.exchange(request).map(response -> { - Flux body = response.body(BodyExtractors.toDataBuffers()); - body = DataBufferUtils.takeUntilByteCount(body, maxByteCount); - return ClientResponse.from(response).body(body).build(); - }); + next.exchange(request).map(response -> + response.mutate() + .body(body -> DataBufferUtils.takeUntilByteCount(body, maxByteCount)) + .build()); } /** diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java index 8ea642a90d..ea94cf843c 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java @@ -260,7 +260,7 @@ class DefaultServerRequest implements ServerRequest { private class DefaultHeaders implements Headers { - + private final HttpHeaders httpHeaders = HttpHeaders.readOnlyHttpHeaders(request().getHeaders()); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilderTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilderTests.java index 8e6c01ef63..70cebb42f4 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilderTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilderTests.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. @@ -64,42 +64,38 @@ public class DefaultClientResponseBuilderTests { } @Test - public void from() { - Flux otherBody = Flux.just("foo", "bar") - .map(s -> s.getBytes(StandardCharsets.UTF_8)) - .map(dataBufferFactory::wrap); + public void mutate() { - ClientResponse other = ClientResponse.create(HttpStatus.BAD_REQUEST, ExchangeStrategies.withDefaults()) + ClientResponse originalResponse = ClientResponse + .create(HttpStatus.BAD_REQUEST, ExchangeStrategies.withDefaults()) .header("foo", "bar") + .header("bar", "baz") .cookie("baz", "qux") - .body(otherBody) + .body(Flux.just("foobar".getBytes(StandardCharsets.UTF_8)).map(dataBufferFactory::wrap)) .build(); - Flux body = Flux.just("baz") - .map(s -> s.getBytes(StandardCharsets.UTF_8)) - .map(dataBufferFactory::wrap); - - ClientResponse result = ClientResponse.from(other) - .headers(httpHeaders -> httpHeaders.set("foo", "baar")) + ClientResponse result = originalResponse.mutate() + .statusCode(HttpStatus.OK) + .headers(headers -> headers.set("foo", "baar")) .cookies(cookies -> cookies.set("baz", ResponseCookie.from("baz", "quux").build())) - .body(body) .build(); - assertThat(result.statusCode()).isEqualTo(HttpStatus.BAD_REQUEST); - assertThat(result.headers().asHttpHeaders().size()).isEqualTo(1); + assertThat(result.statusCode()).isEqualTo(HttpStatus.OK); + assertThat(result.headers().asHttpHeaders().size()).isEqualTo(2); assertThat(result.headers().asHttpHeaders().getFirst("foo")).isEqualTo("baar"); + assertThat(result.headers().asHttpHeaders().getFirst("bar")).isEqualTo("baz"); assertThat(result.cookies().size()).isEqualTo(1); assertThat(result.cookies().getFirst("baz").getValue()).isEqualTo("quux"); StepVerifier.create(result.bodyToFlux(String.class)) - .expectNext("baz") + .expectNext("foobar") .verifyComplete(); } @Test - public void fromCustomStatus() { + public void mutateWithCustomStatus() { ClientResponse other = ClientResponse.create(499, ExchangeStrategies.withDefaults()).build(); - ClientResponse result = ClientResponse.from(other).build(); + ClientResponse result = other.mutate().build(); assertThat(result.rawStatusCode()).isEqualTo(499); assertThatIllegalArgumentException().isThrownBy(result::statusCode); From 0e9ecb6c99471a1efa4deb40149e4ddc48e33b92 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 8 May 2020 21:40:39 +0100 Subject: [PATCH 5/6] Shortcut handling of bodyToFlux(DataBuffer.class) Given that the body is a Flux there probably could be a Flux body(); At least bodyToFlux(DataBuffer.class) which is used when mutating and is a common case should not incur overhead. See gh-24680 --- .../client/DefaultClientResponse.java | 40 +++++++++++-------- .../function/server/DefaultServerRequest.java | 5 ++- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java index 53d6c7fc3c..94eeb137f2 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java @@ -30,6 +30,7 @@ import reactor.core.publisher.Mono; import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.codec.Hints; +import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpRequest; @@ -66,6 +67,8 @@ class DefaultClientResponse implements ClientResponse { private final Supplier requestSupplier; + private final BodyExtractor.Context bodyExtractorContext; + public DefaultClientResponse(ClientHttpResponse response, ExchangeStrategies strategies, String logPrefix, String requestDescription, Supplier requestSupplier) { @@ -76,6 +79,22 @@ class DefaultClientResponse implements ClientResponse { this.logPrefix = logPrefix; this.requestDescription = requestDescription; this.requestSupplier = requestSupplier; + this.bodyExtractorContext = new BodyExtractor.Context() { + @Override + public List> messageReaders() { + return strategies.messageReaders(); + } + + @Override + public Optional serverResponse() { + return Optional.empty(); + } + + @Override + public Map hints() { + return Hints.from(Hints.LOG_PREFIX_HINT, logPrefix); + } + }; } @@ -107,22 +126,7 @@ class DefaultClientResponse implements ClientResponse { @SuppressWarnings("unchecked") @Override public T body(BodyExtractor extractor) { - T result = extractor.extract(this.response, new BodyExtractor.Context() { - @Override - public List> messageReaders() { - return strategies.messageReaders(); - } - - @Override - public Optional serverResponse() { - return Optional.empty(); - } - - @Override - public Map hints() { - return Hints.from(Hints.LOG_PREFIX_HINT, logPrefix); - } - }); + T result = extractor.extract(this.response, this.bodyExtractorContext); String description = "Body from " + this.requestDescription + " [DefaultClientResponse]"; if (result instanceof Mono) { return (T) ((Mono) result).checkpoint(description); @@ -146,8 +150,10 @@ class DefaultClientResponse implements ClientResponse { } @Override + @SuppressWarnings("unchecked") public Flux bodyToFlux(Class elementClass) { - return body(BodyExtractors.toFlux(elementClass)); + return elementClass.equals(DataBuffer.class) ? + (Flux) body(BodyExtractors.toDataBuffers()) : body(BodyExtractors.toFlux(elementClass)); } @Override diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java index ea94cf843c..c98f151fac 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java @@ -36,6 +36,7 @@ import reactor.core.publisher.Mono; import org.springframework.core.ParameterizedTypeReference; import org.springframework.core.codec.DecodingException; import org.springframework.core.codec.Hints; +import org.springframework.core.io.buffer.DataBuffer; import org.springframework.http.HttpCookie; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpRange; @@ -195,8 +196,10 @@ class DefaultServerRequest implements ServerRequest { } @Override + @SuppressWarnings("unchecked") public Flux bodyToFlux(Class elementClass) { - Flux flux = body(BodyExtractors.toFlux(elementClass)); + Flux flux = (elementClass.equals(DataBuffer.class) ? + (Flux) request().getBody() : body(BodyExtractors.toFlux(elementClass))); return flux.onErrorMap(UnsupportedMediaTypeException.class, ERROR_MAPPER) .onErrorMap(DecodingException.class, DECODING_MAPPER); } From 94824e30a4a8f23e4882b556d16904421958d8cc Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Sat, 9 May 2020 09:49:44 +0100 Subject: [PATCH 6/6] 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 --- .../DefaultServerHttpRequestBuilder.java | 38 +++------- .../reactive/ServerHttpRequestTests.java | 61 ++++++++-------- .../client/DefaultClientResponseBuilder.java | 71 ++++++++++++++----- 3 files changed, 96 insertions(+), 74 deletions(-) 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 ce5a3d2b37..e664157ac0 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 @@ -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 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 void copyMultiValueMap(MultiValueMap source, MultiValueMap 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 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 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 cookies, - @Nullable SslInfo sslInfo, Flux body, ServerHttpRequest originalRequest) { + String methodValue, @Nullable SslInfo sslInfo, + Flux 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 initCookies() { - return this.cookies; + return this.originalRequest.getCookies(); } @Nullable diff --git a/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpRequestTests.java b/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpRequestTests.java index c6917031bc..8662d28c0a 100644 --- a/spring-web/src/test/java/org/springframework/http/server/reactive/ServerHttpRequestTests.java +++ b/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 params = createHttpRequest("/path").getQueryParams(); + MultiValueMap params = createRequest("/path").getQueryParams(); assertThat(params.size()).isEqualTo(0); } @Test public void queryParams() throws Exception { - MultiValueMap params = createHttpRequest("/path?a=A&b=B").getQueryParams(); + MultiValueMap 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 params = createHttpRequest("/path?a=1&a=2").getQueryParams(); + MultiValueMap 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 params = createHttpRequest("/path?a=%20%2B+%C3%A0").getQueryParams(); + MultiValueMap 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 params = createHttpRequest("/path?a=").getQueryParams(); + MultiValueMap params = createRequest("/path?a=").getQueryParams(); assertThat(params.size()).isEqualTo(1); assertThat(params.get("a")).isEqualTo(Collections.singletonList("")); } @Test public void queryParamsWithNoValue() throws Exception { - MultiValueMap params = createHttpRequest("/path?a").getQueryParams(); + MultiValueMap params = createRequest("/path?a").getQueryParams(); assertThat(params.size()).isEqualTo(1); assertThat(params.get("a")).isEqualTo(Collections.singletonList(null)); } @Test - public void mutateRequest() throws Exception { - SslInfo sslInfo = mock(SslInfo.class); - ServerHttpRequest request = createHttpRequest("/").mutate().sslInfo(sslInfo).build(); - assertThat(request.getSslInfo()).isSameAs(sslInfo); - - request = createHttpRequest("/").mutate().method(HttpMethod.DELETE).build(); + 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 = createRequest("/").mutate().sslInfo(sslInfo).build(); + assertThat(request.getSslInfo()).isSameAs(sslInfo); + } + + @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()); 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 45ad32585d..f3fe50ea75 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 @@ -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 cookies; + @Nullable + private MultiValueMap cookies; private Flux 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 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> cookiesConsumer) { - cookiesConsumer.accept(this.cookies); + cookiesConsumer.accept(getCookies()); return this; } + @SuppressWarnings("ConstantConditions") + private MultiValueMap getCookies() { + if (this.cookies == null) { + this.cookies = new LinkedMultiValueMap<>(this.originalResponse.cookies()); + } + return this.cookies; + } + @Override public ClientResponse.Builder body(Function, Flux> 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 cookies; private final Flux body; - public BuiltClientHttpResponse(int statusCode, HttpHeaders headers, - MultiValueMap cookies, Flux body) { + @Nullable + private final ClientResponse originalResponse; + + + public BuiltClientHttpResponse(int statusCode, @Nullable HttpHeaders headers, + @Nullable MultiValueMap cookies, Flux 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 getCookies() { - return this.cookies; + return (this.cookies != null ? this.cookies : this.originalResponse.cookies()); } @Override