Address code review comments

reta · reta · commit b0e3f25620a3 · 2024-04-10T08:44:24.000-04:00
Signed-off-by: Andriy Redko &lt;andriy.redko@aiven.io&gt;
diff --git a/guides/generic.md b/guides/generic.md
@@ -13,29 +13,29 @@ 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
 The following sample code sends a simple request that does not require any payload to be provided (typically, `GET` requests).
 
 ```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())) {
    // ...
 }
 ```
 
 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
@@ -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
@@ -30,7 +30,7 @@ public class OpenSearchGenericClient extends ApiClient<OpenSearchTransport, Open
      * Generic client options
      */
     public static final class ClientOptions {
-        public static final ClientOptions DEFAULT = new ClientOptions();
+        private static final ClientOptions DEFAULT = new ClientOptions();
 
         private final Predicate<Integer> error;
 
@@ -125,7 +125,7 @@ public boolean isError(int statusCode) {
         }
 
         @Override
-        public <T extends RuntimeException> T exceptionConverter(Response error) {
+        public <T extends RuntimeException> 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
@@ -98,7 +98,7 @@ default Map<String, String> headers(RequestT request) {
      * @param error error response
      * @return exception instance
      */
-    default <T extends RuntimeException> T exceptionConverter(ErrorT error) {
+    default <T extends RuntimeException> 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
@@ -517,7 +517,7 @@ private <ResponseT, ErrorT> ResponseT prepareResponse(Response clientResp, Endpo
                             entity.getContentType(),
                             content
                         );
-                        throw rawEndpoint.exceptionConverter(error);
+                        throw rawEndpoint.exceptionConverter(statusCode, error);
                     }
                 } else {
                     JsonpDeserializer<ErrorT> errorDeserializer = endpoint.errorDeserializer(statusCode);
@@ -535,7 +535,7 @@ private <ResponseT, ErrorT> 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
@@ -291,7 +291,7 @@ private <ResponseT, ErrorT> ResponseT getHighLevelResponse(
                             content
                         );
 
-                        throw rawEndpoint.exceptionConverter(error);
+                        throw rawEndpoint.exceptionConverter(statusCode, error);
                     }
                 } else {
                     JsonpDeserializer<ErrorT> errorDeserializer = endpoint.errorDeserializer(statusCode);
@@ -309,7 +309,7 @@ private <ResponseT, ErrorT> 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
@@ -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
@@ -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));

Original file line number	Diff line number	Diff line change
`@@ -156,8 +156,8 @@ public OpenSearchClient withTransportOptions(@Nullable TransportOptions transpor`
`156`	`156`	`}`
`157`	`157`
`158`	`158`	`// ----- Child clients`
`159`		`- public OpenSearchGenericClient generic(OpenSearchGenericClient.ClientOptions clientOptions) {`
`160`		`- return new OpenSearchGenericClient(this.transport, this.transportOptions, clientOptions);`
	`159`	`+ public OpenSearchGenericClient generic() {`
	`160`	`+ return new OpenSearchGenericClient(this.transport, this.transportOptions);`
`161`	`161`	`}`
`162`	`162`
`163`	`163`	`public OpenSearchCatClient cat() {`
Original file line number	Diff line number	Diff line change
`@@ -98,7 +98,7 @@ default Map<String, String> headers(RequestT request) {`
`98`	`98`	`* @param error error response`
`99`	`99`	`* @return exception instance`
`100`	`100`	`*/`
`101`		`- default <T extends RuntimeException> T exceptionConverter(ErrorT error) {`
	`101`	`+ default <T extends RuntimeException> T exceptionConverter(int statusCode, @Nullable ErrorT error) {`
`102`	`102`	`throw new OpenSearchException((ErrorResponse) error);`
`103`	`103`	`}`
`104`	`104`	`}`