tools/constraint_checker: validate @cardinality is only on list fields#195
Open
SoundMatt wants to merge 1 commit into
Open
tools/constraint_checker: validate @cardinality is only on list fields#195SoundMatt wants to merge 1 commit into
SoundMatt wants to merge 1 commit into
Conversation
@Cardinality(min, max) describes the size of a list. On a non-list field the cardinality is already fixed by the GraphQL type itself (T is 0..1, T! is 1..1), so any @Cardinality annotation is either redundant or contradicts the GraphQL non-null markers. Until now this misuse went unflagged. Implement has_valid_cardinality() in field.py (closing the long- standing TODO) so it returns True iff the field is list-typed or has no @Cardinality directive. Wire it into ConstraintChecker via a new check_cardinality_on_list_only() method and call it from run(). Two existing test fixtures used 'bar: Int @Cardinality(...)' which is invalid under the new rule; both are switched to 'bar: [Int]' so they continue to test the min<=max constraint they were always meant to test. Five new tests cover the validator end-to-end: - @Cardinality on a nullable scalar is flagged - @Cardinality on a non-null scalar is flagged - @Cardinality on lists of all four nullability flavours passes - a scalar with no @Cardinality directive at all passes Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
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
1 participant
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.
Closes the long-standing TODO at
src/s2dm/exporters/utils/field.py:174.@cardinality(min, max)describes the size of a list. On a non-list field the cardinality is already fixed by the GraphQL type itself (Tis 0..1,T!is 1..1), so any@cardinalityannotation is either redundant or contradicts the GraphQL non-null markers. Until now this misuse went unflagged — a user could writefield: Int! @cardinality(min: 0, max: 5)and the constraint checker would happily report no errors.Changes
src/s2dm/exporters/utils/field.py— replace thehas_valid_cardinality()placeholder (always returnedFalse) with a real implementation: returnsTrueiff the field is list-typed or has no@cardinalitydirective.src/s2dm/tools/constraint_checker.py— addcheck_cardinality_on_list_only()and wire it intoConstraintChecker.run()alongside the existingmin <= maxcheck.tests/test_check_constraint.py— five new tests + two existing fixtures updated.Test fixture updates
Two existing tests (
test_cardinality_min_leq_max,test_cardinality_min_gt_max) usedbar: Int @cardinality(...)which is invalid under the new rule. Both are switched tobar: [Int]so they continue to test what they were always meant to test (themin <= maxconstraint), without leaning on a misuse pattern as test scaffolding.New tests
test_cardinality_on_nullable_scalar_is_flagged—bar: Int @cardinality(...)→ errortest_cardinality_on_non_null_scalar_is_flagged—bar: Int! @cardinality(...)→ errortest_cardinality_on_list_passes— all four list nullability flavours ([Int],[Int]!,[Int!],[Int!]!) → no errortest_no_cardinality_directive_passes—bar: Int(no directive at all) → no errorOut of scope
Other potential cardinality validations — negative
min/max, soft-warn onmin > 0for nullable lists, etc. — are intentionally left for follow-up PRs to keep this one focused on the specific contradiction the existing TODO calls out.Local verification: 536 tests pass.