-
-
Notifications
You must be signed in to change notification settings - Fork 619
fix asdict to handle Some and UNSET values #4231
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| Release type: patch | ||
|
|
||
| `strawberry.asdict` now correctly handles `Some` and `UNSET` values. | ||
| `Some(value)` is unwrapped to its inner value, and fields set to `UNSET` | ||
| are excluded from the resulting dictionary. |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -465,7 +465,11 @@ class MyNode: | |||||||||||||
| def asdict(obj: Any) -> dict[str, object]: | ||||||||||||||
| """Convert a strawberry object into a dictionary. | ||||||||||||||
|
|
||||||||||||||
| This wraps the dataclasses.asdict function to strawberry. | ||||||||||||||
| This wraps the dataclasses.asdict function with additional handling | ||||||||||||||
| for strawberry-specific types like ``Some`` and ``UNSET``. | ||||||||||||||
|
|
||||||||||||||
| - Fields set to ``UNSET`` are excluded from the result. | ||||||||||||||
| - ``Some(value)`` wrappers are unwrapped to their inner value. | ||||||||||||||
|
|
||||||||||||||
| Args: | ||||||||||||||
| obj: The object to convert into a dictionary. | ||||||||||||||
|
|
@@ -486,7 +490,28 @@ class User: | |||||||||||||
| # {"name": "Lorem", "age": 25} | ||||||||||||||
| ``` | ||||||||||||||
| """ | ||||||||||||||
| return dataclasses.asdict(obj) | ||||||||||||||
| 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): | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens when |
||||||||||||||
| result = {} | ||||||||||||||
| for f in dataclasses.fields(obj): | ||||||||||||||
| value = getattr(obj, f.name) | ||||||||||||||
| if value is UNSET: | ||||||||||||||
| continue | ||||||||||||||
|
Comment on lines
+503
to
+504
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you use You need to check if the field annotation is 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 |
||||||||||||||
| result[f.name] = _asdict_inner(value) | ||||||||||||||
|
Comment on lines
+496
to
+505
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (bug_risk): Self-referential objects can now cause infinite recursion compared to dataclasses.asdict’s cycle protection.
|
||||||||||||||
| return result | ||||||||||||||
| if isinstance(obj, (list, tuple)): | ||||||||||||||
| cls = builtins.type(obj) | ||||||||||||||
| return cls(_asdict_inner(v) for v in obj) | ||||||||||||||
|
Comment on lines
+507
to
+509
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NamedTuple reconstruction will break When While this is unlikely to be hit with typical strawberry types (which are dataclasses), it's a subtle regression from the previous
Suggested change
Note: using |
||||||||||||||
| if isinstance(obj, dict): | ||||||||||||||
| return {_asdict_inner(k): _asdict_inner(v) for k, v in obj.items()} | ||||||||||||||
| return obj | ||||||||||||||
|
|
||||||||||||||
| return _asdict_inner(obj) | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| __all__ = [ | ||||||||||||||
|
|
||||||||||||||
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.
Are those necessary for preventing import error? If not, we can move then to module level