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

618 withjson support method for put index template request #657

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

pranishd1
Copy link

Description

Added "withJson" support for PutIndexTemplateRequest

Issues Resolved

Closes [#618]

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.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of requests that support JSON bodies. I think we need to introduce a class or train that supports withJson at a lower level than PutIndexTemplateRequest and then include that in it. WDYT?

@pranishd1
Copy link
Author

Yes, that would be great. I will look into that.

@pranishd1 pranishd1 requested a review from dblock October 21, 2023 18:28
@VachaShah
Copy link
Collaborator

@pranishd1 Can you fix merge conflicts please? Also, you might have to run ./gradlew spotlessApply to fix formatting.

Copy link
Collaborator

@VachaShah VachaShah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also mention in the USER_GUIDE that how to use withJson?

@dblock
Copy link
Member

dblock commented Nov 2, 2023

This is a lot better! Can we apply this to at least some minimum number of requests, not just put index template? I'd also love a guide (maybe copy from opensearch-project/opensearch-py#542?) and a working sample as part of this PR, please?

@pranishd1
Copy link
Author

Can we also mention in the USER_GUIDE that how to use withJson?

sure, I will add a section about how to use this to the user guide.

@pranishd1
Copy link
Author

This is a lot better! Can we apply this to at least some minimum number of requests, not just put index template? I'd also love a guide (maybe copy from opensearch-project/opensearch-py#542?) and a working sample as part of this PR, please?

Yeah, sure I will add this to a few other requests. Thanks for the feedback.

@dblock
Copy link
Member

dblock commented Dec 4, 2023

@pranishd1 Want to finish this?

@pranishd1
Copy link
Author

@dblock , sorry about the delay. I am in the process of implementing the interface in create, search and delete requests. While doing so, I am stuck at object deserialization process. This method does not cover all the objects.

@Jai2305
Copy link
Contributor

Jai2305 commented Jul 26, 2024

Thanks for this guys, much needed, do you need any help with this?
Also after reading the code I got to know the problem -
that as of now we rely on static deserializer which creates a new builder everytime, referring to this code exactly -

public static final JsonpDeserializer<SearchRequest> _DESERIALIZER = ObjectBuilderDeserializer.lazy(
        Builder::new,
        SearchRequest::setupSearchRequestDeserializer
    );

Although some suggestions -

  • Can you please also create a method which takes Readable as an argument kind of like
 default<T> B withJson(Reader reader) {
             ........ 
  • Also now that we have default mapper in JsonpUtils, we can use that rather than relying on a specific implementation
    please refer here .

@dblock
Copy link
Member

dblock commented Jul 26, 2024

@Jai2305 looks like the PR hasn't been touched in a long time, so yes, would love it if you could pick it up and finish it!

Note that we added support for a generic interface since which takes/returns raw JSON, so some parts of that may be reusable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants