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

Support builder pattern like Jackson's @JsonPOJOBuilder #191

Open
misl opened this issue Sep 13, 2019 · 8 comments
Open

Support builder pattern like Jackson's @JsonPOJOBuilder #191

misl opened this issue Sep 13, 2019 · 8 comments
Labels
enhancement New feature or request

Comments

@misl
Copy link

misl commented Sep 13, 2019

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 aguibert added the enhancement New feature or request label Sep 13, 2019
@aguibert
Copy link
Contributor

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
Copy link

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
Copy link
Author

misl commented Oct 2, 2019

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
Copy link

@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
Copy link
Author

misl commented Oct 2, 2019

@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 eclipse-ee4j/yasson#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
Copy link

@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
Copy link
Contributor

m0mus commented Oct 3, 2019

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants