add support for including pydantic computed fields #3798
Conversation
Reviewer's Guide by SourceryThis pull request introduces support for including Pydantic computed fields in Strawberry types generated using the Sequence diagram for including computed fieldssequenceDiagram
participant User
participant strawberry.type
participant PydanticCompat
participant PydanticModel
User->>strawberry.type: Call type(model, include_computed=True)
strawberry.type->>PydanticCompat: from_model(model)
strawberry.type->>PydanticCompat: get_model_fields(model, include_computed=True)
PydanticCompat->>PydanticModel: model.model_fields
PydanticCompat->>PydanticModel: model.model_computed_fields
PydanticCompat-->>strawberry.type: model_fields
strawberry.type-->>User: StrawberryType
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tylernisonoff - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case that uses a computed field with a different name than the pydantic field.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| result = schema.execute_sync(query) | ||
| assert not result.errors | ||
| assert result.data["user"]["age"] == 1 | ||
| assert result.data["user"]["nextAge"] == 2 |
There was a problem hiding this comment.
suggestion (testing): Add test verifying the behavior when include_computed is False
It's important to explicitly test the negative case where include_computed=False. Verify that the computed field is not included in the schema and is not resolvable.
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].messageNote:
- Ensure that the function build_schema exists and accepts the include_computed flag.
- Adjust the error message check as needed to match the actual error output from your GraphQL library.
- If you prefer, you could wrap the new test in a dedicated test class or module that groups computed field tests.
| ) | ||
| return new_fields | ||
|
|
||
| def get_model_fields( |
There was a problem hiding this comment.
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 and get_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:
def _convert_field(self, name: str, field: Any, *, computed: bool = False) -> CompatModelField:
type_val = field.return_type if computed else field.annotation
return CompatModelField(
name=name,
type_=type_val,
outer_type_=type_val,
default=None if computed else field.default,
default_factory=None if computed else field.default_factory, # type: ignore
required=False if computed else field.is_required(),
alias=field.alias,
allow_none=False,
has_alias=field is not None,
description=field.description,
_missing_type=self.PYDANTIC_MISSING_TYPE,
is_v1=False,
)Then update your methods as follows:
def get_model_fields(self, model: type[BaseModel], include_computed: bool = False
) -> dict[str, CompatModelField]:
basic_fields = {
name: self._convert_field(name, field)
for name, field in model.model_fields.items()
}
if include_computed:
computed_fields = {
name: self._convert_field(name, field, computed=True)
for name, field in model.model_computed_fields.items()
}
basic_fields |= computed_fields
return basic_fields
def get_model_computed_fields(self, model: type[BaseModel]) -> dict[str, CompatModelField]:
return {
name: self._convert_field(name, field, computed=True)
for name, field in model.model_computed_fields.items()
}This approach maintains current functionality and reduces complexity by consolidating repeated logic.
|
Thanks for adding the Here's a preview of the changelog: Adds the ability to include pydantic computed fields when using pydantic.type decorator. Example: 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:
passWill allow Here's the tweet text: |
|
@patrick91 sorry for the ping, but would love to do whatever's needed to get this merged if this change would be accepted! Is there someone that can review this PR from the strawberry team? |
| 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.
| new_fields = {} | |
| del include_computed | |
| new_fields = {} |
There was a problem hiding this comment.
Just to keep linters happy :)
|
@tylernisonoff looks good to me! Thanks for contributing this fix. Could you also please add a sentence to the pydantic docs. Something along the lines of: If you want to include all of the fields from your Pydantic model, you can
instead pass `all_fields=True` to the decorator.
-> **Note** Care should be taken to avoid accidentally exposing fields that ->
weren't meant to be exposed on an API using this feature.
```python
import strawberry
from .models import User
@strawberry.experimental.pydantic.type(model=User, all_fields=True)
class UserType:
passAddition below: By default, computed fields are excluded. To also include all computed fields pass `include_computed=True` to the decorator.
```python
import strawberry
from .models import User
@strawberry.experimental.pydantic.type(model=User, all_fields=True, include_computed=True)
class UserType:
pass |
|
@tylernisonoff closing was a mistake 👀 I'll reopen if possible |
| import strawberry | ||
|
|
||
|
|
||
| def test_computed_field(): |
There was a problem hiding this comment.
Since we're using sets it won't be an issue, but it might be nice to add a test that has include_computed=True and also declares the computed field next_age with strawberry.auto to make sure we don't have duplication.
There was a problem hiding this comment.
Pydantic doesn't seem to like me declaring both a computed field and another field, with or without the strawberry.auto type -- for example for
class UserModel(pydantic.BaseModel):
age: int
next_age: strawberry.auto
@computed_field
@property
def next_age(self) -> int:
return self.age + 1I get
ValueError: you can't override a field with a computed field
If you were thinking of something different though, let me know, happy to write another test!
There was a problem hiding this comment.
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.autoThis will touch the include_computed flow as well as the regular field specifier flow. Currently this test should pass fine as we are using a set to dedupe the fields, the purpose would be to check if ever this behavior changes in the future.
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! 🚀
|
@skilkis thanks a ton for reviewing! I added the documentation. Wasn't able to get the test you were suggesting working, but I may have misunderstood the approach. Happy to add more tests if you'd like. |
Adds the ability to include pydantic computed fields when using
pydantic.typedecorator.Description
Fixes #3607
Reserves backwards compatibility by only enabling this behavior with a bool flag.
Types of Changes
Issues Fixed or Closed by This PR
all_fieldsdoes not include computed fields #3607Checklist
Summary by Sourcery
Adds support for including Pydantic computed fields in Strawberry types when using the
strawberry.experimental.pydantic.typedecorator. This feature is enabled via theinclude_computedflag.New Features:
pydantic.typedecorator by introducing a newinclude_computedflag.Tests:
include_computedflag is enabled.