Rewrite ExampleJsonGenerator

[timonback]

Describe the feature request
Instead of using the manual approach (basically StringBuilder), we should use a library like jackson (using JsonNode and similar objects) to build the example payload as a tree and the convert it to string at the end.

Motivation
When we removed swagger-inflector, we started with a very manual approach to create an example json object from an asyncapi schema.
This approach has some short-comings.

Technical details
The class in question: https://github.com/springwolf/springwolf-core/blob/master/springwolf-core/src/main/java/io/github/stavshamir/springwolf/schemas/example/ExampleJsonGenerator.java

It does have decent test coverage already, although test can always be added.

Describe alternatives you've considered

[Sakshi-75]

Hi @timonback , can I work on this?

[timonback]

Sure, @Sakshi-75 . Go ahead.

If you have any questions, feel free to discuss here or join us on Discord

[Sakshi-75]

Hi @timonback ,
I had a couple of questions regarding this:

1. Is it okay if the order of items in the JSON is changed?
   Input: ObjectSchema schema = new ObjectSchema();
   schema.addProperty("s", new StringSchema());
   schema.addProperty("b", new BooleanSchema());
   Current output: "{"b": true,"s": "string"}"
   New output: "{"s":"string","b":true}"
2. Is it okay that the space between ':' and the value is removed?

[timonback]

The sorted keys are new, so I am open to go back to the previous behaviour (properties are ordered by appearance).

I guess that is also a matter of how the ObjectMapper is configured (same with your second question).
We have this (resolved) issue from a couple weeks ago (#357), which encourages to reuse existing (possibly user-configured) ObjectMappers. I can imagine that we use the autowired objectmapper (and just take its configuration), or use the objectmapper of the DefaultAsyncSerializerService - no further configuration needed.

tldr: Yes, to both if avoiding it is too much of a hassle

[Sakshi-75]

Hi @timonback ,
I was able to fix the order issue. As for the spacing, I tried using the object mapper and pretty printer from the DefaultAsyncApiSerializerService but that is changing the output to this:
"{
"b": true,
"s": "string"
}"

I tried to add the spaces manually as well but that was not working properly in case of composite objects. Is it okay if I go ahead without adding the spaces or should I use the pretty printer version?

[timonback]

Let's take the simple route and we go from there :)

[Sakshi-75]

Hi @timonback ,
I've raised a PR for this with my current changes. Please review and let me know if anything else is required for this.

[timonback]

Hi @Sakshi-75
This is great, thank you for your first contribution!
The change in spaces is no issue from my point of view and ready to be merged.

In a later stage, we hope to support the allOf property of the schema annotation, which currently (wrongly) behaves like the oneOf property. We imagine that the rewrite of the class will help to resolve this issue - besides the error prone string concatenations.

Are you up to continue the rewrite to ObjectNode/etc for the internal methods of the class as well?
This would mean that probably most methods return objectNodes and only the public interface does return the json string.

Feel free to continue on the branch. If you prefer, we can also merge it at this point. Whatever you prefer.

[Sakshi-75]

Hi @timonback ,
Currently I have some other tasks on my hand so I won't be able to pick this up right now. You can go ahead and create an issue for this maybe, and then by the time I get free from my other tasks if this hasn't already been picked up by someone else, I can pick it.

[dipeshsingh253]

In a later stage, we hope to support the allOf property of the schema annotation, which currently (wrongly) behaves like the oneOf property. We imagine that the rewrite of the class will help to resolve this issue - besides the error prone string concatenations.

Hello @timonback , I would like to work on this issue if it's still available.

[timonback]

Hi @dipeshsingh253
sure, go ahead.

I merged @Sakshi-75 code, so you can use on her contributions.

[dipeshsingh253]

Hi @timonback , Thanks for your quick response.

I am unsure where to start on this issue, so if you can share the file or module then it will be faster for me.
Also, provide any additional info if it requires my attention to work on this issue.

[timonback]

Hi @dipeshsingh253,
as written in the issue description, the ExampleJsonGenerator is located at https://github.com/springwolf/springwolf-core/blob/master/springwolf-core/src/main/java/io/github/stavshamir/springwolf/schemas/example/ExampleJsonGenerator.java

The goal is the following:

Are you up to continue the rewrite to ObjectNode/etc for the internal methods of the class as well?
This would mean that probably most methods return ObjectNodes and only the public interface does return the json String.

This is a pure technical refactoring so that we avoid passing around strings and concatenating them together. Instead, we let Jackson (via ObjectNode) handle this.

[dipeshsingh253]

Hello @timonback , I have successfully completed the setup, and I greatly appreciate your assistance with it. In addition, I've made some adjustments to the ExampleJsonGenerator.java file, which you can find here: link.

However, I've encountered an issue: Previously, for string schemas(most of the datatypes like string, boolean), we were returning "string" directly. But now, in internal methods, we need to return an ObjectNode. Unfortunately, this results in an output like {"string":"string"}. I've attempted to find a solution for creating an ObjectNode without a key, but none of the solutions I found online seem to work.

I would greatly appreciate your guidance on whether I am on the right path or if there's an alternative approach I should consider.

Thanks once again, @timonback! Working on this issue is an exciting opportunity for me to contribute to open source!

[timonback]

As these are internal methods, you are free to refactor/change them as you need - including their return type - as long as the tests are passing at the end.

I had a look at the docs, it seems there is a TextNode to represent a string:
https://fasterxml.github.io/jackson-core/javadoc/1.9/org/codehaus/jackson/node/TextNode.html

[timonback]

We can also start small. Feel free to open a draft PR and then we go from there. PRs are easier to review for us, as they only show the actual changes and we can run the tests on them.

Happy to assist

[sam0r040]

Hi @dipeshsingh253 , thanks a lot for your contribution. Overall your change looks good, but there are some minor changes we would like to see. See my comments in the PR.

[dipeshsingh253]

Hi @timonback @sam0r040 , I think we can close this issue now.

[timonback]

The final refactoring was released. Thank you for the contributions!