diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index 86ac2bfd7..e87321ae3 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -79,7 +79,9 @@ class DeleteMixin: ) ) - collector = CustomCollector(using=using) + # Pass origin=self (matching Django's Model.delete) so signal receivers can tell that + # cascaded child objects are being deleted as part of deleting this object. + collector = CustomCollector(using=using, origin=self) collector.collect([self], keep_parents=keep_parents) return collector.delete() diff --git a/netbox/utilities/counters.py b/netbox/utilities/counters.py index 16b66a57d..877b27572 100644 --- a/netbox/utilities/counters.py +++ b/netbox/utilities/counters.py @@ -1,5 +1,5 @@ from django.apps import apps -from django.db.models import Count, F, OuterRef, Subquery +from django.db.models import Count, F, OuterRef, QuerySet, Subquery from django.db.models.signals import post_delete, post_save, pre_delete from netbox.registry import registry @@ -63,22 +63,54 @@ def post_save_receiver(sender, instance, created, **kwargs): update_counter(parent_model, new_pk, counter_name, 1) +def _parent_is_being_deleted(origin, parent_model, parent_pk): + """ + Return True if `origin` (the object or queryset that `delete()` was called on) indicates that + the parent identified by (parent_model, parent_pk) is itself being deleted as part of the same + operation. In that case, decrementing its counter is wasted work: the parent row is going away, + so the UPDATE would be a no-op. Skipping it avoids an N+1 storm of pointless UPDATEs when a + parent with many tracked children is deleted (e.g. a Device with thousands of Interfaces). + """ + if origin is None: + return False + if isinstance(origin, QuerySet): + # A bulk delete; every collected child belongs to an object in this queryset by construction + return origin.model is parent_model + # A single object delete + return isinstance(origin, parent_model) and origin.pk == parent_pk + + def pre_delete_receiver(sender, instance, origin, **kwargs): - model = instance._meta.model - if not model.objects.filter(pk=instance.pk).exists(): - instance._previously_removed = True + """ + Before a tracked object is deleted, check whether its row has already been removed (e.g. by an + earlier cascade) and, if so, flag it so post_delete_receiver skips the now-redundant counter + update. The existence check is skipped when the tracked parent is itself being deleted, since + the counter update would be skipped regardless — this avoids a SELECT per cascaded child. + """ + for field_name, counter_name in get_counters_for_model(sender): + parent_model = sender._meta.get_field(field_name).related_model + parent_pk = getattr(instance, field_name, None) + if parent_pk is None or _parent_is_being_deleted(origin, parent_model, parent_pk): + continue + # A tracked parent will survive this operation, so the double-delete guard is needed + if not sender.objects.filter(pk=instance.pk).exists(): + instance._previously_removed = True + return def post_delete_receiver(sender, instance, origin, **kwargs): """ Update counter fields on related objects when a TrackingModelMixin subclass is deleted. """ + if hasattr(instance, '_previously_removed'): + return + for field_name, counter_name in get_counters_for_model(sender): parent_model = sender._meta.get_field(field_name).related_model parent_pk = getattr(instance, field_name, None) - # Decrement the parent's counter by one - if parent_pk is not None and not hasattr(instance, '_previously_removed'): + # Decrement the parent's counter by one, unless the parent is itself being deleted + if parent_pk is not None and not _parent_is_being_deleted(origin, parent_model, parent_pk): update_counter(parent_model, parent_pk, counter_name, -1) diff --git a/netbox/utilities/tests/test_counters.py b/netbox/utilities/tests/test_counters.py index 15dc54a26..d1c211965 100644 --- a/netbox/utilities/tests/test_counters.py +++ b/netbox/utilities/tests/test_counters.py @@ -1,3 +1,5 @@ +from django.db import connection +from django.test.utils import CaptureQueriesContext from django.urls import reverse from dcim.models import * @@ -64,6 +66,33 @@ class CountersTestCase(TestCase): self.assertEqual(device1.interface_count, 1) self.assertEqual(device2.interface_count, 1) + def test_counter_skipped_when_parent_deleted(self): + """ + Deleting a parent object should not issue a counter update for each cascaded child on that + same parent (the row is being removed, so the UPDATE is a no-op). Counters on surviving + related objects must still be updated. + """ + device1 = Device.objects.get(name='Device 1') + device_type = device1.device_type + self.assertEqual(device_type.device_count, 2) + + with CaptureQueriesContext(connection) as ctx: + device1.delete() + + # No counter UPDATE should target the interface counter of the Device being deleted + interface_counter_updates = [ + q['sql'] for q in ctx.captured_queries + if q['sql'].upper().startswith('UPDATE') and '_interface_count' in q['sql'] + ] + self.assertEqual( + interface_counter_updates, [], + "Deleting a Device must not update its own interface counter for each cascaded Interface" + ) + + # The counter on the surviving parent (DeviceType) must still be decremented + device_type.refresh_from_db() + self.assertEqual(device_type.device_count, 1) + def test_interface_count_move(self): """ When a tracked object (Interface) is moved, the tracking counter should be updated.