-
-
Notifications
You must be signed in to change notification settings - Fork 593
Patch default injection for annotated fields #3985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Patch default injection for annotated fields #3985
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis patch refines the default-injection mechanism for fields annotated with File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `strawberry/types/object_type.py:137` </location>
<code_context>
+ setattr(cls, name, None)
+ elif (
+ isinstance(attr := getattr(cls, name), StrawberryField)
+ and isinstance(attr.default, dataclasses._MISSING_TYPE)
+ and isinstance(attr.default_factory, dataclasses._MISSING_TYPE)
+ ):
</code_context>
<issue_to_address>
Using dataclasses._MISSING_TYPE is relying on a private API.
Use dataclasses.MISSING instead, as it is the public API and more stable across Python versions.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
setattr(cls, name, None) | ||
elif ( | ||
isinstance(attr := getattr(cls, name), StrawberryField) | ||
and isinstance(attr.default, dataclasses._MISSING_TYPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Using dataclasses._MISSING_TYPE is relying on a private API.
Use dataclasses.MISSING instead, as it is the public API and more stable across Python versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR fixes a bug in Strawberry GraphQL's handling of strawberry.Maybe
input fields when they are explicitly defined using strawberry.field()
. The core issue was that Maybe fields with field annotations or descriptions would incorrectly be treated as required fields instead of optional ones with default None
values.
The fix involves two complementary changes:
-
Enhanced Maybe detection (
maybe.py
): The_annotation_is_maybe()
function now recursively handlestyping.Annotated
wrappers, which are created when usingstrawberry.field()
with descriptions or other metadata. Previously, the function would fail to recognize Maybe types when they were wrapped inAnnotated
. -
Improved default injection (
object_type.py
): The_inject_default_for_maybe_annotations()
function now handles cases where aStrawberryField
object already exists on the class but lacks explicit default values. The function checks forStrawberryField
instances where bothdefault
anddefault_factory
are missing and injectsNone
as the default value.
This change preserves the existing behavior for simple Maybe field declarations (e.g., name: strawberry.Maybe[str]
) while extending support to field-annotated Maybe types (e.g., name: strawberry.Maybe[str] = strawberry.field(description="...")
). The fix ensures that Maybe semantics work consistently across different field definition styles in input types, which is crucial for the GraphQL schema's type system integrity.
Confidence score: 4/5
- This PR addresses a specific bug with a targeted fix that should be safe to merge with minimal risk
- Score reflects well-isolated changes to core type handling logic, though the lack of tests in the current PR reduces confidence slightly
- Pay close attention to the Maybe type detection logic and default injection mechanisms, as these are foundational to input type behavior
2 files reviewed, 1 comment
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:
|
CodSpeed Performance ReportMerging #3985 will not alter performanceComparing Summary
|
Important
This PR is still a draft as I will not have time to write tests for the change for awhile.
Help in this regard would be appreciated.
Description
This patch fixes a bug where input
strawberry.Maybe
input fields would fail to have a defaultNone
injected when annotated or set withstrawberry.field
.Previously, this would cause this example input to require
name
to be set, despite it being markedMaybe
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by Sourcery
Fix default None injection for input fields annotated with strawberry.Maybe and improve detection of Maybe annotations
Bug Fixes:
Enhancements: