+1 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 mer. 13 avr. 2022 à 08:44, Mark Struberg <strub...@yahoo.de.invalid> a écrit : > Will start a release soonish. > > LieGrue, > strub > > > Am 12.04.2022 um 10:07 schrieb Mark Struberg <strub...@yahoo.de.INVALID > >: > > > > took your PR and enhanced on top of it. > > > > Plz review! > > > > txs and LieGrue, > > strub > > > >> Am 10.04.2022 um 20:08 schrieb Romain Manni-Bucau < > rmannibu...@gmail.com>: > >> > >> 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 > >>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>> > >>>>>> > >>>> > >>>> > > > >
