Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selectors reworked ... #98

Closed
wants to merge 2 commits into from
Closed

Selectors reworked ... #98

wants to merge 2 commits into from

Conversation

goessner
Copy link
Collaborator

... Filter selectors added

Copy link
Collaborator

@glyn glyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's wonderful to see a PR covering filters and descendants. Well done @goessner!

The downside is that the PR is quite large and I ran out of steam in my initial sitting. In other words, I'll probably have more detailed comments next time I look.

I would have preferred a separate PR for descendants and then a series of PRs gradually filling out filter expressions, so that detailed review is more feasible. Once we get two or three people's comments on this PR, it's likely to become unmanageable.

draft-ietf-jsonpath-base.md Show resolved Hide resolved
draft-ietf-jsonpath-base.md Show resolved Hide resolved
draft-ietf-jsonpath-base.md Show resolved Hide resolved
draft-ietf-jsonpath-base.md Show resolved Hide resolved
draft-ietf-jsonpath-base.md Show resolved Hide resolved
draft-ietf-jsonpath-base.md Show resolved Hide resolved
* Parenthesis can be used with `boolean-expr` for grouping. So filter selection syntax in the original proposal `'[?(<expr)]'` is naturally contained in the current lean syntax `'[?<expr]'` as a special case.
* Comparisons are restricted to primitive values `number`, `string`, `true`, `false`, `null`. Comparisons with complex values will fail, i.e. no selection occurs.
* Implicite type conversions during comparisons are not performed. So `"13 == '13'"` selects nothing.
* A member or element value by itself is *falsy* only, if it does not exist. Otherwise it is *truthy*, resulting in its value. To be more specific explicite comparisons are necessary. This existence test &ndash; as an exception of the general rule &ndash; also works with complex values.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please define the terms truthy and falsy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps instead simply drop "truthy" and "falsy" in favor of just saying that a rel-path-value by itself select the item if the relative path can be resolved (or some such language).

Having "truthy" and "falsy" at all contradicts the "explicit comparisons are necessary" statement since these terms are typically used in systems with loose comparisons.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead define it, dropping is better.

draft-ietf-jsonpath-base.md Show resolved Hide resolved
* Regular expression tests can be applied to `string` values only.
* Containment tests work with arrays and objects.
* Explicite boolean type conversion is done by the not operator `neg-op`.
* The behaviour of operators is consistent with the 'C'-family of programming languages.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement seems out of place in the syntax section. Do we need an "Informal Semantics" section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be moved. Not sure when to move to semantic or informal semantics section.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goessner wrote:

will be moved. Not sure when to move to semantic or informal semantics section.

If by "when" you meant "whether", I think the statement should be moved to a new informal semantics section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should perhaps read better:

Syntax and behaviour of operators is consistent with the 'C'-family of programming languages.

Otherwise we might need to add a table of operator precedence ... which would be no problem though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Have a look at how RFC 8949 solves that problem:
https://www.rfc-editor.org/rfc/rfc8949.html#section-1.2-6

The IESG will insist on a reference to the C standard, I can tell you this much :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glyn: By "when" I mean "whenever" in comparable situations, should it goto

  • syntax
  • semantics
  • informal semantics

Are there specific rules especially to allocate to the last two.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glyn: By "when" I mean "whenever" in comparable situations, should it goto

* syntax

* semantics

* informal semantics

Are there specific rules especially to allocate to the last two.

No rules, but the idea with array slices was to introduce the semantics gently in the (informative) informal semantics section and then spell out the (normative) semantics, possibly more indigestibly, in the semantics section. But I am not wedded to this convention so long as we don't confuse normative and informative text.


rel-path-val = "@" *(dot-selector / index-selector)
calc_val = func "(" [rel-path-val / json-path] ")"
func = "index"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please define the semantics of index in the semantics section.

@cabo
Copy link
Member

cabo commented Jun 11, 2021

This PR (actually PR #98) appears to revert all the fixes in 9be270d
Is this intentional?

@goessner
Copy link
Collaborator Author

goessner commented Jun 11, 2021 via email

@cabo
Copy link
Member

cabo commented Jun 11, 2021

Indeed, I do prefer readable documents over unreadable ones :-)

We can discuss this, but my question really was about undoing a change that I thought we had agreed to make.

@cabo
Copy link
Member

cabo commented Jun 11, 2021

I will change that (back).

Please use the branch pr-98 or merge it, before generating more changes.

(My tools have a hard time juggling multiple branches that are called main, hence I created that branch to make my fixes.)

Copy link
Collaborator

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that these sets of changes (the selectors and defining the filters) could have been better served in two PRs, but this looks like a good start.


#### Semantics
The Argument &mdash; the root JSON value &ndash; is anonymous by nature. By getting assigned the universal name `'$'` it becomes the root node.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. You're claiming the thing that's named "Argument" is anonymous, but once referenced with $ it suddenly has a name? I'm not sure this provides any benefit over just saying that the Argument is the root node and can be referenced with $.

Comment on lines +629 to +637
dot-selector = "." dot-member-name
dot-member-name = 1*(
DIGIT /
ALPHA /
"_" / ; _
%x80-10FFFF ; any non-ASCII Unicode character
)
DIGIT = %x30-39 ; 0-9
ALPHA = %x41-5A / %x61-7A ; A-Z / a-z
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definition also resolves #16 (and duplicate #94). 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still needs to be discussed, if a dot-member-name is allowed to start with a DIGIT.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does applying .1 to [1, 2, 3] mean?

draft-ietf-jsonpath-base.md Show resolved Hide resolved
Comment on lines +650 to +651
The behaviour of an implementation is undefined for member names which do
not encode Unicode characters.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of "undefined behavior" 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot happen.

Comment on lines +684 to +685
quoted-member-name = %x22 *double-quoted %x22 / ; "string"
%x27 *single-quoted %x27 ; 'string'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the * for in *double-quoted? The other components (e.g. ESC and unescaped) don't have them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

' simply means multiple ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should point to the string literal syntax that we are using for comparisons as well.

json-path ; any value

rel-path-val = "@" *(dot-selector / index-selector)
calc_val = func "(" [rel-path-val / json-path] ")"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we have a precedence or decision on functions/calculated values yet (index() or otherwise). It's probably best to leave this out until we can identify a family of functions that we'd like to include so that we can decide on a format that works for everything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in fact i am quite reluctant here. It's a proposal needing discussion ... will most certainly result in an issue..

comp-expr = (rel-path-val /
calc-val) [(comp-op comparable / ; comparison
regex-op regex / ; RegEx test
in-op container )] ; containment test
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

container needs a definition

Copy link
Collaborator Author

@goessner goessner Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes ... or more simply

in-op (array / object)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array and object don't have a definition either.
Do you mean expressions that return an array or object?
Or maybe just literals of these types?

Copy link
Collaborator Author

@goessner goessner Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both ...

[[email protected] in ['red,'green','blue']]

or

[[email protected] in $.validColors]


If the result is `true`, the current item, represented by `'@'`, is selected. In case of a `false` result, it is not.

~~~~ abnf
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we decide against arithmetic operations? These are widely supported and should (perhaps eventually) be supported.

* Parenthesis can be used with `boolean-expr` for grouping. So filter selection syntax in the original proposal `'[?(<expr)]'` is naturally contained in the current lean syntax `'[?<expr]'` as a special case.
* Comparisons are restricted to primitive values `number`, `string`, `true`, `false`, `null`. Comparisons with complex values will fail, i.e. no selection occurs.
* Implicite type conversions during comparisons are not performed. So `"13 == '13'"` selects nothing.
* A member or element value by itself is *falsy* only, if it does not exist. Otherwise it is *truthy*, resulting in its value. To be more specific explicite comparisons are necessary. This existence test &ndash; as an exception of the general rule &ndash; also works with complex values.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps instead simply drop "truthy" and "falsy" in favor of just saying that a rel-path-value by itself select the item if the relative path can be resolved (or some such language).

Having "truthy" and "falsy" at all contradicts the "explicit comparisons are necessary" statement since these terms are typically used in systems with loose comparisons.

calc_val / ; calculated value
json-path ; any value

rel-path-val = "@" *(dot-selector / index-selector)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest support for any selector except another filter selector (but it may be supported later). This would allow further support for things like $[?(@..foo contains 42)] which would return true for any item that contains a foo property with a value of 42 at any descended level.

@goessner
Copy link
Collaborator Author

goessner commented Jun 11, 2021 via email

@glyn
Copy link
Collaborator

glyn commented Jun 14, 2021

@goessner please rebase

@goessner
Copy link
Collaborator Author

goessner commented Jun 14, 2021 via email

@cabo
Copy link
Member

cabo commented Jun 14, 2021

... and I went ahead and did what I think is a full rebase, with all the changes, no conflicts, and a correct build. Now #102.

@cabo
Copy link
Member

cabo commented Jun 14, 2021

#102.

... and I put in a short review there as well.

cabo added a commit that referenced this pull request Jun 15, 2021
cabo added a commit that referenced this pull request Jun 15, 2021
cabo added a commit that referenced this pull request Jun 15, 2021
cabo added a commit that referenced this pull request Jun 15, 2021
@cabo cabo closed this Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants