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