-
-
Notifications
You must be signed in to change notification settings - Fork 561
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
add support for including pydantic computed fields #3798
Changes from all commits
0072e2d
ccda2b7
144a414
127d100
533f179
9d08135
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,23 @@ | ||
Release type: minor | ||
|
||
Adds the ability to include pydantic computed fields when using pydantic.type decorator. | ||
|
||
Example: | ||
```python | ||
class UserModel(pydantic.BaseModel): | ||
age: int | ||
|
||
@computed_field | ||
@property | ||
def next_age(self) -> int: | ||
return self.age + 1 | ||
|
||
|
||
@strawberry.experimental.pydantic.type( | ||
UserModel, all_fields=True, include_computed=True | ||
) | ||
class User: | ||
pass | ||
``` | ||
|
||
Will allow `nextAge` to be requested from a user entity. |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -12,7 +12,7 @@ | |||||||
from strawberry.experimental.pydantic.exceptions import UnsupportedTypeError | ||||||||
|
||||||||
if TYPE_CHECKING: | ||||||||
from pydantic.fields import FieldInfo | ||||||||
from pydantic.fields import ComputedFieldInfo, FieldInfo | ||||||||
|
||||||||
IS_PYDANTIC_V2: bool = PYDANTIC_VERSION.startswith("2.") | ||||||||
IS_PYDANTIC_V1: bool = not IS_PYDANTIC_V2 | ||||||||
|
@@ -128,7 +128,33 @@ | |||||||
|
||||||||
return PydanticUndefined | ||||||||
|
||||||||
def get_model_fields(self, model: type[BaseModel]) -> dict[str, CompatModelField]: | ||||||||
def get_model_computed_fields( | ||||||||
self, model: type[BaseModel] | ||||||||
) -> dict[str, CompatModelField]: | ||||||||
computed_field_info: dict[str, ComputedFieldInfo] = model.model_computed_fields | ||||||||
new_fields = {} | ||||||||
# Convert it into CompatModelField | ||||||||
for name, field in computed_field_info.items(): | ||||||||
new_fields[name] = CompatModelField( | ||||||||
name=name, | ||||||||
type_=field.return_type, | ||||||||
outer_type_=field.return_type, | ||||||||
default=None, | ||||||||
default_factory=None, | ||||||||
required=False, | ||||||||
alias=field.alias, | ||||||||
# v2 doesn't have allow_none | ||||||||
allow_none=False, | ||||||||
has_alias=field is not None, | ||||||||
description=field.description, | ||||||||
_missing_type=self.PYDANTIC_MISSING_TYPE, | ||||||||
is_v1=False, | ||||||||
) | ||||||||
return new_fields | ||||||||
|
||||||||
def get_model_fields( | ||||||||
self, model: type[BaseModel], include_computed: bool = False | ||||||||
) -> dict[str, CompatModelField]: | ||||||||
field_info: dict[str, FieldInfo] = model.model_fields | ||||||||
new_fields = {} | ||||||||
# Convert it into CompatModelField | ||||||||
|
@@ -148,6 +174,8 @@ | |||||||
_missing_type=self.PYDANTIC_MISSING_TYPE, | ||||||||
is_v1=False, | ||||||||
) | ||||||||
if include_computed: | ||||||||
new_fields |= self.get_model_computed_fields(model) | ||||||||
return new_fields | ||||||||
|
||||||||
@cached_property | ||||||||
|
@@ -175,7 +203,10 @@ | |||||||
def PYDANTIC_MISSING_TYPE(self) -> Any: # noqa: N802 | ||||||||
return dataclasses.MISSING | ||||||||
|
||||||||
def get_model_fields(self, model: type[BaseModel]) -> dict[str, CompatModelField]: | ||||||||
def get_model_fields( | ||||||||
self, model: type[BaseModel], include_computed: bool = False | ||||||||
) -> dict[str, CompatModelField]: | ||||||||
"""`include_computed` is a noop for PydanticV1Compat.""" | ||||||||
new_fields = {} | ||||||||
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.
Suggested change
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. Just to keep linters happy :) |
||||||||
# Convert it into CompatModelField | ||||||||
for name, field in model.__fields__.items(): # type: ignore[attr-defined] | ||||||||
|
@@ -284,7 +315,6 @@ | |||||||
smart_deepcopy, | ||||||||
) | ||||||||
|
||||||||
|
||||||||
__all__ = [ | ||||||||
"PydanticCompat", | ||||||||
"get_args", | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
import textwrap | ||
|
||
import pydantic | ||
import pytest | ||
from pydantic.version import VERSION as PYDANTIC_VERSION | ||
|
||
import strawberry | ||
|
||
IS_PYDANTIC_V2: bool = PYDANTIC_VERSION.startswith("2.") | ||
|
||
if IS_PYDANTIC_V2: | ||
from pydantic import computed_field | ||
|
||
|
||
@pytest.mark.skipif( | ||
not IS_PYDANTIC_V2, reason="Requires Pydantic v2 for computed_field" | ||
) | ||
def test_computed_field(): | ||
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. Since we're using sets it won't be an issue, but it might be nice to add a test that has 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. Pydantic doesn't seem to like me declaring both a computed field and another field, with or without the class UserModel(pydantic.BaseModel):
age: int
next_age: strawberry.auto
@computed_field
@property
def next_age(self) -> int:
return self.age + 1 I get
If you were thinking of something different though, let me know, happy to write another test! 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. Ahh sorry for the confusion I meant something more along the lines of: @strawberry.experimental.pydantic.type(
UserModel, all_fields=True, include_computed=True
)
class User:
next_age: strawberry.auto This will touch the Not the most important test though for this PR as this can occur with or without computed fields, I'll already approve the PR, thanks again for contributing this! 🚀 |
||
class UserModel(pydantic.BaseModel): | ||
age: int | ||
|
||
@computed_field | ||
@property | ||
def next_age(self) -> int: | ||
return self.age + 1 | ||
|
||
@strawberry.experimental.pydantic.type( | ||
UserModel, all_fields=True, include_computed=True | ||
) | ||
class User: | ||
pass | ||
|
||
@strawberry.experimental.pydantic.type(UserModel, all_fields=True) | ||
class UserNoComputed: | ||
pass | ||
|
||
@strawberry.type | ||
class Query: | ||
@strawberry.field | ||
def user(self) -> User: | ||
return User.from_pydantic(UserModel(age=1)) | ||
|
||
@strawberry.field | ||
def user_no_computed(self) -> UserNoComputed: | ||
return UserNoComputed.from_pydantic(UserModel(age=1)) | ||
|
||
schema = strawberry.Schema(query=Query) | ||
|
||
expected_schema = """ | ||
type Query { | ||
user: User! | ||
userNoComputed: UserNoComputed! | ||
} | ||
|
||
type User { | ||
age: Int! | ||
nextAge: Int! | ||
} | ||
|
||
type UserNoComputed { | ||
age: Int! | ||
} | ||
""" | ||
|
||
assert str(schema) == textwrap.dedent(expected_schema).strip() | ||
|
||
query = "{ user { age nextAge } }" | ||
|
||
result = schema.execute_sync(query) | ||
assert not result.errors | ||
assert result.data["user"]["age"] == 1 | ||
assert result.data["user"]["nextAge"] == 2 | ||
Comment on lines
+69
to
+72
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. suggestion (testing): Add test verifying the behavior when include_computed is False It's important to explicitly test the negative case where Suggested implementation: assert result.data["user"]["age"] == 1
assert result.data["user"]["nextAge"] == 2
# New test for behavior when include_computed is False
def test_computed_not_included_when_disabled():
# Assuming build_schema accepts an include_computed flag.
schema = build_schema(include_computed=False)
expected_schema = """
type UserNoComputed {
age: Int!
}
"""
# Check that the printed schema does not include computed fields.
assert str(schema) == textwrap.dedent(expected_schema).strip()
query = "{ user { age nextAge } }"
# Execute a query that attempts to fetch the computed field.
result = schema.execute_sync(query)
# We expect an error since "nextAge" should not be resolvable.
assert result.errors
# Optionally, verify the error message indicates nextAge is not a valid field.
assert "nextAge" in result.errors[0].message Note:
|
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 (complexity): Consider extracting the shared field conversion logic into a helper function with a flag to handle computed fields differently, and then call this helper from both
get_model_fields
andget_model_computed_fields
functions.Consider extracting the shared conversion logic into a single helper that toggles computed behavior via a flag. This reduces duplication while keeping both functionalities intact. For example:
Then update your methods as follows:
This approach maintains current functionality and reduces complexity by consolidating repeated logic.