jakartaee / jsonp-api

Jakarta JSON Processing
https://eclipse.org/ee4j/jsonp
Other
141 stars 61 forks source link

#153 implemented abstract JsonObject and JsonArray #227

Open amihaiemil opened 4 years ago

amihaiemil commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

m0mus commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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