Do not enforce uniqueItems when it is set to false - #577
Open
gaoflow wants to merge 1 commit into
Open
Conversation
Validator#validate dispatches an attribute whenever its keyword is present in the schema, so UniqueItemsAttribute ran for `uniqueItems: false` just as it did for `true` -- it never read the keyword's value. Per draft3 5.15, draft4 5.3.4 and draft6 6.13, false is the default and imposes no constraint, so a schema that explicitly opts out (or that overrides an inherited `uniqueItems: true`) was rejecting valid data. Read the value in the attribute and return early when it is falsy. This is the smallest possible behaviour change: the metaschema already constrains uniqueItems to a boolean, so only `false` is affected. Adds the false and omitted cases to the shared UniqueItemsTests module, which draft2, draft3 and draft4 already include, and includes that module in the draft6 tests, which had no uniqueItems coverage at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three-way control on
[1, 1]against master:uniqueItems: trueis correctly invalid, the keyword absent is correctly valid, anduniqueItems: falseis wrongly invalid — same in draft2, draft3, draft4 and draft6. The cause is thatValidator#validatedispatches an attribute whenever its keyword is present in the schema, andUniqueItemsAttribute.validatenever readscurrent_schema.schema['uniqueItems'], so presence alone enforces the constraint; draft-04 §5.3.4.2 ("If this keyword has boolean value false, the instance validates successfully") and §5.3.4.3, which makesfalsethe default, say it must be a no-op, as do draft-03 §5.15 and draft-06 §6.13. That matters in practice because an explicitfalseis the normal way to override an inheriteduniqueItems: true, which is exactly the report in #461. The fix reads the value and returns early when it is falsy — the metaschema already constrainsuniqueItemsto a boolean, sofalseis the only value whose behaviour changes.This survived because the official test suite does cover it — 36 cases across draft3/4/6, every one named
uniqueItems=false …— buttest/common_test_suite_test.rbbuilds its test methods fromDir[...]at class-definition time, andtest/test-suiteis empty in a plain checkout, so the file contributes nothing:.github/workflows/test.ymlchecks out withoutsubmodules:, and.gitmodulesstill points atgit://github.com/…, which GitHub switched off in 2022 (git ls-remoteon it hangs). I've left all of that alone since #387 is already open on it and it's your call — and no unit-test fixture had ever setuniqueItemsto anything buttrue, so nothing else caught it either.I ran
bundle exec rake test(383 → 396 tests, 2359 → 2551 assertions, 0 failures) andbundle exec rubocopclean; reverting only thelib/change while keeping the new tests fails 8 of them, and disabling the keyword outright fails the pre-existingtest_unique_itemsin all four drafts. Cloning the pinned submodule commit637f0aclocally and running draft3/4/6 through it takes failures from 161 to 125 with errors unchanged at 92 — exactly the 36uniqueItemscases, and no other suite file's count moves. I also audited every other attribute class for the same "ignores its own keyword value" defect: all 38 keyword/class pairs registered across drafts 1–6, plus a true/false/absent differential over every boolean-valued keyword (additionalProperties,additionalItems,exclusiveMaximum/exclusiveMinimum,maximumCanEqual/minimumCanEqual, draft3required, draft1/2optional, draft6propertyNames) — all of them read their value correctly, souniqueItemsis the only offender rather than a pattern needing a wider change.The tests go into the shared
ArrayValidation::UniqueItemsTests, which draft2/3/4 already include; I also included that module inDraft6Test, which had nouniqueItemscoverage at all.Closes #461.