Le dim. 10 avr. 2022 à 17:33, Mark Struberg <strub...@yahoo.de.invalid> a écrit :
> No, because it was not spec compliant. It clearly didn't respect > JsonbVisibility rules. > We all agreed on that but proposal was ;) > LieGrue, > strub > > > Am 10.04.2022 um 17:23 schrieb Romain Manni-Bucau <rmannibu...@gmail.com > >: > > > > 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 > >>>>>> > >>>>> > >>>>> > >>> > >> > >> > >
