search

strawberry-graphql / strawberry-django

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

`mutations.delete` and `mutations.update` do not support both `filters` _and_ `handle_django_errors` #544

Open diesieben07 opened 3 weeks ago

diesieben07 commented 3 weeks ago

Describe the Bug

When handle_django_errors is turned on (either globally or for an individual mutation) for mutations.delete and mutations.update then filters on those mutations has no effect.

System Information

Additional Context

The problem is here: https://github.com/strawberry-graphql/strawberry-django/blob/90e1878959d83af471fc20d8f4c611ce2e767fa8/strawberry_django/filters.py#L320-L321

When handle_django_errors is turned on, is_list is no longer true, because the return type is now a union with OperationInfo. Additionally, the return type is now no longer even a list.

Upvote & Fund

Fund with Polar

bellini666 commented 3 weeks ago

Hi @diesieben07 ,

Yes, this has not been developed yet.

TBH I'm not sure what should be the proper solution in here, because:

1) GraphQL doesn't allow us to create a union of a type with a list. In the case of update, a list is returned.

An alternative would be to return a type that inside contains the returned list, and union that type with the OperationInfo. But that changes the return value too much to a point where I start to become uncomfortable 😅

2) Another option would be to change list[RetVal] to list[Retval | OperationInfo], which allows for a set of object to succeed and others to fail. But I'm not sure if that is something that the users would want, specially as I don't usually use update/delete with filters (I think it is too risky =P)

What are your thoughts on this?

diesieben07 commented 3 weeks ago

Firstly, for delete: For "delete" I currently use my own version of DjangoDeleteMutation which doesn't return a list, but instead a union of DeleteResult and OperationInfo (I'd like it to just be OperationInfo | None, more on this below). If something is being deleted, it seems strange to return it back to the client. Unfortunately I currently cannot return OperationInfo | None, because DjangoPermissionExtension then prefers returning None instead of OperationInfo in case of a permission error.

For "update" I agree, it is difficult to accomplish. I've hit the problem of GraphQL not allowing unions with lists multiple times, but it makes sense for why it isn't allowed. The best resolution here indeed seems to be a wrapper type, it would be nice if this could be accomplished in some way and still use the logic from DjangoUpdateMutation (I am currently investigating making my own version).

As for why I allow filters for these mutations: These mutations are used for an administrative interface (similar to the Django Admin), which allows bulk deletion. Therefor I have a filter class, which allows filtering on ID using the in_list lookup.