strawberry-graphql / strawberry-django

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

The m2m_changed signal is not triggered when adding objects #401

Open Mapiarz opened 11 months ago

Mapiarz commented 11 months ago

Hi! I encountered an issue with the m2m_changed built in signal. The signal is triggered only when removing objects from the m2m relation, but not when adding. Imagine I have a Book and Label models. A book can have multiple labels and each label can be used in multiple books, your standard m2m scenario. For my API, I use the built in strawberry_django.mutations.update mutation to update a book, nothing extra. The input node has the standard annotation for m2m: labels: Optional[strawberry_django.ListInput[strawberry.relay.GlobalID]]. So far so good. I also have a signal receiver like this:

@receiver(models.signals.m2m_changed, sender=Book.labels.through)
def do_something(sender, instance, **kwargs):
    action = kwargs["action"]
    if action in ["post_add", "post_remove"]:
        print('foo')

In my frontend, for the ListInput, I'm only ever using set. I start with a Book with no labels. I add the first label and the signal is not triggered. I then remove the label and the signal is indeed triggered.

Describe the Bug

The reason why the signal is not triggered when a label is added is because of the way the through models are created in https://github.com/strawberry-graphql/strawberry-graphql-django/blob/8f82b84141c182980b5001b4f0f562c787fe2dda/strawberry_django/mutations/resolvers.py#L479

I do not fully understand the implications of the above logic or why it's done the way it's done. Why not simply use the managers add method? You can pass the through_defaults to add. I also don't understand when the condition hasattr(manager, "through") would be False as all managers have the through model, even if it's auto generated?

Generally speaking I'm not familiar with Django or Strawberry Django internals and I have hard time understanding the intents here since it's not documented. For this reason I'm not suggesting any particular fix.

As a workaround, I can hook up to the post_save signal of the through model.

System Information

Upvote & Fund

Fund with Polar

bellini666 commented 11 months ago

Hey @Mapiarz ,

TBH, its been awhile since I wrote that code (back on strawberry-django-plus), and it has been adjusted over the past 2 years.

Maybe we could indeed call add and pass through_defaults in this case and, worst case scenario hook up the post_save signal.

Do you want to try to open a PR for this? I can help you through the development as needed :)