Better error message for HostAndPort containing a protocol

changelog/@unreleased/pr-983.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Better error message for HostAndPort containing a protocol
+  links:
+  - https://github.com/palantir/conjure-java-runtime-api/pull/983

service-config/src/main/java/com/palantir/conjure/java/api/config/service/HostAndPort.java

@@ -34,8 +34,10 @@
 
 import com.palantir.logsafe.Preconditions;
 import com.palantir.logsafe.SafeArg;
+import com.palantir.logsafe.UnsafeArg;
+import org.jetbrains.annotations.VisibleForTesting;
 
-/** See Guava's {@code HostAndPort}. */
+/** See Guava's {@code HostAndPort}. This class is a re-implementation that throws safelog exceptions. */
 public final class HostAndPort {
     /** Magic value indicating the absence of a port number. */
     private static final int NO_PORT = -1;
@@ -92,6 +94,16 @@ public static HostAndPort fromString(String hostPortString) {
                     isValidPort(port), "Port number out of range", SafeArg.of("port", hostPortString));
         }
 
+        // Additional validation for common errors. An alternative to this diff between conjure HostAndPort and
+        // Guava HostAndPort would be to use Guava's HostSpecifier.from() here instead.  But that would require
+        // either re-adding a Guava dependency (removed earlier in this repo's history), or copying that class
+        // and its dependencies (InetAddresses, InternetDomainName, Ascii.toLowerCase, Splitter, Joiner, and more)
+        // into this repo like we did with HostAndPort.
+        Preconditions.checkArgument(
+                !host.contains("://"),
+                "hostPortString must not contain a protocol prefix like http:// or https://",
+                UnsafeArg.of("hostPortString", hostPortString));
+
         return new HostAndPort(host, port);
     }
 
@@ -147,4 +159,14 @@ public String toString() {
     private static boolean isValidPort(int port) {
         return port >= 0 && port <= 65535;
     }
+
+    @VisibleForTesting
+    String getHost() {
+        return host;
+    }
+
+    @VisibleForTesting
+    int getPort() {
+        return port;
+    }
 }

service-config/src/test/java/com/palantir/conjure/java/api/config/service/HostAndPortTest.java

@@ -0,0 +1,54 @@
+/*
+ * (c) Copyright 2023 Palantir Technologies Inc. All rights reserved.
+ *
+ * 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 com.palantir.conjure.java.api.config.service;
+
+import static com.palantir.logsafe.testing.Assertions.assertThatLoggableExceptionThrownBy;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.palantir.logsafe.UnsafeArg;
+import org.junit.jupiter.api.Test;
+
+class HostAndPortTest {
+
+    @Test
+    void fromString_onHostAndPort_succeeds() {
+        HostAndPort hostAndPort = HostAndPort.fromString("example.com:8080");
+        assertThat(hostAndPort.getHost()).isEqualTo("example.com");
+        assertThat(hostAndPort.getPort()).isEqualTo(8080);
+    }
+
+    @Test
+    void fromString_onHostAndPortWithHttpProtocolPrefix_doesNotThrow() {
+        assertThatLoggableExceptionThrownBy(() -> HostAndPort.fromString("http://example.com:8080"))
+                .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://")
+                .containsArgs(UnsafeArg.of("hostPortString", "http://example.com:8080"));
+    }
+
+    @Test
+    void fromString_onHostAndPortWithHttpsProtocolPrefix_doesNotThrow() {
+        assertThatLoggableExceptionThrownBy(() -> HostAndPort.fromString("https://example.com:8080"))
+                .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://")
+                .containsArgs(UnsafeArg.of("hostPortString", "https://example.com:8080"));
+    }
+
+    @Test
+    void fromString_onHostAndPortWithAbcProtocolPrefix_doesNotThrow() {
+        assertThatLoggableExceptionThrownBy(() -> HostAndPort.fromString("abc://example.com:8080"))
+                .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://")
+                .containsArgs(UnsafeArg.of("hostPortString", "abc://example.com:8080"));
+    }
+}

service-config/src/test/java/com/palantir/conjure/java/api/config/service/ProxyConfigurationTests.java

@@ -16,6 +16,7 @@
 
 package com.palantir.conjure.java.api.config.service;
 
+import static com.palantir.logsafe.testing.Assertions.assertThatLoggableExceptionThrownBy;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
@@ -24,6 +25,7 @@
 import com.fasterxml.jackson.datatype.jdk8.Jdk8Module;
 import com.google.common.io.Resources;
 import com.palantir.conjure.java.api.ext.jackson.ObjectMappers;
+import com.palantir.logsafe.UnsafeArg;
 import java.io.IOException;
 import java.net.URL;
 import org.junit.jupiter.api.Test;
@@ -147,4 +149,14 @@ public void serDe() throws Exception {
         assertThat(ObjectMappers.newClientObjectMapper().readValue(kebabCase, ProxyConfiguration.class))
                 .isEqualTo(config);
     }
+
+    @Test
+    public void incorrectlyFormattedHostAndPort() {
+        assertThatLoggableExceptionThrownBy(() -> ProxyConfiguration.builder()
+                        .hostAndPort("http://squid:3128")
+                        .type(ProxyConfiguration.Type.HTTP)
+                        .build())
+                .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://")
+                .containsArgs(UnsafeArg.of("hostPortString", "http://squid:3128"));
+    }
 }