Deleting associated model instances from within model.save() not working

Question

I'm having a problem that's somewhat difficult to explain, so please bear with me. First of all, here are the relevant versions in case this ends up mattering: Django 2.0.3, Python 3.6.4, PostgreSQL 10.3.

Essentially, I have this structure for a few of my Django models:

class Capability(models.Model):
    relationships = models.ManyToManyField('self',
                                           symmetrical=False,
                                           through='CapabilityRelationship',
                                           blank=True)
    name = models.CharField(max_length=255)


class CapabilityRelationship(models.Model):
    from_capability = models.ForeignKey(Capability,
                                        on_delete=models.CASCADE,
                                        related_name='from_set',
                                        blank=True,
                                        null=True)
    to_capability = models.ForeignKey(Capability,
                                      on_delete=models.CASCADE,
                                      related_name='to_set')
    number = models.PositiveSmallIntegerField(validators=[MinValueValidator(1)])

    def _get_complete_numbers(self):
        # generate the complete numbers using depth-first search

    def save(self, *args, **kwargs):
        # Get the complete numbers before saving. If we're not able to generate the complete numbers, something is
        # wrong, an error will be raised, and we don't want to save.
        complete_numbers = self._get_complete_numbers()

        super().save(*args, **kwargs)  # Call the "real" save() method.

        # Delete all old complete numbers.
        self.capabilityrelationshipcompletenumber_set.all().delete()

        # Save the new complete numbers.
        for complete_number in complete_numbers:
            CapabilityRelationshipCompleteNumber.objects.create(capability_relationship=self,
                                                                complete_number=complete_number)


class CapabilityRelationshipCompleteNumber(models.Model):
    capability_relationship = models.ForeignKey(CapabilityRelationship, on_delete=models.CASCADE)
    complete_number = models.CharField(max_length=255, unique=True)

To describe these models in words, I have a Capability model, which has a many-to-many relationship with itself (captured in CapabilityRelationship). In practice, this will be a "tree" where each node can have multiple children and multiple parents (i.e., it's a directed acyclic graph). Finally, each relationship instance can have multiple "complete numbers" (captured in CapabilityRelationshipCompleteNumber).

The idea behind "numbers" and "complete numbers" is essentially the Dewey decimal system. A complete number of 1.2.3.4 would have 4 levels of Capability objects, where 1 is the top-most level (i.e., root node) and 4 is the leaf. Because the structure I laid out above is a DAG and not actually a tree, a node can actually have multiple of these "complete" numbers since there can be multiple paths from any given node to its root.

If this description doesn't make since, please let me know, and I can mock something up in Paint.

I'm overriding the CapabilityRelationship.save() method because I need to recompute the complete numbers each time the relationship is changed, since number could've changed. So what I want to do is simply compute the new complete numbers, delete all of the old complete numbers, and then save the new ones.

The problem I'm encountering is that I simply cannot delete the old complete numbers, and it's baffling to me. I'm wondering if there's something about overriding CapabilityRelationship.save() that I'm simply not getting. For example:

def save(self, *args, **kwargs):
    complete_numbers = self._get_complete_numbers()

    print('complete numbers: {}'.format(complete_numbers))

    super().save(*args, **kwargs)  # Call the "real" save() method.

    print('before delete: {}'.format(self.capabilityrelationshipcompletenumber_set.all()))
    self.capabilityrelationshipcompletenumber_set.all().delete()
    print('after delete: {}'.format(self.capabilityrelationshipcompletenumber_set.all()))

    for complete_number in complete_numbers:
        CapabilityRelationshipCompleteNumber.objects.create(capability_relationship=self,
                                                            complete_number=complete_number)

    print('after save: {}'.format(self.capabilityrelationshipcompletenumber_set.all()))

If I visit the admin site and set a leaf node's number to 1, save it, and then modify it to 2, I get this output:

complete numbers: {'1.2.1.1', '2.2.1.1', '1.3.1.1', '1.1.1.1'}
before delete: <QuerySet []>
after delete: <QuerySet []>
after save: <QuerySet [<CapabilityRelationshipCompleteNumber: 1.1.1.1>, <CapabilityRelationshipCompleteNumber: 1.2.1.1>, <CapabilityRelationshipCompleteNumber: 1.3.1.1>, <CapabilityRelationshipCompleteNumber: 2.2.1.1>]>
complete numbers: {'1.1.1.2', '1.2.1.2', '2.2.1.2', '1.3.1.2'}
before delete: <QuerySet [<CapabilityRelationshipCompleteNumber: 1.1.1.1>, <CapabilityRelationshipCompleteNumber: 1.2.1.1>, <CapabilityRelationshipCompleteNumber: 1.3.1.1>, <CapabilityRelationshipCompleteNumber: 2.2.1.1>]>
after delete: <QuerySet []>
after save: <QuerySet [<CapabilityRelationshipCompleteNumber: 1.1.1.2>, <CapabilityRelationshipCompleteNumber: 1.2.1.2>, <CapabilityRelationshipCompleteNumber: 1.3.1.2>, <CapabilityRelationshipCompleteNumber: 2.2.1.2>]>

Now, all of this looks great, but when I visit the admin site to get the listing of all of the complete numbers, I see all 8 of the complete numbers computed so far, not the 4 that are currently correct after changing number to 2. If I open up a Python shell and list out the complete numbers, I see all of them created so far:

> ./manage.py shell
Python 3.6.4 (default, Dec 19 2017, 15:24:51)
[GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from src.apps.api.models.capability import *
>>> CapabilityRelationshipCompleteNumber.objects.all()
<QuerySet [<CapabilityRelationshipCompleteNumber: 1.1.1.1>, <CapabilityRelationshipCompleteNumber: 1.1.1.2>, <CapabilityRelationshipCompleteNumber: 1.2.1.1>, <CapabilityRelationshipCompleteNumber: 1.2.1.2>, <CapabilityRelationshipCompleteNumber: 1.3.1.1>, <CapabilityRelationshipCompleteNumber: 1.3.1.2>, <CapabilityRelationshipCompleteNumber: 2.2.1.1>, <CapabilityRelationshipCompleteNumber: 2.2.1.2>]>

I see the same thing if I look directly at the DB using psql.

Clearly, for whatever reason, the delete call isn't actually happening. I've tried CapabilityRelationshipCompleteNumber.objects.filter(capability_relationship=self).delete(), CapabilityRelationshipCompleteNumber.objects.all().delete(), and issuing a raw SQL DELETE FROM api_capabilityrelationshipcompletenumber; using connection.cursor(). Nothing seems to work at all. I don't understand what's going on. I've read the Django documentation on deleting a QuerySet and overriding save(), but I don't see anything that could help me diagnose my problem.

Does anyone know what's going on here? Any help is greatly appreciated. Please let me know if I can clarify any of this.

That is odd. Are you sure you don't have any pre/post-save signal handlers that are manipulating `CapabilityRelationshipCompleteNumber` objects whenever a `CapabilityRelationship` is saved? From what you've posted it looks as though some other piece of code is running to re-insert these back into the DB. — solarissmoke, Apr 02 '18 at 05:52
Just something to try, could you do the same with a post_save signal and see what happens? — Ramkishore M, Apr 02 '18 at 06:29
instead of overriding save method you should write signals `post_save` if you want to perform action after save; and if you want to perform any action before save then use model's `clean` method — Gahan, Apr 02 '18 at 07:16
@solarissmoke It isn't a good practice to perform action after save by overriding save method and for that we have [signals](https://docs.djangoproject.com/en/2.0/topics/signals/) to use. It's like circular dependency you may never realize you have done wrong, debugging will be quite tough. — Gahan, Apr 02 '18 at 10:11
@Gahan I don't see how circular dependencies come into it. Signals are one approach especially if you need to connect to models from another app, but there is really nothing wrong with overriding `save()` in this way either - both approaches are valid and have their merits. I don't think you can dismiss one as being bad practise. — solarissmoke, Apr 02 '18 at 10:14
@solarissmoke if you use signal you can easily detach functionality from existance but in overriding method you need to test for all cases whether it breaks the current system or not, if you are creating tests then you might not face any such problem but if not then it's better to write it in signal because it will keep model more generic. — Gahan, Apr 02 '18 at 10:18
@Gahan, what do you mean it isn't good practice? The Django docs give examples of it [here](https://docs.djangoproject.com/en/2.0/topics/db/models/#overriding-predefined-model-methods) and never mention that it's bad practice. I can try it in a `post_save` signal, though, just to see if that works. I haven't tried that yet. — Geoff, Apr 02 '18 at 13:07
@solarissmoke, correct, I have zero pre/post-save signal handlers in my entire code base. — Geoff, Apr 02 '18 at 13:08
@Geoff, I tried your code with sqlite database, entirely in django shell (no admin interface), and found no issue at all (`>>> models.CapabilityRelationshipCompleteNumber.objects.all() , , , ]> `). So the issue may be in other code or with postgres. — Hai Lang, Apr 02 '18 at 13:37
@HaiLang, thanks for the information. I'm starting to wonder if this is a PostgreSQL issue (possibly with the library I'm using to connect to the DB). — Geoff, Apr 02 '18 at 15:26
Ok, just tried something different. I have a `__str__(self)` method defined in `Capability`. I put `CapabilityRelationshipCompleteNumber.objects.all().delete()` in that method, and it *does* work there. That tells me there's something specific about `save()` that I'm not understanding. — Geoff, Apr 02 '18 at 16:14
Just created a `post_save` signal that simply calls `CapabilityRelationshipCompleteNumber.objects.all().delete()`. I can verify it's working, but it's doing the same thing as when I override `save()`; that is, if I output `CapabilityRelationshipCompleteNumber.objects.all()` before and after calling `delete()`, it appears that it's empty, but it's not actually deleting anything. — Geoff, Apr 02 '18 at 16:50
Ok, I'm starting to think that this is due to the fact that [M2M relationships are first cleared and then saved after the primary object](https://stackoverflow.com/a/1925784/1269634). — Geoff, Apr 02 '18 at 22:28

Geoff · Accepted Answer · 2018-04-03T14:29:07.583

Ok, I figured it out after debugging and digging around on the internet all day. As far as I can tell, the problem seems to be with how Django saves ManyToMany relationships. As discussed at https://stackoverflow.com/a/1925784/1269634:

When you save a model via admin forms it's not an atomic transaction. The main object gets saved first (to make sure it has a PK), then the M2M is cleared and the new values set to whatever came out of the form. So if you are in the save() of the main object you are in a window of opportunity where the M2M hasn't been updated yet. In fact, if you try to do something to the M2M, the change will get wiped out by the clear(). I ran into this about a year ago.

Although this post initially dates back to 2009, this annoyance is apparently still present today with Django 2.0. After reading this, I believe that this is what's causing the weirdness. Unfortunately, as far as I can tell, this is not documented anywhere in the Django docs, which is why it took so long to track down and fix.

The fix for this issue turned out to be relatively simple. I effectively renamed CapabilityRelationship.save() to CapabilityRelationship.update_complete_numbers(). Then, I modified the Capability admin implementation to override ModelAdmin.save_related(). The admin implementation now looks like this:

@admin.register(Capability)
class CapabilityAdmin(nested_admin.NestedModelAdmin):
    list_display = ['name']
    search_fields = ['name']
    inlines = [CapabilityRelationshipInlineAdmin]

    def save_related(self, request, form, formsets, change):
        super().save_related(request, form, formsets, change)

        for capability_relationship in form.instance.to_set.all():
            capability_relationship.update_complete_numbers()

This works! The magic is that updating the complete numbers happens after the model save is completely finished (which is true when super().save_related(request, form, formsets, change) returns).

Thanks to everyone for their help! I hope this can help someone else eventually.

Deleting associated model instances from within model.save() not working

1 Answers1