feat: automatically include implementers of interfaces in schema #3686
Conversation
Reviewer's Guide by SourceryThis PR implements automatic inclusion of interface implementers in the GraphQL schema. The implementation modifies the schema conversion process to detect and include classes that inherit from interface types, even if they're not explicitly referenced in the schema. This is demonstrated through new test cases using a cheese-themed example. Sequence diagram for schema creation with interface implementerssequenceDiagram
participant Developer
participant SchemaConverter
participant GraphQLSchema
Developer->>SchemaConverter: Create schema
SchemaConverter->>SchemaConverter: Detect interface implementers
SchemaConverter->>GraphQLSchema: Add implementers to schema
GraphQLSchema-->>Developer: Schema with implementers
note right of SchemaConverter: Automatically includes implementers of interfaces
Class diagram for the updated schema conversion processclassDiagram
class SchemaConverter {
+Dict extra_interface_child_map
+Dict type_map
+Config config
+Dict scalar_registry
+Callable get_fields
}
class StrawberryObjectDefinition
class GraphQLSchema
class GraphQLCoreConverter
SchemaConverter --> StrawberryObjectDefinition : extra_interface_child_map
SchemaConverter --> GraphQLSchema
SchemaConverter --> GraphQLCoreConverter
note for SchemaConverter "New attribute extra_interface_child_map added to track interface implementers."
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature.Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) Here's the tweet text: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @erikwrede - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please fix the TODO regarding duplicate processing - while it doesn't cause errors, it could impact performance unnecessarily.
- Consider adding documentation explaining why this specific implementation approach was chosen, given the comment about it being 'somewhat hacky'.
- The PR template is incomplete - please check the appropriate boxes for the type of change and other checklist items.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # We need to find a way to add the extra implementations to the schema after creating it. | ||
| # This is not officially supported by GraphQL core and would be somewhat hacky. | ||
|
|
||
| # TODO: prevent duplicates - no error, but duplicate processing is inefficient |
There was a problem hiding this comment.
suggestion (performance): Consider using a set or bulk extend operation to prevent duplicate processing
Instead of appending types one by one, consider collecting them in a set first or using list.extend() to add them all at once. This would prevent duplicate processing and improve performance.
types = set() # Using a set to automatically prevent duplicates
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3686 +/- ##
==========================================
- Coverage 97.02% 94.83% -2.20%
==========================================
Files 503 499 -4
Lines 33520 32417 -1103
Branches 5632 1673 -3959
==========================================
- Hits 32524 30743 -1781
- Misses 790 1404 +614
- Partials 206 270 +64 |
CodSpeed Performance ReportMerging #3686 will not alter performanceComparing Summary
|
This PR implements automatically adding interface subclasses to the schema.
However, it only acts as a proof of concept as we currently lazily evaluate fields during instantiation of the GraphQL Core Schema (since #1684).
This making it impossible to collect all extra types before instantiating the GraphQL core schema.