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 > >>>>> > >>>> > >>>> > >> > >> > >
