From 62dee9a0ef4f3891bf9d9230f4de14cd0ac0bfeb Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Wed, 13 Jun 2018 17:58:17 +0100 Subject: [PATCH 1/4] SimpleHttpClient: Send to default HTTP/HTTPS ports when port numbers are not specified. --- .../hotels/styx/client/SimpleHttpClient.java | 8 +- .../styx/client/SimpleHttpClientTest.java | 149 ++++++++++++------ 2 files changed, 110 insertions(+), 47 deletions(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java index 7524d27e06..fc52b339c7 100644 --- a/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java @@ -84,7 +84,13 @@ private static Origin originFromRequest(FullHttpRequest request) { .orElseThrow(() -> new IllegalArgumentException("Cannot send request " + request + " as URL is not absolute and no HOST header is present")); }); - return newOriginBuilder(HostAndPort.fromString(hostAndPort)).build(); + HostAndPort host = HostAndPort.fromString(hostAndPort); + + if (host.getPortOrDefault(-1) < 0) { + host = host.withDefaultPort(request.isSecure() ? 443 : 80); + } + + return newOriginBuilder(host).build(); } /** diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/SimpleHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/SimpleHttpClientTest.java index 7c17a611a1..5bd7b4b67b 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/SimpleHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/SimpleHttpClientTest.java @@ -22,6 +22,7 @@ import com.hotels.styx.api.HttpRequest; import com.hotels.styx.api.HttpResponse; import com.hotels.styx.api.client.Connection; +import com.hotels.styx.api.client.Origin; import com.hotels.styx.api.service.TlsSettings; import org.mockito.ArgumentCaptor; import org.testng.annotations.BeforeMethod; @@ -36,6 +37,7 @@ import static com.hotels.styx.api.FullHttpRequest.get; import static com.hotels.styx.api.HttpHeaderNames.HOST; import static com.hotels.styx.api.HttpHeaderNames.USER_AGENT; +import static com.hotels.styx.api.HttpResponse.response; import static com.hotels.styx.api.messages.HttpResponseStatus.OK; import static com.hotels.styx.client.Protocol.HTTP; import static com.hotels.styx.client.Protocol.HTTPS; @@ -62,7 +64,7 @@ public void setUp() { } @Test - public void sendsHttp() throws IOException { + public void sendsHttp() { withOrigin(HTTP, port -> { FullHttpResponse response = await(httpClient().sendRequest(httpRequest(port))); assertThat(response.status(), is(OK)); @@ -70,7 +72,7 @@ public void sendsHttp() throws IOException { } @Test - public void sendsHttps() throws IOException { + public void sendsHttps() { withOrigin(HTTPS, port -> { FullHttpResponse response = await(httpsClient().sendRequest(httpsRequest(port))); assertThat(response.status(), is(OK)); @@ -78,7 +80,7 @@ public void sendsHttps() throws IOException { } @Test(expectedExceptions = Exception.class) - public void cannotSendHttpsWhenConfiguredForHttp() throws IOException { + public void cannotSendHttpsWhenConfiguredForHttp() { withOrigin(HTTPS, port -> { FullHttpResponse response = await(httpClient().sendRequest(httpsRequest(port))); assertThat(response.status(), is(OK)); @@ -93,50 +95,10 @@ public void cannotSendHttpWhenConfiguredForHttps() throws IOException { }); } - private SimpleHttpClient httpClient() { - return new SimpleHttpClient.Builder() - .build(); - } - - private SimpleHttpClient httpsClient() { - return new SimpleHttpClient.Builder() - .connectionSettings(new ConnectionSettings(1000)) - .tlsSettings(new TlsSettings.Builder() - .authenticate(false) - .build()) - .responseTimeoutMillis(6000) - .build(); - } - - private FullHttpRequest httpRequest(int port) { - return get("http://localhost:" + port) - .header(HOST, "localhost:" + port) - .build(); - } - - private FullHttpRequest httpsRequest(int port) { - return get("https://localhost:" + port) - .header(HOST, "localhost:" + port) - .build(); - } - - private void withOrigin(Protocol protocol, IntConsumer portConsumer) { - WireMockServer server = new WireMockServer(wireMockConfig().dynamicPort().dynamicHttpsPort()); - - try { - server.start(); - int port = protocol == HTTP ? server.port() : server.httpsPort(); - server.stubFor(WireMock.get(urlStartingWith("/")).willReturn(aResponse().withStatus(200))); - portConsumer.accept(port); - } finally { - server.stop(); - } - } - @Test public void willNotSetAnyUserAgentIfNotSpecified() { Connection mockConnection = mock(Connection.class); - when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(HttpResponse.response().build())); + when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); SimpleHttpClient client = new SimpleHttpClient.Builder() .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) @@ -152,7 +114,7 @@ public void willNotSetAnyUserAgentIfNotSpecified() { @Test public void setsTheSpecifiedUserAgentWhenSpecified() { Connection mockConnection = mock(Connection.class); - when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(HttpResponse.response().build())); + when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); SimpleHttpClient client = new SimpleHttpClient.Builder() .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) @@ -169,7 +131,7 @@ public void setsTheSpecifiedUserAgentWhenSpecified() { @Test public void retainsTheUserAgentStringFromTheRequest() { Connection mockConnection = mock(Connection.class); - when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(HttpResponse.response().build())); + when(mockConnection.write(any(HttpRequest.class))).thenReturn(Observable.just(response().build())); SimpleHttpClient client = new SimpleHttpClient.Builder() .setConnectionFactory((origin, connectionSettings) -> Observable.just(mockConnection)) @@ -194,4 +156,99 @@ public void requestWithNoHostOrUrlAuthorityCausesException() { await(client.sendRequest(request)); } + + @Test + public void sendsToDefaultHttpPort() { + Connection.Factory connectionFactory = mockConnectionFactory(mockConnection(response(OK).build())); + + SimpleHttpClient client = new SimpleHttpClient.Builder() + .setConnectionFactory(connectionFactory) + .build(); + + await(client.sendRequest( + FullHttpRequest.get("/") + .header(HOST, "localhost") + .build() + )); + + ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); + verify(connectionFactory).createConnection(originCaptor.capture(), any(Connection.Settings.class)); + + assertThat(originCaptor.getValue().host().getPort(), is(80)); + } + + @Test + public void sendsToDefaultHttpsPort() { + Connection.Factory connectionFactory = mockConnectionFactory(mockConnection(response(OK).build())); + + SimpleHttpClient client = new SimpleHttpClient.Builder() + .setConnectionFactory(connectionFactory) + .build(); + + await(client.sendRequest( + FullHttpRequest.get("/") + .secure(true) + .header(HOST, "localhost") + .build() + )); + + ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); + verify(connectionFactory).createConnection(originCaptor.capture(), any(Connection.Settings.class)); + + assertThat(originCaptor.getValue().host().getPort(), is(443)); + } + + private SimpleHttpClient httpClient() { + return new SimpleHttpClient.Builder() + .build(); + } + + private SimpleHttpClient httpsClient() { + return new SimpleHttpClient.Builder() + .connectionSettings(new ConnectionSettings(1000)) + .tlsSettings(new TlsSettings.Builder() + .authenticate(false) + .build()) + .responseTimeoutMillis(6000) + .build(); + } + + private FullHttpRequest httpRequest(int port) { + return get("http://localhost:" + port) + .header(HOST, "localhost:" + port) + .build(); + } + + private FullHttpRequest httpsRequest(int port) { + return get("https://localhost:" + port) + .header(HOST, "localhost:" + port) + .build(); + } + + private void withOrigin(Protocol protocol, IntConsumer portConsumer) { + WireMockServer server = new WireMockServer(wireMockConfig().dynamicPort().dynamicHttpsPort()); + + try { + server.start(); + int port = protocol == HTTP ? server.port() : server.httpsPort(); + server.stubFor(WireMock.get(urlStartingWith("/")).willReturn(aResponse().withStatus(200))); + portConsumer.accept(port); + } finally { + server.stop(); + } + } + + private Connection mockConnection(HttpResponse response) { + Connection connection = mock(Connection.class); + when(connection.write(any(HttpRequest.class))).thenReturn(Observable.just(response)); + return connection; + } + + private Connection.Factory mockConnectionFactory(Connection connection) { + Connection.Factory factory = mock(Connection.Factory.class); + when(factory.createConnection(any(Origin.class), any(Connection.Settings.class))) + .thenReturn(Observable.just(connection)); + return factory; + } + } From 2083996367eb8047efe554a75b0a2f740399defe Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Wed, 13 Jun 2018 17:59:32 +0100 Subject: [PATCH 2/4] Tidy up. --- .../main/java/com/hotels/styx/client/SimpleHttpClient.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java index fc52b339c7..74785dfcb3 100644 --- a/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java @@ -43,6 +43,9 @@ * A client that uses netty as transport. */ public final class SimpleHttpClient implements FullHttpClient { + private static final int DEFAULT_HTTPS_PORT = 443; + private static final int DEFAULT_HTTP_PORT = 80; + private final Optional userAgent; private final ConnectionSettings connectionSettings; private final int maxResponseSize; @@ -87,7 +90,7 @@ private static Origin originFromRequest(FullHttpRequest request) { HostAndPort host = HostAndPort.fromString(hostAndPort); if (host.getPortOrDefault(-1) < 0) { - host = host.withDefaultPort(request.isSecure() ? 443 : 80); + host = host.withDefaultPort(request.isSecure() ? DEFAULT_HTTPS_PORT : DEFAULT_HTTP_PORT); } return newOriginBuilder(host).build(); From bc50eeacc3f59ebc2abb9beb93a688cccaa6ad69 Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Wed, 13 Jun 2018 18:01:09 +0100 Subject: [PATCH 3/4] Tidy up. --- .../src/main/java/com/hotels/styx/client/SimpleHttpClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java b/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java index 74785dfcb3..58e28021d2 100644 --- a/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java +++ b/components/client/src/main/java/com/hotels/styx/client/SimpleHttpClient.java @@ -45,7 +45,7 @@ public final class SimpleHttpClient implements FullHttpClient { private static final int DEFAULT_HTTPS_PORT = 443; private static final int DEFAULT_HTTP_PORT = 80; - + private final Optional userAgent; private final ConnectionSettings connectionSettings; private final int maxResponseSize; From 11ca525ed18ffefaa0149a856984f079c2d47b57 Mon Sep 17 00:00:00 2001 From: Mikko Karjalainen Date: Thu, 14 Jun 2018 10:58:20 +0100 Subject: [PATCH 4/4] Remove unnecessary await(). --- .../styx/client/SimpleHttpClientTest.java | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/components/client/src/test/unit/java/com/hotels/styx/client/SimpleHttpClientTest.java b/components/client/src/test/unit/java/com/hotels/styx/client/SimpleHttpClientTest.java index 5bd7b4b67b..0538ddfae0 100644 --- a/components/client/src/test/unit/java/com/hotels/styx/client/SimpleHttpClientTest.java +++ b/components/client/src/test/unit/java/com/hotels/styx/client/SimpleHttpClientTest.java @@ -165,11 +165,9 @@ public void sendsToDefaultHttpPort() { .setConnectionFactory(connectionFactory) .build(); - await(client.sendRequest( - FullHttpRequest.get("/") - .header(HOST, "localhost") - .build() - )); + client.sendRequest(get("/") + .header(HOST, "localhost") + .build()); ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); verify(connectionFactory).createConnection(originCaptor.capture(), any(Connection.Settings.class)); @@ -185,12 +183,10 @@ public void sendsToDefaultHttpsPort() { .setConnectionFactory(connectionFactory) .build(); - await(client.sendRequest( - FullHttpRequest.get("/") - .secure(true) - .header(HOST, "localhost") - .build() - )); + client.sendRequest(get("/") + .secure(true) + .header(HOST, "localhost") + .build()); ArgumentCaptor originCaptor = ArgumentCaptor.forClass(Origin.class); verify(connectionFactory).createConnection(originCaptor.capture(), any(Connection.Settings.class));