support for withJson-618

[Jai2305]

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]

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]

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

[Jai2305]

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]

@dblock any thoughts on above ?

[dblock]

@dblock any thoughts on above ?

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

[dblock]

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?

[dblock]

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

[Jai2305]

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

[Jai2305]

@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]

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

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

[Jai2305]

• 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]

@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]

@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]

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

2. 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

3. 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]

@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]

• 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]

@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]

• 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]

@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]

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]

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]

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]

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]

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]

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]

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]

I'm good with this, @reta?

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

[reta]

I'm good with this, @reta?

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

[Jai2305]

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]

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

[dblock]

Great job, thanks!

[dblock]

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