fix #1982:json_pointer.contains() exception is incorrectly raised

[dota17]

Refer to #1982 and #1942

Thanks @RobBeyer.
This PR is based on #1990, and add some checks for array when call json_pointer.contains().

[nlohmann]

Could you please rebase to the latest develop branch where I merged the changes from #1990 and also added some CI fixes?

[dota17]

Maybe it should add some testcases, and i will do it.

[dota17]

Ready to go.

[nlohmann]

This change will break existing code. I think the best option would be to add a bool option to the contains function to suppress parse errors. What do you think?

[dota17]

This change will break existing code. I think the best option would be to add a bool option to the contains function to suppress parse errors. What do you think?

@nlohmann , it is great.
However, if we add a bool option (default close the new logic) to keep the original logic, and people should open the option to use the new logic.
But from the discussion - #1942 and #1982 , maybe the old logic has something wrongs thus we fix it instead of keeping.

About adding a bool option, what do you think? @RobBeyer.

[dota17]

About adding a bool option, slow down and wait for the conclusion of the discussion.
@nlohmann @RobBeyer

[RobBeyer]

This change will break existing code. I think the best option would be to add a bool option to the contains function to suppress parse errors. What do you think?

@nlohmann , it is great.
However, if we add a bool option (default close the new logic) to keep the original logic, and people should open the option to use the new logic.
But from the discussion - #1942 and #1982 , maybe the old logic has something wrongs thus we fix it instead of keeping.

About adding a bool option, what do you think? @RobBeyer.

From what I understand, and from the use cases contains() is to address, this change should not break/affect existing code.

The change ensures 2 aspects: First that the object under test is not evaluated for the given pointer path. Second that the object under test doesn't raise exceptions instead of reporting false for contains() when the path is not contained.

The breaking change as far as understand would be returning false instead of raising an exception due to object evaluation .

In this case, the two expected actions existing code could perform would be to: Check for contains(), and if false, then either 1). attempt an assignment operation (which is when the path evaluation/assignment is to take place), or 2). not perform any further processing because the path is not contained in the object under test. For 1). the exception would then correctly be raised for the failed assignment, however this would happen at assignment instead of during the (incorrect) evaluation in contains(). And in 2). the code continues execution correctly as before without any exception being raised.

Parse errors (as per RFC) are still raised correctly - i.e. trying to use an invalid path (leading zero on a numerical index).

From my understanding of the use cases for contains(), the proposed fix will not adversely affect existing code, except for the location at which the exception is raised - i.e. at the point of path evaluation/assignment instead of at the path check for contains().

I don't believe a bool option should be added, as the original implementation operates incorrectly by evaluating the path in contains() instead of during a (potential) subsequent assignment operation.

[nlohmann]

Looks good to me.

[nlohmann]

Thanks!

[nlohmann]

---

🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

• ✨ New Feature
• 🐛 Bug Fix
• ⚡️ Improvement
• 🔨 Further Change
• 🔥 Deprecated function

Description

• Fix incorrectly raised exception in json_pointer::contains(). json_pointer.contains() exceptions when path not found #1942 json_pointer.contains() exception is incorrectly raised #1982 catch exceptions for json_pointer : ..../+99 #1990 fix #1982:json_pointer.contains() exception is incorrectly raised #2019

---