Merge pull request #22763 from netbox-community/22497-delete

22497 Avoid redundant counter updates when deleting an object's parent

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()

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,59 @@ 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).
+
+    Note: only the *direct* parent is detected, since `origin` is just the top-level object/queryset
+    delete() was called on. In a deeper cascade (DeviceType -> Device -> Interface) `origin` stays
+    the DeviceType, so intermediate Devices' interface counters still get the (harmless) no-op
+    UPDATE. Suppressing that would require the full deletion set, which the signals don't expose.
+    """
+    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)
 
 

netbox/utilities/tests/test_counters.py

@@ -1,7 +1,9 @@
+from unittest.mock import patch
+
 from django.urls import reverse
 
 from dcim.models import *
-from utilities.counters import connect_counters
+from utilities.counters import connect_counters, update_counter
 from utilities.testing.base import TestCase
 from utilities.testing.utils import create_test_device
 
@@ -64,6 +66,63 @@ 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)
+
+        # The Device must have tracked children for the suppression to be meaningful; otherwise the
+        # assertions below would pass trivially with nothing to suppress
+        self.assertEqual(device1.interfaces.count(), 2)
+
+        # Wrap update_counter so the real counter logic still runs while we record each call
+        with patch('utilities.counters.update_counter', wraps=update_counter) as mock_update:
+            device1.delete()
+
+        # The Device's own interface counter must not be updated per cascaded Interface, since the
+        # Device is itself being deleted
+        counter_names = [call.args[2] for call in mock_update.call_args_list]
+        self.assertNotIn('interface_count', counter_names)
+
+        # The counter on the surviving parent (DeviceType) must still be decremented
+        self.assertIn('device_count', counter_names)
+        device_type.refresh_from_db()
+        self.assertEqual(device_type.device_count, 1)
+
+        # Exactly one update should fire (DeviceType.device_count). Without the optimization the two
+        # cascaded Interfaces on Device 1 would each have triggered an interface_count update.
+        self.assertEqual(mock_update.call_count, 1)
+
+    def test_counter_skipped_when_parent_deleted_via_queryset(self):
+        """
+        A bulk QuerySet delete (e.g. Device.objects.filter(...).delete(), as used by scripts,
+        plugins, and programmatic callers) sets `origin` to the QuerySet rather than a single
+        object. Counter updates for children whose parent belongs to that QuerySet must be
+        suppressed, while counters on surviving related objects are still updated.
+        """
+        device1 = Device.objects.get(name='Device 1')
+        device_type = device1.device_type
+        self.assertEqual(device_type.device_count, 2)
+
+        # The Device must have tracked children for the suppression to be meaningful
+        self.assertEqual(device1.interfaces.count(), 2)
+
+        # Wrap update_counter so the real counter logic still runs while we record each call
+        with patch('utilities.counters.update_counter', wraps=update_counter) as mock_update:
+            Device.objects.filter(name='Device 1').delete()
+
+        # The deleted Device's interface_count must not be decremented per cascaded Interface, and
+        # the only update should be the surviving DeviceType's device_count
+        counter_names = [call.args[2] for call in mock_update.call_args_list]
+        self.assertEqual(counter_names, ['device_count'])
+        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.