From 2cc73d84b270bd92e00bc265f4561fdd60548e36 Mon Sep 17 00:00:00 2001 From: Daniel Kec Date: Fri, 1 Sep 2023 18:35:07 +0200 Subject: [PATCH] Review issues 2 --- .../webclient/api/ClientRequestBase.java | 20 +++- .../io/helidon/webclient/api/ClientUri.java | 37 +------- .../helidon/webclient/api/ClientUriTest.java | 9 -- .../webclient/http1/Http1ClientImpl.java | 2 +- .../webclient/http1/Http1ClientTest.java | 21 +++++ .../websocket/WsClientConfigBlueprint.java | 1 - .../websocket/WsClientConfigSupport.java | 48 ---------- .../webclient/websocket/WsClientImpl.java | 19 +++- .../webclient/websocket/WsClientUri.java | 92 ------------------- .../webclient/websocket/WsClientTest.java | 48 ++++++++++ 10 files changed, 106 insertions(+), 191 deletions(-) delete mode 100644 webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientConfigSupport.java delete mode 100644 webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientUri.java create mode 100644 webclient/websocket/src/test/java/io/helidon/webclient/websocket/WsClientTest.java diff --git a/webclient/api/src/main/java/io/helidon/webclient/api/ClientRequestBase.java b/webclient/api/src/main/java/io/helidon/webclient/api/ClientRequestBase.java index 6190969460d..d39b466bf84 100644 --- a/webclient/api/src/main/java/io/helidon/webclient/api/ClientRequestBase.java +++ b/webclient/api/src/main/java/io/helidon/webclient/api/ClientRequestBase.java @@ -24,6 +24,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicLong; @@ -56,6 +57,7 @@ public abstract class ClientRequestBase, R extends Ht public static final Http.Header USER_AGENT_HEADER = Http.Headers.create(Http.HeaderNames.USER_AGENT, "Helidon " + Version.VERSION); private static final Map COUNTERS = new ConcurrentHashMap<>(); + private static final Set SUPPORTED_SCHEMES = Set.of("https", "http"); private final Map pathParams = new HashMap<>(); private final HttpClientConfig clientConfig; @@ -145,7 +147,7 @@ public T uri(String uri) { @Override public ClientUri resolvedUri() { // we do not want to update our own URI, as this method may be called multiple times - return resolveUri(this.clientUri.copy()); + return resolveUri(ClientUri.create(this.clientUri)); } @Override @@ -249,7 +251,7 @@ public T proxy(Proxy proxy) { @Override public R request() { headers.setIfAbsent(USER_AGENT_HEADER); - return doSubmit(BufferData.EMPTY_BYTES); + return validateAndSubmit(BufferData.EMPTY_BYTES); } @Override @@ -258,7 +260,7 @@ public final R submit(Object entity) { rejectHeadWithEntity(); } headers.setIfAbsent(USER_AGENT_HEADER); - return doSubmit(entity); + return validateAndSubmit(entity); } @Override @@ -435,6 +437,18 @@ private static String nextRequestId(String protocolId) { return "client-" + protocolId + "-" + Long.toHexString(counter.getAndIncrement()); } + private R validateAndSubmit(Object entity) { + if (!SUPPORTED_SCHEMES.contains(uri().scheme())) { + throw new IllegalArgumentException( + String.format("Not supported scheme %s, client supported schemes are: %s", + uri().scheme(), + String.join(", ", SUPPORTED_SCHEMES) + ) + ); + } + return doSubmit(entity); + } + private String resolvePathParams(String path) { String result = path; for (Map.Entry entry : pathParams.entrySet()) { diff --git a/webclient/api/src/main/java/io/helidon/webclient/api/ClientUri.java b/webclient/api/src/main/java/io/helidon/webclient/api/ClientUri.java index e9d4d8864e8..45149e92469 100644 --- a/webclient/api/src/main/java/io/helidon/webclient/api/ClientUri.java +++ b/webclient/api/src/main/java/io/helidon/webclient/api/ClientUri.java @@ -17,7 +17,6 @@ package io.helidon.webclient.api; import java.net.URI; -import java.util.Set; import io.helidon.common.uri.UriFragment; import io.helidon.common.uri.UriInfo; @@ -29,34 +28,30 @@ * URI abstraction for WebClient. */ public class ClientUri implements UriInfo { - private static final Set DEFAULT_SUPPORTED_SCHEMES = Set.of("http", "https"); private final UriInfo base; private final UriQueryWriteable query; private UriInfo.Builder uriBuilder; private boolean skipUriEncoding = false; - protected ClientUri() { + private ClientUri() { this.base = null; this.query = UriQueryWriteable.create(); this.uriBuilder = UriInfo.builder(); } - protected ClientUri(ClientUri baseUri) { - validateScheme(baseUri.scheme()); + private ClientUri(ClientUri baseUri) { this.base = baseUri; this.uriBuilder = UriInfo.builder(base); this.skipUriEncoding = baseUri.skipUriEncoding; this.query = UriQueryWriteable.create().from(baseUri.query()); - validateScheme(baseUri.scheme()); } - protected ClientUri(UriInfo baseUri) { + private ClientUri(UriInfo baseUri) { this.base = baseUri; this.uriBuilder = UriInfo.builder(baseUri); this.skipUriEncoding = false; this.query = UriQueryWriteable.create().from(baseUri.query()); - validateScheme(baseUri.scheme()); } /** @@ -98,15 +93,6 @@ public static ClientUri create(URI baseUri) { return create().resolve(baseUri); } - /** - * Create copy of this client uri. - * - * @return copy of this client uri - */ - public ClientUri copy() { - return ClientUri.create(this); - } - @Override public String toString() { UriInfo info = uriBuilder.query(this.query).build(); @@ -135,7 +121,6 @@ public URI toUri() { * @return updated instance */ public ClientUri scheme(String scheme) { - validateScheme(scheme); uriBuilder.scheme(scheme); return this; } @@ -197,7 +182,6 @@ public ClientUri resolve(URI uri) { } if (uri.getScheme() != null) { - validateScheme(uri.getScheme()); uriBuilder.scheme(uri.getScheme()); } if (uri.getHost() != null) { @@ -337,10 +321,6 @@ public String pathWithQueryAndFragment() { return path; } - protected Set supportedSchemes() { - return DEFAULT_SUPPORTED_SCHEMES; - } - private String extractQuery(String path) { if (path != null) { int i = path.indexOf('?'); @@ -379,15 +359,4 @@ private String resolvePath(String path, String resolvePath) { + "/" + resolvePath; } - - private void validateScheme(String scheme) { - if (!supportedSchemes().contains(scheme)) { - throw new IllegalArgumentException( - String.format("Not supported scheme %s, client supported schemes are: %s", - scheme, - String.join(", ", supportedSchemes()) - ) - ); - } - } } diff --git a/webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java b/webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java index ce80675e821..004064ae63c 100644 --- a/webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java +++ b/webclient/api/src/test/java/io/helidon/webclient/api/ClientUriTest.java @@ -24,7 +24,6 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertThrows; class ClientUriTest { @Test @@ -105,12 +104,4 @@ void testResolveAll() { assertThat(helper.port(), is(80)); assertThat(helper.scheme(), is("https")); } - - @Test - void testSchemeValidation() { - ClientUri validClientUri = ClientUri.create(URI.create("http://localhost")); - assertThrows(IllegalArgumentException.class, () -> ClientUri.create(URI.create("ftp://localhost"))); - assertThrows(IllegalArgumentException.class, () -> validClientUri.scheme("git")); - assertThrows(IllegalArgumentException.class, () -> validClientUri.resolve(URI.create("ldap://localhost"))); - } } \ No newline at end of file diff --git a/webclient/http1/src/main/java/io/helidon/webclient/http1/Http1ClientImpl.java b/webclient/http1/src/main/java/io/helidon/webclient/http1/Http1ClientImpl.java index dfb03c59717..f925f02bf77 100644 --- a/webclient/http1/src/main/java/io/helidon/webclient/http1/Http1ClientImpl.java +++ b/webclient/http1/src/main/java/io/helidon/webclient/http1/Http1ClientImpl.java @@ -43,7 +43,7 @@ class Http1ClientImpl implements Http1Client, HttpClientSpi { @Override public Http1ClientRequest method(Http.Method method) { ClientUri clientUri = clientConfig.baseUri() - .map(ClientUri::copy) // create from base config + .map(ClientUri::create) // create from base config .orElseGet(ClientUri::create); // create as empty clientConfig.baseFragment().ifPresent(clientUri::fragment); diff --git a/webclient/tests/http1/src/test/java/io/helidon/webclient/http1/Http1ClientTest.java b/webclient/tests/http1/src/test/java/io/helidon/webclient/http1/Http1ClientTest.java index 2d7a2d0f41a..f1092984b00 100644 --- a/webclient/tests/http1/src/test/java/io/helidon/webclient/http1/Http1ClientTest.java +++ b/webclient/tests/http1/src/test/java/io/helidon/webclient/http1/Http1ClientTest.java @@ -36,6 +36,7 @@ import io.helidon.http.media.EntityWriter; import io.helidon.http.media.MediaContext; import io.helidon.http.media.MediaContextConfig; +import io.helidon.webclient.http2.Http2Client; import io.helidon.webserver.testing.junit5.ServerTest; import io.helidon.webserver.testing.junit5.SetUpRoute; import io.helidon.webclient.api.ClientConnection; @@ -56,9 +57,11 @@ import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.startsWith; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.core.IsNot.not; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.fail; /* This class uses package local API to validate connection cache, and at the same time benefits from @ServerTest @@ -76,9 +79,11 @@ class Http1ClientTest { private final String baseURI; private final WebClient injectedHttp1client; + private final int plainPort; Http1ClientTest(WebServer webServer, WebClient client) { baseURI = "http://localhost:" + webServer.port(); + this.plainPort = webServer.port(); injectedHttp1client = client; } @@ -376,6 +381,22 @@ void testReadTimeoutPerRequest() { assertThat(ste.getCause(), instanceOf(SocketTimeoutException.class)); } + @Test + void testSchemeValidation() { + try (var r = Http1Client.builder() + .baseUri("test://localhost:" + plainPort + "/") + .shareConnectionCache(false) + .build() + .get("/") + .request()) { + + fail("Should have failed because of invalid scheme."); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), startsWith("Not supported scheme test")); + } + } + + private static void validateSuccessfulResponse(Http1Client client) { String requestEntity = "Sending Something"; Http1ClientRequest request = client.put("/test"); diff --git a/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientConfigBlueprint.java b/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientConfigBlueprint.java index c7d764d0d03..55237b6fb7c 100644 --- a/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientConfigBlueprint.java +++ b/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientConfigBlueprint.java @@ -29,7 +29,6 @@ * io.helidon.webclient.spi.ProtocolConfig) */ @Prototype.Blueprint -@Prototype.CustomMethods(WsClientConfigSupport.WsCustomMethods.class) @Configured interface WsClientConfigBlueprint extends HttpClientConfig, Prototype.Factory { /** diff --git a/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientConfigSupport.java b/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientConfigSupport.java deleted file mode 100644 index 73fca1a547d..00000000000 --- a/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientConfigSupport.java +++ /dev/null @@ -1,48 +0,0 @@ -/* - * Copyright (c) 2023 Oracle and/or its affiliates. - * - * 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 io.helidon.webclient.websocket; - -import java.net.URI; - -import io.helidon.builder.api.Prototype; - -class WsClientConfigSupport { - - static class WsCustomMethods { - /** - * Base URI of the client. - * - * @param builder builder to update - * @param baseUri base URI to use, query is extracted to base query (if any) - */ - @Prototype.BuilderMethod - static void baseUri(WsClientConfig.BuilderBase builder, URI baseUri) { - builder.baseUri(WsClientUri.create().resolve(baseUri)); - } - - /** - * Base URI of the client. - * - * @param builder builder to update - * @param baseUri base URI to use, query is extracted to base query (if any) - */ - @Prototype.BuilderMethod - static void baseUri(WsClientConfig.BuilderBase builder, String baseUri) { - baseUri(builder, URI.create(baseUri)); - } - } -} diff --git a/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientImpl.java b/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientImpl.java index 0569317b032..5eef11bfd2f 100644 --- a/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientImpl.java +++ b/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientImpl.java @@ -23,6 +23,7 @@ import java.security.SecureRandom; import java.util.Base64; import java.util.Random; +import java.util.Set; import io.helidon.common.LazyValue; import io.helidon.common.socket.SocketContext; @@ -31,6 +32,7 @@ import io.helidon.http.ClientResponseHeaders; import io.helidon.http.Http; import io.helidon.webclient.api.ClientConnection; +import io.helidon.webclient.api.ClientUri; import io.helidon.webclient.api.HttpClientResponse; import io.helidon.webclient.api.WebClient; import io.helidon.webclient.http1.Http1Client; @@ -56,7 +58,7 @@ class WsClientImpl implements WsClient { private static final byte[] KEY_SUFFIX = "258EAFA5-E914-47DA-95CA-C5AB0DC85B11".getBytes(StandardCharsets.US_ASCII); private static final int KEY_SUFFIX_LENGTH = KEY_SUFFIX.length; private static final Base64.Encoder B64_ENCODER = Base64.getEncoder(); - + private static final Set SUPPORTED_SCHEMES = Set.of("wss", "ws", "https", "http"); private final ClientRequestHeaders headers; private final WebClient webClient; private final Http1Client http1Client; @@ -93,10 +95,20 @@ public void connect(URI uri, WsListener listener) { .uri(uri); UriInfo resolvedUri = upgradeRequest.resolvedUri(); String scheme = resolvedUri.scheme(); + + if (!SUPPORTED_SCHEMES.contains(scheme)) { + throw new IllegalArgumentException( + String.format("Not supported scheme %s, web socket client supported schemes are: %s", + scheme, + String.join(", ", SUPPORTED_SCHEMES) + ) + ); + } + if ("ws".equals(scheme)) { - upgradeRequest.uri(WsClientUri.create(resolvedUri).scheme("http")); + upgradeRequest.uri(ClientUri.create(resolvedUri).scheme("http")); } else if ("wss".equals(scheme)) { - upgradeRequest.uri(WsClientUri.create(resolvedUri).scheme("https")); + upgradeRequest.uri(ClientUri.create(resolvedUri).scheme("https")); } upgradeRequest.headers(headers -> headers.setIfAbsent(Http.Headers.create(Http.HeaderNames.HOST, resolvedUri @@ -166,4 +178,5 @@ protected String hash(SocketContext ctx, String wsKey) { throw new IllegalStateException("SHA-1 not provided", e); } } + } diff --git a/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientUri.java b/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientUri.java deleted file mode 100644 index 2ce00e1a464..00000000000 --- a/webclient/websocket/src/main/java/io/helidon/webclient/websocket/WsClientUri.java +++ /dev/null @@ -1,92 +0,0 @@ -/* - * Copyright (c) 2023 Oracle and/or its affiliates. - * - * 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 io.helidon.webclient.websocket; - -import java.net.URI; -import java.util.Set; - -import io.helidon.common.uri.UriInfo; -import io.helidon.webclient.api.ClientUri; - -/** - * URI abstraction for WsClient. - */ -public class WsClientUri extends ClientUri { - - private static final Set SUPPORTED_SCHEMES = Set.of("http", "https", "ws", "wss"); - - private WsClientUri() { - super(); - } - - private WsClientUri(ClientUri baseUri) { - super(baseUri); - } - - private WsClientUri(UriInfo baseUri) { - super(baseUri); - } - - /** - * Create an empty URI helper. - * - * @return uri helper - */ - public static WsClientUri create() { - return new WsClientUri(); - } - - /** - * Create a new client uri. - * - * @param baseUri base URI - * @return a new client uri - */ - public static WsClientUri create(ClientUri baseUri) { - return new WsClientUri(baseUri); - } - - /** - * Create a new client uri. - * - * @param baseUri base URI - * @return a new client uri - */ - public static WsClientUri create(UriInfo baseUri) { - return new WsClientUri(baseUri); - } - - /** - * Create a new client URI from an existing URI. - * - * @param baseUri base URI - * @return a new client uri - */ - public static ClientUri create(URI baseUri) { - return create().resolve(baseUri); - } - - @Override - public WsClientUri copy() { - return WsClientUri.create(this); - } - - @Override - protected Set supportedSchemes() { - return SUPPORTED_SCHEMES; - } -} diff --git a/webclient/websocket/src/test/java/io/helidon/webclient/websocket/WsClientTest.java b/webclient/websocket/src/test/java/io/helidon/webclient/websocket/WsClientTest.java new file mode 100644 index 00000000000..9e8fb20238d --- /dev/null +++ b/webclient/websocket/src/test/java/io/helidon/webclient/websocket/WsClientTest.java @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2023 Oracle and/or its affiliates. + * + * 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 io.helidon.webclient.websocket; + +import io.helidon.websocket.WsListener; +import io.helidon.websocket.WsSession; + +import org.junit.jupiter.api.Test; + +import static org.hamcrest.CoreMatchers.startsWith; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.fail; + +public class WsClientTest { + @Test + void testSchemeValidation() { + try { + WsClient.builder() + .baseUri("test://localhost:8888/") + .shareConnectionCache(false) + .build() + .connect("/whatever", new WsListener() { + @Override + public void onMessage(WsSession session, String text, boolean last) { + //not used + } + }); + + fail("Should have failed because of invalid scheme."); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), startsWith("Not supported scheme test")); + } + } +}