From b0e3f25620a3710ff4fe327a35d8b5fbf549644f Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 10 Apr 2024 08:44:24 -0400 Subject: [PATCH] Address code review comments Signed-off-by: Andriy Redko --- guides/generic.md | 16 ++++++++-------- .../client/opensearch/OpenSearchClient.java | 4 ++-- .../generic/OpenSearchGenericClient.java | 12 ++++++++++-- .../opensearch/client/transport/Endpoint.java | 2 +- .../httpclient5/ApacheHttpClient5Transport.java | 4 ++-- .../rest_client/RestClientTransport.java | 4 ++-- .../integTest/AbstractGenericClientIT.java | 16 ++++++++-------- .../integTest/AbstractPingAndInfoIT.java | 5 +---- 8 files changed, 34 insertions(+), 29 deletions(-) diff --git a/guides/generic.md b/guides/generic.md index 3e0937dd2d..95eb8bdb23 100644 --- a/guides/generic.md +++ b/guides/generic.md @@ -13,13 +13,13 @@ There are rare circumstances when the typed OpenSearch client APIs are too const The following sample code gets the `OpenSearchGenericClient` from the `OpenSearchClient` instance. ```java -final OpenSearchGenericClient generic = javaClient().generic(ClientOptions.DEFAULT); +final OpenSearchGenericClient generic = javaClient().generic(); ``` The generic client with default options (`ClientOptions.DEFAULT`) returns the responses as those were received from the server. The generic client could be instructed to raise an `OpenSearchClientException` exception instead if the HTTP status code is not indicating the successful response, for example: ```java -final OpenSearchGenericClient generic = javaClient().generic(ClientOptions.throwOnHttpErrors()); +final OpenSearchGenericClient generic = javaClient().generic().witClientOptions(ClientOptions.throwOnHttpErrors()); ``` ## Sending Simple Request @@ -27,7 +27,7 @@ The following sample code sends a simple request that does not require any paylo ```java // compare with what the low level client outputs -try (Response response = javaClient().generic(ClientOptions.DEFAULT).execute(Requests.builder().endpoint("/").method("GET").build())) { +try (Response response = javaClient().generic().execute(Requests.builder().endpoint("/").method("GET").build())) { // ... } ``` @@ -35,7 +35,7 @@ try (Response response = javaClient().generic(ClientOptions.DEFAULT).execute(Req Please notice that the `Response` instance should be closed explicitly in order to free up any allocated resource (like response input streams or buffers), the [`try-with-resource`](https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html) pattern is encouraged. ```java -try (Response response = javaClient().generic(ClientOptions.DEFAULT).execute(...)) { +try (Response response = javaClient().generic().execute(...)) { // ... } ``` @@ -44,7 +44,7 @@ The generic client never interprets status codes and provides the direct access ```java // compare with what the low level client outputs -try (Response response = javaClient().generic(ClientOptions.DEFAULT).execute(...)) { +try (Response response = javaClient().generic().execute(...)) { if (response.getStatus() != 200) { // Request was not successful } @@ -55,7 +55,7 @@ try (Response response = javaClient().generic(ClientOptions.DEFAULT).execute(... The following sample code a simple request with JSON body. ```java - try (Response response = javaClient().generic(ClientOptions.DEFAULT) + try (Response response = javaClient().generic() .execute( Requests.builder() .endpoint("/" + index + "/_search") @@ -89,7 +89,7 @@ final CreateIndexRequest request = CreateIndexRequest.of( .settings(settings -> settings.sort(s -> s.field("name").order(SegmentSortOrder.Asc))) ); -try (Response response = javaClient().generic(ClientOptions.DEFAULT) +try (Response response = javaClient().generic() .execute( Requests.builder() .endpoint("/" + index).method("PUT") @@ -106,7 +106,7 @@ try (Response response = javaClient().generic(ClientOptions.DEFAULT) Dealing with strings or POJOs could be daunting sometimes, using structured JSON APIs is a middle ground of both approaches, as per following sample code that uses (`jakarta.json.Json`)[https://jakarta.ee/specifications/jsonp]. ```java -try (Response response = javaClient().generic(ClientOptions.DEFAULT) +try (Response response = javaClient().generic() .execute( Requests.builder() .endpoint("/" + index) diff --git a/java-client/src/main/java/org/opensearch/client/opensearch/OpenSearchClient.java b/java-client/src/main/java/org/opensearch/client/opensearch/OpenSearchClient.java index c38c36236b..ee83bcc20d 100644 --- a/java-client/src/main/java/org/opensearch/client/opensearch/OpenSearchClient.java +++ b/java-client/src/main/java/org/opensearch/client/opensearch/OpenSearchClient.java @@ -156,8 +156,8 @@ public OpenSearchClient withTransportOptions(@Nullable TransportOptions transpor } // ----- Child clients - public OpenSearchGenericClient generic(OpenSearchGenericClient.ClientOptions clientOptions) { - return new OpenSearchGenericClient(this.transport, this.transportOptions, clientOptions); + public OpenSearchGenericClient generic() { + return new OpenSearchGenericClient(this.transport, this.transportOptions); } public OpenSearchCatClient cat() { diff --git a/java-client/src/main/java/org/opensearch/client/opensearch/generic/OpenSearchGenericClient.java b/java-client/src/main/java/org/opensearch/client/opensearch/generic/OpenSearchGenericClient.java index 1b82ee0b27..948444e150 100644 --- a/java-client/src/main/java/org/opensearch/client/opensearch/generic/OpenSearchGenericClient.java +++ b/java-client/src/main/java/org/opensearch/client/opensearch/generic/OpenSearchGenericClient.java @@ -30,7 +30,7 @@ public class OpenSearchGenericClient extends ApiClient error; @@ -125,7 +125,7 @@ public boolean isError(int statusCode) { } @Override - public T exceptionConverter(Response error) { + public T exceptionConverter(int statusCode, @Nullable Response error) { throw new OpenSearchClientException(error); } } @@ -136,6 +136,10 @@ public OpenSearchGenericClient(OpenSearchTransport transport) { this(transport, null, ClientOptions.DEFAULT); } + public OpenSearchGenericClient(OpenSearchTransport transport, @Nullable TransportOptions transportOptions) { + this(transport, transportOptions, ClientOptions.DEFAULT); + } + public OpenSearchGenericClient( OpenSearchTransport transport, @Nullable TransportOptions transportOptions, @@ -145,6 +149,10 @@ public OpenSearchGenericClient( this.clientOptions = clientOptions; } + public OpenSearchGenericClient withClientOptions(ClientOptions clientOptions) { + return new OpenSearchGenericClient(this.transport, this.transportOptions, clientOptions); + } + @Override public OpenSearchGenericClient withTransportOptions(@Nullable TransportOptions transportOptions) { return new OpenSearchGenericClient(this.transport, transportOptions, this.clientOptions); diff --git a/java-client/src/main/java/org/opensearch/client/transport/Endpoint.java b/java-client/src/main/java/org/opensearch/client/transport/Endpoint.java index 6f46210f1a..38f9c0a46a 100644 --- a/java-client/src/main/java/org/opensearch/client/transport/Endpoint.java +++ b/java-client/src/main/java/org/opensearch/client/transport/Endpoint.java @@ -98,7 +98,7 @@ default Map headers(RequestT request) { * @param error error response * @return exception instance */ - default T exceptionConverter(ErrorT error) { + default T exceptionConverter(int statusCode, @Nullable ErrorT error) { throw new OpenSearchException((ErrorResponse) error); } } diff --git a/java-client/src/main/java/org/opensearch/client/transport/httpclient5/ApacheHttpClient5Transport.java b/java-client/src/main/java/org/opensearch/client/transport/httpclient5/ApacheHttpClient5Transport.java index 218b06c196..313ccc5437 100644 --- a/java-client/src/main/java/org/opensearch/client/transport/httpclient5/ApacheHttpClient5Transport.java +++ b/java-client/src/main/java/org/opensearch/client/transport/httpclient5/ApacheHttpClient5Transport.java @@ -517,7 +517,7 @@ private ResponseT prepareResponse(Response clientResp, Endpo entity.getContentType(), content ); - throw rawEndpoint.exceptionConverter(error); + throw rawEndpoint.exceptionConverter(statusCode, error); } } else { JsonpDeserializer errorDeserializer = endpoint.errorDeserializer(statusCode); @@ -535,7 +535,7 @@ private ResponseT prepareResponse(Response clientResp, Endpo InputStream content = entity.getContent(); try (JsonParser parser = mapper.jsonProvider().createParser(content)) { ErrorT error = errorDeserializer.deserialize(parser, mapper); - throw endpoint.exceptionConverter(error); + throw endpoint.exceptionConverter(statusCode, error); } } catch (MissingRequiredPropertyException errorEx) { // Could not decode exception, try the response type diff --git a/java-client/src/main/java/org/opensearch/client/transport/rest_client/RestClientTransport.java b/java-client/src/main/java/org/opensearch/client/transport/rest_client/RestClientTransport.java index 9b6a62b1b4..1d90fba827 100644 --- a/java-client/src/main/java/org/opensearch/client/transport/rest_client/RestClientTransport.java +++ b/java-client/src/main/java/org/opensearch/client/transport/rest_client/RestClientTransport.java @@ -291,7 +291,7 @@ private ResponseT getHighLevelResponse( content ); - throw rawEndpoint.exceptionConverter(error); + throw rawEndpoint.exceptionConverter(statusCode, error); } } else { JsonpDeserializer errorDeserializer = endpoint.errorDeserializer(statusCode); @@ -309,7 +309,7 @@ private ResponseT getHighLevelResponse( InputStream content = entity.getContent(); try (JsonParser parser = mapper.jsonProvider().createParser(content)) { ErrorT error = errorDeserializer.deserialize(parser, mapper); - throw endpoint.exceptionConverter(error); + throw endpoint.exceptionConverter(statusCode, error); } } catch (MissingRequiredPropertyException errorEx) { // Could not decode exception, try the response type diff --git a/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractGenericClientIT.java b/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractGenericClientIT.java index f6119b2ef2..c5267e2bc6 100644 --- a/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractGenericClientIT.java +++ b/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractGenericClientIT.java @@ -36,7 +36,7 @@ public void shouldReturnSearchResults() throws Exception { createIndex(index); try ( - Response response = javaClient().generic(ClientOptions.DEFAULT) + Response response = javaClient().generic() .execute( Requests.builder() .endpoint("/" + index + "/_search") @@ -98,8 +98,7 @@ public void shouldReturn404() throws IOException { createIndex(index); try ( - Response response = javaClient().generic(ClientOptions.DEFAULT) - .execute(Requests.builder().endpoint("/" + index + "/_doc/10").method("GET").build()) + Response response = javaClient().generic().execute(Requests.builder().endpoint("/" + index + "/_doc/10").method("GET").build()) ) { assertThat(response.getStatus(), equalTo(404)); assertThat(response.getBody().isPresent(), equalTo(true)); @@ -113,7 +112,8 @@ public void shouldThrow() throws IOException { final OpenSearchClientException ex = assertThrows(OpenSearchClientException.class, () -> { try ( - Response response = javaClient().generic(ClientOptions.throwOnHttpErrors()) + Response response = javaClient().generic() + .withClientOptions(ClientOptions.throwOnHttpErrors()) .execute(Requests.builder().endpoint("/" + index + "/_doc/10").method("GET").build()) ) {} }); @@ -135,7 +135,7 @@ private void createTestDocuments(String index) throws IOException { private void createTestDocument(String index, String id, ShopItem document) throws IOException { try ( - Response response = javaClient().generic(ClientOptions.DEFAULT) + Response response = javaClient().generic() .execute( Requests.builder() .endpoint("/" + index + "/_doc/" + id) @@ -161,7 +161,7 @@ private void createIndexUntyped(String index) throws IOException { final JsonpMapper jsonpMapper = javaClient()._transport().jsonpMapper(); try ( - Response response = javaClient().generic(ClientOptions.DEFAULT) + Response response = javaClient().generic() .execute( Requests.builder() .endpoint("/" + index) @@ -215,7 +215,7 @@ private void createIndexTyped(String index) throws IOException { ); try ( - Response response = javaClient().generic(ClientOptions.DEFAULT) + Response response = javaClient().generic() .execute(Requests.builder().endpoint("/" + index).method("PUT").json(request, jsonpMapper).build()) ) { assertThat(response.getStatus(), equalTo(200)); @@ -231,7 +231,7 @@ private void createIndexTyped(String index) throws IOException { private void refreshIndex(String index) throws IOException { try ( - Response response = javaClient().generic(ClientOptions.DEFAULT) + Response response = javaClient().generic() .execute(Requests.builder().endpoint("/" + index + "/_refresh").method("POST").build()) ) { assertThat(response.getStatus(), equalTo(200)); diff --git a/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractPingAndInfoIT.java b/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractPingAndInfoIT.java index 1335509bbd..d951799c33 100644 --- a/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractPingAndInfoIT.java +++ b/java-client/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractPingAndInfoIT.java @@ -17,7 +17,6 @@ import org.opensearch.client.opensearch.OpenSearchClient; import org.opensearch.client.opensearch.core.InfoResponse; import org.opensearch.client.opensearch.generic.Bodies; -import org.opensearch.client.opensearch.generic.OpenSearchGenericClient.ClientOptions; import org.opensearch.client.opensearch.generic.Requests; import org.opensearch.client.opensearch.generic.Response; import org.opensearch.client.transport.endpoints.BooleanResponse; @@ -33,9 +32,7 @@ public void testInfo() throws IOException { InfoResponse info = openSearchClient.info(); // compare with what the low level client outputs - try ( - Response response = javaClient().generic(ClientOptions.DEFAULT).execute(Requests.builder().endpoint("/").method("GET").build()) - ) { + try (Response response = javaClient().generic().execute(Requests.builder().endpoint("/").method("GET").build())) { assertThat(response.getStatus(), equalTo(200)); assertThat(response.getProtocol(), equalTo("HTTP/1.1")); assertThat(response.getBody().isEmpty(), is(false));