Hey Romain, Your email was clear on how to do it. But thanks for creating the PR. To be honest, I don't really care if you prefer this way. Essentially, instead of adding a property into a file, I'll now add a Maven dependency into my pom file.
Does not change much. If on the other hand, it makes OSGi deployments easier, I'm fine with it. I just need some updates in your PR if I may. Le mer. 2 déc. 2020 à 13:13, Romain Manni-Bucau <rmannibu...@gmail.com> a écrit : > 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 > > > -- Jean-Louis
