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.

Sign up for GitHub

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

Add tests for incorrect filter usage #56

Closed
wants to merge 2 commits into from
Closed

Add tests for incorrect filter usage #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40c8e74
Add tests for incorrect filter usage
Marcono1234 Feb 25, 2024
a9fcedf
Address review feedback
Marcono1234 Feb 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions cts.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,26 @@
}
]
},
{
"name": "filter, existence without selector",
"selector": "$[?@]",
"document": {
"a": {},
"b": 1
},
"result": [
{},
1
]
},
{
"name": "filter, existence test on primitive, selects nothing",
"selector": "$.a[?@]",
"document": {
"a": 1
},
"result": []
},
{
"name": "filter, equals string, single quotes",
"selector": "$[?@.a=='b']",
Expand Down Expand Up @@ -1844,6 +1864,43 @@
]
]
},
{
"name": "filter, on primitive, selects nothing",
"selector": "$.a[?@ == 1]",
"document": {
"a": 1
},
"result": []
},
{
"name": "filter, on primitive object member value, selects nothing",
"selector": "$[?@.a == 1]",
"document": {
"a": 1
},
"result": []
},
{
"name": "filter, name selector on array",
"selector": "$[?@['0'] == 5]",
"document": [
[
5,
6
]
],
"result": []
},
{
"name": "filter, index selector on object",
"selector": "$[?@[0] == 5]",
"document": [
{
"0": 5
}
],
"result": []
},
{
"name": "filter, relative non-singular query, index, equal",
"selector": "$[?(@[0, 0]==42)]",
Expand Down Expand Up @@ -4096,6 +4153,22 @@
],
"result": []
},
{
"name": "functions, length, filter on primitive array element, selects nothing",
"selector": "$[?length(@)==1]",
Copy link
Contributor

Choose a reason for hiding this comment

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

This test could be made broader, thus:

Suggested change
"selector": "$[?length(@)==1]",
"selector": "$[?length(@)>=0]",

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, it would be good to add a test for the comparison of two Nothing values if we don't already, e.g.:

$[?length(@)==length(@)]

should select all the elements of an array argument.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/jsonpath-standard/jsonpath-compliance-test-suite/blob/main/tests%2Ffilter.json

We have Nothing equality.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@glyn I think these changes are moot given the conversation I've raised. What these tests are trying to verify can be accomplished with several more targeted tests.

"document": [
1
],
"result": []
},
{
"name": "functions, length, filter on primitive object member value, selects nothing",
"selector": "$[?length(@)==1]",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"selector": "$[?length(@)==1]",
"selector": "$[?length(@)>=0]",

"document": {
"a": 1
},
"result": []
},
{
"name": "functions, length, result must be compared",
"selector": "$[?length(@.a)]",
Expand Down
36 changes: 36 additions & 0 deletions tests/filter.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@
{"a": null, "d": "e"}
]
},
{
"name": "existence without selector",
"selector": "$[?@]",
"document": {"a": {}, "b": 1},
"result": [{}, 1]
},
{
"name": "existence test on primitive, selects nothing",
"selector": "$.a[?@]",
"document": {"a": 1},
"result": []
},
{
"name": "equals string, single quotes",
"selector" : "$[?@.a=='b']",
Expand Down Expand Up @@ -478,6 +490,30 @@
[42]
]
},
{
"name": "on primitive, selects nothing",
"selector": "$.a[?@ == 1]",
"document": {"a": 1},
"result": []
},
Comment on lines +493 to +498
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be improved since the .a isn't necessary.

Suggested change
{
"name": "on primitive, selects nothing",
"selector": "$.a[?@ == 1]",
"document": {"a": 1},
"result": []
},
{
"name": "on primitive, selects nothing",
"selector": "$[?@ == 1]",
"document": 1,
"result": []
},

Also, I don't think having a filter here improves the test. It could be any selector.

Suggested change
{
"name": "on primitive, selects nothing",
"selector": "$.a[?@ == 1]",
"document": {"a": 1},
"result": []
},
{
"name": "on primitive, selects nothing",
"selector": "$['a']",
"document": 1,
"result": []
},

would do just as well. If we use this, then we need to have a series of these, and

  • they should be put in basic.json.
  • we should test all of the JSON types. (We already have one testing for an array index on an object and a couple, single- and double-quote versions, testing for a string index on an array.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be improved since the .a isn't necessary.

It seems that would be the first test then with primitive top-level value. I had created #57 to propose in general adding tests for that.

Also, I don't think having a filter here improves the test. It could be any selector.

But those are (at least in the specification) two separate cases which independently say that only arrays and objects are supported, so maybe it would be good to have separate tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(I thought we had those tests for primitives but we only have tests for e.g. "index selector against object".)

{
"name": "on primitive object member value, selects nothing",
"selector": "$[?@.a == 1]",
Comment on lines +500 to +501
Copy link
Collaborator

Choose a reason for hiding this comment

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

This and the following three tests seem to be checking that a relative path (@<...>) can have the same path syntax as a global path ($<...>). If that's something we're testing, that's fine, but I'd argue that we'd need to almost replicate the entire suite for relative paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one was based on hiltontj/serde_json_path#49 where the reporter assumed @ refers to the JSON object instead of the member value.

And the tests "name selector on array" and "index selector on object" are supposed to cover the same issue in serde_json_path where the underlying bug was that an unexpected type (which should select nothing) erroneously always passed the filter. I assume such a bug pattern could affect other libraries as well.

Copy link
Collaborator

@gregsdennis gregsdennis Feb 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

where the reporter assumed @ refers to the JSON object instead of the member value.

Two things here:

  1. We shouldn't be interested in testing what an end user expects, even (especially) if that expectation is wrong. We should be testing implementation behavior per the spec. If the implementation is doing it right and the user expected something different, then the solution is to correct the user. We don't need a test for it.
  2. If we are going to test this, then we should test for interpreting @ directly; we don't need the .a. "$[?@ == 1]" would be a much more targeted test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. We shouldn't be interested in testing what an end user expects, even (especially) if that expectation is wrong. We should be testing implementation behavior per the spec.

Fair point. But I think here if the document is {"a": 1}, then it is necessary to have .a. If you used $[?@ == 1], then [1] is the correct result.

But if you add .a (i.e. ?@.a == 1), then you should get no result. However, libraries might get this wrong if they silently ignore the .a because it cannot be applied to a non-object. And with hiltontj/serde_json_path#49 there is a precedent for that exact issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I view this differently. The test should be that @ represents the 1. This is the simple test that verifies the implementation has the correct behavior.

Separately, we test that .a doesn't apply to a non-object (which is your primitive tests).

That .a can't be applied to a non-object in the context of @ is merely a consequence of the correct behavior and doesn't specifically need a test.

Copy link

Choose a reason for hiding this comment

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

singular-querys, i.e., relative paths with @, are defined separately in the standard ABNF. So, this leads implementations to have separate parsing, AST representations, evaluation mechanisms, etc. - as was the case for serde_json_path.

Therefore, I think there is value in having tests that ensure the behaviour of queries against primitives for both absolute and relative paths. Relative paths can only appear in filters, so having separate tests for those requires queries like @Marcono1234 has used here.

I suppose that the motivation for this test case is that one implementor (me) made a logic error in the handling of singular queries, so, having this test would prevent another from doing the same. It is not possible to come up with tests for every conceivable logic error, or misinterpretation of JSONPath, but I don't see the harm in having a test that guards against one that did happen (and the .a was relevant in that case).

Copy link
Collaborator

@gregsdennis gregsdennis Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

singular-querys, i.e., relative paths with @, are defined separately in the standard ABNF.

While true, @-paths can also be existence tests, which can handle all selector constructs. I believe (on mobile now; can verify letter) that we have some tests that ensure only singular paths are used in comparisons, though. If not, we should add them in another PR.

I suppose that the motivation for this test case is that one implementor (me) made a logic error in the handling of singular queries, so, having this test would prevent another from doing the same.

Okay. That this was an implementation error wasn't understood. I thought this was a user that expected something incorrect.

However, I still maintain, that this test is overly complex and can be broken down into smaller constituent pieces that still achieve the goal of verifying proper operation.

Copy link

Choose a reason for hiding this comment

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

Sorry for taking so long to get back...

While true, @-paths can also be existence tests

This escaped me, thanks for pointing that out. I was incorrect there. Relative path != singular query. The singular query defined in the ABNF is only relevant to comparisons. Relative paths in existence tests or as function arguments can be non-singular.

"document": {"a": 1},
"result": []
},
{
"name": "name selector on array",
"selector": "$[?@['0'] == 5]",
"document": [[5, 6]],
"result": []
},
{
"name": "index selector on object",
"selector": "$[?@[0] == 5]",
"document": [{"0": 5}],
"result": []
},
{
"name": "relative non-singular query, index, equal",
"selector": "$[?(@[0, 0]==42)]",
Expand Down
12 changes: 12 additions & 0 deletions tests/functions/length.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,18 @@
"document" : [{"d": "f"}],
"result": []
},
{
"name": "filter on primitive array element, selects nothing",
"selector": "$[?length(@)==1]",
"document": [1],
"result": []
},
Comment on lines +55 to +60
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're already effectively testing this with "number arg" on line 32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as with #56 (comment); the intention was to make sure no library misinterprets @ as representing the JSON array (instead of its elements).

{
"name": "filter on primitive object member value, selects nothing",
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 the effectively the same test as before: checking to see what length() does with a number input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right; I mainly added this to make sure no implementation misinterprets @ as being the whole JSON object (instead of just the member value 1).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that can be tested without the length() function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you provide an example for such a test please?

My intention was that if a library misinterprets @ as being the array, then length(@) == 1 passes and it erroneously selects the array as result. Arguably this is not so much about the length function.

Maybe using @==@ could work here though (and this could be in filter.json then)? The expected result is [1], but a library which erroneously considers @ as referring to the JSON array would have [[]] or [[], 1] or similar as result?

Copy link
Collaborator

@gregsdennis gregsdennis Feb 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

$[?@ == 1] against either an object or an array works just fine to test this. (And, yes, this should be in filter.json.)

Copy link
Contributor Author

@Marcono1234 Marcono1234 Feb 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

But that would not detect @ being misinterpreted as referring also to the array (instead of or in addition to just its items). It would in all cases only have the 1 as result.

Copy link
Collaborator

@gregsdennis gregsdennis Feb 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It would in all cases only have the 1 as result.

Yes, exactly, and in doing so, it correctly tests that the implementation is interpreting @ not as the container but as the value within it. (Which it seems is the result you're after.)

Once it's known that the implementation interprets @ as the value, and assuming it passes the rest of the test suite, it's safe to assume that it will interpret @.a correctly.

"selector": "$[?length(@)==1]",
"document": {"a": 1},
"result": []
},
{
"name": "result must be compared",
"selector" : "$[?length(@.a)]",
Expand Down
Loading