From 61c4adcc08604a366a75ba0a4e6885f661600d77 Mon Sep 17 00:00:00 2001 From: Tomasz Kowalczewski Date: Thu, 5 May 2022 23:16:12 +0200 Subject: [PATCH] Re #87: Added more refactorings and bugfixes. --- .../http/ClientConfigurationBuilder.java | 82 ++----------- .../tjahzi/http/ConnectionParams.java | 36 ++++++ .../tjahzi/http/ConnectionParamsFactory.java | 110 ++++++++++++++++++ .../http/ClientConfigurationBuilderTest.java | 81 ++++++++++--- 4 files changed, 223 insertions(+), 86 deletions(-) create mode 100644 core/src/main/java/pl/tkowalcz/tjahzi/http/ConnectionParams.java create mode 100644 core/src/main/java/pl/tkowalcz/tjahzi/http/ConnectionParamsFactory.java diff --git a/core/src/main/java/pl/tkowalcz/tjahzi/http/ClientConfigurationBuilder.java b/core/src/main/java/pl/tkowalcz/tjahzi/http/ClientConfigurationBuilder.java index 4059653..8fad5f0 100644 --- a/core/src/main/java/pl/tkowalcz/tjahzi/http/ClientConfigurationBuilder.java +++ b/core/src/main/java/pl/tkowalcz/tjahzi/http/ClientConfigurationBuilder.java @@ -97,13 +97,19 @@ public class ClientConfigurationBuilder { throw new IllegalArgumentException("Property maxRequestsInFlight must be greater than 0"); } - validateAndConfigureConnectionParameters(); - - return new ClientConfiguration( - logEndpoint, + ConnectionParams connectionParams = ConnectionParamsFactory.create( + url, host, port, - useSSL, + logEndpoint, + useSSL + ); + + return new ClientConfiguration( + connectionParams.getLogEndpoint(), + connectionParams.getHost(), + connectionParams.getPort(), + connectionParams.isUseSSL(), username, password, connectionTimeoutMillis, @@ -112,70 +118,4 @@ public class ClientConfigurationBuilder { maxRetries ); } - - private void validateAndConfigureConnectionParameters() { - if (url != null && host != null) { - throw new IllegalArgumentException("Only one of 'url' or 'host' can be configured. " + - "Current configuration sets url to '" + url + "' and host to '" + host + "'"); - } - - if (url == null && host == null) { - throw new IllegalArgumentException("One of 'url' or 'host' must be configured."); - } - - if (host != null) { - validateAndConfigureConnectionParametersNoUrl(); - } - - validateAndConfigureConnectionParametersWithUrl(); - } - - private void validateAndConfigureConnectionParametersWithUrl() { - try { - URL parsedUrl = new URL(url); - - String protocolScheme = parsedUrl.getProtocol(); - if (!HTTPS_STRING.equalsIgnoreCase(protocolScheme) && !HTTP_STRING.equalsIgnoreCase(protocolScheme)) { - throw new IllegalArgumentException("Unknown protocol scheme, must be one of 'https' or 'http' (case insensitive). Provided: '" + protocolScheme + "'"); - } - - host = parsedUrl.getHost(); - port = parsedUrl.getPort(); - - if (port == -1) { - if (parsedUrl.getProtocol().equalsIgnoreCase(HTTPS_STRING)) { - port = HTTPS_PORT; - } else { - port = HTTP_PORT; - } - } - - if (parsedUrl.getPath() != null && !parsedUrl.getPath().isEmpty()) { - if (logEndpoint != null) { - throw new IllegalArgumentException("If Loki connection URL contains path part then you cannot at the " + - "same time define log endpoint. Url: '" + url + "', log endpoint: '" + logEndpoint + "'"); - } - - logEndpoint = parsedUrl.getPath(); - } else { - logEndpoint = DEFAULT_LOG_ENDPOINT; - } - - if (HTTPS_STRING.equalsIgnoreCase(parsedUrl.getProtocol())) { - useSSL = true; - } - } catch (MalformedURLException e) { - throw new IllegalArgumentException("Creating configuration with Loki URL failed, url: '" + url + "'", e); - } - } - - private void validateAndConfigureConnectionParametersNoUrl() { - if (logEndpoint == null) { - logEndpoint = DEFAULT_LOG_ENDPOINT; - } - - if (!useSSL) { - useSSL = port == HTTPS_PORT; - } - } } diff --git a/core/src/main/java/pl/tkowalcz/tjahzi/http/ConnectionParams.java b/core/src/main/java/pl/tkowalcz/tjahzi/http/ConnectionParams.java new file mode 100644 index 0000000..484f354 --- /dev/null +++ b/core/src/main/java/pl/tkowalcz/tjahzi/http/ConnectionParams.java @@ -0,0 +1,36 @@ +package pl.tkowalcz.tjahzi.http; + +import java.net.MalformedURLException; +import java.net.URL; + +public class ConnectionParams { + + private final String host; + private final int port; + + private final String logEndpoint; + private final boolean useSSL; + + public ConnectionParams(String host, int port, String logEndpoint, boolean useSSL) { + this.host = host; + this.port = port; + this.logEndpoint = logEndpoint; + this.useSSL = useSSL; + } + + public String getHost() { + return host; + } + + public int getPort() { + return port; + } + + public String getLogEndpoint() { + return logEndpoint; + } + + public boolean isUseSSL() { + return useSSL; + } +} diff --git a/core/src/main/java/pl/tkowalcz/tjahzi/http/ConnectionParamsFactory.java b/core/src/main/java/pl/tkowalcz/tjahzi/http/ConnectionParamsFactory.java new file mode 100644 index 0000000..47ab96b --- /dev/null +++ b/core/src/main/java/pl/tkowalcz/tjahzi/http/ConnectionParamsFactory.java @@ -0,0 +1,110 @@ +package pl.tkowalcz.tjahzi.http; + +import java.net.MalformedURLException; +import java.net.URL; + +public class ConnectionParamsFactory { + + public static final int HTTPS_PORT = 443; + public static final int HTTP_PORT = 80; + public static final String HTTPS_STRING = "https"; + public static final String HTTP_STRING = "http"; + + public static final String DEFAULT_LOG_ENDPOINT = "/loki/api/v1/push"; + + public static ConnectionParams create(String url, String host, int port, String logEndpoint, boolean useSSL) { + if (url != null && host != null) { + throw new IllegalArgumentException("Only one of 'url' or 'host' can be configured. " + + "Current configuration sets url to '" + url + "' and host to '" + host + "'"); + } + + if (url != null && port != 0) { + throw new IllegalArgumentException("Only one of 'url' or 'port' can be configured. " + + "If using url you must configure port as part of url string. Current configuration sets url to '" + url + "' and port to '" + port + "'"); + } + + if (url != null && useSSL) { + throw new IllegalArgumentException("Only one of 'url' or 'useSSL' can be configured. " + + "Please use 'https' prefix in url to enable SSL. Current configuration sets url to '" + url + "' and useSSL to '" + useSSL + "'"); + } + + if (url == null && host == null) { + throw new IllegalArgumentException("One of 'url' or 'host' must be configured."); + } + + if (host != null) { + return validateAndConfigureConnectionParametersNoUrl(host, port, logEndpoint, useSSL); + } + + return validateAndConfigureConnectionParametersWithUrl(url, logEndpoint, useSSL); + } + + private static ConnectionParams validateAndConfigureConnectionParametersNoUrl(String host, int port, String logEndpoint, boolean useSSL) { + if (logEndpoint == null) { + logEndpoint = DEFAULT_LOG_ENDPOINT; + } + + if (!useSSL) { + useSSL = port == HTTPS_PORT; + } + + return new ConnectionParams(host, port, logEndpoint, useSSL); + } + + private static ConnectionParams validateAndConfigureConnectionParametersWithUrl(String url, String logEndpoint, boolean useSSL) { + try { + URL parsedUrl = new URL(url); + String host = parsedUrl.getHost(); + int port = configurePort(parsedUrl); + + useSSL = validateProtocolSchemeAndConfigureSSL(parsedUrl); + logEndpoint = configureLogEndpoint(parsedUrl, logEndpoint); + + return new ConnectionParams(host, port, logEndpoint, useSSL); + } catch (MalformedURLException e) { + throw new IllegalArgumentException("Creating configuration with Loki URL failed, url: '" + url + "'", e); + } + } + + private static String configureLogEndpoint(URL parsedUrl, String logEndpoint) { + if (logEndpoint != null) { + throw new IllegalArgumentException("Only one of 'url' or 'logEndpoint' can be configured. " + + "Embed log endpoint path into url. " + + "Log endpoint: '" + logEndpoint + "', url: '" + parsedUrl + "'"); + } + + if (parsedUrl.getPath() != null && !parsedUrl.getPath().isEmpty()) { + return parsedUrl.getPath(); + } else { + return DEFAULT_LOG_ENDPOINT; + } + } + + private static int configurePort(URL parsedUrl) { + int port = parsedUrl.getPort(); + + if (port == -1) { + if (HTTPS_STRING.equalsIgnoreCase(parsedUrl.getProtocol())) { + return HTTPS_PORT; + } else { + return HTTP_PORT; + } + } + + return port; + } + + private static boolean validateProtocolSchemeAndConfigureSSL(URL parsedUrl) { + String protocolScheme = parsedUrl.getProtocol(); + + if (HTTPS_STRING.equalsIgnoreCase(protocolScheme)) { + return true; + } + + if (HTTP_STRING.equalsIgnoreCase(protocolScheme)) { + return false; + } + + throw new IllegalArgumentException("Unknown protocol scheme, must be one of 'https' or 'http' (case insensitive). Provided: '" + protocolScheme + "'"); + } +} diff --git a/core/src/test/java/pl/tkowalcz/tjahzi/http/ClientConfigurationBuilderTest.java b/core/src/test/java/pl/tkowalcz/tjahzi/http/ClientConfigurationBuilderTest.java index 58b6049..516e869 100644 --- a/core/src/test/java/pl/tkowalcz/tjahzi/http/ClientConfigurationBuilderTest.java +++ b/core/src/test/java/pl/tkowalcz/tjahzi/http/ClientConfigurationBuilderTest.java @@ -1,5 +1,6 @@ package pl.tkowalcz.tjahzi.http; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -11,7 +12,7 @@ import static org.assertj.core.api.Assertions.assertThat; class ClientConfigurationBuilderTest { - static Stream configOptions() { + static Stream validConfigOptions() { return Stream.of( Arguments.of( "Should take arguments from url", @@ -30,7 +31,7 @@ class ClientConfigurationBuilderTest { false ), Arguments.of( - "Should use default logEndpoint is none is provided", + "Should use default logEndpoint if none is provided", "http://example.net:8443", "example.net", 8443, @@ -38,23 +39,67 @@ class ClientConfigurationBuilderTest { false ), Arguments.of( - "Should use default logEndpoint is none is provided", + "Should use default http port if none is provided", "http://example.net", "example.net", - 8443, + 80, "/loki/api/v1/push", false ), + Arguments.of( + "Should use default https port if none is provided", + "https://example.net", + "example.net", + 443, + "/loki/api/v1/push", + true + ) + ); + } + + static Stream invalidConfigOptions() { + return Stream.of( Arguments.of( "Should reject unknown protocol", - "loki://example.net" + ClientConfiguration.builder().withUrl("file://example.net"), + "Unknown protocol scheme, must be one of 'https' or 'http' (case insensitive). Provided: 'file'" + ), + Arguments.of( + "Should not allow url and host", + ClientConfiguration.builder().withUrl("file://example.net").withHost("blah"), + "Only one of 'url' or 'host' can be configured. Current configuration sets url to 'file://example.net' and host to 'blah'" + ), + Arguments.of( + "Should not allow url and port", + ClientConfiguration.builder().withUrl("file://example.net").withPort(10), + "Only one of 'url' or 'port' can be configured. If using url you must configure port as part of url string. Current configuration sets url to 'file://example.net' and port to '10'" + ), + Arguments.of( + "Should not allow url and useSSL to be defined", + ClientConfiguration.builder().withUrl("file://example.net").withUseSSL(true), + "Only one of 'url' or 'useSSL' can be configured. Please use 'https' prefix in url to enable SSL. Current configuration sets url to 'file://example.net' and useSSL to 'true'" + ), + Arguments.of( + "Should require max requests in flight to be positive", + ClientConfiguration.builder().withHost("example.net").withMaxRequestsInFlight(-10), + "Property maxRequestsInFlight must be greater than 0" + ), + Arguments.of( + "Should require at least url or host to be specified", + ClientConfiguration.builder(), + "One of 'url' or 'host' must be configured." + ), + Arguments.of( + "Should not allow url and logEndpoint to be specified at the same time", + ClientConfiguration.builder().withUrl("http://example.net").withLogEndpoint("blah") , + "Only one of 'url' or 'logEndpoint' can be configured. Embed log endpoint path into url. Log endpoint: 'blah', url: 'http://example.net'" ) ); } @ParameterizedTest(name = "{0}") - @MethodSource("configOptions") - void shouldGetConnectionParamsFromUrl( + @MethodSource("validConfigOptions") + void shouldCorrectlyInterpretUrl( String testCase, String url, String expectedHost, @@ -77,14 +122,20 @@ class ClientConfigurationBuilderTest { assertThat(configuration.isUseSSL()).isEqualTo(expectedUseSSL); } - @Test - void shouldRejectUnknownProtocolScheme() { - // Given - ClientConfigurationBuilder configurationBuilder = new ClientConfigurationBuilder(); - + @ParameterizedTest(name = "{0}") + @MethodSource("invalidConfigOptions") + void shouldRejectUnknownProtocolScheme( + String testCase, + ClientConfigurationBuilder clientConfigurationBuilder, + String expectedExceptionMessage + ) { // When - ClientConfiguration configuration = configurationBuilder - .withUrl("loki://example.net:8443/monitoring/loki/api/v1/push") - .build(); + IllegalArgumentException exception = Assertions.assertThrows( + IllegalArgumentException.class, + clientConfigurationBuilder::build + ); + + // Then + assertThat(exception).hasMessage(expectedExceptionMessage); } }