Add tests for incorrect filter usage #56
Conversation
Using filter on primitives should have no result, see RFC 9535 section 2.3.5.2
There was a problem hiding this comment.
Thank you @Marcono1234 for opening the issue and for CC'ing me. I have left a couple of comments for suggestion, but feel that approval should come from one of the other contributors that has worked on this test suite more extensively than I.
One general comment that I wanted to make was this: I had thought of adding a test case back when hiltontj/serde_json_path#49 was resolved, but felt the problem in that case was more due to a logical error in serde_json_path's code, as opposed to some violation of the standard.
So, that at least was the excuse I made for not taking the time to do what you have done here 🙂. Having these test cases will help prevent other future implementors from making the same logical errors I did, so I would give a 👍.
|
Thanks for the feedback! I hope I addressed everything as you had it in mind. As part of that I removed the "@ refers to ..." from the names, hopefully it is still clear why the queries should have no result, respectively don't work as one might expect. To the maintainers: Please feel free to squash the commits on merge, or let me know if I should squash them for a cleaner Git history. |
There was a problem hiding this comment.
There's some duplication of existing tests in here. I'd like to be sure we have good reason to add them and that the test cases are minimal.
Also, I'd like @glyn's opinion.
FWIW, these all pass in my implementation.
| { | ||
| "name": "on primitive, selects nothing", | ||
| "selector": "$.a[?@ == 1]", | ||
| "document": {"a": 1}, | ||
| "result": [] | ||
| }, |
There was a problem hiding this comment.
This could be improved since the .a isn't necessary.
| { | |
| "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.
| { | |
| "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.)
There was a problem hiding this comment.
This could be improved since the
.aisn'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?
There was a problem hiding this comment.
(I thought we had those tests for primitives but we only have tests for e.g. "index selector against object".)
| { | ||
| "name": "filter on primitive array element, selects nothing", | ||
| "selector": "$[?length(@)==1]", | ||
| "document": [1], | ||
| "result": [] | ||
| }, |
There was a problem hiding this comment.
We're already effectively testing this with "number arg" on line 32
There was a problem hiding this comment.
Same as with #56 (comment); the intention was to make sure no library misinterprets @ as representing the JSON array (instead of its elements).
| "result": [] | ||
| }, | ||
| { | ||
| "name": "filter on primitive object member value, selects nothing", |
There was a problem hiding this comment.
This is the effectively the same test as before: checking to see what length() does with a number input.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I think that can be tested without the length() function.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
$[?@ == 1] against either an object or an array works just fine to test this. (And, yes, this should be in filter.json.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| "name": "on primitive object member value, selects nothing", | ||
| "selector": "$[?@.a == 1]", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
where the reporter assumed @ refers to the JSON object instead of the member value.
Two things here:
- 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.
- 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.
There was a problem hiding this comment.
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -4096,6 +4153,22 @@ | |||
| ], | |||
| "result": [] | |||
| }, | |||
| { | |||
| "name": "functions, length, filter on primitive array element, selects nothing", | |||
| "selector": "$[?length(@)==1]", | |||
There was a problem hiding this comment.
This test could be made broader, thus:
| "selector": "$[?length(@)==1]", | |
| "selector": "$[?length(@)>=0]", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We have Nothing equality.
There was a problem hiding this comment.
@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.
| }, | ||
| { | ||
| "name": "functions, length, filter on primitive object member value, selects nothing", | ||
| "selector": "$[?length(@)==1]", |
There was a problem hiding this comment.
| "selector": "$[?length(@)==1]", | |
| "selector": "$[?length(@)>=0]", |
|
Thanks for all your reviews! Sorry that I did not approach this in a very structured way, and also mixed multiple test cases in this pull request. @gregsdennis, if you prefer feel free to add tests in a more organized and simplified way, then I will close this pull request. Otherwise, should I only address @glyn's comments, or try to organize and simplify the tests more as you suggested @gregsdennis? |
|
I think @glyn and I (at a minimum) should agree on the approach for this. I'm still of the mind that these tests are too complex and the desired behavior can be covered with a few more targeted test cases. |
@Marcono1234 May I suggest that you close this PR (or convert it into a draft PR) and then submit some small PRs, each targetting one case? I hope that won't be much more work and it would mean that we can quickly merge the non-contentious PRs. (If you have a better suggestion, I'm all ears!) |
This mainly covers using filter on primitives, which should have no result, see RFC 9535 section 2.3.5.2:
(This was inspired by hiltontj/serde_json_path#49; CC @hiltontj)
Please let me know if any of the tests are incorrect, or if I should adjust them or split this into separate pull requests. Any feedback is appreciated!