strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
409 stars 118 forks source link

Issue with custom field class and type inheritance #414

Closed cpd67 closed 10 months ago

cpd67 commented 10 months ago

Describe the Bug

Hi there 😄

I've created a custom field class that has inherited from StrawberryDjangoField, and i'm using it in a few of my object types. Some of these types have common fields, so I factored those out into a base type and inherited from it. I've noticed that the fields for some of the subtypes becomes StrawberryDjangoField and not my custom field class. Here's some code demonstrating what i'm trying to do:

class CustomStrawberryDjangoField(StrawberryDjangoField):
    # custom things that I need to do...

@strawberry.type
class BaseType:
    field_1: int = CustomStrawberryDjangoField()
    field_2: str = CustomStrawberryDjangoField()
    field_3: bool = strawberry_django.field()

    # I've also created a custom field decorator that acts similar to strawberry.field()
    # so I can have a similar API for attaching resolvers to my custom field
    @custom_field
    def field_4(self, info: Info) -> int:
        return 42

@strawberry_django.type(SomeModel)
class SomeModelType(BaseType):
    field_5: bool

@strawberry_django.type(SomeOtherModel)
class SomeOtherModelType(BaseType):
    field_6: list

@strawberry_django.type(AnotherModel)
class AnotherModelType(BaseType):
    field_7: str

If I look at what field_1, field_2 and field_4 are on SomeModelType's strawberry object definition, they're my CustomStrawberryDjangoField. But if I look at what those fields are on SomeOtherModelType & AnotherModelType's object definitions, they're StrawberryDjangoFields and not my CustomStrawberryDjangoField. I think the issue is in this code block inside of _process_type(), where we loop through the type's fields and process each one:

if isinstance(f, StrawberryDjangoField) and not f.origin_django_type:
    # If the field is a StrawberryDjangoField and it is the first time
    # seeing it, just update its annotations/description/etc
    f.type_annotation = type_annotation
    f.description = description
elif (
    not isinstance(f, StrawberryDjangoField)
    and getattr(f, "base_resolver", None) is not None
):
    # If this is not a StrawberryDjangoField, but has a base_resolver, no need
    # avoid forcing it to be a StrawberryDjangoField
    new_fields.append(f)
    continue
else:
    f = field_cls(  # noqa: PLW2901
        django_name=django_name,
        description=description,
        type_annotation=type_annotation,
        python_name=f.python_name,
        graphql_name=getattr(f, "graphql_name", None),
        origin=getattr(f, "origin", None),
        is_subscription=getattr(f, "is_subscription", False),
        base_resolver=getattr(f, "base_resolver", None),
        permission_classes=getattr(f, "permission_classes", ()),
        default=getattr(f, "default", dataclasses.MISSING),
        default_factory=getattr(f, "default_factory", dataclasses.MISSING),
        deprecation_reason=getattr(f, "deprecation_reason", None),
        directives=getattr(f, "directives", ()),
        pagination=getattr(f, "pagination", UNSET),
        filters=getattr(f, "filters", UNSET),
        order=getattr(f, "order", UNSET),
        extensions=getattr(f, "extensions", ()),
    )

I think i'm hitting the else block when SomeOtherModelType & AnotherModelType are turned into strawberry django types, and StrawberryDjangoFields are created for field_1, field_2 & field_4 (because field_cls is StrawberryDjangoField by default in _process_type()). Is there any way I could make it so that field_1, field_2 & field_4 are CustomStrawberryDjangoField across all subtypes? Maybe _process_type() could be updated to not create a copy of the field if it's one we've already seen and only update django_name, description, and type_annotation on the field?

System Information

Additional Context

I tried accomplishing the same thing using strawberry.interface and the same thing happened. Do I need to use strawberry_django.interface?

Upvote & Fund

Fund with Polar

bellini666 commented 10 months ago

The field_cls that is used for any attribute that doesn't have a = strawberry_or_django_field(...) can be passed to strawberry_django.type like:

@strawberry_django.type(SomeModel, field_cls=CustomStrawberryDjangoField)
class SomeModelType(BaseType):
    field_5: bool

In this example, field_5 will be turned into a CustomStrawberryDjangoField instance

I tried accomplishing the same thing using strawberry.interface and the same thing happened. Do I need to use strawberry_django.interface?

strawberry_django.interface is to strawberry.interface the same as strawberry_django.type is to strawberry.type. It basically adds the extra functionality (like auto type conversion based on the ORM introspection) to it. Note that it calls the same _process_type, it just has some different defaults

cpd67 commented 10 months ago

Thanks for the reply! The issue isn't with field_5, though. The issue is that the inherited field_1, field_2 & field_4 fields from BaseType become StrawberryDjangoFields on SomeModelType & AnotherModelType and not CustomStrawberryDjangoFields. I can make a quick reproduction later today that shows this.

bellini666 commented 10 months ago

@cpd67 oh I see. Well, the suggestion I have would workaround this, but I agree that there is a bug in this as well for field_1, field_2 and field_4.

Will be waiting for the repro :)

cpd67 commented 10 months ago

@bellini666 Thank you :) I have a repro made here. I created a small Django project with strawberry & strawberry-django. The strawberry types and custom fields are located at mysite/myapp/types.py, and I created a custom GraphQLView located at mysite/myapp/views.py that prints out the fields for each of the types to the console whenever you go to localhost:8000/graphql. Here's a screenshot of the output:

image

Notice that the field types for SomeModelType are my custom ones, but then they are StrawberryDjangoFields for the other subtypes.

Please let me know if there's anything else you need from me. I'd be happy to help contribute a fix for this, as well! :smile:

cpd67 commented 10 months ago

@bellini666 I just wanted to let you know that the fix worked :smile: Thanks a ton!

bellini666 commented 10 months ago

@cpd67 Awesome! :)