588 - [FEATURE] serialize requests to JSON

[Jai2305]

Serialize requests to JSON

• This pull request adds an interface PlainJsonSerializable extending JsonpSerializable having a default method to streamline serialization process ( get a string representation of instances of all classes implementing PlainJsonSerializable interface ) , which abstracts away the implementation of having to call serialize method with mapper and generator.
• As a result the classes implementing JsonpSerializable may now implement PlainJsonSerializable if they are required to carry the added default method otherwise they may implement JsonpSerializable
• Added test cases to ensure correctness of the code.

• Self Check
• Added Test Cases
• ChangeLog.md

Issues Resolved - Issue #588

SignOff -

I understand and agree that this project and the contribution are public and that a record of the contribution (including
all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Please check Developer Certificate of Origin here.

[dblock]

Btw, not sure this will work as part of this PR, but we also want serialization of ndjson requests (e.g. bulk).

[reta]

Btw, not sure this will work as part of this PR, but we also want serialization of ndjson requests (e.g. bulk).

There are 2 ways to tackle that (off the top of my head, there are more for sure):

• since this is not a correct Json (but ndjson), we could not implement PlainJsonSerializable (but fe have PlainNdJsonSerializable)
• do override the toJsonString for BulkRequest & co

[Jai2305]

There are 2 ways to tackle that (off the top of my head, there are more for sure):

• since this is not a correct Json (but ndjson), we could not implement PlainJsonSerializable (but fe have PlainNdJsonSerializable)
• do override the toJsonString for BulkRequest & co

• For both of the above discussions, why not we make a general interface, where we will declare only serialize method , it will be one level above JsonpSerializable , and NdJsonSerializable .
• This way we have an optout option from JsonpSerializable default method and in future we could implement different Serializable interfaces if the need arise for eg: NdJsonSerializable

public interface JsonSerializable {
    void serialize(JsonGenerator generator, JsonpMapper mapper);
}

public interface JsonpSerializable extends JsonSerializable  {
    default String toJsonString() {
        try (StringWriter writer = new StringWriter()) {
            try (JsonGenerator generator = JsonpUtils.DEFAULT_PROVIDER.createGenerator(writer)) {
                serialize(generator, JsonpUtils.DEFAULT_JSONP_MAPPER);
            }
            return writer.toString();
        } catch (IOException ex) {
            throw new UncheckedIOException(ex);
        }
    }
}

public interface NDJsonSerializable extends JsonSerializable  {
   .... 
}

[reta]

• For both of the above discussions, why not we make a general interface, where we will declare only serialize method , it will be one level above JsonpSerializable , and NdJsonSerializable .

• This way we have an optout option from JsonpSerializable default method and in future we could implement different Serializable interfaces if the need arise for eg: NdJsonSerializable

I think this is what is being suggested (keeping JsonpSerializable)? #1064 (comment)

[Jai2305]

I think this is what is being suggested (keeping JsonpSerializable)? #1064 (comment)

Yes , I am suggesting a new parent interface and keep JsonpSerializable extending that new interface

[reta]

By defining a top layer , we won't be touching the current implementations and uses (all classes ) of JsonpSerializable , at the same time provide an option to opt out from the default method , which was our concern.

I think it circles back to the reasons why not to do that (#1064 (comment)). The goal is to have 2 separate hierarchies, PlainJsonSerializable & PlainNdJsonSerializable, so toJsonString would be only implemented once by the right class in question (if we change JsonpSerializable, every single class will inherit its toJsonString right away, including the ones that use NdJson type of serialization)

public interface PlainNdJsonSerializable extends JsonpSerializable  {
    default Collection<String> toJsonStrings() {
    }
}

[Jai2305]

The goal is to have 2 separate hierarchies, PlainJsonSerializable & PlainNdJsonSerializable, so toJsonString would be only implemented once by the right class in question (if we change JsonpSerializable, every single class will inherit its toJsonString right away, including the ones that use NdJson type of serialization)

• One last clarification, we already have NdJsonpSerializable interface right ?

public interface NdJsonpSerializable {
    Iterator<?> _serializables();
}

how does this fit in the suggested model.

[reta]

One last clarification, we already have NdJsonpSerializable interface right ? how does this fit in the suggested model.

Correct, it fits super well I believe, for example:

public interface PlainNdJsonSerializable extends NdJsonpSerializable, JsonpSerializable  {
    default Iterator<String> toJsonStrings() {
     ....
    }
}

[Jai2305]

@reta added a separate commit for a new interface PlainJsonSerializable , which extends JsonpSerializable and has the new default toJsonString method .

• As of now I have made every class implementing JsonpSerializable, implement PlainJsonSerializable , as a result many files were changed due to the import statement
• Only concern is if we want to Opt out any class from PlainJsonSerializable now and implement JsonpSerializable , so that we do not inherit the newly added default method, as there are many classes , how will we determine which class to opt in or out ?

[reta]

Thanks @Jai2305

• As of now I have made every class implementing JsonpSerializable, implement PlainJsonSerializable , as a result many files were changed due to the import statement

Expected, thank you!

Only concern is if we want to Opt out any class from PlainJsonSerializable now and implement JsonpSerializable , so that we do not inherit the newly added default method, as there are many classes , how will we determine which class to opt in or out ?

That could be done on case by case I believe , the one of such is BulkRequest fe (it is NdJson)

[reta]

Super nit, could you please revert this change? (so the commit would be clean)

[reta]

Suggested change

	public class JsonpSerializableTest extends Assert {
	public class PlainJsonSerializableTest extends Assert {

[Jai2305]

Fixed according to suggestions , also changed CHANGELOG.md to reflect the addition of new interface, I will have a check on the code, but please let me know , if you find anything out of order.

[reta]

@dblock could you take a look as well please? thank you!

[dblock]

I have some nits on CHANGELOG and errors.

More importantly, can we update https://github.com/opensearch-project/opensearch-java/blob/main/USER_GUIDE.md or one of the guides with this (possibly a new one called json.md)? Even something basic would be great.

[dblock]

Remove the period and add a space to match other CHANGELOG lines.

I think we should explain the purpose of this Add support for serializing requests and responses to plain JSON and nd-JSON?

[Jai2305]

Sorry for the delay, I have made a change on this, tried to keep it short and clear in meaning, please let me know if any work is remaining , and when can we merge this PR and withJson , very much needed .
thanks a lot :)

[dblock]

I am good with merging this.

There's a build failure with the generator, which I think is legit, and a template needs to be modified, please take a look?

Thank you!

[Jai2305]

There's a build failure with the generator, which I think is legit, and a template needs to be modified, please take a look?

• Yes , I think I accidently changed some generated files, Fixed it.
  But the white source Security check is failing and the only message that I see is
  Oops! An error occurred while running the Security Check.The Scanner was unable to connect to the repository or branch and clone it. , is it trying to connect with my forked repo? sorry not familiar with this myself .

[dblock]

There's a build failure with the generator, which I think is legit, and a template needs to be modified, please take a look?

• Yes , I think I accidently changed some generated files, Fixed it.
  But the white source Security check is failing and the only message that I see is
  Oops! An error occurred while running the Security Check.The Scanner was unable to connect to the repository or branch and clone it. , is it trying to connect with my forked repo? sorry not familiar with this myself .

Ignore it. I think someone was looking into it.

[dblock]

Merged, thanks.

@Jai2305, will you document this in USERS_GUIDE, maybe a new json.md guide in guides and a working sample?