opensearch-java icon indicating copy to clipboard operation
opensearch-java copied to clipboard

Add .bodyAsJson() to generic response

Open dblock opened this issue 1 year ago • 5 comments

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 avatar May 14 '24 12:05 dblock

@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?

dblock avatar May 14 '24 12:05 dblock

@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 avatar May 14 '24 12:05 reta

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

  • jakarta.json.JsonStructure
  • com.fasterxml.jackson.databind.JsonNode

(depending which mapper is configured)

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

reta avatar May 14 '24 12:05 reta

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?

dblock avatar May 14 '24 13:05 dblock

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

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

  • JsonbJsonpMapper (Jakarta)
  • JacksonJsonpMapper (Jackson)

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.

reta avatar May 14 '24 13:05 reta

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

dblock avatar Jul 16 '24 15:07 dblock