Hi Markus, Globally it looks ok but noticed some issues with johnzon design (not speaking of import order which is not always respected):
* pure reflection instead of accessmode abstraction which uses meta annotation and things like that so the find annotation should use the access mode (mainly in handler) * reflection to create the mapping config, please pass a function in mapperconfig instead of a Class to use the constructor (sthg like Function<MapperConfig, MappingConfig> maybe) -> to stay graalvm friendly without needing a META-INF/native-image or to register the jsonbmappingconfig class * as mentionned, please keep current ClassMapping constructor like it is, it is used in libs (fine to add a new one) * The code to the current polymorphism dropped, we must be sure we have a strict equivalent solution, including the streaming one on generator side (the jsonb is not) - so doc should be kept and updated with the new solution if there is one else we just keep the old way * on pure code style maybe take care to import order but more important wildcard imports ;) But globally it looks like a good solution for me and more than acceptable in terms of mapper/jsonb design. 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 ven. 14 avr. 2023 à 22:15, Markus Jung <m...@markus-jung.dev> a écrit : > 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> 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> >> wrote: >> > >> > Le mer. 12 avr. 2023 à 16:24, Markus Jung <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> 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 >> > >> >>>> 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> >> >> 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> 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 >> >>>> >> >> >> >> >
