Repository: zalando/logbook
Pull request #115 (2016-06-27 20:11:02)

Allow passing of URIs without defined port
Changes in `RequestURI` and `LocalRequest` to handle an URI with an undefined port more explicit. The reason for this change is:

```
URI.create("https://example.com/no-port").getPort() == -1
```


Patch:
diff --git a/logbook-api/src/main/java/org/zalando/logbook/BaseHttpRequest.java b/logbook-api/src/main/java/org/zalando/logbook/BaseHttpRequest.java
index 27438bf9..a43e78d7 100644
--- a/logbook-api/src/main/java/org/zalando/logbook/BaseHttpRequest.java
+++ b/logbook-api/src/main/java/org/zalando/logbook/BaseHttpRequest.java
@@ -20,6 +20,8 @@ package org.zalando.logbook;
  * #L%
  */
 
+import java.util.Optional;
+
 public interface BaseHttpRequest extends BaseHttpMessage {
 
     String getRemote();
@@ -41,7 +43,7 @@ public interface BaseHttpRequest extends BaseHttpMessage {
 
     String getHost();
 
-    int getPort();
+    Optional<Integer> getPort();
 
     String getPath();
 
diff --git a/logbook-api/src/main/java/org/zalando/logbook/ForwardingHttpRequest.java b/logbook-api/src/main/java/org/zalando/logbook/ForwardingHttpRequest.java
index 8e948af3..06357e9a 100644
--- a/logbook-api/src/main/java/org/zalando/logbook/ForwardingHttpRequest.java
+++ b/logbook-api/src/main/java/org/zalando/logbook/ForwardingHttpRequest.java
@@ -20,6 +20,8 @@ package org.zalando.logbook;
  * #L%
  */
 
+import java.util.Optional;
+
 public abstract class ForwardingHttpRequest extends ForwardingHttpMessage implements HttpRequest {
 
     @Override
@@ -51,7 +53,7 @@ public abstract class ForwardingHttpRequest extends ForwardingHttpMessage implem
     }
 
     @Override
-    public int getPort() {
+    public Optional<Integer> getPort() {
         return delegate().getPort();
     }
 
diff --git a/logbook-api/src/main/java/org/zalando/logbook/ForwardingRawHttpRequest.java b/logbook-api/src/main/java/org/zalando/logbook/ForwardingRawHttpRequest.java
index 16cc917b..6339fbfd 100644
--- a/logbook-api/src/main/java/org/zalando/logbook/ForwardingRawHttpRequest.java
+++ b/logbook-api/src/main/java/org/zalando/logbook/ForwardingRawHttpRequest.java
@@ -24,6 +24,7 @@ import java.io.IOException;
 import java.nio.charset.Charset;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 
 public abstract class ForwardingRawHttpRequest implements RawHttpRequest {
 
@@ -75,7 +76,7 @@ public abstract class ForwardingRawHttpRequest implements RawHttpRequest {
     }
 
     @Override
-    public int getPort() {
+    public Optional<Integer> getPort() {
         return delegate().getPort();
     }
 
diff --git a/logbook-api/src/main/java/org/zalando/logbook/RequestURI.java b/logbook-api/src/main/java/org/zalando/logbook/RequestURI.java
index b764dbee..ddf37a16 100644
--- a/logbook-api/src/main/java/org/zalando/logbook/RequestURI.java
+++ b/logbook-api/src/main/java/org/zalando/logbook/RequestURI.java
@@ -21,6 +21,7 @@ package org.zalando.logbook;
  */
 
 import java.util.EnumSet;
+import java.util.Optional;
 import java.util.Set;
 
 import static java.util.Arrays.asList;
@@ -50,7 +51,7 @@ final class RequestURI {
     private static String reconstruct(final BaseHttpRequest request, final Set<Component> components) {
         final String scheme = request.getScheme();
         final String host = request.getHost();
-        final int port = request.getPort();
+        final Optional<Integer> port = request.getPort();
         final String path = request.getPath();
         final String query = request.getQuery();
 
@@ -63,9 +64,12 @@ final class RequestURI {
         if (components.contains(AUTHORITY)) {
             url.append("//").append(host);
 
-            if (isNotStandardPort(scheme, port)) {
-                url.append(':').append(port);
-            }
+            port.ifPresent(p -> {
+                if (isNotStandardPort(scheme, p)) {
+                    url.append(':').append(p);
+                }
+            });
+
         } else if (components.contains(SCHEME)) {
             url.append("//");
         }
diff --git a/logbook-httpclient/src/main/java/org/zalando/logbook/httpclient/LocalRequest.java b/logbook-httpclient/src/main/java/org/zalando/logbook/httpclient/LocalRequest.java
index 9f8d6ab1..d44a9fe8 100644
--- a/logbook-httpclient/src/main/java/org/zalando/logbook/httpclient/LocalRequest.java
+++ b/logbook-httpclient/src/main/java/org/zalando/logbook/httpclient/LocalRequest.java
@@ -106,9 +106,8 @@ final class LocalRequest implements RawHttpRequest, org.zalando.logbook.HttpRequ
     }
 
     @Override
-    public int getPort() {
-        final int port = originalRequestUri.getPort();
-        return port == -1 ? 80 : port; // TODO(whiskeysiera): is that safe to do?
+    public Optional<Integer> getPort() {
+        return Optional.of(originalRequestUri.getPort()).filter(p -> p != -1);
     }
 
     @Override
diff --git a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java
index f9fbf2dd..8dacea88 100644
--- a/logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java
+++ b/logbook-servlet/src/main/java/org/zalando/logbook/servlet/RemoteRequest.java
@@ -75,8 +75,8 @@ final class RemoteRequest extends HttpServletRequestWrapper implements RawHttpRe
     }
 
     @Override
-    public int getPort() {
-        return getServerPort();
+    public Optional<Integer> getPort() {
+        return Optional.of(getServerPort());
     }
 
     @Override


Test patch:
diff --git a/logbook-api/src/test/java/org/zalando/logbook/BaseHttpRequestTest.java b/logbook-api/src/test/java/org/zalando/logbook/BaseHttpRequestTest.java
index b6fd720a..6791a441 100644
--- a/logbook-api/src/test/java/org/zalando/logbook/BaseHttpRequestTest.java
+++ b/logbook-api/src/test/java/org/zalando/logbook/BaseHttpRequestTest.java
@@ -22,6 +22,8 @@ package org.zalando.logbook;
 
 import org.junit.Test;
 
+import java.util.Optional;
+
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertThat;
 import static org.mockito.Mockito.spy;
@@ -34,7 +36,7 @@ public final class BaseHttpRequestTest {
         final BaseHttpRequest unit = spy(BaseHttpRequest.class);
         when(unit.getScheme()).thenReturn("http");
         when(unit.getHost()).thenReturn("localhost");
-        when(unit.getPort()).thenReturn(80);
+        when(unit.getPort()).thenReturn(Optional.empty());
         when(unit.getPath()).thenReturn("/test");
         when(unit.getQuery()).thenReturn("limit=1");
 
diff --git a/logbook-api/src/test/java/org/zalando/logbook/ForwardingHttpRequestTest.java b/logbook-api/src/test/java/org/zalando/logbook/ForwardingHttpRequestTest.java
index 7d6aa54a..9d113d6e 100644
--- a/logbook-api/src/test/java/org/zalando/logbook/ForwardingHttpRequestTest.java
+++ b/logbook-api/src/test/java/org/zalando/logbook/ForwardingHttpRequestTest.java
@@ -23,6 +23,7 @@ package org.zalando.logbook;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.Optional;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Collections.emptyMap;
@@ -51,7 +52,7 @@ public final class ForwardingHttpRequestTest {
         assertThat(unit.getRequestUri(), is("http://localhost/"));
         assertThat(unit.getScheme(), is("http"));
         assertThat(unit.getHost(), is("localhost"));
-        assertThat(unit.getPort(), is(80));
+        assertThat(unit.getPort(), is(Optional.of(8080)));
         assertThat(unit.getPath(), is("/"));
         assertThat(unit.getQuery(), is(emptyString()));
         assertThat(unit.getProtocolVersion(), is("HTTP/1.1"));
@@ -71,7 +72,7 @@ public final class ForwardingHttpRequestTest {
         when(request.getRequestUri()).thenReturn("http://localhost/");
         when(request.getScheme()).thenReturn("http");
         when(request.getHost()).thenReturn("localhost");
-        when(request.getPort()).thenReturn(80);
+        when(request.getPort()).thenReturn(Optional.of(8080));
         when(request.getPath()).thenReturn("/");
         when(request.getQuery()).thenReturn("");
         when(request.getProtocolVersion()).thenReturn("HTTP/1.1");
diff --git a/logbook-api/src/test/java/org/zalando/logbook/ForwardingRawHttpRequestTest.java b/logbook-api/src/test/java/org/zalando/logbook/ForwardingRawHttpRequestTest.java
index 8e2c90a7..6f14c575 100644
--- a/logbook-api/src/test/java/org/zalando/logbook/ForwardingRawHttpRequestTest.java
+++ b/logbook-api/src/test/java/org/zalando/logbook/ForwardingRawHttpRequestTest.java
@@ -23,6 +23,7 @@ package org.zalando.logbook;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.Optional;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static java.util.Collections.emptyMap;
@@ -50,7 +51,7 @@ public final class ForwardingRawHttpRequestTest {
             when(request.getRequestUri()).thenReturn("http://localhost/");
             when(request.getScheme()).thenReturn("http");
             when(request.getHost()).thenReturn("localhost");
-            when(request.getPort()).thenReturn(80);
+            when(request.getPort()).thenReturn(Optional.of(8080));
             when(request.getPath()).thenReturn("/");
             when(request.getQuery()).thenReturn("");
             when(request.getProtocolVersion()).thenReturn("HTTP/1.1");
@@ -78,7 +79,7 @@ public final class ForwardingRawHttpRequestTest {
         assertThat(unit.getRequestUri(), is("http://localhost/"));
         assertThat(unit.getScheme(), is("http"));
         assertThat(unit.getHost(), is("localhost"));
-        assertThat(unit.getPort(), is(80));
+        assertThat(unit.getPort(), is(Optional.of(8080)));
         assertThat(unit.getPath(), is("/"));
         assertThat(unit.getQuery(), is(emptyString()));
         assertThat(unit.getProtocolVersion(), is("HTTP/1.1"));
@@ -98,7 +99,7 @@ public final class ForwardingRawHttpRequestTest {
         assertThat(request.getRequestUri(), is("http://localhost/"));
         assertThat(request.getScheme(), is("http"));
         assertThat(request.getHost(), is("localhost"));
-        assertThat(request.getPort(), is(80));
+        assertThat(request.getPort(), is(Optional.of(8080)));
         assertThat(request.getPath(), is("/"));
         assertThat(request.getQuery(), is(emptyString()));
         assertThat(request.getProtocolVersion(), is("HTTP/1.1"));
diff --git a/logbook-api/src/test/java/org/zalando/logbook/RequestURITest.java b/logbook-api/src/test/java/org/zalando/logbook/RequestURITest.java
index d50f7972..fbb9e756 100644
--- a/logbook-api/src/test/java/org/zalando/logbook/RequestURITest.java
+++ b/logbook-api/src/test/java/org/zalando/logbook/RequestURITest.java
@@ -26,6 +26,8 @@ import org.junit.runner.RunWith;
 import org.mockito.Mock;
 import org.mockito.runners.MockitoJUnitRunner;
 
+import java.util.Optional;
+
 import static org.hamcrest.Matchers.is;
 import static org.junit.Assert.assertThat;
 import static org.mockito.Mockito.when;
@@ -45,7 +47,7 @@ public final class RequestURITest {
     public void setUp() {
         when(request.getScheme()).thenReturn("http");
         when(request.getHost()).thenReturn("localhost");
-        when(request.getPort()).thenReturn(80);
+        when(request.getPort()).thenReturn(Optional.empty());
         when(request.getPath()).thenReturn("/admin");
         when(request.getQuery()).thenReturn("limit=1");
 
@@ -56,23 +58,30 @@ public final class RequestURITest {
         assertThat(reconstruct(request), is("http://localhost/admin?limit=1"));
     }
 
+    @Test
+    public void shouldNotIncludeStandardHttpPort() {
+        when(request.getScheme()).thenReturn("http");
+        when(request.getPort()).thenReturn(Optional.of(80));
+        assertThat(reconstruct(request), is("http://localhost/admin?limit=1"));
+    }
+
     @Test
     public void shouldNotIncludeStandardHttpsPort() {
         when(request.getScheme()).thenReturn("https");
-        when(request.getPort()).thenReturn(443);
+        when(request.getPort()).thenReturn(Optional.of(443));
         assertThat(reconstruct(request), is("https://localhost/admin?limit=1"));
     }
 
     @Test
     public void shouldIncludeNonStandardHttpPort() {
-        when(request.getPort()).thenReturn(8080);
+        when(request.getPort()).thenReturn(Optional.of(8080));
         assertThat(reconstruct(request), is("http://localhost:8080/admin?limit=1"));
     }
 
     @Test
     public void shouldIncludeNonStandardHttpsPort() {
         when(request.getScheme()).thenReturn("https");
-        when(request.getPort()).thenReturn(1443);
+        when(request.getPort()).thenReturn(Optional.of(1443));
         assertThat(reconstruct(request), is("https://localhost:1443/admin?limit=1"));
     }
 
diff --git a/logbook-httpclient/src/test/java/org/zalando/logbook/httpclient/LocalRequestTest.java b/logbook-httpclient/src/test/java/org/zalando/logbook/httpclient/LocalRequestTest.java
index b3bc121c..f3d809d3 100644
--- a/logbook-httpclient/src/test/java/org/zalando/logbook/httpclient/LocalRequestTest.java
+++ b/logbook-httpclient/src/test/java/org/zalando/logbook/httpclient/LocalRequestTest.java
@@ -31,7 +31,6 @@ import org.apache.http.client.utils.URIUtils;
 import org.apache.http.conn.routing.HttpRoute;
 import org.apache.http.entity.StringEntity;
 import org.apache.http.message.BasicHttpRequest;
-import org.apache.http.util.EntityUtils;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
diff --git a/logbook-test/src/main/java/org/zalando/logbook/MockHttpRequest.java b/logbook-test/src/main/java/org/zalando/logbook/MockHttpRequest.java
index 1d81ad76..d73e7569 100644
--- a/logbook-test/src/main/java/org/zalando/logbook/MockHttpRequest.java
+++ b/logbook-test/src/main/java/org/zalando/logbook/MockHttpRequest.java
@@ -55,7 +55,7 @@ public final class MockHttpRequest implements MockHttpMessage, HttpRequest {
             @Nullable final String method,
             @Nullable final String scheme,
             @Nullable final String host,
-            final int port,
+            @Nullable final Integer port,
             @Nullable final String path,
             @Nullable final String query,
             @Nullable final Map<String, List<String>> headers,
@@ -68,7 +68,7 @@ public final class MockHttpRequest implements MockHttpMessage, HttpRequest {
         this.method = Optional.ofNullable(method).orElse("GET");
         this.scheme = Optional.ofNullable(scheme).orElse("http");
         this.host = Optional.ofNullable(host).orElse("localhost");
-        this.port = port == 0 ? 80 : port;
+        this.port = Optional.ofNullable(port).orElse(80);
         this.path = Optional.ofNullable(path).orElse("/");
         this.query = Optional.ofNullable(query).orElse("");
         this.headers = firstNonNullNorEmpty(headers, emptyMap());
@@ -108,8 +108,8 @@ public final class MockHttpRequest implements MockHttpMessage, HttpRequest {
     }
 
     @Override
-    public int getPort() {
-        return port;
+    public Optional<Integer> getPort() {
+        return Optional.of(port);
     }
 
     @Override
diff --git a/logbook-test/src/main/java/org/zalando/logbook/MockRawHttpRequest.java b/logbook-test/src/main/java/org/zalando/logbook/MockRawHttpRequest.java
index 70427087..6ec44b7e 100644
--- a/logbook-test/src/main/java/org/zalando/logbook/MockRawHttpRequest.java
+++ b/logbook-test/src/main/java/org/zalando/logbook/MockRawHttpRequest.java
@@ -40,7 +40,7 @@ public final class MockRawHttpRequest implements MockHttpMessage, RawHttpRequest
     private final String method;
     private final String scheme;
     private final String host;
-    private final int port;
+    private final Integer port;
     private final String path;
     private final String query;
     private final Map<String, List<String>> headers;
@@ -56,7 +56,7 @@ public final class MockRawHttpRequest implements MockHttpMessage, RawHttpRequest
             @Nullable final String method,
             @Nullable final String scheme,
             @Nullable final String host,
-            final int port,
+            @Nullable final Integer port,
             @Nullable final String path,
             @Nullable final String query,
             @Nullable final Map<String, List<String>> headers,
@@ -69,7 +69,7 @@ public final class MockRawHttpRequest implements MockHttpMessage, RawHttpRequest
         this.method = Optional.ofNullable(method).orElse("GET");
         this.scheme = Optional.ofNullable(scheme).orElse("http");
         this.host = Optional.ofNullable(host).orElse("localhost");
-        this.port = port == 0 ? 80 : port;
+        this.port = Optional.ofNullable(port).orElse(80);
         this.path = Optional.ofNullable(path).orElse("/");
         this.query = Optional.ofNullable(query).orElse("");
         this.headers = firstNonNullNorEmpty(headers, emptyMap());
@@ -104,8 +104,8 @@ public final class MockRawHttpRequest implements MockHttpMessage, RawHttpRequest
     }
 
     @Override
-    public int getPort() {
-        return port;
+    public Optional<Integer> getPort() {
+        return Optional.of(port);
     }
 
     @Override
diff --git a/logbook-test/src/test/java/org/zalando/logbook/MockHttpMessageTester.java b/logbook-test/src/test/java/org/zalando/logbook/MockHttpMessageTester.java
index 666c093e..a65d122c 100644
--- a/logbook-test/src/test/java/org/zalando/logbook/MockHttpMessageTester.java
+++ b/logbook-test/src/test/java/org/zalando/logbook/MockHttpMessageTester.java
@@ -21,6 +21,7 @@ package org.zalando.logbook;
  */
 
 import java.io.IOException;
+import java.util.Optional;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.hamcrest.Matchers.empty;
@@ -40,7 +41,7 @@ public interface MockHttpMessageTester {
         assertThat(unit.getRequestUri(), is("http://localhost/"));
         assertThat(unit.getScheme(), is("http"));
         assertThat(unit.getHost(), is("localhost"));
-        assertThat(unit.getPort(), is(80));
+        assertThat(unit.getPort(), is(Optional.of(80)));
         assertThat(unit.getPath(), is("/"));
         assertThat(unit.getQuery(), is(emptyString()));
         assertThat(unit.getProtocolVersion(), is("HTTP/1.1"));
diff --git a/logbook-test/src/test/java/org/zalando/logbook/MockRawHttpRequestTest.java b/logbook-test/src/test/java/org/zalando/logbook/MockRawHttpRequestTest.java
index 2e366d9d..28629258 100644
--- a/logbook-test/src/test/java/org/zalando/logbook/MockRawHttpRequestTest.java
+++ b/logbook-test/src/test/java/org/zalando/logbook/MockRawHttpRequestTest.java
@@ -23,6 +23,7 @@ package org.zalando.logbook;
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.Optional;
 
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.hamcrest.Matchers.is;
@@ -49,7 +50,7 @@ public final class MockRawHttpRequestTest implements MockHttpMessageTester {
     public void shouldUseNonDefaultPort() {
         final MockRawHttpRequest unit = MockRawHttpRequest.request().port(8080).build();
 
-        assertThat(unit.getPort(), is(8080));
+        assertThat(unit.getPort(), is(Optional.of(8080)));
     }
 
 }
\ No newline at end of file