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

Update jsonb.adoc - Annotate adapter on a class #340

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mkarg
Copy link
Contributor

@mkarg mkarg commented May 10, 2023

It would be rather comfortable if we could allow to annotate any POJO with @JsonbTypeAdapter, so ANY use of that POJO in ANY location (as field content, in collections, as keys or values in maps) would trigger the use of the named adapter class.

It would be rather comfortable if we could allow to annotate any POJO with `@JsonbTypeAdapter`, so ANY use of that POJO in ANY location (as field content, in collections, as keys or values in maps) would trigger the use of the named adapter class.
@mkarg
Copy link
Contributor Author

mkarg commented May 10, 2023

@m0mus @rmannibucau I would kindly invite you into the discussion of this feature proposal. :-)

@rmannibucau
Copy link

What about deprecating adapters in favor of (de)serializers? I always found it inconsistent and misleading in their usage cause they have hidden limitations even if idea looks great upfront.

@mkarg
Copy link
Contributor Author

mkarg commented May 11, 2023

What about deprecating adapters in favor of (de)serializers? I always found it inconsistent and misleading in their usage cause they have hidden limitations even if idea looks great upfront.

From the user's perspective I need to say that most people I met really love adapters but dislike serializers, so we should keep them but remove the limitations. In this particular case, there not even seems to be a technical constraint, as e. g. Yasson already supports in some situations.

BTW, according to the Jakarta EE WG's documents it is forbidden to remove existing features.

@rmannibucau
Copy link

we should keep them but remove the limitations

At some point it will not be possible, like adapters loop handling or defining if a chain should be supported (forbidden today), it also probably goes way too far in terms of mapping and will have deep implications when mixed with cdi too much which looks wrong from a design perspective so shouldnt be allowed even if technically we can go a bit further.

I need to say that most people I met really love adapters but dislike serializers

So they probably dislike adapters since you can code adapters as (de)serializer (it is one line) but not the opposite so maybe a doc issue or comm issue but technically it is not just lower level, it is lower+same level (context).

BTW, according to the Jakarta EE WG's documents it is forbidden to remove existing features.

Fully agree but also fully inconsistent with Jakarta EE WG acts since it is at eclipse, all releases broke and removed features.
That said I never said to drop without warning, just to deprecate in next release with a remove for N+2 or 3 which is allowed by jakarta (was already by EE).

So summary looks like:

  • only pro of adapters is to be serializer+deserializer in a single class -> ok to have a codec interface handling both de+serializer side, no other advantage
  • it has pitfalls we'll never fix right and which will be very misleading for users
  • it is ok to deprecate

this is why I think working on them is making the spec more complex with no user gain

@m0mus
Copy link
Contributor

m0mus commented May 19, 2023

Thanks for your PR @mkarg. I checked the source code, @JsonbTypeAdapter can be placed on type, field, method, and parameter. @JsonbTypeAdapter on a method is uncovered my the doc. It would be good to add it. Also, it may make sense updating the JavaDoc accordingly.

@rmannibucau
Copy link

@m0mus what about dropping adapters so freezing the features as this (ie we don't care about class case), seems logicial with the high level api of (de)serializer today, just miss a serial+deserial API but since it includes the mapping API, serializer is included easily too and avoids multiple entry points which is always a nightmare for end users to pick the right API. I know we discussed it for 1.0 but the defintion concurrency of the two API didn't finish/timed very well but since we have now this feedback we can fix it and deprecate adapter concept in favor of a clean mapping. This also makes the view concept quite pointless (keeping the API minimal and light is always good for such multifacet library IMHO). Can finish a PR this way if it helps.

@mkarg
Copy link
Contributor Author

mkarg commented Dec 23, 2024

Thanks for your PR @mkarg. I checked the source code, @JsonbTypeAdapter can be placed on type, field, method, and parameter. @JsonbTypeAdapter on a method is uncovered my the doc. It would be good to add it. Also, it may make sense updating the JavaDoc accordingly.

@jakartaee/ee4j-jsonb-project-leads Kindly asking to consider this PR for merge.

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