Repository: apple/servicetalk (java)

Issue:
HttpRequestMetaData scheme case sensitivity
`HttpRequestMetaData#scheme()` javadocs state the result will be lowercase, but this is not always the case. `DefaultMultiAddressUrlHttpClientBuilder` has a few instances (`CachingKeyFactory`, `UrlKey`) which rely upon the lowercase behavior describe in `HttpRequestMetaData#scheme()`. In general the comparisons should be done in a case insensitive fashion [1].

[1] https://tools.ietf.org/html/rfc3986#section-3.1
> Although schemes are case-
   insensitive, the canonical form is lowercase and documents that
   specify schemes must do so with lowercase letters.  An implementation
   should accept uppercase letters as equivalent to lowercase in scheme
   names (e.g., allow "HTTP" as well as "http") for the sake of
   robustness but should only produce lowercase scheme names for
   consistency.

original context: https://github.com/apple/servicetalk/pull/1387#discussion_r589684620

Pull request:
Convert scheme to lower case when parsing URI
Motivation:

`HttpRequestMetaData#scheme()` javadocs state the result will be lowercase, but this is not always the case because `Uri3986` parses the scheme as-is. As a result, `DefaultMultiAddressUrlHttpClientBuilder` may have more than 1 client instance for the same address if users use URI with different scheme letters case. 
See https://github.com/apple/servicetalk/issues/1423 for details.

Modifications:

- Convert scheme to lower case when parsing `Uri3986` reusing well-known schemes ("http" and "https")

Result:

Fixes #1423.

Patch:
diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/Uri3986.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/Uri3986.java
index f5f03b8a6..799d72b12 100644
--- a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/Uri3986.java
+++ b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/Uri3986.java
@@ -17,6 +17,7 @@
 package io.servicetalk.http.api;
 
 import java.nio.charset.Charset;
+import java.util.Locale;
 import java.util.Objects;
 import javax.annotation.Nullable;
 
@@ -38,6 +39,8 @@ import static io.servicetalk.http.api.UriUtils.parsePort;
 final class Uri3986 implements Uri {
     @SuppressWarnings({"StringOperationCanBeSimplified", "PMD.StringInstantiation"})
     private static final String NULL_COMPONENT = new String(""); // instance equality required!
+    private static final String HTTP_SCHEME = "http";
+    private static final String HTTPS_SCHEME = "https";
     private final String uri;
     @Nullable
     private final String scheme;
@@ -169,7 +172,7 @@ final class Uri3986 implements Uri {
                 if (i == 0) {
                     throw new IllegalArgumentException("Invalid URI format: no scheme before colon (':')");
                 }
-                parsedScheme = uri.substring(0, i);
+                parsedScheme = getLowerCaseScheme(uri, i);
                 begin = ++i;
                 // We don't enforce the following, browsers still generate these types of requests.
                 // https://tools.ietf.org/html/rfc3986#section-3.3
@@ -196,6 +199,15 @@ final class Uri3986 implements Uri {
         this.uri = uri;
     }
 
+    private static String getLowerCaseScheme(final String uri, final int endIndex) {
+        if (endIndex == 4 && uri.regionMatches(true, 0, HTTP_SCHEME, 0, 4)) {
+            return HTTP_SCHEME;
+        } else if (endIndex == 5 && uri.regionMatches(true, 0, HTTPS_SCHEME, 0, 5)) {
+            return HTTPS_SCHEME;
+        }
+        return uri.substring(0, endIndex).toLowerCase(Locale.ENGLISH);
+    }
+
     @Override
     public String uri() {
         return uri;


Test patch:
diff --git a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/Uri3986Test.java b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/Uri3986Test.java
index 482af6505..4de5f11cc 100644
--- a/servicetalk-http-api/src/test/java/io/servicetalk/http/api/Uri3986Test.java
+++ b/servicetalk-http-api/src/test/java/io/servicetalk/http/api/Uri3986Test.java
@@ -31,7 +31,10 @@ import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.regex.Pattern.compile;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalToIgnoringCase;
 import static org.hamcrest.Matchers.lessThan;
+import static org.hamcrest.Matchers.nullValue;
+import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
@@ -162,8 +165,21 @@ class Uri3986Test {
     }
 
     @Test
-    void schemeParsedCaseSensative() {
-        verifyUri3986("HTTP://test.com", "HTTP", null, "test.com", -1, "", "", null, null, null);
+    void schemeParsedCaseInsensitive() {
+        verifyUri3986("HTTP://test.com", "http", null, "test.com", -1, "", "", null, null, null);
+        verifyUri3986("HTTPS://test.com", "https", null, "test.com", -1, "", "", null, null, null);
+        verifyUri3986("ANY://test.com", "any", null, "test.com", -1, "", "", null, null, null);
+    }
+
+    @Test
+    void reuseWellKnownScheme() {
+        String http1 = new Uri3986("http://test.com").scheme();
+        String http2 = new Uri3986("HTTP://test.com").scheme();
+        assertThat("different http", http1, sameInstance(http2));
+
+        String https1 = new Uri3986("https://test.com").scheme();
+        String https2 = new Uri3986("HTTPS://test.com").scheme();
+        assertThat("different https", https1, sameInstance(https2));
     }
 
     @Test
@@ -637,7 +653,9 @@ class Uri3986Test {
         // Validate against the RFC's regex
         Matcher matcher = VALID_PATTERN.matcher(expectedUri);
         assertThat(true, is(matcher.matches()));
-        assertThat("invalid scheme()", uri.scheme(), is(matcher.group(2)));
+        String matcherScheme = matcher.group(2);
+        assertThat("invalid scheme()", uri.scheme(),
+                matcherScheme == null ? is(nullValue()) : equalToIgnoringCase(matcherScheme));
         assertThat("invalid authority()", uri.authority(), is(matcher.group(4)));
         assertThat("invalid path()", uri.path(), is(matcher.group(5)));
         assertThat("invalid query()", uri.query(), is(matcher.group(7)));
diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilderTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilderTest.java
index 4c9ac9260..4dff5b0a9 100644
--- a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilderTest.java
+++ b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/DefaultMultiAddressUrlHttpClientBuilderTest.java
@@ -37,6 +37,7 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 import java.net.InetSocketAddress;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static io.servicetalk.buffer.api.Matchers.contentEqualTo;
@@ -222,6 +223,28 @@ class DefaultMultiAddressUrlHttpClientBuilderTest {
         }
     }
 
+    @Test
+    void internalClientSchemeShouldBeCaseInsensitive() throws Exception {
+        AtomicInteger counter = new AtomicInteger();
+
+        try (ServerContext serverContext = HttpServers.forAddress(localAddress(0))
+                .listenStreamingAndAwait((ctx, request, responseFactory) -> succeeded(responseFactory.ok()));
+             BlockingHttpClient blockingHttpClient = HttpClients.forMultiAddressUrl(getClass().getSimpleName())
+                     .initializer((scheme, address, builder) -> counter.incrementAndGet())
+                     .buildBlocking()) {
+
+            HttpResponse response = blockingHttpClient.request(
+                    blockingHttpClient.get("http://" + serverHostAndPort(serverContext)));
+            assertThat(response.status(), is(OK));
+
+            response = blockingHttpClient.request(
+                    blockingHttpClient.get("HTTP://" + serverHostAndPort(serverContext)));
+            assertThat(response.status(), is(OK));
+
+            assertThat(counter.get(), is(1));
+        }
+    }
+
     private static void assertExecutionContext(HttpExecutionContext expected, HttpExecutionContext actual) {
         assertThat(actual, is(notNullValue()));
         assertThat(actual.ioExecutor(), is(sameInstance(expected.ioExecutor())));