Skip to content

Commit

Permalink
Address code review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Andriy Redko <[email protected]>
  • Loading branch information
@reta
reta committed Apr 10, 2024
1 parent 0d15e40 commit b0e3f25
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 29 deletions.
16 changes: 8 additions & 8 deletions guides/generic.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(...)) {
// ...
}
```
Expand All @@ -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
}
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions java-client/src/main/java/org/opensearch/client/opensearch/OpenSearchClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
12 changes: 10 additions & 2 deletions ...lient/src/main/java/org/opensearch/client/opensearch/generic/OpenSearchGenericClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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,
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion java-client/src/main/java/org/opensearch/client/transport/Endpoint.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
4 changes: 2 additions & 2 deletions ...src/main/java/org/opensearch/client/transport/httpclient5/ApacheHttpClient5Transport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions ...client/src/main/java/org/opensearch/client/transport/rest_client/RestClientTransport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down
16 changes: 8 additions & 8 deletions ...t/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractGenericClientIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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));
Expand All @@ -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())
) {}
});
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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));
Expand All @@ -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));
Expand Down
5 changes: 1 addition & 4 deletions ...ent/src/test/java11/org/opensearch/client/opensearch/integTest/AbstractPingAndInfoIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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));
Expand Down

0 comments on commit b0e3f25

Please sign in to comment.