diff --git a/netbox/circuits/apps.py b/netbox/circuits/apps.py index 1cc517700..d1d4fe9e7 100644 --- a/netbox/circuits/apps.py +++ b/netbox/circuits/apps.py @@ -1,20 +1,4 @@ from django.apps import AppConfig -from django.db.models.signals import pre_delete - - -def _clear_circuit_termination_pointer(sender, **kwargs): - from .models import CircuitTermination - from .signals import clear_circuit_termination_pointer - - if sender is CircuitTermination: - clear_circuit_termination_pointer(**kwargs) - - -# This module is imported in populate() phase 1, ahead of the models phase which connects -# core.signals.handle_deleted_object. Connecting here records the Circuit pointer clear before the -# termination's own DELETE; branch revert replays newest-first and needs the termination restored -# before the pointer referencing it. (#23134) -pre_delete.connect(_clear_circuit_termination_pointer) class CircuitsConfig(AppConfig): diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 2cbb68543..e5e0847dc 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -421,9 +421,9 @@ class CircuitTermination( termination_name = f'termination_{self.term_side.lower()}' updates.setdefault(self.circuit_id, {})[termination_name] = self.pk - # Ordered by PK so concurrent saves take the circuit locks in the same order. The - # delete path is unordered (see circuits.signals), so a bulk delete racing a save - # can still deadlock. + # Ordered by PK so concurrent saves take the circuit locks in the same order. + # delete() locks in queryset order, so a bulk delete under an enclosing transaction + # racing a save can still deadlock. for circuit_id in sorted(updates): self._set_circuit_terminations(circuit_id, updates[circuit_id], using=using) @@ -481,6 +481,31 @@ class CircuitTermination( # because the Circuit was just re-fetched circuit.save(using=using, update_fields=[*fields, 'last_updated']) + def delete(self, *args, **kwargs): + # Clear the parent Circuit's cached pointer before the deletion starts, so that its change + # record precedes this row's DELETE. on_delete=SET_NULL clears the column with a bulk + # UPDATE, and related_name='+' hides the relation from Circuit._meta.related_objects, so + # neither path records an ObjectChange. (#23134) + # + # Not a pre_delete receiver: core.signals.handle_deleted_object connects during the models + # import phase, ahead of any app's ready(), and Django dispatches in connection order, so a + # receiver here would run only after the DELETE had been recorded. + # + # Cascades (e.g. deleting the terminating Site, or the Circuit itself) reach the row through + # the collector rather than here, and remain unrecorded. + using = kwargs.get('using') or router.db_for_write(type(self)) + with transaction.atomic(using=using): + if self.term_side: + self._set_circuit_terminations( + self.circuit_id, + {f'termination_{self.term_side.lower()}': None}, + using=using, + only_if_references=self.pk, + ) + return super().delete(*args, **kwargs) + + delete.alters_data = True + def cache_related_objects(self): self._provider_network = self._region = self._site_group = self._site = self._location = None if self.termination_type: diff --git a/netbox/circuits/signals.py b/netbox/circuits/signals.py index c9d9609fb..4db765c80 100644 --- a/netbox/circuits/signals.py +++ b/netbox/circuits/signals.py @@ -3,7 +3,7 @@ from django.dispatch import receiver from dcim.signals import rebuild_paths -from .models import Circuit, CircuitTermination +from .models import CircuitTermination @receiver((post_save, post_delete), sender=CircuitTermination) @@ -15,26 +15,3 @@ def rebuild_cablepaths(instance, raw=False, **kwargs): peer_termination = instance.get_peer_termination() if peer_termination: rebuild_paths([peer_termination]) - - -def clear_circuit_termination_pointer(instance, using=None, origin=None, **kwargs): - """ - Clear the parent Circuit's cached `termination_a`/`termination_z` pointer with a change-logged - save. on_delete=SET_NULL clears it via a bulk UPDATE, and related_name='+' hides the relation - from Circuit._meta.related_objects, so neither path records an ObjectChange. (#23134) - - Connected in CircuitsConfig, not here, so that it precedes handle_deleted_object. - """ - if not instance.term_side: - return - - # The pointer goes away with the circuit, so a change record for it would be spurious - if isinstance(origin, Circuit) or getattr(origin, 'model', None) is Circuit: - return - - # only_if_references matches what on_delete=SET_NULL would have cleared: the in-memory - # term_side may not be what the pointer actually references - field_name = f'termination_{instance.term_side.lower()}' - CircuitTermination._set_circuit_terminations( - instance.circuit_id, {field_name: None}, using=using, only_if_references=instance.pk - ) diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 162ddde3e..d439207a4 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -491,36 +491,37 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): @tag('regression') # Ref: #23134 def test_bulk_deletion_records_circuit_update(self): - # A queryset delete() passes the queryset as the signal's origin rather than an instance + # NetBox's bulk delete views iterate obj.delete() rather than calling queryset.delete() termination = self._tracked(lambda: CircuitTermination.objects.create( circuit=self.circuits[0], term_side='A', termination=self.sites[0], )) ObjectChange.objects.all().delete() termination_pk = termination.pk - self._tracked(CircuitTermination.objects.filter(pk=termination_pk).delete) + def _bulk_delete(): + for obj in CircuitTermination.objects.filter(pk=termination_pk): + obj.delete() + + self._tracked(_bulk_delete) changes = self._circuit_changes(self.circuits[0]) self.assertEqual(changes.count(), 1) self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk) self.assertIsNone(changes[0].postchange_data['termination_a']) - def test_cascade_from_termination_parent_records_circuit_update(self): - # The circuit survives the cascade, so the pointer clear still has to be recorded - termination = self._tracked(lambda: CircuitTermination.objects.create( + def test_cascade_deletion_leaves_pointer_unrecorded(self): + # Deleting the terminating Site reaches the termination through the collector, which does + # not call delete(). on_delete=SET_NULL still clears the column, but nothing records it. + self._tracked(lambda: CircuitTermination.objects.create( circuit=self.circuits[0], term_side='A', termination=self.sites[0], )) ObjectChange.objects.all().delete() - termination_pk = termination.pk self._tracked(self.sites[0].delete) self.circuits[0].refresh_from_db() self.assertIsNone(self.circuits[0].termination_a_id) - - changes = self._circuit_changes(self.circuits[0]) - self.assertEqual(changes.count(), 1) - self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk) + self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) def test_deletion_leaves_pointer_for_another_termination(self): # An in-memory term_side which diverges from the persisted one must not clear a pointer