diff --git a/changelog/@unreleased/pr-983.v2.yml b/changelog/@unreleased/pr-983.v2.yml new file mode 100644 index 000000000..c08986dca --- /dev/null +++ b/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 diff --git a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/HostAndPort.java b/service-config/src/main/java/com/palantir/conjure/java/api/config/service/HostAndPort.java index a8975b71b..30a384bc7 100644 --- a/service-config/src/main/java/com/palantir/conjure/java/api/config/service/HostAndPort.java +++ b/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; + } } diff --git a/service-config/src/test/java/com/palantir/conjure/java/api/config/service/HostAndPortTest.java b/service-config/src/test/java/com/palantir/conjure/java/api/config/service/HostAndPortTest.java new file mode 100644 index 000000000..977547864 --- /dev/null +++ b/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")); + } +} diff --git a/service-config/src/test/java/com/palantir/conjure/java/api/config/service/ProxyConfigurationTests.java b/service-config/src/test/java/com/palantir/conjure/java/api/config/service/ProxyConfigurationTests.java index f3958852e..346848e20 100644 --- a/service-config/src/test/java/com/palantir/conjure/java/api/config/service/ProxyConfigurationTests.java +++ b/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")); + } }