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..989f5251d 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,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) diff --git a/netbox/utilities/tests/test_counters.py b/netbox/utilities/tests/test_counters.py index 15dc54a26..5c1ed0ba5 100644 --- a/netbox/utilities/tests/test_counters.py +++ b/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.