Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33369 closed New feature (wontfix)

Add through_defaults as argument for m2m_changed signal

Reported by: ShmuelTreiger Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: m2m_changed
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Use case:

In English:
I have many policies, each of which has a status from a table of statuses and is not unique to that policy. I associate these policies to my many domains through the many_to_many table. Domains can have many policies, but only one of each type of policy. Likewise, the same policy can apply to multiple domains. (In practice, there are a few thousand domains and a dozen or so policies.) In the following code, I have successfully implemented this relationship at the database level.

In pseudocode:

Domain(models.Model)
    <fields>

PolicyStatus(models.Model):
    status = models.CharField(<>)

Policy(models.Model)
    status = models.ForeignKey(PolicyStatus, <>)
    domains = models.ManyToManyField(Domain, through="Domain_Policy_m2m", <>)
    <fields>

Domain_Policy_m2m(models.Model):
    class Meta:
        constraints = [
            models.UniqueConstraint(
                fields=["domain", "status", ],
                name="unique_constraint"
            )
        ]
    domain = models.ForeignKey(Domain, <>)
    policy = models.ForeignKey(Policy, <>)
    status = models.ForeignKey(PolicyStatus, <>)

Scenario:
I go to associate a new policy with a domain:
domain.policy_set.add(policy, through_defaults={"status": policy.status})

However, the domain already has a policy with this status and raises IntegrityError. Instead, I would like to replace the existing policy with the new one. I can do this in my code:

        new_policy = Policy(<>)
        existing_policy = domain.policy_set.filter(status=new_policy.status).first()
        if existing_policy:
            domain.policy_set.remove(existing_policy)
        domain.policy_set.add(policy, through_defaults={"status": policy.status})

Instead, I wanted to take care of this in a more permanent way, so I created a m2m_change signal receive:

from <> import Policy

@receiver(m2m_changed, sender=Domain_Policy_m2m)
def delete_old_m2m(action, instance, pk_set, **kwargs):
    if action == "pre_add":

        pk = min(pk_set)
        policy = Policy.objects.get(pk=pk)
        status = policy.status
        existing_policy = Domain_Policy_m2m.objects.filter(status=status, domain=instance)

        if existing_policy.exists():
            existing_policy.delete()

This works, but it's not pretty. It would be excellent if I could have the through_defaults as an argument, so I could do something like:

@receiver(m2m_changed, sender=Domain_Policy_m2m)
def delete_old_m2m(action, instance, through_defaults, **kwargs):
    if action == "pre_add":

        status = through_defaults["status"]
        existing_policy = Domain_Policy_m2m.objects.filter(status=status, domain=instance)

        if existing_policy.exists():
            existing_policy.delete()

Even the object(s) being added would be so much more convenient than the pk:

@receiver(m2m_changed, sender=Domain_Policy_m2m)
def delete_old_m2m(action, instance, model_set, **kwargs):
    if action == "pre_add":

        model = min(model_set)
        existing_policy = Domain_Policy_m2m.objects.filter(status=model.status, domain=instance)

        if existing_policy.exists():
            existing_policy.delete()

Change History (6)

comment:1 by ShmuelTreiger, 3 years ago

Version: 4.0dev

comment:2 by Mariusz Felisiak, 3 years ago

Resolution: invalid
Status: newclosed

Thanks for the report , however the m2m_changed signal was created for automatically created intermediate models. If you have an explicit through model, you can attached Model signals to it, i.e. pre_save, post_save, pre_delete, and post_delete. Personally, I would use add this logic to the Domain_Policy_m2m.save() and don't use signals at all, this is not a situation where signals are needed. If have further questions, see TicketClosingReasons/UseSupportChannels for ways to get help.

comment:3 by ShmuelTreiger, 3 years ago

Resolution: invalid
Status: closednew

domain.policy_set.add(<>) does not call save() on the m2m model. From the documentation here https://docs.djangoproject.com/en/4.0/ref/models/relations/#django.db.models.fields.related.RelatedManager.add

Using add() with a many-to-many relationship, however, will not call any save() methods (the bulk argument doesn’t exist), but rather create the relationships using QuerySet.bulk_create(). If you need to execute some custom logic when a relationship is created, listen to the m2m_changed signal, which will trigger pre_add and post_add actions.

I tried save, it didn't work, so I found the documentation which told me to use m2m_changed.

I suppose instead of add, I could do Domain_Policy_m2m.objects.create(<>) but that's just seems less than ideal.

comment:4 by Mariusz Felisiak, 3 years ago

Resolution: wontfix
Status: newclosed

Please don't reopen closed ticket. You have a custom intermediate models, so in your case .create() or .save() seem ideal to handle this logic. As you already described there are at least two ways for your use case. We don't want to add a third way. Moreover, signals are required in really rare cases and it's definitely not one of them.

comment:5 by ShmuelTreiger, 3 years ago

I'm literally doing what your documentation tells me to do, and it's shitty, hacky, and horrible. Your response is "Won't fix". Excellent.

comment:6 by Mariusz Felisiak, 3 years ago

You can start a discussion on DevelopersMailingList if you don't agree, where you'll reach a wider audience and see what other think, and follow the guidelines with regards to requesting features.

Also, please don't use offensive language, and be aware that Code of Conduct applies to the Trac.

Note: See TracTickets for help on using tickets.
Back to Top