Le dim. 10 avr. 2022 à 17:19, Mark Struberg <strub...@yahoo.de.invalid> a écrit :
> PS: Ad JsonbAccessMode design: the problem is that you need different > behaviour depending on whether @JsonbVisibility is there or not. > Any custom AccessMode needs to implement the same if they want Jsonb > support. Of course the design is imo not really sustainable any more, but > cleaning that up would likely break backward compat. We could do that in > the next release. > Previous proposal with the additional flag was making it matching both cases cleanly so real reason to break the api and design IMHO > LieGrue, > strub > > > > Am 10.04.2022 um 17:15 schrieb Mark Struberg <strub...@yahoo.de>: > > > > 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? > > > > 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 > >>>> > >>> > >>> > > > >
