No, because it was not spec compliant. It clearly didn't respect JsonbVisibility rules.
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 >>>>>> >>>>> >>>>> >>> >> >>
