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 <[email protected]> wrote: > > Le mer. 12 avr. 2023 à 16:24, Markus Jung <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>> 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 <[email protected]> >> 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 <[email protected]> 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 >>>> >>
