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
