strawberry-graphql / strawberry-django

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

Django LazyObject is not considered in mutation resolvers #333

Closed ryanprobus closed 3 months ago

ryanprobus commented 1 year ago

Describe the Bug

The user from Django's request.user is set to a LazyObject in the AuthenticationMiddleware. I believe a LazyObject is always an Iterable even if the underlying object isn't because LazyObject implements iter. When passing the request.user to mutations resolvers.update, it will think its an Iterable and call list(instance). However, this causes an error because the user object isn't actually an iterable.

This could be resolved by doing a check at the beginning of the resolvers similar to how its done in django:

if isinstance(instance, LazyObject):
  instance = instance.__reduce__()[1][0]

Though for me to fix this, I just passed user._wrapped to the resolver.

This would also affect the delete mutation resolver as well.

However, I believe this exact scenario is the only case where this would happen. I didn't see references to LazyObject wrapping models except for this case. So since its obscure, maybe its not worth considering? Just creating this issue to track what I ran into and see if y'all have any thoughts.

Upvote & Fund

Fund with Polar

bellini666 commented 1 year ago

Hey @ryanprobus,

Interesting... I don't think I have any use case for those in my projects, so I've never stumbled upon this.

I think it is fine for us to add an if isinstance(instance, LazyObject) there and in other places that makes sense. Do you want to try to open a PR for that? I'll gladly review it :)

bellini666 commented 3 months ago

Fixed by https://github.com/strawberry-graphql/strawberry-django/pull/338