Define PartialType metaclass #3994
Conversation
Reviewer's GuideThis PR introduces a new PartialType metaclass that automatically makes all inherited Strawberry type fields optional, integrates it into the public API, adds corresponding documentation and release notes, and provides comprehensive tests to validate its behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In PartialType you unconditionally set all defaults to strawberry.UNSET, which will override any explicit defaults from base classes—consider preserving user-specified defaults when copying fields.
- The metaclass currently assumes every base has a _type_definition.fields attribute; add a guard or skip logic for bases that aren’t Strawberry types to avoid errors when mixing in non-GraphQL classes.
- You wrap every annotation in Optional without checking if it’s already optional, which can lead to nested Optional types—add a check to avoid double-wrapping annotations that are already Optional/StrawberryOptional.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In PartialType you unconditionally set all defaults to strawberry.UNSET, which will override any explicit defaults from base classes—consider preserving user-specified defaults when copying fields.
- The metaclass currently assumes every base has a _type_definition.fields attribute; add a guard or skip logic for bases that aren’t Strawberry types to avoid errors when mixing in non-GraphQL classes.
- You wrap every annotation in Optional without checking if it’s already optional, which can lead to nested Optional types—add a check to avoid double-wrapping annotations that are already Optional/StrawberryOptional.
## Individual Comments
### Comment 1
<location> `strawberry/tools/partialtype.py:12-18` </location>
<code_context>
+ mro = super().__new__(cls, name, bases, namespaces, **kwargs).mro()
+ annotations = namespaces.get("__annotations__", {})
+ fields: list[str] = []
+ for base in mro[:-1]: # the object class has no __annotations__ attr
+ for k, v in base.__annotations__.items():
+ # To prevent overriding the higher attr annotation
+ if k not in annotations:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Accessing __annotations__ directly may raise AttributeError for some base classes.
Use getattr(base, "__annotations__", {}) to safely access annotations and prevent exceptions.
```suggestion
for base in mro[:-1]: # the object class has no __annotations__ attr
for k, v in getattr(base, "__annotations__", {}).items():
# To prevent overriding the higher attr annotation
if k not in annotations:
annotations[k] = v
fields.extend(field.name for field in base._type_definition.fields)
```
</issue_to_address>
### Comment 2
<location> `strawberry/tools/partialtype.py:18` </location>
<code_context>
+ if k not in annotations:
+ annotations[k] = v
+
+ fields.extend(field.name for field in base._type_definition.fields)
+
+ for field in annotations:
</code_context>
<issue_to_address>
**suggestion:** Assumes all bases have _type_definition.fields, which may not always be true.
Use hasattr or a try/except block to avoid AttributeError when a base lacks _type_definition.
```suggestion
if hasattr(base, "_type_definition") and hasattr(base._type_definition, "fields"):
fields.extend(field.name for field in base._type_definition.fields)
```
</issue_to_address>
### Comment 3
<location> `strawberry/tools/partialtype.py:20-22` </location>
<code_context>
+
+ fields.extend(field.name for field in base._type_definition.fields)
+
+ for field in annotations:
+ if not field.startswith("_"):
+ annotations[field] = Optional[annotations[field]]
+
+ namespaces["__annotations__"] = annotations
</code_context>
<issue_to_address>
**suggestion:** Wrapping all non-private fields in Optional may not preserve existing Optional types.
Check if a field's annotation is already Optional before applying another Optional wrapper to avoid redundancy.
```suggestion
from typing import get_origin
for field, annotation in annotations.items():
if not field.startswith("_"):
# Check if annotation is already Optional
origin = get_origin(annotation)
if origin is not Optional:
annotations[field] = Optional[annotation]
```
</issue_to_address>
### Comment 4
<location> `tests/tools/test_partialtype.py:8-17` </location>
<code_context>
+def test_partialtype():
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for edge cases such as inherited fields with default values, fields with complex types, and fields with leading underscores.
Adding these cases will help verify that the metaclass correctly handles all field scenarios, including skipping fields with leading underscores.
</issue_to_address>
### Comment 5
<location> `docs/guides/tools.md:99` </location>
<code_context>
+### `PartialType`
+
+`PartialType` metaclass is used to extend your type but makes its all field optional. Consider
+you have different types for each operation on the same model such as `UserCreate`, `UserUpdate`
+and `UserQuery`. `UserQuery` should have id field but the other does not. All fields of
</code_context>
<issue_to_address>
**issue (typo):** Correct grammar: 'makes its all field optional' should be 'makes all its fields optional.'
The correct phrasing is 'makes all its fields optional.'
```suggestion
`PartialType` metaclass is used to extend your type but makes all its fields optional. Consider
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| for base in mro[:-1]: # the object class has no __annotations__ attr | ||
| for k, v in base.__annotations__.items(): | ||
| # To prevent overriding the higher attr annotation | ||
| if k not in annotations: | ||
| annotations[k] = v | ||
|
|
||
| fields.extend(field.name for field in base._type_definition.fields) |
There was a problem hiding this comment.
suggestion (bug_risk): Accessing annotations directly may raise AttributeError for some base classes.
Use getattr(base, "annotations", {}) to safely access annotations and prevent exceptions.
| for base in mro[:-1]: # the object class has no __annotations__ attr | |
| for k, v in base.__annotations__.items(): | |
| # To prevent overriding the higher attr annotation | |
| if k not in annotations: | |
| annotations[k] = v | |
| fields.extend(field.name for field in base._type_definition.fields) | |
| for base in mro[:-1]: # the object class has no __annotations__ attr | |
| for k, v in getattr(base, "__annotations__", {}).items(): | |
| # To prevent overriding the higher attr annotation | |
| if k not in annotations: | |
| annotations[k] = v | |
| fields.extend(field.name for field in base._type_definition.fields) |
| if k not in annotations: | ||
| annotations[k] = v | ||
|
|
||
| fields.extend(field.name for field in base._type_definition.fields) |
There was a problem hiding this comment.
suggestion: Assumes all bases have _type_definition.fields, which may not always be true.
Use hasattr or a try/except block to avoid AttributeError when a base lacks _type_definition.
| fields.extend(field.name for field in base._type_definition.fields) | |
| if hasattr(base, "_type_definition") and hasattr(base._type_definition, "fields"): | |
| fields.extend(field.name for field in base._type_definition.fields) |
| for field in annotations: | ||
| if not field.startswith("_"): | ||
| annotations[field] = Optional[annotations[field]] |
There was a problem hiding this comment.
suggestion: Wrapping all non-private fields in Optional may not preserve existing Optional types.
Check if a field's annotation is already Optional before applying another Optional wrapper to avoid redundancy.
| for field in annotations: | |
| if not field.startswith("_"): | |
| annotations[field] = Optional[annotations[field]] | |
| from typing import get_origin | |
| for field, annotation in annotations.items(): | |
| if not field.startswith("_"): | |
| # Check if annotation is already Optional | |
| origin = get_origin(annotation) | |
| if origin is not Optional: | |
| annotations[field] = Optional[annotation] |
docs/guides/tools.md
Outdated
|
|
||
| ### `PartialType` | ||
|
|
||
| `PartialType` metaclass is used to extend your type but makes its all field optional. Consider |
There was a problem hiding this comment.
issue (typo): Correct grammar: 'makes its all field optional' should be 'makes all its fields optional.'
The correct phrasing is 'makes all its fields optional.'
| `PartialType` metaclass is used to extend your type but makes its all field optional. Consider | |
| `PartialType` metaclass is used to extend your type but makes all its fields optional. Consider |
|
It was closed when I deleted the fork. Here is the original PR |
|
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: |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR introduces a new PartialType metaclass to the strawberry.tools module that addresses a common code duplication problem in GraphQL schema design. The metaclass automatically converts all fields inherited from parent classes into optional fields with default values of strawberry.UNSET. This allows developers to create partial types (like UserUpdate, UserQuery) that inherit from a base type (UserCreate) without manually redefining every field as optional.
The implementation works by:
- Traversing the Method Resolution Order (MRO) to collect field annotations from base classes
- Wrapping all collected annotations with
Optional[]types - Setting default values to
strawberry.UNSETfor fields that don't already have values - Integrating with Strawberry's existing type definition system
The feature is exported from strawberry.tools, includes comprehensive tests validating field properties and inheritance behavior, and provides documentation with usage examples. This fits into Strawberry's ecosystem as a developer productivity tool that reduces boilerplate code when working with related GraphQL types that have different optionality requirements.
PR Description Notes:
- Missing import statement in the second code example (
from typing import Optional) - Inconsistent spacing in code blocks
Confidence score: 2/5
- This PR has significant implementation issues that could cause runtime errors and unexpected behavior in production
- Score reflects critical flaws in the metaclass implementation including unsafe MRO traversal, potential attribute access errors, and lack of proper error handling
- Pay close attention to
strawberry/tools/partialtype.py- the core implementation needs thorough review and likely refactoring before merge
Context used:
Context - When documenting features, ensure that explanations are clear and provide context on why certain functionalities are useful, especially when dealing with integrations like Pydantic. (link)
Context - In tests involving Pydantic models, ensure that the tests accurately reflect the intended behavior by using the correct instances and values. Avoid copy-pasting code that may lead to confusion. (link)
5 files reviewed, 9 comments
| assert query_role.graphql_name is None | ||
| assert query_role.default is strawberry.UNSET | ||
| assert isinstance(query_role.type, StrawberryOptional) | ||
| assert query_role.type.of_type is RoleInput |
There was a problem hiding this comment.
style: Missing newline at end of file
|
|
||
| ```py |
There was a problem hiding this comment.
syntax: Missing required imports. The example uses Optional and strawberry.ID but doesn't import them.
| ```py | |
| from typing import Optional | |
| from strawberry.tools import PartialType | |
| @strawberry.type |
| @strawberry.type | ||
| class UserUpdate(UserCreate, metaclass=PartialType): | ||
| pass | ||
|
|
There was a problem hiding this comment.
syntax: Missing default value assignment. Based on the implementation, fields should have = strawberry.UNSET when not explicitly provided.
| id: Optional[strawberry.ID] = strawberry.UNSET |
| } | ||
| ``` | ||
|
|
||
| ### `PartialType` |
There was a problem hiding this comment.
style: Missing section separator. Should have --- before the PartialType section to match the formatting of other sections.
| id: Optional[strawberry.ID] | ||
| ``` |
There was a problem hiding this comment.
style: Incomplete code block structure. The </CodeGrid> tag appears without proper context or corresponding GraphQL output example like the other sections.
| @@ -0,0 +1,18 @@ | |||
| Release type: minor | |||
|
|
|||
| Add `PartialType` metaclass to make fields optional dynamically. | |||
There was a problem hiding this comment.
style: The description could be more specific about what 'dynamically' means here - that it automatically wraps inherited fields with Optional[] and sets defaults to strawberry.UNSET
Context Used: Context - When documenting features, ensure that explanations are clear and provide context on why certain functionalities are useful, especially when dealing with integrations like Pydantic. (link)
| if k not in annotations: | ||
| annotations[k] = v | ||
|
|
||
| fields.extend(field.name for field in base._type_definition.fields) |
There was a problem hiding this comment.
logic: Accessing _type_definition may fail if base classes haven't been processed by strawberry decorators yet during MRO traversal
|
|
||
| for field in annotations: | ||
| if not field.startswith("_"): | ||
| annotations[field] = Optional[annotations[field]] |
There was a problem hiding this comment.
logic: Already Optional fields will be double-wrapped as Optional[Optional[T]], potentially causing type resolution issues
|
|
||
| class PartialType(ABCMeta): | ||
| def __new__(cls, name: str, bases: tuple, namespaces: dict, **kwargs): | ||
| mro = super().__new__(cls, name, bases, namespaces, **kwargs).mro() |
There was a problem hiding this comment.
logic: Creates class instance to call mro() but doesn't use the proper instance for subsequent operations
To prevent defining the same field for different types on the same model I defined a metaclass called PartialType that makes fields optional.
Currently, we define the same fields in multiple types for the same model since the field might be optional for one type but not for the other.
Imagine that
UserCreatehas a bunch of fields. One has to define all of them in theUserUpdatetype. WithPartialTypemetaclass reusing is possible.Description
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Introduce PartialType to streamline defining partial GraphQL types by inheriting existing types and making all fields optional with default UNSET
New Features:
Documentation:
Tests:
Chores: