From c2d1cb2c701f3c0dd06a6db298cd78e06d039c8f Mon Sep 17 00:00:00 2001 From: ayudovin Date: Mon, 17 Jun 2019 09:38:59 -0700 Subject: [PATCH 1/3] Chain predicates in PropertyMapper when methods Update `PropertyMapper` to correctly combine predicates when repeated calls are made to `when` and `whenNot`. Prior to this commit, subsequent invocations would replace the previous predicate. Fixes gh-17225 --- .../boot/context/properties/PropertyMapper.java | 5 +++-- .../boot/context/properties/PropertyMapperTests.java | 12 ++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/PropertyMapper.java b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/PropertyMapper.java index f5486abb291..72e8cd50ecd 100644 --- a/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/PropertyMapper.java +++ b/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/properties/PropertyMapper.java @@ -50,6 +50,7 @@ import org.springframework.util.StringUtils; * {@link Source#toInstance(Function) new instance}. * * @author Phillip Webb + * @author Artsiom Yudovin * @since 2.0.0 */ public final class PropertyMapper { @@ -288,7 +289,7 @@ public final class PropertyMapper { */ public Source whenNot(Predicate predicate) { Assert.notNull(predicate, "Predicate must not be null"); - return new Source<>(this.supplier, predicate.negate()); + return when(predicate.negate()); } /** @@ -299,7 +300,7 @@ public final class PropertyMapper { */ public Source when(Predicate predicate) { Assert.notNull(predicate, "Predicate must not be null"); - return new Source<>(this.supplier, predicate); + return new Source<>(this.supplier, (this.predicate != null) ? this.predicate.and(predicate) : predicate); } /** diff --git a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/PropertyMapperTests.java b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/PropertyMapperTests.java index 49a8b04aa25..0e7cd005a45 100644 --- a/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/PropertyMapperTests.java +++ b/spring-boot-project/spring-boot/src/test/java/org/springframework/boot/context/properties/PropertyMapperTests.java @@ -28,6 +28,7 @@ import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException * Tests for {@link PropertyMapper}. * * @author Phillip Webb + * @author Artsiom Yudovin */ public class PropertyMapperTests { @@ -190,6 +191,17 @@ public class PropertyMapperTests { assertThat(source.getCount()).isOne(); } + @Test + public void whenWhenValueNotMatchesShouldSupportChainedCalls() { + this.map.from("123").when("456"::equals).when("123"::equals).toCall(Assert::fail); + } + + @Test + public void whenWhenValueMatchesShouldSupportChainedCalls() { + String result = this.map.from("123").when((s) -> s.contains("2")).when("123"::equals).toInstance(String::new); + assertThat(result).isEqualTo("123"); + } + @Test public void alwaysApplyingWhenNonNullShouldAlwaysApplyNonNullToSource() { this.map.alwaysApplyingWhenNonNull().from(() -> null).toCall(Assert::fail); From b0e4c716d396077dbe606489b9c430aee0151e4e Mon Sep 17 00:00:00 2001 From: ayudovin Date: Mon, 17 Jun 2019 09:39:26 -0700 Subject: [PATCH 2/3] Fix connection timeout configuration for Netty Update `NettyWebServerFactoryCustomizer` to deal with the fact that Netty treats `0` and negative connection timeout values differently to Tomcat, Undertow and Jetty. See gh-16535 --- .../NettyWebServerFactoryCustomizer.java | 8 +++-- .../NettyWebServerFactoryCustomizerTests.java | 32 +++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java index 54029cdd076..d962b0cdc7c 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java @@ -58,10 +58,12 @@ public class NettyWebServerFactoryCustomizer @Override public void customize(NettyReactiveWebServerFactory factory) { factory.setUseForwardHeaders(getOrDeduceUseForwardHeaders(this.serverProperties, this.environment)); - PropertyMapper propertyMapper = PropertyMapper.get(); - propertyMapper.from(this.serverProperties::getMaxHttpHeaderSize).whenNonNull().asInt(DataSize::toBytes) + PropertyMapper propertyMapper = PropertyMapper.get().alwaysApplyingWhenNonNull(); + propertyMapper.from(this.serverProperties::getMaxHttpHeaderSize).asInt(DataSize::toBytes) .to((maxHttpRequestHeaderSize) -> customizeMaxHttpHeaderSize(factory, maxHttpRequestHeaderSize)); - propertyMapper.from(this.serverProperties::getConnectionTimeout).whenNonNull().asInt(Duration::toMillis) + propertyMapper.from(this.serverProperties::getConnectionTimeout).asInt(Duration::toMillis) + .whenNot((connectionTimout) -> connectionTimout.equals(0)) + .as((connectionTimeout) -> connectionTimeout.equals(-1) ? 0 : connectionTimeout) .to((duration) -> factory.addServerCustomizers(getConnectionTimeOutCustomizer(duration))); } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java index d5429548dd6..05ca5c6e72f 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java @@ -16,21 +16,27 @@ package org.springframework.boot.autoconfigure.web.embedded; +import java.time.Duration; + import org.junit.Before; import org.junit.Test; import org.springframework.boot.autoconfigure.web.ServerProperties; import org.springframework.boot.context.properties.source.ConfigurationPropertySources; import org.springframework.boot.web.embedded.netty.NettyReactiveWebServerFactory; +import org.springframework.boot.web.embedded.netty.NettyServerCustomizer; import org.springframework.mock.env.MockEnvironment; +import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; /** * Tests for {@link NettyWebServerFactoryCustomizer}. * * @author Brian Clozel + * @author Artsiom Yudovin */ public class NettyWebServerFactoryCustomizerTests { @@ -48,6 +54,12 @@ public class NettyWebServerFactoryCustomizerTests { this.customizer = new NettyWebServerFactoryCustomizer(this.environment, this.serverProperties); } + private void clear() { + this.serverProperties.setUseForwardHeaders(null); + this.serverProperties.setMaxHttpHeaderSize(null); + this.serverProperties.setConnectionTimeout(null); + } + @Test public void deduceUseForwardHeaders() { this.environment.setProperty("DYNO", "-"); @@ -71,4 +83,24 @@ public class NettyWebServerFactoryCustomizerTests { verify(factory).setUseForwardHeaders(true); } + @Test + public void setConnectionTimeoutAsZero() { + clear(); + this.serverProperties.setConnectionTimeout(Duration.ZERO); + + NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class); + this.customizer.customize(factory); + verify(factory, times(0)).addServerCustomizers(any(NettyServerCustomizer.class)); + } + + @Test + public void setConnectionTimeoutAsMinusOne() { + clear(); + this.serverProperties.setConnectionTimeout(Duration.ofNanos(-1)); + + NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class); + this.customizer.customize(factory); + verify(factory, times(1)).addServerCustomizers(any(NettyServerCustomizer.class)); + } + } From 692bda1595a0d6a16697629eaedb5b0b1ae4727e Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 17 Jun 2019 11:10:56 -0700 Subject: [PATCH 3/3] Polish "Fix connection timeout configuration for Netty" See gh-16535 --- .../NettyWebServerFactoryCustomizer.java | 24 +++---- .../NettyWebServerFactoryCustomizerTests.java | 62 ++++++++++++++----- 2 files changed, 60 insertions(+), 26 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java index d962b0cdc7c..2344b9755a1 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java @@ -24,7 +24,6 @@ import org.springframework.boot.autoconfigure.web.ServerProperties; import org.springframework.boot.cloud.CloudPlatform; import org.springframework.boot.context.properties.PropertyMapper; import org.springframework.boot.web.embedded.netty.NettyReactiveWebServerFactory; -import org.springframework.boot.web.embedded.netty.NettyServerCustomizer; import org.springframework.boot.web.server.WebServerFactoryCustomizer; import org.springframework.core.Ordered; import org.springframework.core.env.Environment; @@ -59,12 +58,10 @@ public class NettyWebServerFactoryCustomizer public void customize(NettyReactiveWebServerFactory factory) { factory.setUseForwardHeaders(getOrDeduceUseForwardHeaders(this.serverProperties, this.environment)); PropertyMapper propertyMapper = PropertyMapper.get().alwaysApplyingWhenNonNull(); - propertyMapper.from(this.serverProperties::getMaxHttpHeaderSize).asInt(DataSize::toBytes) + propertyMapper.from(this.serverProperties::getMaxHttpHeaderSize) .to((maxHttpRequestHeaderSize) -> customizeMaxHttpHeaderSize(factory, maxHttpRequestHeaderSize)); - propertyMapper.from(this.serverProperties::getConnectionTimeout).asInt(Duration::toMillis) - .whenNot((connectionTimout) -> connectionTimout.equals(0)) - .as((connectionTimeout) -> connectionTimeout.equals(-1) ? 0 : connectionTimeout) - .to((duration) -> factory.addServerCustomizers(getConnectionTimeOutCustomizer(duration))); + propertyMapper.from(this.serverProperties::getConnectionTimeout) + .to((connectionTimeout) -> customizeConnectionTimeout(factory, connectionTimeout)); } private boolean getOrDeduceUseForwardHeaders(ServerProperties serverProperties, Environment environment) { @@ -75,14 +72,17 @@ public class NettyWebServerFactoryCustomizer return platform != null && platform.isUsingForwardHeaders(); } - private void customizeMaxHttpHeaderSize(NettyReactiveWebServerFactory factory, Integer maxHttpHeaderSize) { - factory.addServerCustomizers((NettyServerCustomizer) (httpServer) -> httpServer.httpRequestDecoder( - (httpRequestDecoderSpec) -> httpRequestDecoderSpec.maxHeaderSize(maxHttpHeaderSize))); + private void customizeMaxHttpHeaderSize(NettyReactiveWebServerFactory factory, DataSize maxHttpHeaderSize) { + factory.addServerCustomizers((httpServer) -> httpServer.httpRequestDecoder( + (httpRequestDecoderSpec) -> httpRequestDecoderSpec.maxHeaderSize((int) maxHttpHeaderSize.toBytes()))); } - private NettyServerCustomizer getConnectionTimeOutCustomizer(int duration) { - return (httpServer) -> httpServer.tcpConfiguration( - (tcpServer) -> tcpServer.selectorOption(ChannelOption.CONNECT_TIMEOUT_MILLIS, duration)); + private void customizeConnectionTimeout(NettyReactiveWebServerFactory factory, Duration connectionTimeout) { + if (!connectionTimeout.isZero()) { + long timeoutMillis = connectionTimeout.isNegative() ? 0 : connectionTimeout.toMillis(); + factory.addServerCustomizers((httpServer) -> httpServer.tcpConfiguration((tcpServer) -> tcpServer + .selectorOption(ChannelOption.CONNECT_TIMEOUT_MILLIS, (int) timeoutMillis))); + } } } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java index 05ca5c6e72f..7a1e434246d 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java @@ -17,18 +17,29 @@ package org.springframework.boot.autoconfigure.web.embedded; import java.time.Duration; +import java.util.Map; +import io.netty.bootstrap.ServerBootstrap; +import io.netty.channel.ChannelOption; import org.junit.Before; import org.junit.Test; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.MockitoAnnotations; +import reactor.netty.http.server.HttpServer; +import reactor.netty.tcp.TcpServer; import org.springframework.boot.autoconfigure.web.ServerProperties; import org.springframework.boot.context.properties.source.ConfigurationPropertySources; import org.springframework.boot.web.embedded.netty.NettyReactiveWebServerFactory; import org.springframework.boot.web.embedded.netty.NettyServerCustomizer; import org.springframework.mock.env.MockEnvironment; +import org.springframework.test.util.ReflectionTestUtils; -import static org.mockito.Mockito.any; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -46,20 +57,18 @@ public class NettyWebServerFactoryCustomizerTests { private NettyWebServerFactoryCustomizer customizer; + @Captor + private ArgumentCaptor customizerCaptor; + @Before public void setup() { + MockitoAnnotations.initMocks(this); this.environment = new MockEnvironment(); this.serverProperties = new ServerProperties(); ConfigurationPropertySources.attach(this.environment); this.customizer = new NettyWebServerFactoryCustomizer(this.environment, this.serverProperties); } - private void clear() { - this.serverProperties.setUseForwardHeaders(null); - this.serverProperties.setMaxHttpHeaderSize(null); - this.serverProperties.setConnectionTimeout(null); - } - @Test public void deduceUseForwardHeaders() { this.environment.setProperty("DYNO", "-"); @@ -85,22 +94,47 @@ public class NettyWebServerFactoryCustomizerTests { @Test public void setConnectionTimeoutAsZero() { - clear(); - this.serverProperties.setConnectionTimeout(Duration.ZERO); - + setupConnectionTimeout(Duration.ZERO); NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class); this.customizer.customize(factory); - verify(factory, times(0)).addServerCustomizers(any(NettyServerCustomizer.class)); + verifyConnectionTimeout(factory, null); } @Test public void setConnectionTimeoutAsMinusOne() { - clear(); - this.serverProperties.setConnectionTimeout(Duration.ofNanos(-1)); + setupConnectionTimeout(Duration.ofNanos(-1)); + NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class); + this.customizer.customize(factory); + verifyConnectionTimeout(factory, 0); + } + @Test + public void setConnectionTimeout() { + setupConnectionTimeout(Duration.ofSeconds(1)); NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class); this.customizer.customize(factory); - verify(factory, times(1)).addServerCustomizers(any(NettyServerCustomizer.class)); + verifyConnectionTimeout(factory, 1000); + } + + @SuppressWarnings("unchecked") + private void verifyConnectionTimeout(NettyReactiveWebServerFactory factory, Integer expected) { + if (expected == null) { + verify(factory, never()).addServerCustomizers(any(NettyServerCustomizer.class)); + return; + } + verify(factory, times(1)).addServerCustomizers(this.customizerCaptor.capture()); + NettyServerCustomizer serverCustomizer = this.customizerCaptor.getValue(); + HttpServer httpServer = serverCustomizer.apply(HttpServer.create()); + TcpServer tcpConfiguration = ReflectionTestUtils.invokeMethod(httpServer, "tcpConfiguration"); + ServerBootstrap bootstrap = tcpConfiguration.configure(); + Map options = (Map) ReflectionTestUtils.getField(bootstrap, "options"); + assertThat(options).containsEntry(ChannelOption.CONNECT_TIMEOUT_MILLIS, expected); + } + + private void setupConnectionTimeout(Duration connectionTimeout) { + this.serverProperties.setUseForwardHeaders(null); + this.serverProperties.setMaxHttpHeaderSize(null); + this.serverProperties.setConnectionTimeout(connectionTimeout); } }