strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
MIT License
394 stars 115 forks source link

How to write custom model resolvers that pass Python typing? #256

Open humphrey opened 1 year ago

humphrey commented 1 year ago

When I write a custom resolver for a strawberry_django type, I need to specify the graphql type as the resolver's return type. However, since my resolvers needs to actually return an instance of the model, I get python typing errors.

Is there a way that I can get Python typing to be happy, or is there a way we can update strawberry_django to handle this scenario?

See the following code as an example. It works as I desire, but my Python type checker gives the following complaint about the last line of code.

Expected type 'UserPreferencesType', got 'UserPreferences' instead 
import strawberry
import strawberry_django
from strawberry import auto
from strawberry.types import Info

from userprefs.models import UserPreferences

@strawberry_django.type(UserPreferences, name='UserPreferences')
class UserPreferencesType:
    date_format: auto

def resolve_user_preferences(info: Info) -> UserPreferencesType:
    user = info.context.request.user
    if user.is_authenticated:
        if prefs := UserPreferences.objects.filter(user__id=user.id).first():
            return prefs

    # Return an unsaved model instance if user has not customised preferences yet (or is not logged in)
    # as that model will contain the default preference values.
    return UserPreferences()

Upvote & Fund

Fund with Polar

bellini666 commented 1 year ago

Hi @humphrey ,

That is indeed a problem without a solution atm. What I do in my projects is to cast the return value (i.e. return cast(UserPreferencesType, UserPreferences()) in your case).

The fact is that strawberry really expects UserPreferencesType to be returned, but is able to handle a django object because the type acts as a protocol for the model, and a getattr(model, field) will work for all fields defined in the type as long as the fields without a custom resolver are defined in the model.

We discussed that internally but still haven't find a proper fix/workaround. Maybe PEP 712 will help with that

humphrey commented 1 year ago

Thanks!

That is indeed a problem without a solution atm. What I do in my projects is to cast the return value (i.e. return cast(UserPreferencesType, UserPreferences()) in your case).

Yeah that makes sense. As my number of migrations is starting to scale upwards, I just wanted to check that there wasn't something obvious that I'd missed!

The fact is that strawberry really expects UserPreferencesType to be returned, but is able to handle a django object because the type acts as a protocol for the model, and a getattr(model, field) will work for all fields defined in the type as long as the fields without a custom resolver are defined in the model.

Ah, I had wondered how that worked behind the scenes.

I assume that explains why "self" is actually the Django model instance in the following code. I've been working around this by specifying the model as the type. PyCharm is ok with the following code, but mypy doesn't like it.

@strawberry_django.type(User, name='User')
class UserType:
    id: int
    email: auto

    @strawberry.field
    def full_name(self: ApiUser) -> str:
        return self.get_full_name() or self.username

We discussed that internally but still haven't find a proper fix/workaround. Maybe PEP 712 will help with that

Yeah, I expect there isn't an easy/obvious solution 🤷‍♂️ I also write a lot of TypeScript, and Python type hinting seems quite immature in comparison.

I'm just taking a stab in the dark, but might be there a solution that involves strawberry_django.type() defining a new type that could be used as the return type that both strawberry understands and satisfies the type checker?

UserPreferencesType.ReturnType = UserPreferencesType | UserPreferences
def resolve_user_preferences(info: Info) -> UserPreferencesType.ReturnType: ...

But I'm definitely happy with either using cast() or ignoring the errors for now.

bellini666 commented 1 year ago

I assume that explains why "self" is actually the Django model instance in the following code. I've been working around this by specifying the model as the type. PyCharm is ok with the following code, but mypy doesn't like it.

You can ignore self and use root, that's what I do :)

@strawberry_django.type(User, name='User')
class UserType:
    id: int
    email: auto

    @strawberry.field
    def full_name(self, root: ApiUser) -> str:
        return root.get_full_name() or root.username

self and root will be the same object, but you can type root without making mypy/pyright unhappy

I'm just taking a stab in the dark, but might be there a solution that involves strawberry_django.type() defining a new type that could be used as the return type that both strawberry understands and satisfies the type checker?

The type checker would be satisfied, but since strawberry uses that for the schema, it would be an error for it (specially because a model is not a graphql type)

I'll try soon to add support for the converter on strawberry and use it to "fool" mypy/pyright into thinking that there's a converter in there to satisfy the type without needing to cast. I would suggest you doing the cast for now, and as soon as this is supported (and hopefully, you will not need to change anything at your side), they will say that the cast is unneeded and you can remove them.

humphrey commented 1 year ago

You can ignore self and use root, that's what I do :)

Oh brilliant!!

I'll try soon to add support for the converter on strawberry and use it to "fool" mypy/pyright into thinking that there's a converter in there to satisfy the type without needing to cast. I would suggest you doing the cast for now, and as soon as this is supported (and hopefully, you will not need to change anything at your side), they will say that the cast is unneeded and you can remove them.

Sounds awesome :) Thanks. I just read up a bit of those and it looks pretty powerful, and that Django might get better typing support in the future also 👌

hyusetiawan commented 10 months ago

one more typing issue i came across while manually instantiating the types, the typing requires filling in all the possible properties. For ex:


@strawberry_django.type(models.User, name="User")
class TypeUser:
    gid: str
    first_name: auto
    last_name: auto
    email: auto

    chats: list[
        Annotated["ChatObject", strawberry.lazy(".chat")]
    ] = strawberry_django.field(field_name="chat_set", default_factory=lambda: [])

instantiating TypeUser, I need to fill all of that, including chats, if not, it will throw an error. Now, I am not sure if this is the right way to do it but, I added default_factory=lambda: [] on chats property, is there a better way? Also generally, is there a better way to describe django relationships here?

Screenshot 2023-08-28 at 1 00 00 PM
bellini666 commented 10 months ago

instantiating TypeUser, I need to fill all of that, including chats, if not, it will throw an error. Now, I am not sure if this is the right way to do it but, I added default_factory=lambda: [] on chats property, is there a better way? Also generally, is there a better way to describe django relationships here?

The main thing here is that when using a @strawberry_django.type, you are mostly using it as an interface and you usually will not instantiate the type itself (although you can).

For curiosity, is there a reason to why you want to instantiate the type directly?

And btw, the way you are defining the django relation there is correct :)

hyusetiawan commented 10 months ago

@bellini666 the specific use case for me is to describe a session object, it's not really derived from a django model but it's more of a computed object where I would pull various objects/scalars and put it under session type.

Is there a more succinct way of creating the relationship? it's a rather mouthful to type it all out :D

bellini666 commented 10 months ago

@bellini666 the specific use case for me is to describe a session object, it's not really derived from a django model but it's more of a computed object where I would pull various objects/scalars and put it under session type.

Is there a more succinct way of creating the relationship? it's a rather mouthful to type it all out :D

For cases when you are not actually using a model, you can use a strawberry type directly :)

hyusetiawan commented 10 months ago

just following up, is there a more succinct way to express the django relationship? Also, perhaps you can point me to the docs? The only example I see is from the README.md

bellini666 commented 10 months ago

just following up, is there a more succinct way to express the django relationship? Also, perhaps you can point me to the docs? The only example I see is from the README.md

The documentation for that is here: https://strawberry-graphql.github.io/strawberry-graphql-django/guide/fields/#relationships

You can also take a look at this demo I created to make a live on discord the other day: https://github.com/bellini666/strawberry-django-demo . I still want to improve it and merge it here in the examples directory.

cpontvieux-systra commented 4 months ago

I use @staticmethod with self: models.MyObject type hint. Mypy is then happy with it. In fact it’s exactly that as the method is not a real one, and is called statically.