From 6eb0e9e44bc7e1dc4fefe8a68231d8e3c0d7a6f8 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 11 Sep 2019 21:47:07 +0100 Subject: [PATCH 1/4] Unwrap decorated request or response Closes: gh-23598 --- .../upgrade/JettyRequestUpgradeStrategy.java | 37 ++++++++++++++----- .../ReactorNettyRequestUpgradeStrategy.java | 18 ++++++++- .../upgrade/TomcatRequestUpgradeStrategy.java | 37 ++++++++++++++----- .../UndertowRequestUpgradeStrategy.java | 18 +++++++-- 4 files changed, 87 insertions(+), 23 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/JettyRequestUpgradeStrategy.java b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/JettyRequestUpgradeStrategy.java index 92b6ba4335..f688756b22 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/JettyRequestUpgradeStrategy.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/JettyRequestUpgradeStrategy.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,6 +18,7 @@ package org.springframework.web.reactive.socket.server.upgrade; import java.io.IOException; import java.util.function.Supplier; + import javax.servlet.ServletContext; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -32,7 +33,9 @@ import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.http.server.reactive.AbstractServerHttpRequest; import org.springframework.http.server.reactive.AbstractServerHttpResponse; import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.http.server.reactive.ServerHttpRequestDecorator; import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.http.server.reactive.ServerHttpResponseDecorator; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.web.reactive.socket.HandshakeInfo; @@ -144,8 +147,8 @@ public class JettyRequestUpgradeStrategy implements RequestUpgradeStrategy, Life ServerHttpRequest request = exchange.getRequest(); ServerHttpResponse response = exchange.getResponse(); - HttpServletRequest servletRequest = getHttpServletRequest(request); - HttpServletResponse servletResponse = getHttpServletResponse(response); + HttpServletRequest servletRequest = getNativeRequest(request); + HttpServletResponse servletResponse = getNativeResponse(response); HandshakeInfo handshakeInfo = handshakeInfoFactory.get(); DataBufferFactory factory = response.bufferFactory(); @@ -173,14 +176,30 @@ public class JettyRequestUpgradeStrategy implements RequestUpgradeStrategy, Life return Mono.empty(); } - private HttpServletRequest getHttpServletRequest(ServerHttpRequest request) { - Assert.isInstanceOf(AbstractServerHttpRequest.class, request); - return ((AbstractServerHttpRequest) request).getNativeRequest(); + private static HttpServletRequest getNativeRequest(ServerHttpRequest request) { + if (request instanceof AbstractServerHttpRequest) { + return ((AbstractServerHttpRequest) request).getNativeRequest(); + } + else if (request instanceof ServerHttpRequestDecorator) { + return getNativeRequest(((ServerHttpRequestDecorator) request).getDelegate()); + } + else { + throw new IllegalArgumentException( + "Couldn't find HttpServletRequest in " + request.getClass().getName()); + } } - private HttpServletResponse getHttpServletResponse(ServerHttpResponse response) { - Assert.isInstanceOf(AbstractServerHttpResponse.class, response); - return ((AbstractServerHttpResponse) response).getNativeResponse(); + private static HttpServletResponse getNativeResponse(ServerHttpResponse response) { + if (response instanceof AbstractServerHttpResponse) { + return ((AbstractServerHttpResponse) response).getNativeResponse(); + } + else if (response instanceof ServerHttpResponseDecorator) { + return getNativeResponse(((ServerHttpResponseDecorator) response).getDelegate()); + } + else { + throw new IllegalArgumentException( + "Couldn't find HttpServletResponse in " + response.getClass().getName()); + } } private void startLazily(HttpServletRequest request) { diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/ReactorNettyRequestUpgradeStrategy.java b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/ReactorNettyRequestUpgradeStrategy.java index ddc2da441d..4112bc790a 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/ReactorNettyRequestUpgradeStrategy.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/ReactorNettyRequestUpgradeStrategy.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. @@ -24,6 +24,7 @@ import reactor.netty.http.server.HttpServerResponse; import org.springframework.core.io.buffer.NettyDataBufferFactory; import org.springframework.http.server.reactive.AbstractServerHttpResponse; import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.http.server.reactive.ServerHttpResponseDecorator; import org.springframework.lang.Nullable; import org.springframework.web.reactive.socket.HandshakeInfo; import org.springframework.web.reactive.socket.WebSocketHandler; @@ -72,7 +73,7 @@ public class ReactorNettyRequestUpgradeStrategy implements RequestUpgradeStrateg @Nullable String subProtocol, Supplier handshakeInfoFactory) { ServerHttpResponse response = exchange.getResponse(); - HttpServerResponse reactorResponse = ((AbstractServerHttpResponse) response).getNativeResponse(); + HttpServerResponse reactorResponse = getNativeResponse(response); HandshakeInfo handshakeInfo = handshakeInfoFactory.get(); NettyDataBufferFactory bufferFactory = (NettyDataBufferFactory) response.bufferFactory(); @@ -85,4 +86,17 @@ public class ReactorNettyRequestUpgradeStrategy implements RequestUpgradeStrateg }); } + private static HttpServerResponse getNativeResponse(ServerHttpResponse response) { + if (response instanceof AbstractServerHttpResponse) { + return ((AbstractServerHttpResponse) response).getNativeResponse(); + } + else if (response instanceof ServerHttpResponseDecorator) { + return getNativeResponse(((ServerHttpResponseDecorator) response).getDelegate()); + } + else { + throw new IllegalArgumentException( + "Couldn't find native response in " + response.getClass().getName()); + } + } + } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/TomcatRequestUpgradeStrategy.java b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/TomcatRequestUpgradeStrategy.java index c308fe221c..fb4ef36dd0 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/TomcatRequestUpgradeStrategy.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/TomcatRequestUpgradeStrategy.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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,6 +19,7 @@ package org.springframework.web.reactive.socket.server.upgrade; import java.io.IOException; import java.util.Collections; import java.util.function.Supplier; + import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -32,7 +33,9 @@ import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.http.server.reactive.AbstractServerHttpRequest; import org.springframework.http.server.reactive.AbstractServerHttpResponse; import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.http.server.reactive.ServerHttpRequestDecorator; import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.http.server.reactive.ServerHttpResponseDecorator; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.web.reactive.socket.HandshakeInfo; @@ -130,8 +133,8 @@ public class TomcatRequestUpgradeStrategy implements RequestUpgradeStrategy { ServerHttpRequest request = exchange.getRequest(); ServerHttpResponse response = exchange.getResponse(); - HttpServletRequest servletRequest = getHttpServletRequest(request); - HttpServletResponse servletResponse = getHttpServletResponse(response); + HttpServletRequest servletRequest = getNativeRequest(request); + HttpServletResponse servletResponse = getNativeResponse(response); HandshakeInfo handshakeInfo = handshakeInfoFactory.get(); DataBufferFactory bufferFactory = response.bufferFactory(); @@ -155,14 +158,30 @@ public class TomcatRequestUpgradeStrategy implements RequestUpgradeStrategy { return Mono.empty(); } - private HttpServletRequest getHttpServletRequest(ServerHttpRequest request) { - Assert.isInstanceOf(AbstractServerHttpRequest.class, request, "ServletServerHttpRequest required"); - return ((AbstractServerHttpRequest) request).getNativeRequest(); + private static HttpServletRequest getNativeRequest(ServerHttpRequest request) { + if (request instanceof AbstractServerHttpRequest) { + return ((AbstractServerHttpRequest) request).getNativeRequest(); + } + else if (request instanceof ServerHttpRequestDecorator) { + return getNativeRequest(((ServerHttpRequestDecorator) request).getDelegate()); + } + else { + throw new IllegalArgumentException( + "Couldn't find HttpServletRequest in " + request.getClass().getName()); + } } - private HttpServletResponse getHttpServletResponse(ServerHttpResponse response) { - Assert.isInstanceOf(AbstractServerHttpResponse.class, response, "ServletServerHttpResponse required"); - return ((AbstractServerHttpResponse) response).getNativeResponse(); + private static HttpServletResponse getNativeResponse(ServerHttpResponse response) { + if (response instanceof AbstractServerHttpResponse) { + return ((AbstractServerHttpResponse) response).getNativeResponse(); + } + else if (response instanceof ServerHttpResponseDecorator) { + return getNativeResponse(((ServerHttpResponseDecorator) response).getDelegate()); + } + else { + throw new IllegalArgumentException( + "Couldn't find HttpServletResponse in " + response.getClass().getName()); + } } private WsServerContainer getContainer(HttpServletRequest request) { diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/UndertowRequestUpgradeStrategy.java b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/UndertowRequestUpgradeStrategy.java index 5a322bbc06..56b69515a4 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/UndertowRequestUpgradeStrategy.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/socket/server/upgrade/UndertowRequestUpgradeStrategy.java @@ -33,6 +33,7 @@ import reactor.core.publisher.Mono; import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.http.server.reactive.AbstractServerHttpRequest; import org.springframework.http.server.reactive.ServerHttpRequest; +import org.springframework.http.server.reactive.ServerHttpRequestDecorator; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.web.reactive.socket.HandshakeInfo; @@ -55,9 +56,7 @@ public class UndertowRequestUpgradeStrategy implements RequestUpgradeStrategy { public Mono upgrade(ServerWebExchange exchange, WebSocketHandler handler, @Nullable String subProtocol, Supplier handshakeInfoFactory) { - ServerHttpRequest request = exchange.getRequest(); - Assert.isInstanceOf(AbstractServerHttpRequest.class, request); - HttpServerExchange httpExchange = ((AbstractServerHttpRequest) request).getNativeRequest(); + HttpServerExchange httpExchange = getNativeRequest(exchange.getRequest()); Set protocols = (subProtocol != null ? Collections.singleton(subProtocol) : Collections.emptySet()); Hybi13Handshake handshake = new Hybi13Handshake(protocols, false); @@ -77,6 +76,19 @@ public class UndertowRequestUpgradeStrategy implements RequestUpgradeStrategy { return Mono.empty(); } + private static HttpServerExchange getNativeRequest(ServerHttpRequest request) { + if (request instanceof AbstractServerHttpRequest) { + return ((AbstractServerHttpRequest) request).getNativeRequest(); + } + else if (request instanceof ServerHttpRequestDecorator) { + return getNativeRequest(((ServerHttpRequestDecorator) request).getDelegate()); + } + else { + throw new IllegalArgumentException( + "Couldn't find HttpServerExchange in " + request.getClass().getName()); + } + } + private class DefaultCallback implements WebSocketConnectionCallback { From 07b0fa132ee724920d32f0eb1108377672d444ae Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 12 Sep 2019 10:25:25 +0100 Subject: [PATCH 2/4] doFilterNestedErrorDispatch delegates to filter chain Closes: gh-23596 --- .../org/springframework/web/filter/OncePerRequestFilter.java | 2 +- .../web/filter/OncePerRequestFilterTests.java | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/filter/OncePerRequestFilter.java b/spring-web/src/main/java/org/springframework/web/filter/OncePerRequestFilter.java index a08d0df4da..403e7d127b 100644 --- a/spring-web/src/main/java/org/springframework/web/filter/OncePerRequestFilter.java +++ b/spring-web/src/main/java/org/springframework/web/filter/OncePerRequestFilter.java @@ -247,7 +247,7 @@ public abstract class OncePerRequestFilter extends GenericFilterBean { protected void doFilterNestedErrorDispatch(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException { - doFilter(request, response, filterChain); + filterChain.doFilter(request, response); } } diff --git a/spring-web/src/test/java/org/springframework/web/filter/OncePerRequestFilterTests.java b/spring-web/src/test/java/org/springframework/web/filter/OncePerRequestFilterTests.java index 9f97ca6741..3898a07846 100644 --- a/spring-web/src/test/java/org/springframework/web/filter/OncePerRequestFilterTests.java +++ b/spring-web/src/test/java/org/springframework/web/filter/OncePerRequestFilterTests.java @@ -16,6 +16,7 @@ package org.springframework.web.filter; import java.io.IOException; + import javax.servlet.DispatcherType; import javax.servlet.FilterChain; import javax.servlet.ServletException; @@ -31,7 +32,6 @@ import org.springframework.mock.web.test.MockHttpServletRequest; import org.springframework.mock.web.test.MockHttpServletResponse; import org.springframework.web.util.WebUtils; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -184,9 +184,10 @@ public class OncePerRequestFilterTests { @Override protected void doFilterNestedErrorDispatch(HttpServletRequest request, HttpServletResponse response, - FilterChain filterChain) { + FilterChain filterChain) throws ServletException, IOException { this.didFilterNestedErrorDispatch = true; + super.doFilterNestedErrorDispatch(request, response, filterChain); } } From b7eaab4c5d1b2beed13389ed79a07161329ea686 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 12 Sep 2019 14:14:11 +0100 Subject: [PATCH 3/4] Handle very early completion notification On a Servlet container a completion notification may come at any time even in the UNSUBSCRIBED state, i.e. before the write Publisher has called onSubscribe. See: gh-23553 --- .../reactive/AbstractListenerWriteFlushProcessor.java | 11 +++++++++++ .../reactive/AbstractListenerWriteProcessor.java | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerWriteFlushProcessor.java b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerWriteFlushProcessor.java index 329cd3dbca..81c8098d32 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerWriteFlushProcessor.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerWriteFlushProcessor.java @@ -246,6 +246,17 @@ public abstract class AbstractListenerWriteFlushProcessor implements Processo super.onSubscribe(processor, subscription); } } + + @Override + public void onComplete(AbstractListenerWriteFlushProcessor processor) { + // This can happen on (very early) completion notification from container.. + if (processor.changeState(this, COMPLETED)) { + processor.resultPublisher.publishComplete(); + } + else { + processor.state.get().onComplete(processor); + } + } }, REQUESTED { diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerWriteProcessor.java b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerWriteProcessor.java index 509076d02b..4f05d5e1e0 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerWriteProcessor.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractListenerWriteProcessor.java @@ -322,6 +322,12 @@ public abstract class AbstractListenerWriteProcessor implements Processor void onComplete(AbstractListenerWriteProcessor processor) { + // This can happen on (very early) completion notification from container.. + processor.changeStateToComplete(this); + } }, REQUESTED { From 23be5dfb0ea52ac08854aad2a5ed0caf696877c3 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 12 Sep 2019 20:57:30 +0100 Subject: [PATCH 4/4] Handle invalid MediaType in Jetty/Tomcat adapters See: gh-23553 --- .../server/reactive/AbstractServerHttpResponse.java | 9 +++++++-- .../server/reactive/JettyHttpHandlerAdapter.java | 9 ++++++++- .../server/reactive/ServletServerHttpResponse.java | 13 ++++++++++++- .../server/reactive/TomcatHttpHandlerAdapter.java | 9 ++++++++- 4 files changed, 35 insertions(+), 5 deletions(-) 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 3936a6824a..4f2f51214d 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 @@ -262,8 +262,13 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { protected abstract void applyStatusCode(); /** - * Apply header changes from {@link #getHeaders()} to the underlying response. - * This method is called once only. + * Invoked when the response is getting committed allowing sub-classes to + * make apply header values to the underlying response. + *

Note that most sub-classes use an {@link HttpHeaders} instance that + * wraps an adapter to the native response headers such that changes are + * propagated to the underlying response on the go. That means this callback + * is typically not used other than for specialized updates such as setting + * the contentType or characterEncoding fields in a Servlet response. */ protected abstract void applyHeaders(); 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 b29a592dab..4f2c7128e9 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 @@ -101,8 +101,15 @@ public class JettyHttpHandlerAdapter extends ServletHttpHandlerAdapter { @Override protected void applyHeaders() { - MediaType contentType = getHeaders().getContentType(); HttpServletResponse response = getNativeResponse(); + MediaType contentType = null; + try { + contentType = getHeaders().getContentType(); + } + catch (Exception ex) { + String rawContentType = getHeaders().getFirst(HttpHeaders.CONTENT_TYPE); + response.setContentType(rawContentType); + } if (response.getContentType() == null && contentType != null) { response.setContentType(contentType.toString()); } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java index ffcddb680f..429c665da8 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java @@ -118,7 +118,14 @@ class ServletServerHttpResponse extends AbstractListenerServerHttpResponse { this.response.addHeader(headerName, headerValue); } }); - MediaType contentType = getHeaders().getContentType(); + MediaType contentType = null; + try { + contentType = getHeaders().getContentType(); + } + catch (Exception ex) { + String rawContentType = getHeaders().getFirst(HttpHeaders.CONTENT_TYPE); + this.response.setContentType(rawContentType); + } if (this.response.getContentType() == null && contentType != null) { this.response.setContentType(contentType.toString()); } @@ -126,6 +133,10 @@ class ServletServerHttpResponse extends AbstractListenerServerHttpResponse { if (this.response.getCharacterEncoding() == null && charset != null) { this.response.setCharacterEncoding(charset.name()); } + long contentLength = getHeaders().getContentLength(); + if (contentLength != -1) { + this.response.setContentLengthLong(contentLength); + } } @Override 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 2bdf901b1c..78ad8d2970 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 @@ -204,7 +204,14 @@ public class TomcatHttpHandlerAdapter extends ServletHttpHandlerAdapter { @Override protected void applyHeaders() { HttpServletResponse response = getNativeResponse(); - MediaType contentType = getHeaders().getContentType(); + MediaType contentType = null; + try { + contentType = getHeaders().getContentType(); + } + catch (Exception ex) { + String rawContentType = getHeaders().getFirst(HttpHeaders.CONTENT_TYPE); + response.setContentType(rawContentType); + } if (response.getContentType() == null && contentType != null) { response.setContentType(contentType.toString()); }