Send a PR with the "SPI" option which enables to have this toggle *at will* and drop it when not desired anymore without any config. Hope it illustrates better than words one toggle option which wouldnt depend on the env.
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. 2 déc. 2020 à 11:49, Jean-Louis MONTEIRO <jeano...@gmail.com> a écrit : > I understand it's a nice feature and the RFC does not address it. > What I'm not happy with is that adding this feature breaks what's actually > in the spec. > > I would prefer us to implement this feature without breaking standard > features. > I'll push a proposal for now and we can improve. > > > > > Le mar. 1 déc. 2020 à 15:13, Romain Manni-Bucau <rmannibu...@gmail.com> a > écrit : > > > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore < > > jonathan.gallim...@gmail.com> a écrit : > > > > > I'll address a few points inline below, but at a high level, what are > we > > > looking to achieve from a spec/tck challenge? > > > > > > I can see a case for some clarification and updates to the Javadoc. > > > > > > The assertions that /- will return an error (as that references an > index > > to > > > append to after the *end* of an array - i.e. array.length) are tested > in > > > the TCK, and other implementations must be passing that TCK. It's hard > to > > > see a spec change happening, as there is no spec document beyond the > RFCs > > > that I can find. A TCK change that would enable Johnzon to pass, and > > > require other currently passing implementations to make a change seems > > > unlikely. Jakarta EE 8's TCK has been around a while and has > > > implementations that pass. The Jakarta EE 9 TCK is basically "done" and > > is > > > essentially the same as EE8, bar the namespace change. I guess adding a > > > test exclude is possible, but serves to make this more vague and vendor > > > dependent (and non-portable) which feels like it defeats the purpose - > > > surely having it better defined and tested is the way to go. > > > > > > > Well, here the fact is that it does not impact other vendors since it is > a > > johnzon vendor specific feature we put in a shadow of the (javax/jakarta) > > spec handling in a custom fashion an error case. > > Typically the case where we can exclude the TCK since it is irrelevant > for > > our impl but I understand also it is not perfect. > > > > > > > > > > I appreciate that this introduces a backwards incompatible change, and > > that > > > there may be other consumers of the library that would have an issue if > > > this just changed. This seems like a fairly straightforward case that > > could > > > be easily and quickly solved with a feature switch, and passing the TCK > > is > > > a worthwhile goal, both for Johnzon and TomEE. I suspect the TCK > > challenge > > > will take a bit of time, and we'll likely end up back at the feature > > switch > > > anyway. > > > > > > > Issue is we dont have a Json.createPointerFactory(mapWithToggle) so it > is a > > global flag which means it breaks some deployments anyway - at least at > > tomee level - when > 1 app is deployed (or >= 1 app + 1 extension). > > > > > > > > > > On Fri, Nov 27, 2020 at 10:44 AM Romain Manni-Bucau < > > rmannibu...@gmail.com > > > > > > > wrote: > > > > > > > Hi JL, > > > > > > > > As discussed together - but sharing for others - we must take into > > > account > > > > some points: > > > > > > > > 1. reading both spec, JSON-Patch enables to handle /- as your first > did > > > (ie > > > > consider it is last element). JSON-Patch uses JSON-Pointer but > nowhere > > it > > > > is written it behaves as JSON-Pointer in all cases and it is > typically > > > > "integration" definition which can extend an underlying spec > (otherwise > > > > most of EE wouldn't be right? ;)) > > > > > > > > > > I think the idea is that it references a non-existent element, *after* > > the > > > last element in an array. So if you have an array [0, 1, 2, 3, 4], then > > > "/-" would reference element _5_ (assuming you start your numbering at > > 0), > > > and not the last element in the array (index 4). > > > > > > > This is the jsonpointer spec right, but JSONPatch never requires to not > > handle the case as we do, it is just not written (and why we used it > also). > > Issue on jsonpointer side being we can't have another character which > means > > "last element". > > > > > > > > > > > > > > 2. On johnzon point of view we can't break this feature which was > > > requested > > > > by user and transitive users (ie user of products built with johnzon) > > > > without at least a clear migration path so if we want to break we > > should > > > do > > > > a 1.3 (dont think we need a 1.2 maintenance branch, we can do it > > lazily), > > > > document how to migrate from current behavior to new one (i'll detail > > it > > > > after) and communicate on it on our website properly (index.html ref > > and > > > > dedicated page I guess with the release annoucement). Alternative is > to > > > > challenge the TCK, it is a failure case so it is typically the kind > of > > > case > > > > we can plug custom/vendor behavior (we do in other parts of the > JSON-B > > > spec > > > > for ex). Overall idea is to not let users on the road because some > TCK > > > > exist (functional and users over procedural work). > > > > > > > > > > I'd be interested in the history, it helps to be mindful of it when > > making > > > changes. > > > > > > > Goal is to be able to work on the last element, there is nothing in specs > > about this one but it is very common to need that (see it as "length" > > operator). > > Indeed we can enrich jsonlogic module to cover that case but most users > > just bring jsonp+jsonb and not johnzon-jsonlogic. > > > > > > > > > > > > > > > > > > On strict TCK side, we can also do a johnzon-tck module where we wrap > > the > > > > provider to handle that case and pass the TCK, this is purely > technical > > > to > > > > be compliant but would avoid to break anything. > > > > Now if we really want to be strict in our implementation we must > still > > > > enable this last element case. One option not far from what we have > is > > to > > > > use our json-logic module and add some jsonpatch operators. Combining > > > > multiple operators we can manage to fulfill this common patching > need - > > > but > > > > we break the overall API + require a new module to be added to apps). > > > > > > > > Lastly I would note that JSON Pointer *enables* our impl: > > > > > > > > > Note that the use of the "-" character to index an array will > always > > > > > > > > result in such an error condition because by definition it refers > to > > > > a nonexistent array element. Thus, applications of JSON Pointer > > need > > > > to specify how that character is to be handled, if it is to be > > > > useful. > > > > > > > > > > > > > For example, some applications might stop pointer processing upon > an > > > > > > > > error, while others may attempt to recover from missing values by > > > > inserting default ones. > > > > > > > > > > > > Literally means "this is a case we consider as an error but your > > > > application can recover from it" and we do ;). > > > > > > > > > > Sort of. "applications of JSON Pointer need to specify how that > character > > > is to be handled". What's the definition of "application of JSON > > pointer"? > > > In the case of TomEE, I'd suggest the "application" is Jakarta EE, > which > > > has specified that an error should be thrown. In a standalone case, is > > the > > > application whatever is consuming Johnzon, or Johnzon itself? > > > > > > > Well TCK define it but not the JSON-P spec and I'm more than happy to > > request to drop that TCK since it was completely passed under the radar - > > guess TCK were never really reviewed). > > Also note that the JsonPointer javadoc - since there is no pdf or spec > > document - does not mention it must implement the RFC but only that it > must > > respect its syntax and part of its constraints. > > So really it was under the radar more than anything and we must not > assume > > this TCK was intended originally when JsonPointer class was created IMHO. > > > > > > > > > > > > > > Since it is an error case I would start by challenging the TCK to > make > > it > > > > vendor dependent and exclude it from the passing list for now. > > > > If really blocking we can go with plan B and try to have a migration > > path > > > > but it sounds like a lot of effort for everyone for literally 0 gain > > > IMHO. > > > > > > > > > > Personally, I'd prefer a switch that enables us to comply with the > > Jakarta > > > EE spec behaviour, rather than introducing something vendor specific > and > > > non-portable into the spec. > > > > > > > We would have the factory I would be for that but since we don't I see > way > > more pitfalls than advantages - except passing the TCK there is none > > actually but in OSGi env or multiapp containers it would be a real pain > :( > > - so if it is the solution you want (and I fully get it is the fastest to > > pass TCK which is likely current goal) then maybe just wrap JsonProvider > > with a custom TCKJsonPointer(johnzonProvider.createJsonPointer(...)) and > > validate the tck case. > > It is quite trivial to do in tomee-tck setup and will give you a "not > > risky" flagging. > > > > Don't get me wrong, I'm not very happy of that but I just don't want we > > drop an used feature for a test which is not needed at spec level. > > Alternative I discussed with JL was to provide a clear migration path and > > adding some jsonlogic operator to fill the gap, it is also very doable, > > only point I'm not sure is that adding a module will match other users > > expectations. Assuming that yes we can do a 1.2.9 keeping this feature > (we > > have some fixes we should let go out before any breaking change), do the > > changes in jsonlogic module (we can do them before too since it is only > > additions) and do a 1.3.0 fully compliant with the documentation updated. > > Small variation of this option is to have our own SPI for JsonPointer > > factory this way it can be overriden for TCK and we can also keep our > > impl, I'm less a fan of this one since it will bring a proprietary import > > in portable code in a nasty way instead of splitting it properly (like > > jsonlogic module option does). > > > > Hope it makes sense. > > > > > > > > > > Jon > > > > > > > > -- > Jean-Louis >
