From a86976e0195e42077a8da06f7a70bcaef03f6e6f Mon Sep 17 00:00:00 2001 From: Andrew Ash Date: Thu, 20 Apr 2023 15:28:44 -0700 Subject: [PATCH 1/3] Add tests showing current behavior --- .../java/api/config/service/HostAndPort.java | 13 ++++++- .../api/config/service/HostAndPortTest.java | 38 +++++++++++++++++++ .../service/ProxyConfigurationTests.java | 10 +++++ 3 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 service-config/src/test/java/com/palantir/conjure/java/api/config/service/HostAndPortTest.java 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..75f3a7ad4 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,9 @@ import com.palantir.logsafe.Preconditions; import com.palantir.logsafe.SafeArg; +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; @@ -147,4 +148,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..666aed1d8 --- /dev/null +++ b/service-config/src/test/java/com/palantir/conjure/java/api/config/service/HostAndPortTest.java @@ -0,0 +1,38 @@ +/* + * (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 org.assertj.core.api.Assertions.assertThat; + +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() { + HostAndPort hostAndPort = HostAndPort.fromString("http://example.com:8080"); + assertThat(hostAndPort.getHost()).isEqualTo("http://example.com:8080"); + assertThat(hostAndPort.getPort()).isEqualTo(-1); + } +} 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..b268e812a 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 @@ -147,4 +147,14 @@ public void serDe() throws Exception { assertThat(ObjectMappers.newClientObjectMapper().readValue(kebabCase, ProxyConfiguration.class)) .isEqualTo(config); } + + @Test + public void incorrectlyFormattedHostAndPort() { + assertThatThrownBy(() -> ProxyConfiguration.builder() + .hostAndPort("http://squid:3128") + .type(ProxyConfiguration.Type.HTTP) + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("Given hostname does not contain a port number: {hostname=[http://squid:3128]}"); + } } From e3cb600390507bc8545097b26f00d37ff8ce040f Mon Sep 17 00:00:00 2001 From: Andrew Ash Date: Thu, 20 Apr 2023 15:52:15 -0700 Subject: [PATCH 2/3] Add specific error message for HostAndPort containing a protocol --- .../java/api/config/service/HostAndPort.java | 11 ++++++++++ .../api/config/service/HostAndPortTest.java | 22 ++++++++++++++++--- .../service/ProxyConfigurationTests.java | 8 ++++--- 3 files changed, 35 insertions(+), 6 deletions(-) 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 75f3a7ad4..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,6 +34,7 @@ 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}. This class is a re-implementation that throws safelog exceptions. */ @@ -93,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); } 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 index 666aed1d8..977547864 100644 --- 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 @@ -16,8 +16,10 @@ 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 { @@ -31,8 +33,22 @@ void fromString_onHostAndPort_succeeds() { @Test void fromString_onHostAndPortWithHttpProtocolPrefix_doesNotThrow() { - HostAndPort hostAndPort = HostAndPort.fromString("http://example.com:8080"); - assertThat(hostAndPort.getHost()).isEqualTo("http://example.com:8080"); - assertThat(hostAndPort.getPort()).isEqualTo(-1); + 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 b268e812a..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; @@ -150,11 +152,11 @@ public void serDe() throws Exception { @Test public void incorrectlyFormattedHostAndPort() { - assertThatThrownBy(() -> ProxyConfiguration.builder() + assertThatLoggableExceptionThrownBy(() -> ProxyConfiguration.builder() .hostAndPort("http://squid:3128") .type(ProxyConfiguration.Type.HTTP) .build()) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Given hostname does not contain a port number: {hostname=[http://squid:3128]}"); + .hasMessageContaining("hostPortString must not contain a protocol prefix like http:// or https://") + .containsArgs(UnsafeArg.of("hostPortString", "http://squid:3128")); } } From 31afb48b2c743d2489622100bb0f11c6648aa898 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Thu, 20 Apr 2023 23:01:59 +0000 Subject: [PATCH 3/3] Add generated changelog entries --- changelog/@unreleased/pr-983.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-983.v2.yml 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