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