Re #87: Added more refactorings and bugfixes.

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;
-        }
-    }
 }

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;
+    }
+}

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 + "'");
+    }
+}

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<Arguments> configOptions() {
+    static Stream<Arguments> 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<Arguments> 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);
     }
 }