lint: add incoherent_exclusive_limits rule#818
Conversation
🤖 Augment PR SummarySummary: This PR introduces a new linter rule, Changes:
Technical Notes: The rule is non-mutating (lint-only) and reports both keywords as implicated when it fires. 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
2 issues found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
efb534c to
bffc321
Compare
jviotti
left a comment
There was a problem hiding this comment.
I think this one is a good one to have in the common folder for both the linter and canonicalizer (and testing on both), and having a transform that sets the current schema to false or just adds not: true or whatever to make the unsatisfiability obvious.
I believe we have some rules that do something like this for you to take inspiration from
ed368af to
d8625e0
Compare
|
Updated as suggested. The rule is now in |
| } | ||
|
|
||
| auto transform(JSON &schema, const Result &) const -> void override { | ||
| schema.into(JSON{false}); |
There was a problem hiding this comment.
Much better, but you will have to guard against $ref's. Consider a schema like this, which is weird, but definitely valid:
{
"$ref": "#/$defs/test/additionalProperties"
"$defs": {
"test": {
"additionalProperties": {
"type": "string"
},
"exclusiveMaximum": 5,
"exclusiveMinimum": 8
}
}
}By unconditionally converting $defs/test to false you break the reference. So try to think of potential adversarial tests for how you can break the rule. Also maybe if the schema has a $dynamicAnchor, etc.
If I recall correctly, in other cases, for this reason, what we do is NOT convert the whole thing to false but just add not: {}, or if not is already there, add it into an existing or new allOf
d8625e0 to
edb10ef
Compare
Signed-off-by: AcE <kintan0108@gmail.com>
edb10ef to
8de48ab
Compare
|
Thanks for the detailed feedback. I have updated the implementation to address all your points: Rule changes (
Test coverage:
All 39 unit tests are passing cleanly and the full alterschema test suite has been verified. |
Summary
Adds a new lint rule
incoherent_exclusive_limitsthat fires whenexclusiveMinimumis greater than or equal toexclusiveMaximum, making the schema unsatisfiable.Applies to Draft 6, Draft 7, 2019-09, and 2020-12.
Test cases (per dialect)
exclusiveMinimum<exclusiveMaximumexclusiveMinimumpresentexclusiveMaximumpresentexclusiveMinimum==exclusiveMaximumexclusiveMinimum>exclusiveMaximumChanges
src/alterschema/linter/incoherent_exclusive_limits.h- new rulesrc/alterschema/alterschema.cc- rule registered for all four dialectssrc/alterschema/CMakeLists.txt- header addedtest/alterschema/alterschema_lint_draft6_test.cc- tests addedtest/alterschema/alterschema_lint_draft7_test.cc- tests addedtest/alterschema/alterschema_lint_2019_09_test.cc- tests addedtest/alterschema/alterschema_lint_2020_12_test.cc- tests addedPart of sourcemeta/core#1975.