Hi Markus, I did a review on the PR and seems several concerns were not adressed (but don't worry they are globally minor I think). On the polymorphism I think you are right and the main issue is that the doc was deleted instead of updated to explain how to migrate. The overall feedback of the PR is to ensure runtime does not compute things N > 1 times, that we respect our user contract (don't worry, I agree the mapeprconfig one was maybe not obvious) and that we limit the diff to its impact if possible since it is big enough (so a few original changes which are not needed and style should be fixed when possible).
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 dim. 16 avr. 2023 à 17:41, Markus Jung <m...@markus-jung.dev> a écrit : > Hi Romain, > > yet again - thanks for your feedback! > > I’ve addressed all issues except for: > > * 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 > > > Don’t really understand what you mean by that. The old code for > polymorphism is gone, yes. But it should work the same as Mapper just sets > the fields in ClassMapping in a way that it _should_ just work. Also > literally put the new polymorphism handling in the same place the old one > lived in MappingGenerator/Parser. > Am I missing something there? Or do you mean jsonb-extras polymorphism? > > > Thanks > > Markus > > > On 15. Apr 2023, at 09:00, Romain Manni-Bucau <rmannibu...@gmail.com> > wrote: > > 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 > > >
