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): Check for unused strawberry.auto fields when converting BaseModel #1551

Conversation

thejaminator
Copy link
Contributor

@thejaminator thejaminator commented Jan 7, 2022

Description

Checks for AutoFieldsNotInBaseModelError when converting from pydantic models.
It is raised when strawberry.auto is used, but the pydantic model does not have
the particular field defined.

class User(BaseModel):
    age: int


@strawberry.experimental.pydantic.type(User)
class UserType:
    age: strawberry.auto
    password: strawberry.auto # oops

Previously no errors would be raised, and the password field would not appear on graphql schema.

Such mistakes could be common during refactoring. E.g. if you rename age to user_age on the basemodel.
but you forget to change age on the UserType. Then the age field will disappear on the graphql schema
Now, AutoFieldsNotInBaseModelError is raised.

(maybe u want to warn instead of raising error? not sure)

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).

@@ -446,7 +446,7 @@ class BranchAType(BaseType):
class BranchBType(BaseType):
pass

@strawberry.experimental.pydantic.type(User, fields=["age", "interface_field"])
@strawberry.experimental.pydantic.type(User, fields=["interface_field"])
class UserType:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mistakes caught in our tests :D

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #1551 (a3e1379) into main (eac1fe1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1551   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files         130      130           
  Lines        4447     4461   +14     
  Branches      755      758    +3     
=======================================
+ Hits         4363     4377   +14     
  Misses         44       44           
  Partials       40       40           

@botberry
Copy link
Member

botberry commented Jan 7, 2022

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release checks for AutoFieldsNotInBaseModelError when converting from pydantic models.
It is raised when strawberry.auto is used, but the pydantic model does not have
the particular field defined.

class User(BaseModel):
    age: int

@strawberry.experimental.pydantic.type(User)
class UserType:
    age: strawberry.auto
    password: strawberry.auto

Previously no errors would be raised, and the password field would not appear on graphql schema.
Such mistakes could be common during refactoring. Now, AutoFieldsNotInBaseModelError is raised.


Here's the preview release card for twitter:

Here's the tweet text:

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

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

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 great! I've only added a minor comment 😊

RELEASE.MD Outdated Show resolved Hide resolved
RELEASE.MD Outdated Show resolved Hide resolved
strawberry/experimental/pydantic/utils.py Outdated Show resolved Hide resolved


def test_cannot_convert_pydantic_type_to_strawberry_property_auto():
# auto inferring type of a property is not supported
Copy link
Member

Choose a reason for hiding this comment

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

should this be supported in future? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would be nice, though idk how complicated it would be. i think we could isinstance(method, property) to check if its a property. and then we could extract the return type from the code.

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.

<3

@patrick91 patrick91 merged commit 1f81418 into strawberry-graphql:main Jan 7, 2022
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.

3 participants