Skip to content
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

Fix Pydantic Unions #1231

Merged
merged 4 commits into from
Sep 11, 2021
Merged

Conversation

Matt343
Copy link

@Matt343 Matt343 commented Sep 10, 2021

Description

In order to convert Union types from Pydantic models into Strawberry models, we need to check which branch of the Union we have and proceed with the conversion according to that type definition. I had to add a _pydantic_type field to our StrawberryType instances that are created through the converter so that we can get a reference to the original type of each branch for comparison. This seems a little hacky, but I couldn't think of a better way.

This fix only works for Unions of object types, but that should be fine as GraphQL doesn't support other types of unions.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

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

Matt Allen added 2 commits September 10, 2021 14:18
…ma_fix_unions_pydantic

# Conflicts:
#	tests/experimental/pydantic/test_conversion.py
@botberry
Copy link
Member

botberry commented Sep 10, 2021

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Fixes a bug in the Pydantic conversion code around Union values.


Here's the preview release card for twitter:

Here's the tweet text:

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

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #1231 (8fb5d6f) into main (6cff2e4) will decrease coverage by 0.02%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main    #1231      +/-   ##
==========================================
- Coverage   97.58%   97.56%   -0.03%     
==========================================
  Files          88       88              
  Lines        3360     3369       +9     
  Branches      489      493       +4     
==========================================
+ Hits         3279     3287       +8     
  Misses         45       45              
- Partials       36       37       +1     

@patrick91
Copy link
Member

@Matt343 thanks! I'll try to review this over the weekend ^^

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I've only added a note, but it is nothing to worry about for now, we'll deal with it when we finalise some refactoring and when we tackle this issue: #1192

@@ -144,6 +144,7 @@ def wrap(cls):
)

model._strawberry_type = cls # type: ignore
cls._pydantic_type = model # type: ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a patter we want to move away from :)

I think we can keep this for now though, we still need to do some refactoring anyway ^^

@patrick91 patrick91 merged commit 4518706 into strawberry-graphql:main Sep 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pydantic conversion fails for Union types
3 participants