From b73ab316c5e2fc1ba2e1cdf0eb871d1966311ff0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Kraus?= Date: Fri, 20 Jan 2023 15:33:37 +0100 Subject: [PATCH 1/6] Issue 5383: ContentEncodingContext Builder and passing ContentEncodingContext instance from WebServer to Http1Connection. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomáš Kraus --- nima/http/encoding/encoding/pom.xml | 6 +- .../http/encoding/ContentEncodingContext.java | 118 +++++++++++++- .../encoding/ContentEncodingSupportImpl.java | 80 +++------- .../encoding/src/main/java/module-info.java | 3 +- .../http2/webserver/ConnectionConfigTest.java | 14 +- .../io/helidon/nima/webserver/WebServer.java | 6 +- .../nima/webserver/http1/Http1Connection.java | 4 +- .../webserver/http1/ConnectionConfigTest.java | 146 ++---------------- nima/websocket/webserver/pom.xml | 5 + .../WsUpgradeProviderConfigTest.java | 101 ++---------- 10 files changed, 191 insertions(+), 292 deletions(-) diff --git a/nima/http/encoding/encoding/pom.xml b/nima/http/encoding/encoding/pom.xml index ab933198509..fd26e3721f7 100644 --- a/nima/http/encoding/encoding/pom.xml +++ b/nima/http/encoding/encoding/pom.xml @@ -1,6 +1,6 @@ + + helidon-nima-http-encoding-gzip + io.helidon.nima.http.encoding + test + org.junit.jupiter junit-jupiter-api diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java index 9879d4687e6..254824759ab 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java @@ -507,6 +507,7 @@ Map routers() { } List connectionProviders() { + providersConfig.get("discover-services").asBoolean().ifPresent(connectionProviders::useSystemServiceLoader); List providers = connectionProviders.build().asList(); // Send configuration nodes to providers return providers.stream() diff --git a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java index 95612cd71a2..7c385ceed54 100644 --- a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java +++ b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java @@ -20,6 +20,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.List; +import java.util.NoSuchElementException; import org.junit.jupiter.api.Test; @@ -30,11 +31,13 @@ import io.helidon.nima.webserver.Router; import io.helidon.nima.webserver.ServerContext; import io.helidon.nima.webserver.WebServer; -import io.helidon.nima.webserver.spi.ServerConnectionProvider; import io.helidon.nima.webserver.spi.ServerConnectionSelector; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; +import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -75,6 +78,29 @@ void testConnectionConfig() } assertThat("No Http12ConnectionProvider was found", haveHttp1Provider, is(true)); } + @Test + void testConnectionProvidersDisabled() + throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { + + // This will pick up application.yaml from the classpath as default configuration file + Config config = Config.create(); + + // Builds LoomServer instance including connectionProviders list using server2 node. + WebServer.Builder wsBuilder = WebServer.builder() + .config(config.get("server2")); + + // Call wsBuilder.connectionProviders() trough reflection + Method connectionProviders + = WebServer.Builder.class.getDeclaredMethod("connectionProviders", (Class[]) null); + connectionProviders.setAccessible(true); + @SuppressWarnings("unchecked") + List providers + = (List) connectionProviders.invoke(wsBuilder, (Object[]) null); + + // No providers shall be loaded with ServiceLoader disabled for connection providers. + assertThat(providers, notNullValue()); + assertThat(providers, is(empty())); + } // Check that WebServer ContentEncodingContext is disabled when disable is present in config @Test @@ -90,11 +116,28 @@ void testContentEncodingConfig() throws NoSuchFieldException, IllegalAccessExcep Field contentEncodingContextField = WebServer.Builder.class.getDeclaredField("contentEncodingContext"); contentEncodingContextField.setAccessible(true); ContentEncodingContext contentEncodingContext = (ContentEncodingContext) contentEncodingContextField.get(wsBuilder); - - assertThat(contentEncodingContext.contentEncodingEnabled(), is(false)); - assertThat(contentEncodingContext.contentDecodingEnabled(), is(false)); + // helidon-nima-http-encoding-gzip is on classpath so disabling ServiceLoader shall remove them + assertThat(contentEncodingContext.contentEncodingEnabled(), is(true)); + assertThat(contentEncodingContext.contentDecodingEnabled(), is(true)); + failsWith(() -> contentEncodingContext.decoder("gzip"), NoSuchElementException.class); + failsWith(() -> contentEncodingContext.decoder("gzip"), NoSuchElementException.class); + failsWith(() -> contentEncodingContext.encoder("gzip"), NoSuchElementException.class); + failsWith(() -> contentEncodingContext.decoder("x-gzip"), NoSuchElementException.class); + failsWith(() -> contentEncodingContext.encoder("x-gzip"), NoSuchElementException.class); } + // Verify that provided task throws an exception + private static void failsWith(Runnable task, Class exception) { + try { + task.run(); + // Fail the test when no Exception was thrown + fail(String.format("Exception %s was not thrown", exception.getName())); + } catch (Exception ex) { + if (!exception.isAssignableFrom(ex.getClass())) { + throw ex; + } + } + } private static ConnectionContext mockContext() { ConnectionContext ctx = mock(ConnectionContext.class); diff --git a/nima/webserver/webserver/src/test/resources/application.yaml b/nima/webserver/webserver/src/test/resources/application.yaml index f57151440a3..02025644349 100644 --- a/nima/webserver/webserver/src/test/resources/application.yaml +++ b/nima/webserver/webserver/src/test/resources/application.yaml @@ -26,4 +26,11 @@ server: validate-path: false content-encoding: - disable: true + discover-services: false + +server2: + port: 8079 + host: 127.0.0.1 + + connection-providers: + discover-services: false From 3cd08a9623efd677a1b6e4692fee15feabc3db15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Kraus?= Date: Fri, 20 Jan 2023 18:03:24 +0100 Subject: [PATCH 4/6] Fixed checkstyle issue. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomáš Kraus --- .../io/helidon/nima/http/encoding/ContentEncodingContext.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nima/http/encoding/encoding/src/main/java/io/helidon/nima/http/encoding/ContentEncodingContext.java b/nima/http/encoding/encoding/src/main/java/io/helidon/nima/http/encoding/ContentEncodingContext.java index f2715c8adbd..975ef3ae41c 100644 --- a/nima/http/encoding/encoding/src/main/java/io/helidon/nima/http/encoding/ContentEncodingContext.java +++ b/nima/http/encoding/encoding/src/main/java/io/helidon/nima/http/encoding/ContentEncodingContext.java @@ -24,8 +24,8 @@ import java.util.Set; import io.helidon.common.HelidonServiceLoader; -import io.helidon.common.http.Headers; import io.helidon.common.config.Config; +import io.helidon.common.http.Headers; import io.helidon.nima.http.encoding.spi.ContentEncodingProvider; /** From 0fda112f579472accf422540a7229e1281f20d7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Kraus?= Date: Wed, 1 Feb 2023 16:37:29 +0100 Subject: [PATCH 5/6] Review notes applied. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomáš Kraus --- .../nima/http/encoding/ContentEncodingContext.java | 12 +++++++++++- .../java/io/helidon/nima/webserver/WebServer.java | 5 ++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/nima/http/encoding/encoding/src/main/java/io/helidon/nima/http/encoding/ContentEncodingContext.java b/nima/http/encoding/encoding/src/main/java/io/helidon/nima/http/encoding/ContentEncodingContext.java index 975ef3ae41c..90e334b00f8 100644 --- a/nima/http/encoding/encoding/src/main/java/io/helidon/nima/http/encoding/ContentEncodingContext.java +++ b/nima/http/encoding/encoding/src/main/java/io/helidon/nima/http/encoding/ContentEncodingContext.java @@ -41,6 +41,16 @@ static ContentEncodingContext create() { return builder().build(); } + /** + * Create a new encoding support and apply provided configuration. + * + * @param config configuration to use + * @return content encoding support + */ + static ContentEncodingContext create(Config config) { + return builder().config(config).build(); + } + /** * There is at least one content encoder. * @@ -132,7 +142,7 @@ public Builder config(Config config) { } /** - * Disable content encoding support. + * Whether Java Service Loader should be used to load {@link ContentEncodingProvider}. * * @return updated builder */ diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java index 254824759ab..47349aeb1a6 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/WebServer.java @@ -252,9 +252,8 @@ public Builder config(Config config) { }); // Configure content encoding config.get("content-encoding") - .asNode() - .ifPresent(encodingConfig -> - contentEncodingContext(ContentEncodingContext.builder().config(encodingConfig).build())); + .as(ContentEncodingContext::create) + .ifPresent(this::contentEncodingContext); // Store providers config node for later usage. providersConfig = config.get("connection-providers"); return this; From d5353404c3851be187656962092052f94e54e6fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Kraus?= Date: Wed, 1 Feb 2023 17:55:37 +0100 Subject: [PATCH 6/6] Removed reflection from the tests. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Tomáš Kraus --- .../nima/webserver/WebServerConfigTest.java | 89 ++++++++++++ .../webserver/http1/ConnectionConfigTest.java | 132 ++++-------------- 2 files changed, 118 insertions(+), 103 deletions(-) create mode 100644 nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/WebServerConfigTest.java diff --git a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/WebServerConfigTest.java b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/WebServerConfigTest.java new file mode 100644 index 00000000000..4c34b69e3db --- /dev/null +++ b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/WebServerConfigTest.java @@ -0,0 +1,89 @@ +/* + * Copyright (c) 2023 Oracle and/or its affiliates. + * + * 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 + * + * http://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 io.helidon.nima.webserver; + +import java.lang.reflect.InvocationTargetException; +import java.util.List; +import java.util.NoSuchElementException; + +import org.junit.jupiter.api.Test; + +import io.helidon.config.Config; +import io.helidon.nima.http.encoding.ContentEncodingContext; +import io.helidon.nima.webserver.spi.ServerConnectionSelector; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.empty; +import static org.junit.jupiter.api.Assertions.fail; + +public class WebServerConfigTest { + + @Test + void testConnectionProvidersEnabled() { + // This will pick up application.yaml from the classpath as default configuration file + Config config = Config.create(); + WebServer.Builder wsBuilder = WebServer.builder().config(config.get("server")); + List providers = wsBuilder.connectionProviders(); + // Providers shall be loaded with ServiceLoader. + assertThat(providers, notNullValue()); + assertThat(providers, is(not(empty()))); + } + + @Test + void testConnectionProvidersDisabled() { + // This will pick up application.yaml from the classpath as default configuration file + Config config = Config.create(); + WebServer.Builder wsBuilder = WebServer.builder().config(config.get("server2")); + List providers = wsBuilder.connectionProviders(); + // No providers shall be loaded with ServiceLoader disabled for connection providers. + assertThat(providers, notNullValue()); + assertThat(providers, is(empty())); + } + + // Check that WebServer ContentEncodingContext is disabled when disable is present in config + @Test + void testContentEncodingConfig() { + // This will pick up application.yaml from the classpath as default configuration file + Config config = Config.create(); + WebServer.Builder wsBuilder = WebServer.builder().config(config.get("server")); + ContentEncodingContext contentEncodingContext = wsBuilder.contentEncodingContext(); + assertThat(contentEncodingContext.contentEncodingEnabled(), is(true)); + assertThat(contentEncodingContext.contentDecodingEnabled(), is(true)); + failsWith(() -> contentEncodingContext.decoder("gzip"), NoSuchElementException.class); + failsWith(() -> contentEncodingContext.decoder("gzip"), NoSuchElementException.class); + failsWith(() -> contentEncodingContext.encoder("gzip"), NoSuchElementException.class); + failsWith(() -> contentEncodingContext.decoder("x-gzip"), NoSuchElementException.class); + failsWith(() -> contentEncodingContext.encoder("x-gzip"), NoSuchElementException.class); + } + + // Verify that provided task throws an exception + private static void failsWith(Runnable task, Class exception) { + try { + task.run(); + // Fail the test when no Exception was thrown + fail(String.format("Exception %s was not thrown", exception.getName())); + } catch (Exception ex) { + if (!exception.isAssignableFrom(ex.getClass())) { + throw ex; + } + } + } + +} diff --git a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java index 7c385ceed54..90580b7440d 100644 --- a/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java +++ b/nima/webserver/webserver/src/test/java/io/helidon/nima/webserver/http1/ConnectionConfigTest.java @@ -16,135 +16,61 @@ package io.helidon.nima.webserver.http1; -import java.lang.reflect.Field; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; import java.util.List; -import java.util.NoSuchElementException; +import java.util.function.Function; import org.junit.jupiter.api.Test; -import io.helidon.common.buffers.DataReader; import io.helidon.config.Config; -import io.helidon.nima.http.encoding.ContentEncodingContext; -import io.helidon.nima.webserver.ConnectionContext; -import io.helidon.nima.webserver.Router; -import io.helidon.nima.webserver.ServerContext; import io.helidon.nima.webserver.WebServer; +import io.helidon.nima.webserver.spi.ServerConnectionProvider; import io.helidon.nima.webserver.spi.ServerConnectionSelector; import static org.hamcrest.CoreMatchers.is; -import static org.hamcrest.CoreMatchers.notNullValue; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.empty; -import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; public class ConnectionConfigTest { @Test - void testConnectionConfig() - throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { - + void testConnectionConfig() { // This will pick up application.yaml from the classpath as default configuration file Config config = Config.create(); - - // Builds LoomServer instance including connectionProviders list. - WebServer.Builder wsBuilder = WebServer.builder() - .config(config.get("server")); - - // Call wsBuilder.connectionProviders() trough reflection - Method connectionProviders - = WebServer.Builder.class.getDeclaredMethod("connectionProviders", (Class[]) null); - connectionProviders.setAccessible(true); - @SuppressWarnings("unchecked") - List providers - = (List) connectionProviders.invoke(wsBuilder, (Object[]) null); - - // Check whether at least one Http2ConnectionProvider was found - boolean haveHttp1Provider = false; - - for (ServerConnectionSelector provider : providers) { - if (provider instanceof Http1ConnectionSelector) { - haveHttp1Provider = true; - Http1Connection conn = (Http1Connection) provider.connection(mockContext()); - // Verify values to be updated from configuration file - assertThat(conn.config().maxPrologueLength(), is(4096)); - assertThat(conn.config().maxHeadersSize(), is(8192)); - assertThat(conn.config().validatePath(), is(false)); - assertThat(conn.config().validateHeaders(), is(false)); - } - } - assertThat("No Http12ConnectionProvider was found", haveHttp1Provider, is(true)); + TestProvider provider = new TestProvider(); + WebServer.builder().config(config.get("server")).addConnectionProvider(provider).build(); + assertThat(provider.isConfig(), is(true)); + Http1Config http1Config = provider.config(); + assertThat(http1Config.maxPrologueLength(), is(4096)); + assertThat(http1Config.maxHeadersSize(), is(8192)); + assertThat(http1Config.validatePath(), is(false)); + assertThat(http1Config.validateHeaders(), is(false)); } - @Test - void testConnectionProvidersDisabled() - throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { - - // This will pick up application.yaml from the classpath as default configuration file - Config config = Config.create(); - // Builds LoomServer instance including connectionProviders list using server2 node. - WebServer.Builder wsBuilder = WebServer.builder() - .config(config.get("server2")); + private static class TestProvider implements ServerConnectionProvider { - // Call wsBuilder.connectionProviders() trough reflection - Method connectionProviders - = WebServer.Builder.class.getDeclaredMethod("connectionProviders", (Class[]) null); - connectionProviders.setAccessible(true); - @SuppressWarnings("unchecked") - List providers - = (List) connectionProviders.invoke(wsBuilder, (Object[]) null); + private Http1Config http1Config = null; + private Config config = null; - // No providers shall be loaded with ServiceLoader disabled for connection providers. - assertThat(providers, notNullValue()); - assertThat(providers, is(empty())); - } - - // Check that WebServer ContentEncodingContext is disabled when disable is present in config - @Test - void testContentEncodingConfig() throws NoSuchFieldException, IllegalAccessException { - // This will pick up application.yaml from the classpath as default configuration file - Config config = Config.create(); + @Override + public Iterable configKeys() { + return List.of("http_1_1"); + } - // Builds LoomServer instance including connectionProviders list. - WebServer.Builder wsBuilder = WebServer.builder() - .config(config.get("server")); + @Override + public ServerConnectionSelector create(Function configs) { + config = configs.apply("http_1_1"); + http1Config = DefaultHttp1Config.toBuilder(config).build(); + return mock(ServerConnectionSelector.class); + } - // Access WebServer.Builder.contentEncodingContext trough reflection - Field contentEncodingContextField = WebServer.Builder.class.getDeclaredField("contentEncodingContext"); - contentEncodingContextField.setAccessible(true); - ContentEncodingContext contentEncodingContext = (ContentEncodingContext) contentEncodingContextField.get(wsBuilder); - // helidon-nima-http-encoding-gzip is on classpath so disabling ServiceLoader shall remove them - assertThat(contentEncodingContext.contentEncodingEnabled(), is(true)); - assertThat(contentEncodingContext.contentDecodingEnabled(), is(true)); - failsWith(() -> contentEncodingContext.decoder("gzip"), NoSuchElementException.class); - failsWith(() -> contentEncodingContext.decoder("gzip"), NoSuchElementException.class); - failsWith(() -> contentEncodingContext.encoder("gzip"), NoSuchElementException.class); - failsWith(() -> contentEncodingContext.decoder("x-gzip"), NoSuchElementException.class); - failsWith(() -> contentEncodingContext.encoder("x-gzip"), NoSuchElementException.class); - } + private Http1Config config() { + return http1Config; + } - // Verify that provided task throws an exception - private static void failsWith(Runnable task, Class exception) { - try { - task.run(); - // Fail the test when no Exception was thrown - fail(String.format("Exception %s was not thrown", exception.getName())); - } catch (Exception ex) { - if (!exception.isAssignableFrom(ex.getClass())) { - throw ex; - } + private boolean isConfig() { + return http1Config != null; } - } - private static ConnectionContext mockContext() { - ConnectionContext ctx = mock(ConnectionContext.class); - when(ctx.dataReader()).thenReturn(mock(DataReader.class)); - when(ctx.router()).thenReturn(Router.empty()); - when(ctx.serverContext()).thenReturn(mock(ServerContext.class)); - return ctx; } }