[FEATURE] Enable Generic HTTP Actions in Java Client

[reta]

Description

Enable Generic HTTP Actions in Java Client.
USER_GUIDE.md update is on me once we settle up on the APIs.

Issues Resolved

Closes #377

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[dblock]

Good progress!

Let's talk about how it's going to be used? I think we don't want a generic client. Is there a world where I can mix and match? For example, there's no reason why I cannot make a generic JSON request, but get back a serialized SearchResponse, and vice-versa.

[reta]

Let's talk about how it's going to be used? I think we don't want a generic client. Is there a world where I can mix and match?

Yes and no (if by mix and match you mean reuse existing models), sadly it turned out to be rather difficult, for simple cases it works pretty well (snippets from the tests):

GenericResponse response = javaClient().generic() .execute(
        new GenericRequest(
            "PUT",
            "/" + index + "/_doc/" + id,
            Collections.emptyList(),
            GenericBodies.json(document, javaClient()) // <-- document is arbitrary Java class here
        )
    )

final CreateIndexResponse r = GenericBodies.json(response.getBody().get(), CreateIndexResponse._DESERIALIZER, javaClient());
assertThat(r.acknowledged(), equalTo(true));

But others are very difficult to implement at the moment. It works now :)

[dblock]

I think this is very close! Write some basic docs.

[dblock]

Ok this is great. Did a careful review.

I want to make sure we discuss not wanting to handle errors. Why did you decide not to throw exceptions like the regular client? IMO users want to send POJOs vs. JSON, but error handling is sort of tangent. Doesn't it make it more complicated to reason about all clients when some don't handle errors like others?

[reta]

Ok this is great. Did a careful review.

❤️ thanks a lot @dblock

I want to make sure we discuss not wanting to handle errors. Why did you decide not to throw exceptions like the regular client?

Sure, great question (as always). So here is my reasoning: with generic HTTP client, we give a freedom to basically deal with raw HTTP requests and responses (the generic client is literally just a messenger, it delivers some message and brings some response back, no implied knowledge what it is). Since we don't know the intent (may be caller expects 404), it is better to leave every decision to the caller, including how to handle status codes.

[dblock]

Ok this is great. Did a careful review.

❤️ thanks a lot @dblock

I want to make sure we discuss not wanting to handle errors. Why did you decide not to throw exceptions like the regular client?

Sure, great question (as always). So here is my reasoning: with generic HTTP client, we give a freedom to basically deal with raw HTTP requests and responses (the generic client is literally just a messenger, it delivers some message and brings some response back, no implied knowledge what it is). Since we don't know the intent (may be caller expects 404), it is better to leave every decision to the caller, including how to handle status codes.

From the user requests I've seen the reasons to go to a lower level client are usually that we don't support a specific API, not to get more control. Does this change your mind? Is there a way to have both of these worlds?

[dblock]

I am going to merge, but let's be careful about shipping this - I still want to discuss error handling more.

[reta]

Thanks @dblock

From the user requests I've seen the reasons to go to a lower level client are usually that we don't support a specific API, not to get more control. Does this change your mind?

There are certainly pros and cons to both approaches. From implementation perspective, it is always safer to start simple (return the response as is) and add complexity progressively.

Is there a way to have both of these worlds?

Yes, but I would better be guided by usage patterns to be fair: we have pretty extensive typed model, with full fledged error handling. It seems like it was not enough - the generic client is an opposite of that,

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-910-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c1dcc441e2b0000a23f591e7a458c9956cd41d14
# Push it to GitHub
git push --set-upstream origin backport/backport-910-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-910-to-2.x.

[dblock]

Thanks @dblock

From the user requests I've seen the reasons to go to a lower level client are usually that we don't support a specific API, not to get more control. Does this change your mind?

There are certainly pros and cons to both approaches. From implementation perspective, it is always safer to start simple (return the response as is) and add complexity progressively.

Is there a way to have both of these worlds?

Yes, but I would better be guided by usage patterns to be fair: we have pretty extensive typed model, with full fledged error handling. It seems like it was not enough - the generic client is an opposite of that,

Does the underlying HTTP client throw errors on network failure? This renders error handling inconsistent, doesn't it?

[dblock]

it is always safer to start simple (return the response as is) and add complexity progressively.

In this case it's not actually true. If the server returns a 401 right now there's no error with the generic client. If we change this in the future it will be a breaking change.

[reta]

Does the underlying HTTP client throw errors on network failure?

Not really: the request was sent and response was received. Interpretation of the HTTP status code is subjective - the caller decides what he expected or not expected. Receiving the response is not a failure, everything else is.

We should take into account that we are not building generic HTTP client, but complementary OpenSearch client favour that should only be used when typed ones do not fit.

[dblock]

Not really: the request was sent and response was received. Interpretation of the HTTP status code is subjective - the caller decides what he expected or not expected. Receiving the response is not a failure, everything else is.

This is not true. HTTP is very clear on what's an error and what is not! Just like TCP (which raises timeout errors for example).

[dblock]

We should take into account that we are not building generic HTTP client, but complementary OpenSearch client favour that should only be used when typed ones do not fit.

Correct. Error codes aren't a matter of type. The bodies of errors certainly are.

[reta]

Not really: the request was sent and response was received. Interpretation of the HTTP status code is subjective - the caller decides what he expected or not expected. Receiving the response is not a failure, everything else is.

This is not true. HTTP is very clear on what's an error and what is not! Just like TCP (which raises timeout errors for example).

This is not what I meant, sorry about that (HTTP is very clear indeed), interpretation of the HTTP status code is subjective from the caller perspective (I have tons of examples), but I think we should be discussing that in #927, thanks @dblock !