strawberry icon indicating copy to clipboard operation
strawberry copied to clipboard

Pydantic: Allow inheritance without additional auto class variables

Open movabo opened this issue 3 years ago • 2 comments

Problem

Currently, when trying to inherit from a class which was wrapped by strawberry.experimental.pydantic.type or -.input without any additional class variables with the strawberry.auto-annotation, a strawberry.experimental.pydantic.exceptions.MissingFieldsListError arises.

The reason is the check here:

if not fields_set:
    raise MissingFieldsListError(cls)

Example

example.py:

import strawberry
from strawberry import auto

from pydantic import BaseModel


class NoteModel(BaseModel):
    confidential: str  # do not expose via graphql
    headline: str
    text: str


@strawberry.experimental.pydantic.input(NoteModel)
class NoteInput:
    headline: auto
    text: auto


@strawberry.experimental.pydantic.type(NoteModel)
class Note(NoteInput):
    pass

raises

$ python3.10 example.py

Traceback (most recent call last):
  File "example.py", line 21, in <module>
    class Note(NoteInput):
  File "/venv/lib/python3.10/site-packages/strawberry/experimental/pydantic/object_type.py", line 113, in wrap
    raise MissingFieldsListError(cls)
strawberry.experimental.pydantic.exceptions.MissingFieldsListError: List of fields to copy from <class '__main__.Note'> is empty. Add fields with the `auto` type annotation

Why is...

.. all_fields=True not sufficient?

  • If there are variables which you do not want to expose, see NoteModel.confidential in the example above.

.. empty inheritance desirable?

  • To allow types and inputs with same variables.

.. non-empty inheritance without an auto-attribute desirable?

  • To allow nested inputs and types where the types of the nested objects also differ. E.g.
@strawberry.experimental.pydantic.input(ImageModel, all_fields=True)
class ImageInput:
    pass

@strawberry.experimental.pydantic.input(ImageModel, all_fields=True)
class Image:
    pass

@strawberry.experimental.pydantic.input(NoteModel)
class NoteInput:
    headline: auto
    text: auto
    images: List[ImageInput]


@strawberry.experimental.pydantic.type(NoteModel)
class Note(NoteInput):
    images: List[Image]

Suggestions

My suggestion would be to completely remove the lines 106 and 107. https://github.com/strawberry-graphql/strawberry/blob/main/strawberry/experimental/pydantic/object_type.py#L106-L107

This would solve above problems. Also, a conversion from pydantic to strawberry and vice-versa without the strawberry.auto-annotation is desirable to add the to_pydantic and from_pydantic-methods to a class.

If completely removing is not an option, a check whether any attributes are inherited might be an option, e.g.:

- if not fields_set:
+ inherited_fields = set(
+    name
+    for base in cls.__bases__
+    for name in getattr(base, "__annotations__", {}).keys()
+    if name not in existing_fields
+ )
+ if not fields_set and not inherited_fields:

I would love to open a PR for one of the above mentioned suggestions if you tell me which one you would prefer. :)

movabo avatar Jan 01 '22 20:01 movabo

The behavior we're trying to avoid with that check is someone accidentally leaving the class with no fields at all. If there are inherited fields, I'd think we'd want to find those per your second suggestion, but that's just my thoughts on it

Matt343 avatar Feb 01 '22 14:02 Matt343

This could probably solve another problem:

Test which fails:

def test_basic_type_without_fields_not_throws_an_error():
    class User(pydantic.BaseModel):
        age: int


    @strawberry.experimental.pydantic.type(User)
    class UserType:
        @strawberry.field
        def age(self) -> int:
            return 10

    definition: TypeDefinition = UserType._type_definition
    assert definition.name == "UserType"

    field1 = definition.fields[0]

    assert field1.python_name == "age"
    assert field1.type is int

benzolium avatar Jul 01 '22 08:07 benzolium