json_pointer.contains() exception is incorrectly raised

[RobBeyer]

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_pointer exists in a json object.
• contains() must not validate if a _json_pointer may not exist in a json object.
• contains() must never raise an exception if a _json_pointer can 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.

[t-b]

@RobBeyer Is your example json document correct? I'm not seeing an array there.

[RobBeyer]

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]

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.

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/x or others. They should return false if there is an error condition, right?

[RobBeyer]

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 /b is an array, then /b/0 is contained in the object, and /b/x is not. The bug is that /b/x should not raise an exception just because /b is an array.

I do not want that invalid _json_pointer paths 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]

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.

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 the contains() should catch the exceptions and return false. contains() always return true or false.
Right?

If yes, i think it is a good idea.

[RobBeyer]

No. That's not it at all.

I do NOT want a try/catch around the contains() function. Illegal (by syntax) json path specifications must still raise exceptions, as expected.

The exception is being raised ONLY because the json object being tested for the contains() path can not evaluate in the object, for the path being tested.

Even more explicit example: (showing json followed by contains())

{ "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 json object being tested MUST NOT DETERMINE the operation of contains() - 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.

[RobBeyer]

@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]

@RobBeyer
Oh, i see.
But we still get an exception for contains("/a/11x") or contains("/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() and std::stoi and handle the exceptions.

[dota17]

Fine, I will update and add complete check to deal with reference_token lately.

[RobBeyer]

@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 contains is actually a bit more complicated and involved - contains("/a/11x") is rather evil, while entirely legal.

In those cases std::stoi would 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]

@RobBeyer

the issue of contains is actually a bit more complicated and involved

I had update my code and all testcases passed. Just consider the ABNF in RFC 6901 Sect 4.

[stale]

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.