add support for default values with a field that is a pydantic model #3499
Conversation
for more information, see https://pre-commit.ci
| @@ -17,6 +17,9 @@ | |||
| IS_PYDANTIC_V2: bool = PYDANTIC_VERSION.startswith("2.") | |||
| IS_PYDANTIC_V1: bool = not IS_PYDANTIC_V2 | |||
|
|
|||
| if IS_PYDANTIC_V2: | |||
There was a problem hiding this comment.
I'm not sure this is absolutely necessary, but when I was running the tests locally with pydantic v2 all of the mypy automated tests were failing without this import.
|
Hi, thanks for contributing to Strawberry 🍓! We noticed that this PR is missing a So as soon as this PR is merged, a release will be made 🚀. Here's an example of Release type: patch
Description of the changes, ideally with some examples, if adding a new feature.Release type can be one of patch, minor or major. We use semver, so make sure to pick the appropriate type. If in doubt feel free to ask :) 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 @ppease - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if IS_PYDANTIC_V2: | ||
| import pydantic.v1 |
There was a problem hiding this comment.
question: Clarify the purpose of importing pydantic.v1 under IS_PYDANTIC_V2 condition.
It seems counterintuitive to import a version-specific module conditionally based on the version check. Ensure this is intentional and document the rationale behind this decision.
CodSpeed Performance ReportMerging #3499 will not alter performanceComparing Summary
|
ppease
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3499 +/- ##
==========================================
+ Coverage 94.43% 96.50% +2.07%
==========================================
Files 506 511 +5
Lines 31783 33008 +1225
Branches 3656 5488 +1832
==========================================
+ Hits 30014 31856 +1842
+ Misses 1487 918 -569
+ Partials 282 234 -48 |
for more information, see https://pre-commit.ci
| @@ -27,3 +29,72 @@ def filter(self, input: FilterInput) -> str: | |||
| assert not result.errors | |||
| assert result.data | |||
| assert result.data["filter"] == "Hello nope" | |||
|
|
|||
|
|
|||
| def test_input_with_nonscalar_field_default(): | |||
There was a problem hiding this comment.
I originally thought this had issues, but as long as you specify the default factory, this works as intended. I didn't see any tests explicitly testing this so I added one.
In this PR, I am adding support for having a pydantic model be a default value for a field on an input. Previously this would not generate the proper schema.
Description
My original fix was modifying code in
from_input_fieldinside of the schema converter. This is the point where we are instantiating a graphql input type, and I was checking to see if the default value had a model dump method on it. This seems a little better since it is still within the pydantic specific code. The issue I'm trying to solve is that when we print the schema from withinprint_input_valuewe useast_from_valueon the default value. We fall into theis_input_object_typecase and then we check if there is an object definition for the value. Ifis_input_object_typeevaluates toTruethen we callstrawberry.asdict. I think we'd want to do something similar with a pydantic instance. Today we fail the check and end up returningNonewhich causes the default value to not be printed in the schema. What I'm doing in this MR is detecting if our default value is a pydantic instance, and if it is then I am storing the serialized value for the pydantic model as the default value.I'm curious to know if there is a better way to plug into the printing process because that feels more inline with what we are doing with dataclasses today. I could add code that I had previously into either
print_input_valueorast_from_value, but that felt a little more hacky since we are putting pydantic specific stuff in higher level code.Types of Changes
Issues Fixed or Closed by This PR
This fixes this issue
Checklist