fix asdict to handle Some and UNSET values#4231
Open
Mr-Neutr0n wants to merge 3 commits intostrawberry-graphql:mainfrom
Open
fix asdict to handle Some and UNSET values#4231Mr-Neutr0n wants to merge 3 commits intostrawberry-graphql:mainfrom
Mr-Neutr0n wants to merge 3 commits intostrawberry-graphql:mainfrom
Conversation
Contributor
Reviewer's GuideImplements a custom recursive File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
Member
|
Thanks for adding the Here's a preview of the changelog:
Here's the tweet text: |
Member
|
Thanks for adding the Here's a preview of the changelog:
Here's the tweet text: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The custom
_asdict_innerno longer has the cycle-detection logic thatdataclasses.asdictprovides, so consider adding memoization to avoid infinite recursion on self-referential dataclasses. - The new implementation now also transforms dictionary keys (not just values), which differs from
dataclasses.asdictbehavior; double-check whether this change is intentional for Strawberry objects or if keys should be left untouched. - UNSET handling currently only applies to dataclass fields; if UNSET can appear inside lists, tuples, or dicts, you may want to clarify or extend how those occurrences should be treated (e.g., pruned vs. preserved).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The custom `_asdict_inner` no longer has the cycle-detection logic that `dataclasses.asdict` provides, so consider adding memoization to avoid infinite recursion on self-referential dataclasses.
- The new implementation now also transforms dictionary keys (not just values), which differs from `dataclasses.asdict` behavior; double-check whether this change is intentional for Strawberry objects or if keys should be left untouched.
- UNSET handling currently only applies to dataclass fields; if UNSET can appear inside lists, tuples, or dicts, you may want to clarify or extend how those occurrences should be treated (e.g., pruned vs. preserved).
## Individual Comments
### Comment 1
<location> `strawberry/types/object_type.py:496-505` </location>
<code_context>
+ from strawberry.types.maybe import Some
+ from strawberry.types.unset import UNSET
+
+ def _asdict_inner(obj: Any) -> Any:
+ if isinstance(obj, Some):
+ return _asdict_inner(obj.value)
+ if dataclasses.is_dataclass(obj) and not isinstance(obj, builtins.type):
+ result = {}
+ for f in dataclasses.fields(obj):
+ value = getattr(obj, f.name)
+ if value is UNSET:
+ continue
+ result[f.name] = _asdict_inner(value)
+ return result
+ if isinstance(obj, (list, tuple)):
+ cls = builtins.type(obj)
+ return cls(_asdict_inner(v) for v in obj)
+ if isinstance(obj, dict):
+ return {
+ _asdict_inner(k): _asdict_inner(v) for k, v in obj.items()
+ }
+ return obj
+
+ return _asdict_inner(obj)
</code_context>
<issue_to_address>
**issue (bug_risk):** Self-referential objects can now cause infinite recursion compared to dataclasses.asdict’s cycle protection.
`dataclasses.asdict` tracks visited objects to prevent infinite recursion on self- or mutually-referential structures. This implementation doesn’t, so inputs that previously worked may now hit a `RecursionError`. Please add identity-based cycle tracking (or otherwise align with `dataclasses.asdict`’s behavior) to avoid this regression.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+496
to
+505
| def _asdict_inner(obj: Any) -> Any: | ||
| if isinstance(obj, Some): | ||
| return _asdict_inner(obj.value) | ||
| if dataclasses.is_dataclass(obj) and not isinstance(obj, builtins.type): | ||
| result = {} | ||
| for f in dataclasses.fields(obj): | ||
| value = getattr(obj, f.name) | ||
| if value is UNSET: | ||
| continue | ||
| result[f.name] = _asdict_inner(value) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Self-referential objects can now cause infinite recursion compared to dataclasses.asdict’s cycle protection.
dataclasses.asdict tracks visited objects to prevent infinite recursion on self- or mutually-referential structures. This implementation doesn’t, so inputs that previously worked may now hit a RecursionError. Please add identity-based cycle tracking (or otherwise align with dataclasses.asdict’s behavior) to avoid this regression.
Contributor
Greptile OverviewGreptile SummaryReplaces the simple
Confidence Score: 4/5
Important Files Changed
Flowchartflowchart TD
A["asdict(obj)"] --> B["_asdict_inner(obj)"]
B --> C{"isinstance(obj, Some)?"}
C -->|Yes| D["Unwrap: _asdict_inner(obj.value)"]
D --> B
C -->|No| E{"is_dataclass(obj)?"}
E -->|Yes| F["Iterate fields"]
F --> G{"value is UNSET?"}
G -->|Yes| H["Skip field"]
G -->|No| I["result[name] = _asdict_inner(value)"]
I --> J["Return result dict"]
H --> F
E -->|No| K{"isinstance(obj, list/tuple)?"}
K -->|Yes| L["Reconstruct with _asdict_inner per element"]
K -->|No| M{"isinstance(obj, dict)?"}
M -->|Yes| N["Recurse on keys and values"]
M -->|No| O["Return obj as-is (leaf)"]
Last reviewed commit: a9158e6 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+507
to
+509
| if isinstance(obj, (list, tuple)): | ||
| cls = builtins.type(obj) | ||
| return cls(_asdict_inner(v) for v in obj) |
Contributor
There was a problem hiding this comment.
NamedTuple reconstruction will break
When obj is a NamedTuple instance, isinstance(obj, tuple) is True, so this branch is taken. However, cls(generator) passes the generator as the first positional argument rather than unpacking it, which will fail for NamedTuples with more than one field. The standard library's dataclasses._asdict_inner uses type(obj)(*[...]) to correctly unpack.
While this is unlikely to be hit with typical strawberry types (which are dataclasses), it's a subtle regression from the previous dataclasses.asdict behavior. Consider using unpacking to match the standard library:
Suggested change
| if isinstance(obj, (list, tuple)): | |
| cls = builtins.type(obj) | |
| return cls(_asdict_inner(v) for v in obj) | |
| if isinstance(obj, (list, tuple)): | |
| cls = builtins.type(obj) | |
| return cls(*[_asdict_inner(v) for v in obj]) |
Note: using *[list_comp] instead of *(generator) ensures NamedTuple constructors receive individual positional arguments.
Comment on lines
+503
to
+504
| if value is UNSET: | ||
| continue |
Member
There was a problem hiding this comment.
When you use Maybe annotation, and value is not provided it's going to be None - https://strawberry.rocks/docs/types/maybe
You need to check if the field annotation is maybe - AFAIR there is such a function already in strawberry. If so and the value is None, exclude it from the result.
This is the implementation I've been using in my project - it's not perfect, but may come in handy
from dataclasses import fields, is_dataclass
def strawberry_to_dict(obj) -> dict[str, Any]:
result: dict[str, Any] = {}
for field in fields(obj):
value = getattr(obj, field.name)
is_maybe = _annotation_is_maybe(field.type)
if isinstance(value, Some):
if is_dataclass(value.value):
result[field.name] = _strawberry_to_dict(value.value)
elif isinstance(value.value, list):
result[field.name] = [_strawberry_to_dict(item) if is_dataclass(item) else item for item in value.value]
else:
result[field.name] = value.value
elif value is None and not is_maybe:
result[field.name] = None
elif value is not UNSET and not is_maybe:
if is_dataclass(value):
result[field.name] = _strawberry_to_dict(value)
elif isinstance(value, list):
result[field.name] = [_strawberry_to_dict(item) if is_dataclass(item) else item for item in value]
else:
result[field.name] = value
return result
_maybe_re = re.compile(r"^(?:strawberry\.)?Maybe\[(.+)\]$")
def _annotation_is_maybe(annotation: Any) -> bool:
# copied from strawberry
if isinstance(annotation, str):
# Ideally we would try to evaluate the annotation, but the args inside
# may still not be available, as the module is still being constructed.
# Checking for the pattern should be good enough for now.
return _maybe_re.match(annotation) is not None
orig = typing.get_origin(annotation)
if orig is typing.Annotated:
return _annotation_is_maybe(typing.get_args(annotation)[0])
return orig is Maybe
bellini666
reviewed
Feb 14, 2026
Comment on lines
+493
to
+494
| from strawberry.types.maybe import Some | ||
| from strawberry.types.unset import UNSET |
Member
There was a problem hiding this comment.
Are those necessary for preventing import error? If not, we can move then to module level
| def _asdict_inner(obj: Any) -> Any: | ||
| if isinstance(obj, Some): | ||
| return _asdict_inner(obj.value) | ||
| if dataclasses.is_dataclass(obj) and not isinstance(obj, builtins.type): |
Member
There was a problem hiding this comment.
what happens when dataclasses.is_dataclass(obj) and also isinstance(obj, builtins.type)?
11 tasks
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
4 participants
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.
strawberry.asdictwas just delegating todataclasses.asdict, which doesn't know aboutSomeorUNSET. This meantSome(value)would end up as-is in the dict instead of being unwrapped, andUNSETfields would leak through instead of being excluded.Fixed by replacing the plain
dataclasses.asdictcall with a custom recursive implementation that:Some(value)to justvalueUNSETdataclasses.asdictdoesFixes #4141
Related: #3265
Summary by Sourcery
Update strawberry.asdict to correctly handle framework-specific sentinel and wrapper types while maintaining recursive conversion behavior.
Bug Fixes:
Documentation:
Tests: