Thanks Romain. Should all be merged into the same PR. Fill free to review and close yours. It should be included in mine.
Le mer. 2 déc. 2020 à 15:38, Romain Manni-Bucau <rmannibu...@gmail.com> a écrit : > Let say it enables more control whereas this kind of toggle system > properties can be random when apps don't need the same (once again it is > functional and not about perf there). > Feel free to take back my PR, was just sharing the idea with code to try to > fix it together faster. > > 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 à 15:30, Jean-Louis MONTEIRO <jeano...@gmail.com> a > écrit : > > > 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 > > > -- Jean-Louis
