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

#153 implemented abstract JsonObject and JsonArray #227

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

amihaiemil
Copy link

PR for #153.

Implemented AbstractJsonObject and AbstractJsonArray. These classes are very useful for users who want to implement their own JsonObject or JsonArray objects.

See JavaDocs for examples of usage.

These classes should be provided by the API since they are provider-agnostic.

@amihaiemil
Copy link
Author

amihaiemil commented Feb 2, 2020

@m0mus You said you are skeptical of the idea and that the idea of the library is for concrete classes to be constructed via the Json factory.

But the scope of the Json factory is to provide concrete implementations in a provider-agnostic way.

The 2 classes in this PR are abstract (basically interfaces as well) and provider-agnostic as well.
I don't see any reason why the API should not provide them, particularly since they are encouraging a more object-oriented design.

If you ask me, even the class CollectedJsonArray (which I gave as example in the JavaDoc of AbstractJsonArray) should be provided by the API, but that's another story :)

So the point is that, if the API does not offer class AbstractJsonArray, I have to implement it myself or implement the methods of JsonArray throughout all of the JsonArray implementations, of my application (such as CollectedJsonArray). Same for the case of JsonObject.

cc: @m-reza-rahman

@amihaiemil
Copy link
Author

amihaiemil commented Feb 2, 2020

@m0mus @m-reza-rahman I've also made an Eclipse Account and signed the ECA. Now I get an error message saying:

Please check that each commit has the 'Signed-off-by' footer as part of the
commit message. This can be added by checking out the offending commit
and using git commit -a -s.

I have reset my commit and redone it with the -s flag. Still, the eclipsefdn check fails...
I'm not sure what's wrong now, I see the same message on my ECA profile page.

@amihaiemil amihaiemil requested a review from m0mus February 2, 2020 13:27
@m0mus
Copy link
Contributor

m0mus commented Feb 3, 2020

I am still skeptical.

  1. JsonObject and JsonArray are not designed to be created this way. Adding abstract base classes assumes that instances created by the API extend them, but it's not true. They just implement JsonObject and JsonArray without extending AbstractXXX classes. Forcing it will break all implementations.

  2. Considering above, it may make sense including these classes into an implementation. It cannot be RI though, because in RI JsonArray is de-facto a list and JsonObject is de-facto a map.

  3. I don't see a use case of extending JsonObject and JsonArray. Examples in javadocs are too abstract and more theoretical than practical. JSONP API already has methods of creating new JSON objects based on existing ones. If you thinking of easy creation of Json objects from Java objects, you should look on JSON Binding API.

@amihaiemil
Copy link
Author

amihaiemil commented Feb 3, 2020

@m0mus I'm answering punctually

  1. JsonObject and JsonArray are not designed to be created this way

JsonObject and JsonArray are interfaces. How they are implemented is not important.

1.[...] Adding abstract base classes assumes that instances created by the API extend them, but it's not true. They just implement JsonObject and JsonArray without extending AbstractXXX classes. Forcing it will break all implementations.

But this is not true. Simply adding these 2 abstract classes does not mean other implementations of JsonObject or JsonArray are forced to extend them. Why do you think JSON-P providers would be obliged to use these classes?

  1. Considering above, it may make sense including these classes into an implementation. It cannot be RI though, because in RI JsonArray is de-facto a list and JsonObject is de-facto a map.

Same as above, these 2 classes have nothing to do with other implementations of JsonObject. And again, adding these classes to a provider's implementation doesn't make sense since they are provider agnostic -- they work only with interfaces from the API, nothing concrete.

  1. JSONP API already has methods of creating new JSON objects based on existing ones.

That's exactly the issue: Users are provided only with methods, which they will use as utilities in other static methods throughout their code.

As the CollectedJsonArray example shows, I want to use the static methods of JSON-P inside objects. I don't want a private static buildArray(...) method in my code (or call one from JSON-P). I want an object which I can decouple and test -- then, later, if the methods offered by JSON-P will change, I can simply make the change inside CollectedJsonArray.

It's not such an uncommon usecase. The point is actually about extension. Extension by composition, not inheritance. And in order to create/extend my own JsonObject or JsonArray I need to implement that interface... and I implement it based on an existing JsonObject or JsonArray.

Another usecase: in order to avoid Swagger (it was too big for my needs), I made my JAX-RS classes implement JsonObject and in their constructor, I initialized the key/value pairs with the JAX-RS endpoints offered by that class. So, I would simply return that object to the user which was automatically rendered as JSON.

@amihaiemil
Copy link
Author

@m0mus Maybe the name of these 2 classes is wrong. Maybe Abstract is too generic and that's why developers might think they are obliged to extend them when implementing JsonObject/JsonArray...

But I can't think of a better name. Ideas? :D

@amihaiemil
Copy link
Author

amihaiemil commented Feb 3, 2020

@m0mus The common/uncommon argument is actually irrelevant if you ask me. As long as functionalities respect given interfaces and are provider-agnostic, I don't see any reason why they should not be added.

This is the standard JSON api for Java, but its authors should not impose their way of thinking on users.

The bigger the variety of tools the users are provided with, the better, I would say :D

@amihaiemil
Copy link
Author

@m0mus Another option would be to add these 2 classes in a package named util, maybe? So it's clear that they are "utility" classes for the users, not for implementors of the API.

@m-reza-rahman
Copy link

Design cohesion of an API is very important. That's why it is important to work closely and as best as is possible align with existing design thinking. Unless a substantial portion of existing users and committers feel otherwise, I am inclined to respect Dmitry's opinion on this. Let us try to see if we can collaborate to make a compelling case for this enhancement.

Reza Rahman
Jakarta EE Ambassador, Author, Speaker, Blogger

Please note views expressed here are my own as an individual community member and do not reflect the views of my employer.

@amihaiemil
Copy link
Author

amihaiemil commented Feb 10, 2020

@m0mus @m-reza-rahman This functionality can also be implemented with interfaces and default methods.

We would have interfaces DefaultJsonObject and DefaultJsonArray with a delegate() method to get the underlying JsonObject or JsonArray. Then, all the methods of the interface would be default and would use this.deletate() to delegate all the method calls.

What do you think? Would it be more suitable, would you accept the PR with this implementation?

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.

3 participants