JsonArray.of & JsonObject.of

[MukjepScarlet]

Purpose

Like Java 9 and Guava.

Description

closes #2923

Checklist

• New code follows the Google Java Style Guide
  This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
  Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
• If necessary, new public API validates arguments, for example rejects null
• New public API has Javadoc
  • Javadoc uses @since $next-version$
    ($next-version$ is a special placeholder which is automatically replaced during release)
• If necessary, new unit tests have been added
  • Assertions in unit tests use Truth, see existing tests
  • No JUnit 3 features are used (such as extending class TestCase)
  • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
• mvn clean verify javadoc:jar passes without errors

[Marcono1234]

Thanks for the pull request! To me this looks quite reasonable and the implementation looks good¹, but let's see what the project members think about this.

Footnotes

1. It will need unit tests though, but might be good to wait for the feedback from the project members, to avoid wasted work. ↩

[Marcono1234]

These of overloads make it a bit verbose, though I guess there is no better way at the moment and Guava and the JDK do it like this as well.

The other alternative would be to only have maybe of 1-3 and for the rest require the user to use a different approach, e.g. the above mentioned of(Map).

Would be good to wait for the opinion of one of the project members.

[Marcono1234]

Thanks for the changes!

I think you slightly need to adjust the types so that subtypes of JsonElement don't cause issues. See comments.

[MukjepScarlet]

@Marcono1234 Can we use some codegen technique to generate factory methods for them? I don't know if we can modify an existing class, for example, adding a static method.

[Marcono1234]

Which factory methods are you referring to?

Sorry if my previous comment was a bit confusing; I just meant that the methods you added should use ? extends JsonElement (as shown in the individual comments), but nothing else.

Or do you want to use code generation to generate the JsonObject#of methods automatically to avoid the code duplication? In my opinion the code duplication there is ok since it is quite simple (it is not ideal but there is maybe no better solution). Setting up code generation just for that does not seem to justify the additional complexity.

[Marcono1234]

Thanks for the changes! A few more minor things.

[Marcono1234]

@eamonnmcmanus, what do you think?

[MukjepScarlet]

I will probably split this PR into 2 or more for easier review & merging.

[eamonnmcmanus]

Sorry for not responding earlier. I completely agree that the JsonArray and JsonObject APIs are somewhat awkward, and in particular it is unnecessarily inconvenient to build up a tree of JsonElement objects using those APIs. However I'm not sure that just adding these factory methods is the right way to improve things. My feeling is that we might be better off with a general API that translates an arbitrary tree of Java objects (using List, Map, Number and so on) into the corresponding JsonElement tree. We would need to take the time to specify such an API carefully before sending any PRs. I hope to create a Gson issue shortly where we can discuss approaches.

[MukjepScarlet]

I think extensions about addAll can be a simpler PR, like addAll(JsonElement...)
Also I noticed Gson is going to be Java 8, so we can have collectors for JsonArray and JsonObject too.

[MukjepScarlet]

My feeling is that we might be better off with a general API that translates an arbitrary tree of Java objects (using List, Map, Number and so on) into the corresponding JsonElement tree.

Generally we have something like DTO list/map which can't be simply parsed into JSON tree without serializer. If we only allow Map to JsonObject, this API might be not so useful.

[eamonnmcmanus]

My feeling is that we might be better off with a general API that translates an arbitrary tree of Java objects (using List, Map, Number and so on) into the corresponding JsonElement tree.

Generally we have something like DTO list/map which can't be simply parsed into JSON tree without serializer. If we only allow Map to JsonObject, this API might be not so useful.

I think what you are describing is what Gson already does with its TypeAdapter API. What I'm describing is a way to build JSON trees more conveniently, typically with List.of(...) and Map.of(...), where the only types that appear in the compound expression are List, Map, and primitives (in the JsonPrimitive sense). So I shouldn't have said "an arbitrary tree of Java objects". But I hope to spell all that out in a separate issue to make it clearer.

[softpapers]

1. All the overloaded methods for "of" are not needed.
2. Consider using

      public static JsonObject of(String... keysAndValues){
           int size = keysAndValues.lenght();
           if(size % 2 != 0) throw YourExceptionForInvalidParam("******");
               JsonObject object = new JsonObject();
               for(int i = 0; i < size; i+2;){
                   object.add(keysAndValues[i], keysAndValues[i+1]);
               }
             return object;
     }

[MukjepScarlet]

This can only be a latest overload, like ImmutableMap.

[softpapers]

Why is such a requirement in the first instance?
What problems are you solving with that?

[MukjepScarlet]

var it = JsonObject.of("key", JsonArray.of());
it.toString() -> {"key":[]}

instead of

var it = new JsonObject()
it.add("key", new JsonArray());