Repository navigation
json_pointer.contains() exception is incorrectly raised #1982
Description
When using contains() with a _json_pointer, if the json object being tested has an array in the path, the _json_pointer is incorrectly being restricted to reference an array by the contents of the object, instead of verifying that the _json_pointer is contained in the provided json object.
The operation of contains() demonstrates the following bug:
contains()must validate if a_json_pointerexists in a json object.contains()must not validate if a_json_pointermay not exist in a json object.contains()must never raise an exception if a_json_pointercan not be supported by a json object.
To reproduce:
json test = { { "a", "Ok" }, { "b", { "bad", "exception", "case" } } };
cout << "/a: " << test.contains("/a"_json_pointer) << endl;
cout << "/b/0: " << test.contains("/b/0"_json_pointer) << endl;
cout << "/b/x: " << test.contains("/b/x"_json_pointer) << endl;
Current Behavior:
/a: 1
/b/0: 1
/b/x: **(exception)**
Expected Behavior:
/a: 1
/b/0: 1
/b/x: 0
Compiler: MSVC 2017, GCC
This bug is present in v3.7.3 / master.
Originally submitted as a question, but there has been no discussion for a month: #1942
I believe that RFC 6901 confirms this to be a bug - in that for the contains() operation, the _json_pointer is NOT being EVALUATED at each depth in the path, but the path existence is being tested at each depth. The operation performed is to verify if the pointer is CONTAINED in the object under test. Therefore, contains() pointer path processing should simply stop in this case, and no exception is to be raised.
Background: RFC 6901 on _json_pointer
Section 4:
Implementations will evaluate each reference token against the document's contents and will raise an error condition if it fails to resolve a concrete value for any of the JSON pointer's reference tokens.
For example, if an array is referenced with a non-numeric token, an error condition will be raised. See Section 7 for details.
Section 7:
In the event of an error condition, evaluation of the JSON Pointer fails to complete.
Error conditions include, but are not limited to:
o Invalid pointer syntax
o A pointer that references a nonexistent value
This specification does not define how errors are handled. An application of JSON Pointer SHOULD specify the impact and handling of each type of error.
For example, some applications might stop pointer processing upon an error, while others may attempt to recover from missing values by inserting default ones.
Activity
t-b commentedon Mar 12, 2020
@RobBeyer Is your example json document correct? I'm not seeing an array there.
RobBeyer commentedon Mar 12, 2020
Yes. The
_json_pointer"/b" references an array, as per the definition in the specification.JSON as first-class data type
Reproduce Code Sample:
Screen Capture: Example C++ Code in MSVC 2017 Debugger
dota17 commentedon Mar 16, 2020
The specification does not define how errors are handled.
I think what you want is that the behavior should be same whatever the path is
/b/xor others. They should return false if there is an error condition, right?RobBeyer commentedon Mar 16, 2020
Well, if you want to try and explain the issue "backwards", then yes?
At the moment, the
contains()operation is dependent on the contents of the object under test, instead of verifying that the path MAY be contained in the object.When
/bis an array, then/b/0is contained in the object, and/b/xis not. The bug is that/b/xshould not raise an exception just because/bis an array.I do not want that invalid
_json_pointerpaths return false - any invalid path specifications should raise exceptions as/if required.I want the
contains()function to NOT evaluate the path at each depth, as the function should ONLY be checking for existence (contains) not evaluating and raising exceptions if the path can not be achieved with the provided object.dota17 commentedon Mar 17, 2020
As you said, the
contains()should return true if the path can be achieved, otherwise return false. Also, any invalid path specifications should raise exceptions as/if required. But thecontains()should catch the exceptions and return false.contains()always returntrueorfalse.Right?
If yes, i think it is a good idea.
RobBeyer commentedon Mar 18, 2020
No. That's not it at all.
I do NOT want a try/catch around the
contains()function. Illegal (by syntax)jsonpath specifications must still raise exceptions, as expected.The exception is being raised ONLY because the
jsonobject being tested for thecontains()path can not evaluate in the object, for the path being tested.Even more explicit example: (showing
jsonfollowed bycontains()){ "a" : "hello" }contains("/a") -> true(a exists)contains("/a/0") -> false(a is not an array)contains("/a/x") -> false(a is not an object)All of these are operating as expected. They return false BECAUSE the path is not CONTAINED in the object.
{ "a" : ["hello", "there"] }contains("/a") -> true(path exists)contains("/a/0") -> true(a is an array, and the zero'th item exists)contains("/a/x") -> false(a is an array, and only BECAUSE a is an array,contains()raises an exception)My point is that the
jsonobject being tested MUST NOT DETERMINE the operation ofcontains()- i.e. it should not evaluate the path against the object at any point - only test if the path is CONTAINED in the object.Basically,
contains()must only determine IF (and only if) the given path IS CONTAINED in the json object.That the path can not be (legally) evaluated by the input json object, is not what
contains()is meant to do.fix nlohmann#1982: contains() exceptions are incorrectly raised
RobBeyer commentedon Mar 27, 2020
@dota17 - I believe that adding tests similar to the patch I proposed in #1942 would be much more appropriate than catching exception cases after attempting a path evaluation. Your proposed commit is (again) only testing the object against the path, instead of testing if the path is contained. If I wanted this type of an "exception handled" case, I could just eval the path against the object, and catch the exception myself and therefore assume that the path is not contained. Exceptions should be exceptional, not used for something expected.
The #1942 proposed patch does not perform any further path checking, if the next element in the path (already having been identified as an array) can not possibly be contained in the object under test.
A fix using this methodology would be not only more efficient than trying to evaluate, and then catch exceptions, but it can ensure that the path is "not contained" in the object.
For example, in the current implementation of the contains() function - after identifying the current path element as an array, the array size() is checked to ensure the array index in the path is within the array bounds - and then returns false ... without exceptions.
dota17 commentedon Mar 30, 2020
@RobBeyer
Oh, i see.
But we still get an exception for
contains("/a/11x")orcontains("/001")just to chang that line to:if (JSON_HEDLEY_UNLIKELY((reference_token < "0") || (reference_token > "9"))).
And we should check each char of the reference_token, because "999" > "9".
If we add more tests to check, it will be a lot of added code. Thus, i push the work to
arrayindex()andstd::stoiand handle the exceptions.dota17 commentedon Mar 30, 2020
Fine, I will update and add complete check to deal with
reference_tokenlately.RobBeyer commentedon Mar 30, 2020
@dota17
Yeah ... thank you! That's exactly the concern, and you're absolutely right - my "hack" suggestion/change isn't correct, which is why I said "something like" and wanted to discuss this. I realize now that the issue of
containsis actually a bit more complicated and involved -contains("/a/11x")is rather evil, while entirely legal.In those cases
std::stoiwould need to verify a numeric index is provided as path element before testing, because the JSON spec also states that a/0...path for an array is invalid, but is not invalid when the path is a string. Complicated edge cases. :(dota17 commentedon Mar 31, 2020
@RobBeyer
I had update my code and all testcases passed. Just consider the ABNF in RFC 6901 Sect 4.
stale commentedon Apr 30, 2020
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Merge pull request #2019 from dota17/contains_v2