Fix crash when validating null values in objects with additionalProperties #370
Conversation
…rties Remove erroneous code that added a patch with empty JSON pointer for any null instance. Null is a valid JSON value and should not trigger a patch. Amends commit 59c9d62.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #370 +/- ##
=======================================
Coverage ? 93.49%
=======================================
Files ? 7
Lines ? 1060
Branches ? 0
=======================================
Hits ? 991
Misses ? 69
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| static const json schema = R"( | ||
| { | ||
| "properties": { | ||
| "x": {"type": "object", "additionalProperties": {}} |
There was a problem hiding this comment.
additionalProperties is defaulting to be an empty schema. So can be removed.
There was a problem hiding this comment.
I'm not sure why, but if I remove this, the test succeeds even without the fix.
There was a problem hiding this comment.
Empty schema is seen as maybe seen as false. So additionalProperties are not allowed.
Not sure whether this is expected.
There was a problem hiding this comment.
Definitely something wrong here, additionalProperties: {} should behave the same as if omitted.
There was a problem hiding this comment.
🤷♂️
Without the fix:
- Without
additionalProperties, the test passes - With
additionalProperties, I get this:
ctest -R issue-null --output-on-failure
Test project /home/orgads/Projects/json-schema-validator/build
Start 18: issue-null-values-in-object
1/1 Test #18: issue-null-values-in-object ......Subprocess aborted***Exception: 0.02 sec
terminate called after throwing an instance of 'nlohmann::json_abi_v3_12_0::detail::type_error'
what(): [json.exception.type_error.305] cannot use operator[] with a string argument with null
0% tests passed, 1 tests failed out of 1
There was a problem hiding this comment.
It's late, but I read a little bit further: The default-patch handling seems flawed:
if (instance.is_null()) {
patch.add(nlohmann::json::json_pointer{}, default_value_);
}I don't understand how the, seemingly, global instance patch, can use the root-pointer. Such a patch, when applied, will overwrite everything from the instance.
This is what you see in your test.
The incoherence of your test result regarding additionalProperties (present as {} or omitted) comes from the fact that nested is a null value, and when additionalProperties is {} it enters validation nested, this succeeds, but as it is a null-value a patch is added, with in my eyes a wrong json-path.
I think removing the if as you did, or at least change it:
if (instance.is_null() && default_value_) {
patch.add(ptr, default_value_);
}This would still create a patch for a possible default value of the instance is null. So this is wrong.
Actually a default value would need to be added only if the json-path does not exist in the validated instance.
As it is stands, default values patch creation seems flawed...
Not sure what to do, is the original author still available to check and explain? @Finkman
This change 59c9d62 was adding these lines.
| if (instance.end() == finding) { // if the prop is not in the instance | ||
| const auto &default_value = prop.second->default_value(ptr, instance, e); | ||
| if (!default_value.is_null()) { // if default value is available | ||
| patch.add((ptr / prop.first), default_value); |
There was a problem hiding this comment.
It is done here, isn't it? Not sure why it was added above in the first place. All the tests pass.
3 participants
Remove erroneous code that added a patch with empty JSON pointer for any null instance. Null is a valid JSON value and should not trigger a patch.