strawberry icon indicating copy to clipboard operation
strawberry copied to clipboard

add support for default values with a field that is a pydantic model

Open ppease opened this issue 1 year ago • 3 comments

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_field inside 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 within print_input_value we use ast_from_value on the default value. We fall into the is_input_object_type case and then we check if there is an object definition for the value. If is_input_object_type evaluates to True then we call strawberry.asdict. I think we'd want to do something similar with a pydantic instance. Today we fail the check and end up returning None which 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_value or ast_from_value, but that felt a little more hacky since we are putting pydantic specific stuff in higher level code.

Types of Changes

  • [ ] Core
  • [x] Bugfix
  • [ ] New feature
  • [ ] Enhancement/optimization
  • [ ] Documentation

Issues Fixed or Closed by This PR

This fixes this issue

Checklist

  • [x] My code follows the code style of this project.
  • [ ] My change requires a change to the documentation.
  • [ ] I have updated the documentation accordingly.
  • [x] I have read the CONTRIBUTING document.
  • [x] I have added tests to cover my changes.
  • [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

ppease avatar May 14 '24 18:05 ppease

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Fixes a bug where pydantic models as the default value for an input did not print the proper schema. See this issue.

Here's the tweet text:

🆕 Release (next) is out! Thanks to ppease for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

botberry avatar May 14 '24 18:05 botberry

CodSpeed Performance Report

Merging #3499 will not alter performance

Comparing ppease:feature/default-values (c5f6b60) with main (b7f2881)

Summary

✅ 12 untouched benchmarks

codspeed-hq[bot] avatar May 14 '24 18:05 codspeed-hq[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.56%. Comparing base (b7f2881) to head (c5f6b60).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3499   +/-   ##
=======================================
  Coverage   96.56%   96.56%           
=======================================
  Files         523      523           
  Lines       33485    33551   +66     
  Branches     5554     5566   +12     
=======================================
+ Hits        32336    32400   +64     
- Misses        914      916    +2     
  Partials      235      235           

codecov[bot] avatar May 14 '24 19:05 codecov[bot]

@ppease is there any reason why this is draft? just double checking!

patrick91 avatar Jun 10 '24 13:06 patrick91

@ppease is there any reason why this is draft? just double checking!

@patrick91 This fix resolves my original issue. The only reason I made it a draft is I just wanted to make sure the fix made sense architecturally, and there wasn't a more preferred way to fix the issue.

ppease avatar Jun 10 '24 13:06 ppease

@ppease I think the fix is fine! 😊

patrick91 avatar Jun 10 '24 13:06 patrick91

@patrick91 Sounds good! Thanks for reviewing it! I went ahead and marked it as ready.

ppease avatar Jun 10 '24 14:06 ppease

@ppease thanks! looks like the tests all passed here, so I merged it

Mypy sometimes does weird things locally with pydantic :D

patrick91 avatar Jun 10 '24 14:06 patrick91