Hi Romain, thanks for giving me some ideas, those saved me a lot of time! Basically went down option #2 as it seemed the easiest for me. At first I tried to just use a reader/writer like you suggested, but that ended up looking like going down a whole other rabbit hole to me. I created JsonbMappings that overwrites createClassMapping to inject jsonb polymorphism handling there. This still changes a bit how polymorphism works in Mapper internally, but is not exposed in the API and doesn’t break it.
Not sure though if my solution is the cleanest for this problem. Can you give me some thoughts on that? Would highly appreciate it. Thanks Markus > On 12. Apr 2023, at 19:33, Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > > Well I had in mind a few options - the ones I checked working, there are > others indeed: > > * (my preferred) use jsonbaccessmode to override find reader/writer and if > polymorpshim is enabled then just return (potentially wrapped if one is > already found - this is undefined in jsonb AFAIK) the (de)serializer adding > polymmorphism handling. > * either to enable to define a JsonbClassMapping which extends ClassMapping > and instantiate a JsonbMappings in the mapper (mainly enabling to provide a > custom Mappings instance in the builder). Then it is just a matter of using > an object writer/reader in the mapping to handle the polymorphism. This is a > bit low level but opens some doors for other stuff. > * just do it in to/from json methods of json, unwrapping types until you get > the Class and if polymorpshim is enabled then wrap it there with some handler > as you defined (but scoped to jsonb). What I don't like with this option is > the the fact to keep a map<Type, ClassWithPolymorphismHandlerOrNull> to avoid > to keep recomputing it and the additional check compared to the previous > option but it is way simpler and I can envision a johnzon.skip-polymorphism > toggle if this overhead is not acceptable for the app. > > There are also other ways I didn't investgate like overriding some mapper > methods or things like that but this feature is very particular and > particularly defined so IMHO mapper wouldn't benefit from it. > > Romain Manni-Bucau > @rmannibucau <https://twitter.com/rmannibucau> | Blog > <https://rmannibucau.metawerx.net/> | Old Blog > <http://rmannibucau.wordpress.com/> | Github <https://github.com/rmannibucau> > | LinkedIn <https://www.linkedin.com/in/rmannibucau> | Book > <https://www.packtpub.com/application-development/java-ee-8-high-performance> > > Le mer. 12 avr. 2023 à 19:16, Markus Jung <m...@markus-jung.dev > <mailto:m...@markus-jung.dev>> a écrit : >> Hi Romain, >> >> > I wouldn't leak in mapper the jsonb design we don’t fully ack. >> How do you mean that? Remove MapperBuilder#setPolymorphismHandler again? How >> do I hand down the polymorphism impl from jsonb then, some sort of SPI? >> >> >> Thanks >> >> Markus >> >> > On 12. Apr 2023, at 17:06, Romain Manni-Bucau <rmannibu...@gmail.com >> > <mailto:rmannibu...@gmail.com>> wrote: >> > >> > Le mer. 12 avr. 2023 à 16:24, Markus Jung <m...@markus-jung.dev >> > <mailto:m...@markus-jung.dev>> a écrit : >> > >> >> Hi Romain, >> >> >> >> >> >> thanks for your feedback, I think the upgrade won't go seamless for most >> >> users because of javax.* to jakarta.* migration. >> >> >> > >> > I see, but we already went through this with JSONB 2. >> > >> > >> >> >> >> But if someone only uses mapper directly johnzon 2.x might actually just >> >> be a drop in replacement for johnzon 1.x, depending on how it's used. >> > >> > >> > I didnt understand, mapper is not JSONB. >> > That said we have two cases: >> > >> > * mapper only usage (no jsonb) >> > * @polymorphism from jsonb using mapper api (never said it is good but it >> > is used) >> > >> > >> >> I >> >> agree that we should aim to make this possible, still think it might be >> >> a bit confusing in MapperBuilder though. I added a bit of documentation >> >> on what PolymorphismHandler is, why it exists and what setting it does. >> >> >> > >> > >> > Check a bit your PR, I wouldn't leak in mapper the jsonb design we don't >> > fully ack. >> > We also try to not break Mapper API (constructors). >> > Rest and design looks ok otherwise. >> > >> > Side note: don't forget to close your json instance in tests (we have a >> > rule for that if that helps), we see too much broken snippet not doing it >> > to do it ourselves. >> > >> > >> >> >> >> >> >> Thanks >> >> >> >> Markus >> >> >> >> On 11.04.23 21:32, Romain Manni-Bucau wrote: >> >>> Well thing is we will NOT make it to johnzon-mapper but only in >> >>> johnzon-jsonb so all good for me. This is a jsonb specific thing so will >> >> be >> >>> implemented in jsonb module. Some plumbing can be done in mapper while it >> >>> does not break anything, does not make things slower nor surface in the >> >> api. >> >>> >> >>> Both are different features and one is more relevant in several cases so >> >> we >> >>> should kezp both IMHO. >> >>> >> >>> Side note: we made the upgrade seamless so not sure what you mean. >> >>> >> >>> Sure we can break now but not to drop a feature IMHO - no acceptable >> >>> migration is possible there AFAIK. >> >>> >> >>> Last, TCK does not require much breaking change - or if some is missed we >> >>> will challenge and fix the spec - so not a strong point for me too to >> >>> assume there will be any breakage (if so jakarta would be dead right? >> >> They >> >>> already hurt themselves enough with this kind of behavior and jsonb is >> >> not >> >>> a bad citizen there) >> >>> >> >>> Le mar. 11 avr. 2023 à 21:08, Markus Jung <m...@markus-jung.dev >> >>> <mailto:m...@markus-jung.dev>> a >> >> écrit : >> >>> >> >>>> Hi all, >> >>>> >> >>>> I’ve made some progress on my jsonb 3 polymorphism impl and I’m now at a >> >>>> point where I feel comfortable to open a PR if we can agree on how to >> >> move >> >>>> forward with johnzon-mapper’s polymorphism. >> >>>> >> >>>> @romain I think having two more or less unrelated sets of properties in >> >>>> MapperBuilder for the same thing adds more confusion than what it’s >> >> worth. >> >>>> The code is not really that much more complex, just adds a bit more >> >>>> flexibility I needed for hooking up jsonb polymorphism. For >> >> johnzon-mapper >> >>>> users that rely on a simple way to handle polymorphic types theres >> >>>> JohnzonMapperPolymorphismHandler, though I’d like to move it’s >> >> properties >> >>>> out of MapperBuilder into some other Builder that can be used just like >> >>>> PolymorphicConfig could be on the jsonb side. >> >>>> >> >>>> I’m with JL here - upgrading to johnzon 2.* won’t be seamless anyways, >> >> so >> >>>> quite honestly I’m okay with a breaking change. Just need to clearly >> >>>> document how to migrate in some sort of changelog/johnzon 2.0 migration >> >>>> guide. Feel like this won’t be the only breaking change that will arise >> >>>> from jsonb 3/jsonp 2.1 tck compatibility. >> >>>> >> >>>> >> >>>> Thanks >> >>>> >> >>>> Markus >> >>>> >> >>>>> On 8. Apr 2023, at 09:52, Romain Manni-Bucau <rmannibu...@gmail.com >> >>>>> <mailto:rmannibu...@gmail.com>> >> >>>> wrote: >> >>>>> Hi Markus, >> >>>>> >> >>>>> Great news! >> >>>>> >> >>>>> Personally I'd keep the small wiring we have right now and implement >> >>>> jsonb3 >> >>>>> flavor aside since both have different design and dont converge so no >> >>>> need >> >>>>> to make it complicated. >> >>>>> >> >>>>> Le ven. 7 avr. 2023 à 22:57, Jean-Louis MONTEIRO <jeano...@gmail.com >> >>>>> <mailto:jeano...@gmail.com>> >> >> a >> >>>>> écrit : >> >>>>> >> >>>>>> Hi Markus, >> >>>>>> >> >>>>>> I'll have a look from Monday on. Thanks for jumping into this and >> >>>> helping >> >>>>>> out. >> >>>>>> >> >>>>>> Note that if we have to break something to pass the TCK now is the >> >> good >> >>>>>> time. >> >>>>>> >> >>>>>> We are already breaking backward compatibility because of javax to >> >>>> Jakarta >> >>>>>> namespace. So I don't have any problem. >> >>>>>> >> >>>>>> Best regards >> >>>>>> >> >>>>>> Le ven. 7 avr. 2023, 16:46, Markus Jung <m...@markus-jung.dev >> >>>>>> <mailto:m...@markus-jung.dev>> a >> >> écrit >> >>>> : >> >>>>>>> Hi all, >> >>>>>>> >> >>>>>>> I’m currently working on implementing JSON-B 3 polymorphism. See >> >>>>>>> https://github.com/jungm/johnzon/tree/jsonb3-polymorphism, still WIP >> >>>> as >> >>>>>> I >> >>>>>>> need to write tests + documentation and remove johnzon-jsonb-extras. >> >>>> But >> >>>>>>> it’s passing all TCK polymorphism tests except for one as of right >> >> now >> >>>>>> (And >> >>>>>>> that last one is probably related to locale validation in johnzon). >> >>>>>>> >> >>>>>>> I couldn’t really implement this on top of the current way >> >> polymorphism >> >>>>>>> works in johnzon-mapper because it expects that JSONs will always >> >> only >> >>>>>>> contain one property to describe it’s type vs the JSON-B 3 spec >> >> having >> >>>>>>> multiple properties to describe the type. >> >>>>>>> As of now I have created a default polymorphism implementation that >> >>>>>>> seamlessly mimics the way polymorphism used to work before in >> >>>>>>> johnzon-mapper to avoid breaking changes for users, but this approach >> >>>>>>> creates some redundancies in MapperBuilder. >> >>>>>>> >> >>>>>>> Ideally I’d like to completely remove this old polymorphism code also >> >>>>>> from >> >>>>>>> MapperBuilder and create some sort of PolymorphismBuilder that could >> >> be >> >>>>>>> used instead and plugged into MapperBuilder. But this will obviously >> >>>>>> break >> >>>>>>> the API for anyone using johonzon-mapper directly with polymorphism. >> >>>> Any >> >>>>>>> thoughts on this? >> >>>>>>> >> >>>>>>> >> >>>>>>> Thanks >> >>>>>>> >> >>>>>>> Markus >> >>>> >> >> >>
