You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@johnzon.apache.org by Jonathan Gallimore <jo...@gmail.com> on 2020/12/01 13:40:31 UTC
Re: JSON Pointer/Patch issue with /-
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.
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.
On Fri, Nov 27, 2020 at 10:44 AM Romain Manni-Bucau <rm...@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).
> 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.
>
> 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?
> 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.
Jon
Re: JSON Pointer/Patch issue with /-
Posted by Jean-Louis MONTEIRO <je...@gmail.com>.
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 <rm...@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 <je...@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 <rm...@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 <je...@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 <
> rmannibucau@gmail.com
> > >
> > > a
> > > > écrit :
> > > >
> > > > > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> > > > > jonathan.gallimore@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 <
> > > > > rmannibucau@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
Re: JSON Pointer/Patch issue with /-
Posted by Jean-Louis MONTEIRO <je...@gmail.com>.
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 <rm...@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 <je...@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 <rm...@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 <je...@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 <
> rmannibucau@gmail.com
> > >
> > > a
> > > > écrit :
> > > >
> > > > > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> > > > > jonathan.gallimore@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 <
> > > > > rmannibucau@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
Re: JSON Pointer/Patch issue with /-
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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 <je...@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 <rm...@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 <je...@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 <rmannibucau@gmail.com
> >
> > a
> > > écrit :
> > >
> > > > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> > > > jonathan.gallimore@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 <
> > > > rmannibucau@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
>
Re: JSON Pointer/Patch issue with /-
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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 <je...@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 <rm...@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 <je...@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 <rmannibucau@gmail.com
> >
> > a
> > > écrit :
> > >
> > > > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> > > > jonathan.gallimore@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 <
> > > > rmannibucau@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
>
Re: JSON Pointer/Patch issue with /-
Posted by Jean-Louis MONTEIRO <je...@gmail.com>.
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 <rm...@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 <je...@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 <rm...@gmail.com>
> a
> > écrit :
> >
> > > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> > > jonathan.gallimore@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 <
> > > rmannibucau@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
Re: JSON Pointer/Patch issue with /-
Posted by Jean-Louis MONTEIRO <je...@gmail.com>.
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 <rm...@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 <je...@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 <rm...@gmail.com>
> a
> > écrit :
> >
> > > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> > > jonathan.gallimore@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 <
> > > rmannibucau@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
Re: JSON Pointer/Patch issue with /-
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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 <je...@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 <rm...@gmail.com> a
> écrit :
>
> > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> > jonathan.gallimore@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 <
> > rmannibucau@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
>
Re: JSON Pointer/Patch issue with /-
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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 <je...@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 <rm...@gmail.com> a
> écrit :
>
> > Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> > jonathan.gallimore@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 <
> > rmannibucau@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
>
Re: JSON Pointer/Patch issue with /-
Posted by Jean-Louis MONTEIRO <je...@gmail.com>.
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 <rm...@gmail.com> a
écrit :
> Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> jonathan.gallimore@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 <
> rmannibucau@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
Re: JSON Pointer/Patch issue with /-
Posted by Jean-Louis MONTEIRO <je...@gmail.com>.
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 <rm...@gmail.com> a
écrit :
> Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
> jonathan.gallimore@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 <
> rmannibucau@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
Re: JSON Pointer/Patch issue with /-
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
jonathan.gallimore@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 <rmannibucau@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
>
Re: JSON Pointer/Patch issue with /-
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore <
jonathan.gallimore@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 <rmannibucau@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
>