Fix pydantic generics and add ability to directive json schema #4000
+348
−4
Conversation
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
Reviewer's GuideFix Pydantic generics conversion by handling generic metadata and preserving type parameters in dataclass creation, and add support for attaching schema directives based on Pydantic JSON schema extras. Decorator signatures are extended to accept a File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for adding the Here's a preview of the changelog: This release fixes the pydantic support for generics and allows capture of the Here's the tweet text: |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Extract the JSON schema directive application logic in
_build_dataclass_creation_fieldsinto a separate helper to reduce complexity and improve readability. - Relying on the internal
__pydantic_generic_metadata__to resolve generics is brittle – consider using official Pydantic or typing‐utilities APIs to retrieve generic arguments. - The call to
model.model_json_schema()on every type wrap may be expensive for large models; consider caching or deferring it until directives are actually applied.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the JSON schema directive application logic in `_build_dataclass_creation_fields` into a separate helper to reduce complexity and improve readability.
- Relying on the internal `__pydantic_generic_metadata__` to resolve generics is brittle – consider using official Pydantic or typing‐utilities APIs to retrieve generic arguments.
- The call to `model.model_json_schema()` on every type wrap may be expensive for large models; consider caching or deferring it until directives are actually applied.
## Individual Comments
### Comment 1
<location> `tests/experimental/pydantic/schema/test_basic.py:531-540` </location>
<code_context>
assert result.data["user"]["age"] == 1
assert result.data["user"]["password"] is None
+def test_nested_type_with_resolved_generic():
+
+ A = TypeVar("A")
+ class Hobby(pydantic.BaseModel, Generic[A]):
+ name: A
+
+ @strawberry.experimental.pydantic.type(Hobby)
+ class HobbyType(Generic[A]):
+ name: strawberry.auto
+
+ class User(pydantic.BaseModel):
+ hobby: Hobby[str]
+
+ @strawberry.experimental.pydantic.type(User)
+ class UserType:
+ hobby: strawberry.auto
+
+ @strawberry.type
+ class Query:
+ @strawberry.field
+ def user(self) -> UserType:
+ return UserType(hobby=HobbyType(name="Skii"))
+
+ schema = strawberry.Schema(query=Query)
+
+ query = "{ user { hobby { name } } }"
+
+ result = schema.execute_sync(query)
+
+ assert not result.errors
+ assert result.data["user"]["hobby"]["name"] == "Skii"
+
+def test_nested_type_with_resolved_field_generic():
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for error conditions and invalid generic types.
Please add tests for cases where the generic type is missing, invalid, or mismatched to verify error handling and type resolution.
</issue_to_address>
### Comment 2
<location> `tests/experimental/pydantic/schema/test_basic.py:563-594` </location>
<code_context>
+ assert not result.errors
+ assert result.data["user"]["hobby"]["name"] == "Skii"
+
+def test_nested_type_with_resolved_field_generic():
+ Count: TypeAlias = Annotated[float, pydantic.Field(ge = 0)]
+
+ A = TypeVar("A")
+ class Hobby(pydantic.BaseModel, Generic[A]):
+ count: A
+
+ @strawberry.experimental.pydantic.type(Hobby)
+ class HobbyType(Generic[A]):
+ count: strawberry.auto
+
+ class User(pydantic.BaseModel):
+ hobby: Hobby[Count]
+
+ @strawberry.experimental.pydantic.type(User)
+ class UserType:
+ hobby: strawberry.auto
+
+ @strawberry.type
+ class Query:
+ @strawberry.field
+ def user(self) -> UserType:
+ return UserType(hobby=HobbyType(count=2))
+
+ schema = strawberry.Schema(query=Query)
+
+ query = "{ user { hobby { count } } }"
+
+ result = schema.execute_sync(query)
+
+ assert not result.errors
+ assert result.data["user"]["hobby"]["count"] == 2
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions for field constraints and edge cases.
Consider adding tests that pass invalid values (such as negative numbers) to ensure the constraint is enforced and errors are reported correctly.
```suggestion
def test_nested_type_with_resolved_field_generic():
Count: TypeAlias = Annotated[float, pydantic.Field(ge = 0)]
A = TypeVar("A")
class Hobby(pydantic.BaseModel, Generic[A]):
count: A
@strawberry.experimental.pydantic.type(Hobby)
class HobbyType(Generic[A]):
count: strawberry.auto
class User(pydantic.BaseModel):
hobby: Hobby[Count]
@strawberry.experimental.pydantic.type(User)
class UserType:
hobby: strawberry.auto
@strawberry.type
class Query:
@strawberry.field
def user(self) -> UserType:
return UserType(hobby=HobbyType(count=2))
schema = strawberry.Schema(query=Query)
query = "{ user { hobby { count } } }"
result = schema.execute_sync(query)
assert not result.errors
assert result.data["user"]["hobby"]["count"] == 2
def test_nested_type_with_resolved_field_generic_constraint_violation():
Count: TypeAlias = Annotated[float, pydantic.Field(ge = 0)]
A = TypeVar("A")
class Hobby(pydantic.BaseModel, Generic[A]):
count: A
@strawberry.experimental.pydantic.type(Hobby)
class HobbyType(Generic[A]):
count: strawberry.auto
class User(pydantic.BaseModel):
hobby: Hobby[Count]
@strawberry.experimental.pydantic.type(User)
class UserType:
hobby: strawberry.auto
@strawberry.type
class Query:
@strawberry.field
def user(self) -> UserType:
# Intentionally pass a negative value to violate the constraint
return UserType(hobby=HobbyType(count=-1))
schema = strawberry.Schema(query=Query)
query = "{ user { hobby { count } } }"
result = schema.execute_sync(query)
assert result.errors, "Expected errors due to constraint violation"
assert result.data is None or result.data["user"]["hobby"]["count"] is None
```
</issue_to_address>
### Comment 3
<location> `tests/experimental/pydantic/schema/test_directives.py:8` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Don't import test modules. ([`dont-import-test-modules`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/dont-import-test-modules))
<details><summary>Explanation</summary>Don't import test modules.
Tests should be self-contained and don't depend on each other.
If a helper function is used by multiple tests,
define it in a helper module,
instead of importing one test from the other.
</details>
</issue_to_address>
### Comment 4
<location> `strawberry/experimental/pydantic/object_type.py:95-100` </location>
<code_context>
def _build_dataclass_creation_fields(
field: CompatModelField,
is_input: bool,
existing_fields: dict[str, StrawberryField],
auto_fields_set: set[str],
use_pydantic_alias: bool,
compat: PydanticCompat,
json_schema: dict[str, Any],
json_schema_directive: Optional[builtins.type] = None,
) -> DataclassCreationFields:
field_type = (
get_type_for_field(field, is_input, compat=compat)
if field.name in auto_fields_set
else existing_fields[field.name].type
)
if (
field.name in existing_fields
and existing_fields[field.name].base_resolver is not None
):
# if the user has defined a resolver for this field, always use it
strawberry_field = existing_fields[field.name]
else:
# otherwise we build an appropriate strawberry field that resolves it
existing_field = existing_fields.get(field.name)
graphql_name = None
if existing_field and existing_field.graphql_name:
graphql_name = existing_field.graphql_name
elif field.has_alias and use_pydantic_alias:
graphql_name = field.alias
if json_schema_directive and json_schema:
field_names = {
field.name for field in dataclasses.fields(json_schema_directive)
}
applicable_values = {
key: value
for key, value in json_schema.items()
if key in field_names
}
if applicable_values:
json_directive = json_schema_directive(
**applicable_values
)
directives = (
*(existing_field.directives if existing_field else ()),
json_directive,
)
else:
directives = existing_field.directives if existing_field else ()
else:
directives = ()
strawberry_field = StrawberryField(
python_name=field.name,
graphql_name=graphql_name,
# always unset because we use default_factory instead
default=dataclasses.MISSING,
default_factory=get_default_factory_for_field(field, compat=compat),
type_annotation=StrawberryAnnotation.from_annotation(field_type),
description=field.description,
deprecation_reason=(
existing_field.deprecation_reason if existing_field else None
),
permission_classes=(
existing_field.permission_classes if existing_field else []
),
directives=directives,
metadata=existing_field.metadata if existing_field else {},
)
return DataclassCreationFields(
name=field.name,
field_type=field_type, # type: ignore
field=strawberry_field,
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** We've found these issues:
- Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))
- Low code quality found in \_build\_dataclass\_creation\_fields - 19% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))
```suggestion
if applicable_values := {
key: value
for key, value in json_schema.items()
if key in field_names
}:
```
<br/><details><summary>Explanation</summary>
The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Summary
This PR fixes two important issues in Strawberry's Pydantic integration: generic type conversion and adds JSON schema directive support.
Key Changes:
- Generic Type Fix: Enhanced
replace_types_recursivelyinfields.pyto properly handle Pydantic generic models by checking__pydantic_generic_metadata__and applying type arguments correctly - JSON Schema Directives: Added
json_schema_directiveparameter to@pydantic.type()and@pydantic.input()decorators, enabling automatic application of schema directives based on Pydantic field metadata - Generic Base Handling: Fixed dataclass creation to use
__orig_bases__instead of__bases__for proper generic inheritance - Comprehensive Testing: Added tests for both generic type resolution scenarios and JSON schema directive functionality
The implementation is solid with proper error handling and follows existing code patterns. The JSON schema directive feature integrates cleanly with Strawberry's directive system, automatically extracting relevant metadata from Pydantic's json_schema_extra and applying it as GraphQL directives.
Confidence Score: 4/5
- This PR is safe to merge with good test coverage and minimal risk
- Score reflects solid implementation with comprehensive tests, but minor improvement suggested for error handling in generic type processing
- Pay attention to
test_generic.pywhich is empty and should contain generic-specific tests
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| strawberry/experimental/pydantic/fields.py | 4/5 | Fixed generic type handling by checking for __pydantic_generic_metadata__ and applying args correctly |
| strawberry/experimental/pydantic/object_type.py | 4/5 | Added json schema directive support and fixed generic bases handling using __orig_bases__ |
| tests/experimental/pydantic/schema/test_generic.py | 3/5 | Empty test file that should contain generic-specific tests |
Sequence Diagram
sequenceDiagram
participant U as User Code
participant PT as @pydantic.type decorator
participant RTR as replace_types_recursively
participant BDCF as _build_dataclass_creation_fields
participant SM as Strawberry Model
U->>PT: Define Pydantic model with generics
PT->>RTR: Process field types
alt Pydantic Generic detected
RTR->>RTR: Check __pydantic_generic_metadata__
RTR->>RTR: Apply generic args to replaced_type
else Regular type
RTR->>RTR: Return replaced_type as-is
end
RTR->>BDCF: Return processed type
opt JSON Schema Directive provided
BDCF->>BDCF: Extract field schema properties
BDCF->>BDCF: Create directive instance
BDCF->>BDCF: Add to field directives
end
BDCF->>PT: Return field configuration
PT->>PT: Use __orig_bases__ for generic inheritance
PT->>SM: Create Strawberry dataclass
SM->>U: Return configured type
5 files reviewed, 2 comments
Comment on lines
49
to
50
| if hasattr(basic_type, "__pydantic_generic_metadata__") and basic_type.__pydantic_generic_metadata__["args"]: | ||
| return replaced_type[*basic_type.__pydantic_generic_metadata__["args"]] |
There was a problem hiding this comment.
style: Consider checking if args exists and is not empty to avoid potential KeyError
Suggested change
| if hasattr(basic_type, "__pydantic_generic_metadata__") and basic_type.__pydantic_generic_metadata__["args"]: | |
| return replaced_type[*basic_type.__pydantic_generic_metadata__["args"]] | |
| if hasattr(basic_type, "__pydantic_generic_metadata__") and basic_type.__pydantic_generic_metadata__.get("args"): | |
| return replaced_type[*basic_type.__pydantic_generic_metadata__["args"]] |
Boudewijn26
force-pushed
the
feature/improved-pydantics
branch
from
fc18959 to
bf10d56
Compare
Boudewijn26
force-pushed
the
feature/improved-pydantics
branch
5 times, most recently
from
844bef6 to
048ecf2
Compare
Boudewijn26
force-pushed
the
feature/improved-pydantics
branch
from
048ecf2 to
65beaaf
Compare
bellini666
reviewed
Sep 28, 2025
| schema = strawberry.Schema(query=Query) | ||
|
|
||
| expected_schema = """ | ||
| directive @jsonSchema(test: Int!, exclusiveMinimum: Int = null) on FIELD_DEFINITION |
There was a problem hiding this comment.
Would this also include values from other fields on other types?
And if so, would this become too bloated in case we have N fields, each with M different extras?
I'm not much familiar with the pydantic integration here, so @patrick91 may be the best to check this :)
There was a problem hiding this comment.
Only the values in the json schema which are present on the passed directive are passed to the graphQL schema.
So bloat would be limited by whichever values the user wants in their directive. They could also decide to have different directives for different types to prevent their directives from growing too much
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Description
This fixes the conversion of pydantic models into strawberry models if those models include generics. It also adds the ability to pass a
SchemaDirectiveto the pydantic conversion to collect the pydantic json schema details inTypes of Changes
Checklist
Summary by Sourcery
Fix Pydantic generic model support in Strawberry conversion and introduce json_schema_directive to annotate fields with Pydantic JSON schema metadata as GraphQL directives
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: