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

[FEATURE] Enable Generic HTTP Actions in Java Client #377

Closed
RLashofRegas opened this issue Feb 23, 2023 · 28 comments · Fixed by #910
Closed

[FEATURE] Enable Generic HTTP Actions in Java Client #377

RLashofRegas opened this issue Feb 23, 2023 · 28 comments · Fixed by #910
Assignees
Labels
enhancement New feature or request v2.10.0 Issues and PRs related to version 2.10.0

Comments

@RLashofRegas
Copy link

Is your feature request related to a problem?

Not all requests I need to make to OpenSearch are supported natively by the client. One example is mapping AWS IAM Roles to FGAC roles in the security plug-in. These require doing a post request to the _plugins/_security endpoint. Since the client doesn't natively support the _plugins endpoint I need to send a generic PUT request. Right now the client has generic APIs for get(...) and delete(...) (NOTE: as I'm looking back at the get API even that seems to only support getting documents not generic GET...), but it does not have the same for put(...) or post(...). These are needed to make it easy for clients to use the OpenSearch client to send all the requests they need to make to the cluster with a single client interface.

What solution would you like?

Add PutResponse put(PutRequest request) and PostResponse post(PostRequest request) to the OpenSearchClient API.

What alternatives have you considered?

Right now it seems the only alternative would be to manually create a PutRequest class and pass this directly to transport.performRequest(...) similar to what is done for the delete(...) API here.

Do you have any additional context?

No.

@RLashofRegas RLashofRegas added enhancement New feature or request untriaged labels Feb 23, 2023
@harshavamsi
Copy link
Contributor

@RLashofRegas I agree with your request. I need something similar to this for the hadoop client. We need generic request and response classes for that would match RequestT request, Endpoint<RequestT, ResponseT, ErrorT> endpoint and inherit the RequestBase class for the Request. Although even the delete API you mentioned is used to delete a document. I can pick this up if there's support.

@harshavamsi
Copy link
Contributor

@reta would love your feedback on this as well as opensearch-project/opensearch-hadoop#124

@reta
Copy link
Collaborator

reta commented Feb 25, 2023

@reta would love your feedback on this as well as opensearch-project/opensearch-hadoop#124

My apologies @harshavamsi , I am on vacation at the moment, should be back at the end of upcoming week

@dblock
Copy link
Member

dblock commented Feb 28, 2023

This is a useful feature!

@reta
Copy link
Collaborator

reta commented Mar 3, 2023

@harshavamsi I would agree with you and @dblock that it could be a useful feature, but I would suggest to approach it from the perspective of generic client (as a child client), something along these lines:

public class OpenSearchClient extends ApiClient<OpenSearchTransport, OpenSearchClient> {
        ...
	public OpenSearchGenericClient generic() {
		return new OpenSearchGenericClient(this.transport, this.transportOptions);
	}
}

The OpenSearchGenericClient (or OpenSearchHttpClient , ... naming is difficult) would be just a wrapper for get/put/post/head/delete/patch methods. With that, the OpenSearchClient would stay clean but would let to issue generic HTTP requests. What do you think?

@dblock
Copy link
Member

dblock commented Mar 6, 2023

@reta What's the reason you prefer to prevent users from making arbitrary requests?

In other clients I've seen such as Ruby all API calls are implemented as wrappers of generic HTTP methods, which seems to be appreciated by folks asking for this kind of feature.

Maybe we can do the opposite, extract OpenSearchGenericClient out of OpenSearchClient and make OpenSearchClient extends OpenSearchGenericClient in a way that the generic methods become private? One could instantiate OpenSearchGenericClient to make raw requests?

@reta
Copy link
Collaborator

reta commented Mar 6, 2023

@dblock the OpenSearchClient is a "typed" version, intended to simplify the integration with OpenSearch by providing request / response models.

In other clients I've seen such as Ruby all API calls are implemented as wrappers of generic HTTP methods, which seems to be appreciated by folks asking for this kind of feature.

The users could make any arbitrary requests by just using client.transport() directly, nothing for us to do here

However if you look into OpenSearchClient, it groups the clients by APIs:

public class OpenSearchClient extends ApiClient<OpenSearchTransport, OpenSearchClient> {
    public OpenSearchCatClient cat() { ... } 
    public OpenSearchNodesClient nodes() { ... } 
    ...

My suggestion was to offer the generic client following such conventions, hope it makes sense.

@chenqi0805
Copy link
Contributor

chenqi0805 commented Mar 10, 2023

One other use case is for ISM APIs: https://opensearch.org/docs/latest/im-plugin/ism/api/. We need the generic HTTP actions and requests to support that

@harshavamsi
Copy link
Contributor

@harshavamsi I would agree with you and @dblock that it could be a useful feature, but I would suggest to approach it from the perspective of generic client (as a child client), something along these lines:

public class OpenSearchClient extends ApiClient<OpenSearchTransport, OpenSearchClient> {
        ...
	public OpenSearchGenericClient generic() {
		return new OpenSearchGenericClient(this.transport, this.transportOptions);
	}
}

The OpenSearchGenericClient (or OpenSearchHttpClient , ... naming is difficult) would be just a wrapper for get/put/post/head/delete/patch methods. With that, the OpenSearchClient would stay clean but would let to issue generic HTTP requests. What do you think?

I like this idea @reta. Let me put together a PR that adds this generic client.

@dlvenable
Copy link
Member

This sounds like a good idea. If I understand correctly we'd have a new interface to use which looks somewhat like the following?

public interface OpenSearchGenericClient {
  <T> T get(String path, Class<T> responseClass);
  <T> T post(String path, Class<T> responseClass, Object request);
  ...
}

It might be good to look at existing frameworks like HttpClient, WebClient, and RestTemplate.

@harshavamsi
Copy link
Contributor

Hi @reta, in this implementation how do we de-serialize the response object without knowing the fields in the response? For example, the GetResponse class uses GetResult as the de-serializer to parse the incoming response. This obviously contains fields that are unique to GetResponse.

@reta
Copy link
Collaborator

reta commented Mar 15, 2023

Hi @reta, in this implementation how do we de-serialize the response object without knowing the fields in the response? For example, the GetResponse class uses GetResult as the de-serializer to parse the incoming response. This obviously contains fields that are unique to GetResponse.

Hey @harshavamsi , since the client is generic, the requests and responses are also generic: the consumer would need to deserialize the response body (the same goes with request body serialization) to whatever it thinks it should be.

@RLashofRegas
Copy link
Author

@harshavamsi ElasticSearch already provides a low-level REST client with a generic performRequest(Request request) method. I would also recommend checking that out. I have used it and the API is pretty user-friendly. Source code is here.

@dblock
Copy link
Member

dblock commented Mar 16, 2023

@RLashofRegas we cannot accept/look at non-open-source code. Please feel free to reimplement something similar without copying code that's not-APLv2-compatible.

@reta
Copy link
Collaborator

reta commented Mar 16, 2023

@dblock I think it falls into "use raw transport" alternative (from description):

Right now it seems the only alternative would be to manually create a PutRequest class and pass this directly to transport.performRequest(...) ...

The OpenSearchTransport exposes performRequest() to do raw requests

@dblock
Copy link
Member

dblock commented Mar 16, 2023

The OpenSearchTransport exposes performRequest() to do raw requests

Ah yes! Someone write a sample :)

@dlvenable
Copy link
Member

Hi @reta, in this implementation how do we de-serialize the response object without knowing the fields in the response? For example, the GetResponse class uses GetResult as the de-serializer to parse the incoming response. This obviously contains fields that are unique to GetResponse.

@harshavamsi ,

Other REST frameworks such as Spring's RestTemplate and WebClient support this by having the call provide the class to serialize to.

Take GET for example, you can have a method like the following.

<T> T get(String path, Class<T> responseClass);

The framework will deserialize into the responseClass. It doesn't need to know this in advance. This allows a caller to provide a Map or create their own custom domain object for example.

Map<String, Object> rootResponse = genericClient.get("/", Map.class);
assertThat(rootResponse.get("name", "opensearch-node1");
assertThat(rootResponse.get("cluster_name", "opensearch-cluster");

As it appears the OpenSearchTransport can already provide a lot of this, this generic client could be a fairly light wrapper to make it easier to use.

@reta
Copy link
Collaborator

reta commented Apr 13, 2023

@dlvenable the problem with that is that these frameworks (with this approach) do not provide the access to response status code etc - the developers have to deal with tons of exceptions. This functionality could be provided by base Response class, assuming the response does have the content:

Response::getContent(Class<T> responseClass);

Since this is generic client, providing access to underlying response details is a must (I think).

@dlvenable
Copy link
Member

That is still possible. RestTemplate actually allows for the response to come as a ResponseEntity<T> which has the status code, response headers, and body.

When you use it this way, you do not get exceptions.

@reta
Copy link
Collaborator

reta commented Apr 13, 2023

When you use it this way, you do not get exceptions.

I think we should keep API as minimal as possible: this is not a generic HTTP based client.

@dlvenable
Copy link
Member

I created a proof-of-concept for using the Transport to perform a request. It is definitely not straightforward, mostly because of the deserialization of the response. I ended up copying and pasting some code from opensearch-java because it is package protected.

You can see it all in this one file. (It's a branch of data-prepper, but just look at the one file).

https://github.com/dlvenable/data-prepper/blob/do-not-merge-opensearch-java-client-generic-poc/data-prepper-plugins/opensearch/src/test/java/org/opensearch/dataprepper/plugins/sink/opensearch/GeneralRequestDemoTest.java

@reta
Copy link
Collaborator

reta commented Apr 13, 2023

@dlvenable There is a pull request already [1], would be good to consolidate efforts there

[1] #415

@maryama
Copy link

maryama commented Oct 16, 2023

This is something that my team have also had to work around, and would appreciate a solution for. We migrated from elasticsearch, which as someone stated above, does support such requests.

@rursprung
Copy link
Contributor

i also need this in order to implement opensearch-liquibase (see opensearch-project/opensearch-migrations#148): as i cannot cover all possible use-cases i want to start with a simple "just run any HTTP action you want against OpenSearch" as a liquibase change type (so liquibase is just used to record what was executed but doesn't - yet - abstract away e.g. over different OpenSearch versions (API changes)). i use opensearch-java instead of just using a pure HTTP library to interact with OpenSearch because i also need to store my own data (changeset management). but for this i'm now blocked due to this feature not being present.

@dblock
Copy link
Member

dblock commented Nov 29, 2023

#257 is related.

@reta
Copy link
Collaborator

reta commented Mar 17, 2024

@VachaShah @dblock @dlvenable mind if I pick this issue up, that would help to unblock the dedicated support of the OpenSearch in Liquibase @rursprung is working on. Thanks folks.

@VachaShah
Copy link
Collaborator

@VachaShah @dblock @dlvenable mind if I pick this issue up, that would help to unblock the dedicated support of the OpenSearch in Liquibase @rursprung is working on. Thanks folks.

All yours @reta!

@reta reta self-assigned this Mar 18, 2024
@tloubrieu-jpl
Copy link

Hello,
We are migrating to OpenSearch Serverless on AWS and we need this feature to be able to search our OpenSearch data, since our schema is dynamically updated depending on the documents which are loaded in OpenSearch. Thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v2.10.0 Issues and PRs related to version 2.10.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.