Validate schema directive applications - #4607
Conversation
|
Thanks for adding the Below is the changelog that will be used for the release. This release fixes validation of schema directive applications. Strawberry now validates every supported type-system location during schema Applying a non-repeatable directive more than once to the same schema element now This may expose invalid applications that Strawberry previously omitted from SDL or This release was contributed by @patrick91 in #4607 |
Apollo Federation Subgraph Compatibility Results
Learn more: |
Greptile SummaryThis PR validates schema-directive locations and repeatability during schema construction, adds source-aware errors, and lets the SDL printer emit the resulting validated applications directly.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking concern that unvalidated post-construction metadata or alternate BaseSchema implementations can now produce invalid SDL. Construction-time validation comprehensively covers the normal Schema path, but removing all printer checks leaves the public printing boundary dependent on directive metadata remaining unchanged and on callers having used the concrete Schema collector. Files Needing Attention: strawberry/printer/printer.py
|
| Filename | Overview |
|---|---|
| strawberry/schema/directive_collector.py | Adds location-aware traversal and per-element repeatability validation for directive applications throughout the converted GraphQL type graph. |
| strawberry/printer/printer.py | Removes printer-side location filtering in favor of construction-time validation, leaving a non-blocking validation-boundary concern for mutation and alternate BaseSchema implementations. |
| strawberry/exceptions/schema_directive.py | Adds actionable, source-aware exceptions for invalid locations and duplicate non-repeatable directives. |
| strawberry/federation/schema_directives.py | Marks composeDirective repeatable to preserve valid multiple schema applications. |
| tests/schema/test_schema_directives.py | Adds broad registration, SDL validity, location rejection, duplicate rejection, and separate-element reuse coverage. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Python schema declarations] --> B[GraphQL conversion]
B --> C[SchemaDirectiveCollector]
C --> D{Location and repeatability valid?}
D -- No --> E[Raise source-aware schema error]
D -- Yes --> F[Register directive definitions]
F --> G[Construct GraphQL schema]
G --> H[Print validated directive applications to SDL]
Reviews (1): Last reviewed commit: "Validate schema directive applications" | Re-trigger Greptile
| return "".join( | ||
| print_schema_directive(directive, schema=schema, extras=extras) | ||
| for directive in directives | ||
| for directive in field.directives |
There was a problem hiding this comment.
Preserve printer location validation
The printer now emits directives from mutable field metadata without validating their declared locations. Post-construction mutations and alternate BaseSchema implementations bypass SchemaDirectiveCollector, so printing such metadata produces SDL that GraphQL tooling rejects; retain a location assertion at this boundary or make the validated state immutable.
Knowledge Base Used: Schema conversion and printing
There was a problem hiding this comment.
I wonder if we can consider that use case supported 🤔
maybe we do a public api to do this (modify directives?)
There was a problem hiding this comment.
I’m open to supporting post-construction changes, but mutating field.directives directly cannot be considered supported by this PR: it bypasses SchemaDirectiveCollector, so the GraphQLSchema directive registry, introspection, repeatability/location checks, and Federation-derived metadata can become stale. A public API could make this safe, but it would need to validate the application and invalidate or rebuild the affected schema state—not just append to the list. Until that exists, I’d keep the printer defensive here, or make the directive collections immutable, so raw metadata mutation cannot produce invalid SDL.
There was a problem hiding this comment.
I'm not sure how much use case is there for these things, what are the mutation that one could do? could we have field.add_directive for example?
There was a problem hiding this comment.
I agree the post-construction mutation use case is probably small. The realistic cases are extensions or decorators that add directives while assembling a type, generated fields, or code that runs after @strawberry.type but before Schema construction. Those already work by mutating field.directives, and the collector validates the final state when the schema is built.
A field.add_directive(directive) method could make that supported usage clearer, but I would scope it to pre-schema-construction mutation. It cannot safely support mutation after Schema creation: the GraphQL field, directive registry/introspection, repeatability checks, and Federation metadata may already have been materialized. Supporting that would require a schema-level API that rebuilds or invalidates those structures, rather than simply appending to the Strawberry field.
I would not add field.add_directive as part of this PR unless we want the explicit API/documentation. If we do add it, the collector should remain the validation boundary, and the printer can rely on validated applications rather than reimplementing location filtering.
Summary
Compatibility
This intentionally surfaces schema directive applications that were previously omitted from SDL or emitted as SDL rejected by GraphQL tools. Generated input mutation fields now require directives attached to them to declare INPUT_FIELD_DEFINITION.
Test plan
Summary by Sourcery
Validate schema directive locations and repeatability during schema construction to prevent invalid applications from being omitted or emitted as invalid SDL.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores: