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