jakartaee / jsonb-api

Jakarta JSON Binding
https://eclipse-ee4j.github.io/jsonb-api/
Other
79 stars 39 forks source link

Support builder pattern like Jackson's @JsonPOJOBuilder #191

Open misl opened 5 years ago

misl commented 5 years ago

Currently using JSON-B with immutable objects is hardly doable.

@JsonbCreator is only partially helpful here, but makes all parameters required. But also with many parameters the solution starts to smell. Normally one would use the builder pattern in this scenario. However unlike Jackson (@JsonPOJOBuilder) JSON-B does not support it.

aguibert commented 5 years ago

I like the idea of supporting the builder pattern in JSON-B, since it's a common alternative to a public all-args constructor when creating immutable objects.

rmannibucau commented 5 years ago

I'm -1 to get it as a core feature since it quits the scope of JSON-B because it is basically defining another way to map an object on top of JSON-P (so a sibling and not a child of the spec) but it can be an appendix of the spec (i.e. an optional part/proposal).

misl commented 5 years ago

Without support for Builder Pattern JSON-B is, in my opinion, practically useless for many developers.

It is not really important to me how this can be done as long as it it possible to use the Builder Pattern. An option might be to allow using the @JsonbCreator on the Builder class and refer to the class being created. Or maybe at class level of the original class and have it refer to the builder class and build method.

rmannibucau commented 5 years ago

@misl did you give a try to adapters to implement the builder pattern? it works not bad and avoids to create a concurrent model. Also I kind of disagree on the useless side, jsonbcreator enables immutable objects quite well and mainstream style is still anemic POJO for JSON mapping so don't hide this part of the iceberg under the the builder pattern which is actually the exception today IMHO.

misl commented 5 years ago

@rmannibucau primary reason why I haven't switched to JSON-B yet is my requirement for immutable objects. Initially I thought I could use @JsonbCreator, but it appears in that case all arguments are mandatory. Which also conflicted with my requirements. There is some discussion on this subject in https://github.com/eclipse-ee4j/yasson/issues/237 . I tried to get around this. Even sent in a PR, but it was rejected as current reference implementation is in line with the standard. So my experience is that for immutable objects @JsonbCreator might only work well in edge cases.

But even then, if for a constructor or creator method the number of arguments exceeds some arbitrary limit (say 5 or 10) the code becomes smelly and maintenance unfriendly.

I agree it could be solved with an adapter. JsonbCreator can also be solved with an adapter. Yet there is a separate annotation for that. Also it requires me to write an additional class. In comparison with Jackson, I only needed 2 annotations. My mantra mostly is: more code leads to more maintenance. So when I can do with less code I prefer it.

Is JSON-B only intended for a mainstream coding style?

rmannibucau commented 5 years ago

@misl I will likely repeat a few things spread accross tickets so don't take it badly if obvious but here is my view on that kind of issue:

  1. spec will need a property to not require parameters to be present in the JSON in jsonbcreators (@JsonbCreator(required={}) for example, using same naming than property order annotation). There is no real other option to keep the creator usable and interoperable, validation does not belong to the spec but to a layer on top of the spec (jsonschema being the most obvious but there are also business validations) so no reason to fail here and as you mention it does not support real world very long.
  2. the code smell you mention is where java is going with case classes so not a big deal, the main rule of thumb here is that the constructor limit must apply to classes which are not pure data structures (anemic pojo) only
  3. it is not that we must only support mainstream coding style but we can't not support it (the other way around)
  4. before adding another coding style we must ask ourself the ROI we get from it and here we mainly propose two competitive approach without being able to promote one or the other (you can find a tons of pitfalls to builder pattern too) and at the end the code handling if quite different for a poor gain IMHO - otherwise you didn't do a builder pattern but just used another convention for setters which is often mixed with builders. Note that jackson builder pattern is mainly about calling a method once the json "set" and not a builder pattern which must enforce the order the fields are set. SO mixing required fields in the constructor (jsonbcreator) and setters, you end up with the same solution. You just miss the build() call but it is useless since the building state is fully encapsulated by the mapper.

So long story short I would propose to push to get 1 ASAP instead of trying to get something probably half baked.

Hope it makes sense.

m0mus commented 5 years ago

It's a nice feature, but I disagree that it's must have feature. Deserialization of immutable objects can be achieved by using @JsonbCreator or by annotating class properties with @JsonbProperty or (in future) by using runtime configuration. Possibly we should think how to make runtime configuration API builders friendly.

rmannibucau commented 5 years ago

@m0mus if we get an introspector API (reflection abstraction) as mentionned in a few other tickets then we can let it be set on the JsonbConfig. This enables to handle fluent API (not builder but here the builder = fluent API + factory method) then it is just a matter of adding a deserializer which uses the builder instead of the main class and calls it (it is a one liner).