opensearch-project / opensearch-java

Java Client for OpenSearch
Apache License 2.0
119 stars 182 forks source link

Add .bodyAsJson() to generic response #983

Closed dblock closed 2 months ago

dblock commented 4 months ago

Is your feature request related to a problem?

Given


            OpenSearchGenericClient genericClient = client.generic()
                .withClientOptions(ClientOptions.throwOnHttpErrors());

            try {
                System.out.println(genericClient.execute(
                    Requests.builder()
                        .endpoint(index)
                        .method("PUT")
                        .json("{}")
                        .build()
                    ).getBody().get().bodyAsString());
            } catch (OpenSearchClientException ex) {
                System.out.println(ex.response().getBody().get().bodyAsString());
            }
``

The error caught is the following JSON.

```json
{"error":{"root_cause":[{"type":"resource_already_exists_exception","reason":"index [movies/NKwGTlmvTw6xzs9RURZwRA] already exists","index":"movies","index_uuid":"NKwGTlmvTw6xzs9RURZwRA"}],"type":"resource_already_exists_exception","reason":"index [movies/NKwGTlmvTw6xzs9RURZwRA] already exists","index":"movies","index_uuid":"NKwGTlmvTw6xzs9RURZwRA"},"status":400}}

We'd like to extract the resource_already_exists_exception part, ie. write something like this:

  } catch (OpenSearchClientException ex) {
      ... json = ex.response().getBody().get().bodyAsJson();
      if (json.get('error').get('root_cause').get('type')...
  }

What solution would you like?

Implement Body#bodyAsJson() or .json().

What alternatives have you considered?

Deal with mappers & friends.

dblock commented 4 months ago

@reta What's the magical combination of JsonpMapper, JsonpDeserializer<ErrorCause> or maybe a more generic deserializer for my catch (OpenSearchClientException ex) { above? And another for a successful response?

reta commented 4 months ago

@dblock there are few options, may be the easiest one is to convert it to a map:

            Map<String, Object> responseAsMap = response.getBody()
                .map(b -> Bodies.json(b, Map.class, javaClient()._transport().jsonpMapper()))
                .orElseGet(Collections::emptyMap);
reta commented 4 months ago

As a follow up, we could support "generic" JSON structures, like:

(depending which mapper is configured)

JsonNode json = response.getBody()
                .map(b -> Bodies.json(b, JsonNode.class, javaClient()._transport().jsonpMapper()))
                .orElse(null);
dblock commented 4 months ago

I like the second version a lot more.

                JsonNode json = ex.response().getBody()
                    .map(b -> Bodies.json(b, JsonNode.class, client._transport().jsonpMapper()))
                    .orElse(null);
                String errorType = json.get("error").get("type").textValue();
                if (! errorType.equals("resource_already_exists_exception")) {
                    System.err.println(ex.response().getBody().get().bodyAsString());
                    throw ex;
                }

Any reason not to expose .bodyAsJson() implemented as the above that returns JsonNode?

reta commented 4 months ago

Any reason not to expose .bodyAsJson() implemented as the above that returns JsonNode?

So at the moment, we have 2 implementations of JsonpMapper:

The JsonNode would only work when JacksonJsonpMapper is used (and consequently, Jackson). JsonStructure may work for both I think but needs some work on serde level to recognize this "generic" pattern. No reasons to not have a support for that, to your point.

dblock commented 2 months ago

Can we close this with https://github.com/opensearch-project/opensearch-java/pull/1064 and #1083, @Jai2305, or is there work remaining?