carterkozak commented on a change in pull request #274:
URL: 
https://github.com/apache/httpcomponents-core/pull/274#discussion_r607095304



##########
File path: httpcore5/src/main/java/org/apache/hc/core5/net/URIBuilder.java
##########
@@ -333,7 +333,12 @@ private void digestURI(final URI uri, final Charset 
charset) {
         this.scheme = uri.getScheme();
         this.encodedSchemeSpecificPart = uri.getRawSchemeSpecificPart();
         this.encodedAuthority = uri.getRawAuthority();
-        this.host = uri.getHost();
+        final String uriHost = uri.getHost();
+        // URI.getHost incorrectly returns bracketed (encoded) IPv6 values. 
Brackets are an
+        // encoding detail of the URI and not part of the host string.
+        this.host = uriHost != null && 
InetAddressUtils.isIPv6URLBracketedAddress(uriHost)

Review comment:
       I don't think so, the ipv6 address as a host value matches the ipv6 
pattern which you've pointed out is less ambiguous than the bracketed value 
(when used in isolation, not within a URI), so there's no reason to match the 
java `URI.getHost` brackets.

##########
File path: httpcore5/src/test/java/org/apache/hc/core5/net/TestURIBuilder.java
##########
@@ -813,4 +813,32 @@ public void testNormalizeSyntax() throws Exception {
                 new 
URIBuilder("http:///../../.././";).normalizeSyntax().build().toASCIIString());
     }
 
+    @Test
+    public void testIpv6Host() throws Exception {
+        final URI uri = new URIBuilder("https://[::1]:432/path";).build();
+        Assert.assertEquals(432, uri.getPort());
+        Assert.assertEquals("https", uri.getScheme());
+        Assert.assertEquals("[::1]", uri.getHost());
+        Assert.assertEquals("/path", uri.getPath());
+    }
+
+    @Test
+    public void testIpv6HostWithPortUpdate() throws Exception {
+        // Updating the port clears URIBuilder.encodedSchemeSpecificPart
+        // and bypasses the fast/simple path which preserves input.
+        final URI uri = new 
URIBuilder("https://[::1]:432/path";).setPort(123).build();
+        Assert.assertEquals(123, uri.getPort());
+        Assert.assertEquals("https", uri.getScheme());
+        Assert.assertEquals("[::1]", uri.getHost());

Review comment:
       There's nothing we can do in this case because we're operating on the 
constructed `URI` object, not the `URIBuilder`. `URIBuilder.getHost` does 
return `::1`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to