Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add .bodyAsJson() to generic response #983

Closed
dblock opened this issue May 14, 2024 · 6 comments
Closed

Add .bodyAsJson() to generic response #983

dblock opened this issue May 14, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@dblock
Copy link
Member

dblock commented May 14, 2024

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 dblock added enhancement New feature or request untriaged labels May 14, 2024
@dblock
Copy link
Member Author

dblock commented May 14, 2024

@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
Copy link
Collaborator

reta commented May 14, 2024

@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
Copy link
Collaborator

reta commented May 14, 2024

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);

@dblock
Copy link
Member Author

dblock commented May 14, 2024

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
Copy link
Collaborator

reta commented May 14, 2024

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.

@dblock dblock removed the untriaged label May 16, 2024
@dblock
Copy link
Member Author

dblock commented Jul 16, 2024

Can we close this with #1064 and #1083, @Jai2305, or is there work remaining?

@reta reta closed this as completed Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants