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

support for withJson-618 #1148

Merged
merged 2 commits into from
Sep 3, 2024
Merged

Conversation

Jai2305
Copy link
Contributor

@Jai2305 Jai2305 commented Aug 20, 2024

Description
Added PlainDeserializable interface with methods for streamlining deserialization process .

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.

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 20, 2024

Hi, I am converting this PR into a draft , I wanted to share the general approach , i.e for all the classes annotated with @JsonpDeserializable , I want to add an interface which shall be implemented by respective Builder classes and thus can be chained with the Builder methods to create the instance

Once you are aligned on this approach without any concerns , then changes for all the classes will be implemented in this PR

@dblock
Copy link
Member

dblock commented Aug 20, 2024

I like it! Iterate to green/add some documentation and maybe a sample?

@Jai2305 Jai2305 marked this pull request as ready for review August 21, 2024 07:49
@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 21, 2024

Can you also please clarify -

  • As we are coding to an interface , we have a choice whether a class's Builder implements it or not , should we first do it only for some classes and opt out from others , or are we going for every class which is annotated with @JsonpDeserializable.

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 24, 2024

@dblock any thoughts on above ?

@dblock
Copy link
Member

dblock commented Aug 24, 2024

@dblock any thoughts on above ?

I think users want it everywhere - are there any arguments to limit it?

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.

This looks alright to me, but as you point out I think we want it at a lower level. Basically any builder that accepts a body should also accept withJson, shouldn't it?

import org.opensearch.client.json.ObjectBuilderDeserializer;
import org.opensearch.client.json.ObjectDeserializer;
import org.opensearch.client.json.PlainJsonSerializable;
import org.opensearch.client.json.*;
Copy link
Member

Choose a reason for hiding this comment

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

I am surprised this doesn't get trapped by spotless, let's expand these like in all other places (and check spotless configuration please).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that would be my ide auto import :) , I will fix these.

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 25, 2024

@dblock any thoughts on above ?

I think users want it everywhere - are there any arguments to limit it?

  • I was unsure about the universal application of the withJson feature. For eg. some response classes which are annotated with @JsonpDeserializable , in those cases the data is expected to be returned by search engine so I was not sure about the usecase of withJson in those type of classes , at first I thought of implementing the feature to only the request classes which are annotated with @JsonpDeserializable .
  • With that being said I do not have any concrete argument against it.

My only concern is

with the classes in which Builders are protected, that means the original intent was to never pass these builders around; for those classes, we will not (or should not ?) be able to implement the withJson method.

@dblock
Copy link
Member

dblock commented Aug 25, 2024

I think you have valid concerns, @Xtansia @reta WDYT?

Will we be able to have withNdJson() for bulk?

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 26, 2024

  • If we decide it to be appropriate for every Builder to have withJson then one solution that I have is to make new abstract object Builder class which extends the classes like ObjectBuilders and AbstractBuilders and implements the new PlainDeserializable interface - these are basically the classes extended by every builder . By making an abstract class or different abstract classes we can control the access of self() method - this is the method which returns the Builder of a class.
    - cons - All files will have to implement the extend the new classes, will have to add new abstract classes and I think similar code will have to be repeated ..
    - pro - better control, extensibility

  • Or we can modify ObjectBuilder class itself
    - cons - a bit dangerous, no opt out option,
    - pro -changes will be applied to all the builders in the library at once.

  • Excuse me for the raising the PR first but the above reported concern was an unknown to me till I started working on it and realized the possible complexity. Looking forward to your suggestions or any alternate approach you have, thanks alot :)

@dblock
Copy link
Member

dblock commented Aug 26, 2024

@Jai2305 some builders will need to serialize JSON but others ND-JSON (bulk), and some both that may be implemented differently (you can make nd-json from json). So I think (2) doesn't work, no?

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 26, 2024

@Jai2305 some builders will need to serialize JSON but others ND-JSON (bulk), and some both that may be implemented differently (you can make nd-json from json). So I think (2) doesn't work, no?

You are right. There can be a work around to add new Abstract classes for NdJson cases, but, this is in general a pretty clunky solution, lets rule it out then.

@reta
Copy link
Collaborator

reta commented Aug 26, 2024

@Jai2305 some builders will need to serialize JSON but others ND-JSON (bulk), and some both that may be implemented differently (you can make nd-json from json). So I think (2) doesn't work, no?

@dblock @Jai2305 I have concerns regarding this approach:

  1. I believe we should not deserialize builder from JSON this way, it lets to very weird code constructs like:
     PutIndexTemplateRequest indexTemplateRequest = new PutIndexTemplateRequest.Builder().name("My index 2")
         .indexPatterns("index_pattern3")
         .withJson(inputStream)
         .name("name")
         .build();

(what is the values of name and indexPatterns after that? not to mention calling withJson a few times)

  1. However, some type of requests that accepts pretty much freestyle payloads (like source, template fe) would benefit from this particular feature but it should be limited to these requests only

  2. If we really want to support builder desearlization, than we would need to look for something like that (very rough idea, but essentially have a builder instance deserialized first and tailored later): new PutIndexTemplateRequest.Builder(json).name("My index 2")

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 27, 2024

@reta Thanks so much for the review ,

  • If we have a code like below -
    PutIndexTemplateRequest indexTemplateRequest = new PutIndexTemplateRequest.Builder().name("My index 2")
        .indexPatterns("index_pattern3")
        .withJson(inputStream)
        .name("name")
        .build();
  • The field values consistently reflect the latest assignment. In this specific scenario, the name field will have the value assigned through the name() method because it's called after the withJson thus overwriting the value (i.e if withJson ever set it ) .
  • While withJson may appear unconventional, but it merely acts as a mutator that populates fields within the builder, potentially overwriting previously assigned values and then returning the Builder again for potential tailoring (client's choice).
  • This behavior aligns with the design pattern of builders, where multiple calls to setter methods like name will ultimately result in the value closest to the build invocation being used. This approach mirrors the behavior observed in the High-Level Rest Client, demonstrating a well-established and user-accepted pattern.

@reta
Copy link
Collaborator

reta commented Aug 27, 2024

  • While withJson may appear unconventional, but it merely acts as a mutator that populates fields within the builder, potentially overwriting previously assigned values and then returning the Builder again for potential tailoring (client's choice).

Thanks @Jai2305, so what is the behaviour for properties that have not been specified? Will be replaced by null? Will be they kept untouched? To me, it looks not only unconventional but obscure - the users of the API have to guess what is going to happen. I think we should come up with conventional API that leaves no guesses.

I also don't see a clear use case for full fledged builders serde, we do offer the generic client which could be used to manipulate the APIs in JSON only way.

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 28, 2024

@reta

  • they will be untouched, withJson is behaving exactly as if you are making your object using individual builders, because internally thats what it is doing , it is taking whatever the value of current builder is and setting the values in that builder using whatever values you have provided in your Json request .
  • This way you are getting all the features and validations you get when you use builder pattern to populate the fields.
    Eg in PutIndexTemplateRequest , you will get an error if you call build without setting the name since it is a required property so either you can use something like this -
//  setting name using name field on builder 
String stringTemplate =
            "{\"composed_of\":[\"component1\",\"component2\"],\"index_patterns\":[\"index_pattern1\"]}";
        InputStream inputStream = new ByteArrayInputStream(stringTemplate.getBytes(StandardCharsets.UTF_8));
        
        PutIndexTemplateRequest indexTemplateRequest = new PutIndexTemplateRequest.Builder().name("My index 2")...
            .withJson(inputStream)
            .build();

Or

//  setting name  using withJson
  String stringTemplate =
            "{\"name\":\"My index\",\"composed_of\":[\"component1\",\"component2\"],\"index_patterns\":[\"index_pattern1\"]}";
        InputStream inputStream = new ByteArrayInputStream(stringTemplate.getBytes(StandardCharsets.UTF_8));

        PutIndexTemplateRequest indexTemplateRequest = new PutIndexTemplateRequest.Builder()
            .withJson(inputStream)
            .build();

I think it is perfect that way no?

@reta
Copy link
Collaborator

reta commented Aug 28, 2024

  • they will be untouched, withJson is behaving exactly as if you are making your object using individual builders, because internally thats what it is doing , it is taking whatever the value of current builder is and setting the values in that builder using whatever values you have provided in your Json request .

Thanks @Jai2305, but than how to set specific value to null using JSON? The Builder pattern with per field access is very easily to reason about, the suggested withJson is not (in my opinion). Semantically, withJson is equivalent to creating a new instance of the Builder. And I still don't see any compelling use case for that to be fair (yes, I understand that we replicate what Elasticsearch Java does but it does not mean we should).

@dblock do you have any thoughts on this feature in general?

@dblock
Copy link
Member

dblock commented Aug 28, 2024

@dblock do you have any thoughts on this feature in general?

I was 100% in agreement with @Jai2305's comment in #1148 (comment). A builder's .build() ultimately is a JSON representation of the query, so withJson merges whatever you pass in with whatever is already there. I don't think it's confusing.

The other option is to only have .withJson for free-formed parameters (e.g. an index mapping). I hope we don't have APIs that take 2 free formed JSONs :) It's certainly safer. Is that what you're advocating we do @reta? I like it too.

but than how to set specific value to null using JSON?

By specifying null in the JSON "{ key: null }" which is different from absence of "key".

Semantically, withJson is equivalent to creating a new instance of the Builder.

Right, and the result is the modified builder with whatever you set via withJson.

@dblock
Copy link
Member

dblock commented Aug 28, 2024

Thanks @dblock

The other option is to only have .withJson for free-formed parameters (e.g. an index mapping). I hope we don't have APIs that take 2 free formed JSONs :) It's certainly safer. Is that what you're advocating we do @reta? I like it too.

That's right

@Jai2305 WDYT? Thanks so much for hanging in here with us!

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 29, 2024

Thank you for your insights @dblock and @reta,
sorry but just for my understanding ,
By free-formed parameters do we mean the data that are not strictly structured or constrained? - do we have a decided list for such classes/cases maybe an example will help .
Is the new proposal about limiting the implementation of withJson to certain classes/groups? or we are looking for a complete different implementation other then the idea we have to mutate builders?

@reta
Copy link
Collaborator

reta commented Aug 29, 2024

Thanks @Jai2305

By free-formed parameters do we mean the data that are not strictly structured or constrained? - do we have a decided list for such classes/cases maybe an example will help .

Here is the examples I had in mind:

  • source for index requests (basically the document)
  • mappings for create / update index mappings
  • template for index templates

@Jai2305
Copy link
Contributor Author

Jai2305 commented Aug 29, 2024

So basically we only implement withJson for these type of properties which are free formed. I do agree that it's certainly more safer that way. Do we have a strict list of such parameters ( or any pattern to determine if they are appropriate or can be grouped in as free formed) , do you think that there is possibility for us not anticipating a property or deem it necessary right now to implement withJson feature and in future we will have to be open for changes ?

@dblock
Copy link
Member

dblock commented Aug 29, 2024

So basically we only implement withJson for these type of properties which are free formed. I do agree that it's certainly more safer that way. Do we have a strict list of such parameters ( or any pattern to determine if they are appropriate or can be grouped in as free formed) , do you think that there is possibility for us not anticipating a property or deem it necessary right now to implement withJson feature and in future we will have to be open for changes ?

I don't think we have a list but adding is never a problem, it's removing that breaks backwards compat. So I'd add an interface, implement it for the couple of cases we know and then find the missing ones incrementally as people need them. Of course you can comb through https://github.com/opensearch-project/opensearch-api-specification but ... it's a lot of APIs :)

@Jai2305
Copy link
Contributor Author

Jai2305 commented Sep 2, 2024

I have worked on implementing PlainDeserializable for certain free-form fields within our application. To ensure expected behaviors, I have also added test cases.

I've identified these fields as potential candidates based on initial analysis, but I'm also reviewing various request-response cycles to identify additional fields that might benefit from this implementation.

I'd appreciate any insights or suggestions you may have regarding other fields that could be suitable for the withJson feature.
Or we can go with these fields initially and open an issue so that people can add their opinions and requests for other fields

@dblock
Copy link
Member

dblock commented Sep 2, 2024

Or we can go with these fields initially and open an issue so that people can add their opinions and requests for other fields

I like this better, start small.

dblock
dblock previously approved these changes Sep 2, 2024
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.

I'm good with this, @reta?

Let's add some working examples in https://github.com/opensearch-project/opensearch-java/tree/main/samples please?

CHANGELOG.md Outdated Show resolved Hide resolved
@reta
Copy link
Collaborator

reta commented Sep 3, 2024

I'm good with this, @reta?

Agree @dblock , looks like a great start, thanks a lot @Jai2305 !

@Jai2305
Copy link
Contributor Author

Jai2305 commented Sep 3, 2024

Let's add some working examples in https://github.com/opensearch-project/opensearch-java/tree/main/samples please?

I have also added a working sample and an explanation in docs directory for user's reference.

@reta
Copy link
Collaborator

reta commented Sep 3, 2024

@dblock LGTM, please go ahead if you concerns are resolved, thank a lot @Jai2305 !

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.

Great job, thanks!

@dblock dblock added the backport 2.x Backport to 2.x branch label Sep 3, 2024
@dblock dblock merged commit 17c94e0 into opensearch-project:main Sep 3, 2024
57 of 59 checks passed
@dblock
Copy link
Member

dblock commented Sep 3, 2024

@Jai2305 will you please help backport this to 2.x manually? looks like the automatic workflow failed

@Jai2305 Jai2305 mentioned this pull request Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Provide an easy way to deserialize classes annotated as @JsonpDeserializable
3 participants