Created https://github.com/apache/johnzon/pull/83 to try to share what I meant and try to solve it to enable the release (note I did it a bit in a hurry, the coming weeks are quite busy for me - so happy to get a second pair of eyes + optionally somebody running the jsonb tck)
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. 10 avr. 2022 à 18:42, Romain Manni-Bucau <rmannibu...@gmail.com> a écrit : > > > Le dim. 10 avr. 2022 à 17:34, Mark Struberg <strub...@yahoo.de.invalid> a > écrit : > >> Oki will re-introduce that ct. >> Should then be backward compat, isn't? >> > > If the modifier check is there back yep > > >> LieGrue, >> strub >> >> >> > Am 10.04.2022 um 17:24 schrieb Romain Manni-Bucau < >> rmannibu...@gmail.com>: >> > >> > Le dim. 10 avr. 2022 à 17:16, Mark Struberg <strub...@yahoo.de.invalid> >> a >> > écrit : >> > >> >> Actually there is not much diff. There is one ct which was only used in >> >> JsonbAccessMode as true. Everywhere else it was feed as false. >> >> Now we could keep this flag and feed it as default if you really like. >> >> That should be rather easy to do if you think it's necessary. But >> still I'd >> >> mark that 4param ct as @Deprecated. Wdyt? >> >> >> > >> > Well it is used by users writing custom access mode quite often so we >> must >> > keep it and its associated behavior as a minimum for coming release. >> > No issue to make an abstract class with more children to keep it >> simpler in >> > the code but please dont break it. >> > >> > >> >> LieGrue, >> >> strub >> >> >> >>> Am 10.04.2022 um 16:24 schrieb Romain Manni-Bucau < >> rmannibu...@gmail.com >> >>> : >> >>> >> >>> Hi Mark, >> >>> >> >>> If I read the diff correctly we still have some breaking change in >> >>> FieldAndMethodAccessMode >> >>> constructor/behavior? Can we get it fixed? >> >>> In terms of design, jsonb access mode was originally thought as >> >> delegating >> >>> the research of the writers/readers to a 3rd party access mode and >> just >> >>> refilter on top of it with jsonb rules. >> >>> Now it is kind of mixed in the code (inheritance + composition vs just >> >>> composition), the reason it was not done was to avoid to expose the >> >> default >> >>> delegate access mode when a 4rd party access mode was using and mix >> them >> >> in >> >>> the jsonbaccess mode, not 100% sure if we want to keep the original >> >> design >> >>> there or not but mentionning it since it now prevent us to change it >> >> later. >> >>> >> >>> Overall except the missing constructor it looks ok to me, happy to get >> >>> thoughts on the access mode pattern too. >> >>> >> >>> 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 sam. 9 avr. 2022 à 23:49, Mark Struberg <strub...@yahoo.de.invalid> >> a >> >>> écrit : >> >>> >> >>>> here we go. plz take a look! >> >>>> >> >>>> https://github.com/struberg/johnzon/tree/JOHNZON-364 < >> >>>> https://github.com/struberg/johnzon/tree/JOHNZON-364> >> >>>> >> >>>> txs and LieGrue, >> >>>> strub >> >>>> >> >>>> >> >>>>> Am 09.04.2022 um 23:14 schrieb Mark Struberg >> <strub...@yahoo.de.INVALID >> >>>>> : >> >>>>> >> >>>>> Looked at your proposed solution, but it might be tricky to get it >> >>>> working. The problem is that we have two modules: mapper and jsonb. >> And >> >> in >> >>>> JOHNZON-250 we added a logic to the mapper module which is ONLY >> defined >> >> and >> >>>> useful in Jsonb. This is the alwaysPreferMethodVisibility in >> >>>> org.apache.johnzon.mapper.access.FieldAndMethodAccessMode. This >> actually >> >>>> did break the mapper. Sorry that I did not catch this earlier. >> >>>>> >> >>>>> Note that the FieldAndMethodAccessMode is only used if no other >> >>>> AccessMode is configured. There is also a JsonbAccessMode which >> >> delegates >> >>>> through to the accessMode of the Mapper. Normally means the >> >>>> FieldAndMethodAccessMode, but could be different. >> >>>>> We could implement the logic in JsonbAccessMode, totally ditching >> the >> >>>> accessMode coming from the Mapper. Also thought about having the >> >>>> JsonbAccessMode extending the FieldAndMethodAccessMode and adding the >> >>>> handling as parameter over there. still not really perfect imo. >> >>>>> >> >>>>> Will try to ship a patch proposal tomorrow. >> >>>>> >> >>>>> LieGrue, >> >>>>> strub >> >>>>> >> >>>>> >> >>>>> >> >>>>>> Am 28.03.2022 um 10:22 schrieb Romain Manni-Bucau < >> >>>> rmannibu...@gmail.com>: >> >>>>>> >> >>>>>> Le lun. 28 mars 2022 à 10:05, Mark Struberg >> <strub...@yahoo.de.invalid >> >>>> <mailto:strub...@yahoo.de.invalid>> a >> >>>>>> écrit : >> >>>>>> >> >>>>>>> +1 for getting rid of java.beans. >> >>>>>>> >> >>>>>>> I've created a spec ticket for the open questions: >> >>>>>>> https://github.com/eclipse-ee4j/jsonb-api/issues/325 < >> >>>>>>> https://github.com/eclipse-ee4j/jsonb-api/issues/325> >> >>>>>>> >> >>>>>>> My point is: until my fix we clearly did break 4.6. And class >> >>>> hierarchies >> >>>>>>> are totally not portable right now anyway. Introducing It's just >> >> badly >> >>>>>>> underspecified. >> >>>>>>> So I do not see why the current implementation does make it worse. >> >>>>>>> >> >>>>>> >> >>>>>> Well this one is easy to answer: it changes the behavior and breaks >> >>>> users >> >>>>>> so it is not an option for a patch version and to be honest the >> >> proposed >> >>>>>> fix solves it for both cases so think we should tackle it this >> way, at >> >>>>>> least for now until the spec clarifies it any other way. >> >>>>>> Once again, the source of the issue is more that we lack an >> >> integration >> >>>>>> between mapper and jsonb modules than anything else and this >> >> integration >> >>>>>> had kind of been done but broke mapper so think we should fix it >> back, >> >>>> just >> >>>>>> costs us a constructor we could have avoided without this issue >> AFAIK >> >> so >> >>>>>> not a big deal, no? >> >>>>>> >> >>>>>> >> >>>>>>> >> >>>>>>> LieGrue, >> >>>>>>> strub >> >>>>>>> >> >>>>>>>> Am 21.03.2022 um 09:52 schrieb Romain Manni-Bucau < >> >>>> rmannibu...@gmail.com >> >>>>>>>> : >> >>>>>>>> >> >>>>>>>> Hi, >> >>>>>>>> >> >>>>>>>> From what we discussed on this issue, the thing is that our jsonb >> >>>> access >> >>>>>>>> mode does not enable the visibility to see all readers/writers >> cause >> >>>>>>> there >> >>>>>>>> is some hardcoded filtering in the delegates so I think the >> trick is >> >>>> to >> >>>>>>>> simply add a toggle to the delegates access modes we use to not >> have >> >>>> this >> >>>>>>>> filtering and 100% rely on the visibility filtering in >> jsonbaccess >> >>>> mode >> >>>>>>> we >> >>>>>>>> already have in place. >> >>>>>>>> >> >>>>>>>> Before the release we must also ensure getters computation cache >> is >> >>>>>>> cleaned >> >>>>>>>> up after the ClassMapping is created - we don't want to keep >> that in >> >>>>>>> memory >> >>>>>>>> at runtime and not sure we can clean up at another time since we >> >> don't >> >>>>>>> want >> >>>>>>>> a background thread for that. >> >>>>>>>> I also think it is not worth to depends on java.beans >> (java.desktop >> >>>>>>> module) >> >>>>>>>> so we would reimpl decapitalize as we already did somewhere else >> (it >> >>>> is >> >>>>>>> one >> >>>>>>>> liner anyway ;)). >> >>>>>>>> Lastly, FieldAndMethodAccessMode constructor compatibility must >> be >> >>>>>>> restored >> >>>>>>>> (it is part of our API and used), default should stay 1-1 but as >> >>>>>>> mentionned >> >>>>>>>> I'd add a new toggle (with a constructor calling "this" to keep >> the >> >>>>>>>> backward compat) to ignore the filtering. >> >>>>>>>> >> >>>>>>>> Sounds like this way we solve all issues and can release. >> >>>>>>>> >> >>>>>>>> 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 lun. 21 mars 2022 à 09:00, Mark Struberg >> >> <strub...@yahoo.de.invalid >> >>>>> >> >>>>>>> a >> >>>>>>>> écrit : >> >>>>>>>> >> >>>>>>>>> To sum this up: >> >>>>>>>>> >> >>>>>>>>> The problem lies partly in the JSON-B spec. It doesn't define >> what >> >>>>>>> happens >> >>>>>>>>> if there are class hierarchies like class A extends B. >> >>>>>>>>> >> >>>>>>>>> In 3.7.1 it states that >> >>>>>>>>> For a serialization operation, if a matching public getter >> method >> >>>>>>> exists, >> >>>>>>>>> the method is called to obtain the value of the property. If a >> >>>> matching >> >>>>>>>>> getter method with private, protected, or defaulted to >> package-only >> >>>>>>> access >> >>>>>>>>> exists, then this field is ignored. If no matching getter method >> >>>> exists >> >>>>>>> and >> >>>>>>>>> the field is public, then the value is obtained directly from >> the >> >>>> field. >> >>>>>>>>> >> >>>>>>>>> This can get changed y using a @JsonbVisibility >> >>>>>>>>> >> >>>>>>>>> 4.6 Custom visibility >> >>>>>>>>> To customize scope and field access strategy as specified in >> >> section >> >>>>>>>>> 3.7.1, it is possible to specify >> >>>>>>> javax.json.bind.annotation.JsonbVisibility >> >>>>>>>>> annotation or to override default behavior globally calling >> >>>>>>>>> JsonbConfig::withPropertyVisibilityStrategy method with given >> >> custom >> >>>>>>>>> property visibility strategy. >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> The problem is that that the PropertyVisibilityStrategy is >> simply >> >> not >> >>>>>>>>> ready to implement the rules of 3.7.1 in a portable way when it >> >>>> comes to >> >>>>>>>>> class hierarchies >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>> >> >>>> >> >> >> https://github.com/apache/geronimo-specs/blob/trunk/geronimo-jsonb_1.0_spec/src/main/java/javax/json/bind/config/PropertyVisibilityStrategy.java >> >>>>>>>>> < >> >>>>>>>>> >> >>>>>>> >> >>>> >> >> >> https://github.com/apache/geronimo-specs/blob/trunk/geronimo-jsonb_1.0_spec/src/main/java/javax/json/bind/config/PropertyVisibilityStrategy.java >> >>>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> So either we say that the spec simply does not define what >> should >> >>>> happen >> >>>>>>>>> in case of hierarchic classes (totally valid imo) or we need to >> >> ditch >> >>>>>>> our >> >>>>>>>>> DefaultPropertyVisibility logic completely. It's simply not >> >> possible >> >>>> to >> >>>>>>>>> implement it that way. >> >>>>>>>>> >> >>>>>>>>> The reason why I removed the logic from the Reader is the >> >> following: >> >>>>>>> Even >> >>>>>>>>> if the reader says 'oh there is a getter, so let's hide the >> >> property' >> >>>>>>> you >> >>>>>>>>> STILL are perfectly allowed - as per the spec - to have >> Visibility >> >>>>>>>>> strategies which do disable ALL methods and ONLY use field >> access. >> >>>>>>>>> That means the current impl is surely more correct than before, >> but >> >>>> not >> >>>>>>>>> yet perfect. >> >>>>>>>>> >> >>>>>>>>> LieGrue, >> >>>>>>>>> strub >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>> >> >>>>>>>>>> Am 28.02.2022 um 11:17 schrieb Romain Manni-Bucau < >> >>>>>>> rmannibu...@gmail.com >> >>>>>>>>>> : >> >>>>>>>>>> >> >>>>>>>>>> Hi JL, >> >>>>>>>>>> >> >>>>>>>>>> Last commit broke some stuff. >> >>>>>>>>>> Dicussed it quickly offline with Mark but Im AFK for a few >> days, >> >>>> can it >> >>>>>>>>>> wait some days? Otherwise maybe just pick this commit? >> >>>>>>>>>> >> >>>>>>>>>> Le lun. 28 févr. 2022 à 10:37, Jean-Louis Monteiro < >> >>>>>>>>> jlmonte...@tomitribe.com> >> >>>>>>>>>> a écrit : >> >>>>>>>>>> >> >>>>>>>>>>> Hi all, >> >>>>>>>>>>> >> >>>>>>>>>>> Is it ok to release a 1.2.17? >> >>>>>>>>>>> I'm looking for a Jakarta EE fix - >> >>>>>>>>>>> >> >>>>>>>>>>> >> >>>>>>>>> >> >>>>>>> >> >>>> >> >> >> https://github.com/apache/johnzon/commit/a1c53d99c3280cbfc35b369ac4ce138e12f1d5ff >> >>>>>>>>>>> >> >>>>>>>>>>> Do you guys have some additional fixes to push? >> >>>>>>>>>>> -- >> >>>>>>>>>>> Jean-Louis Monteiro >> >>>>>>>>>>> http://twitter.com/jlouismonteiro >> >>>>>>>>>>> http://www.tomitribe.com >> >>>>> >> >>>> >> >>>> >> >> >> >> >> >>
