From ed9069449f858965a87f163dcb84058227dcd914 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 16 Sep 2026 10:22:14 -0700 Subject: [PATCH] handle cascade delete --- netbox/circuits/models/circuits.py | 35 +++++++++++++--------- netbox/circuits/tests/test_models.py | 45 ++++++++++++++++++++++++++-- netbox/netbox/models/deletion.py | 14 ++++++++- 3 files changed, 77 insertions(+), 17 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 6c095de9b..6fbbce8a5 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -241,13 +241,14 @@ class CircuitGroupAssignment(CustomFieldsMixin, ExportTemplatesMixin, TagsMixin, return reverse('circuits:circuitgroupassignment', args=[self.pk]) -def _set_circuit_termination(circuit, field_name, value): +def _set_circuit_terminations(circuit, fields): """ - Set or clear a Circuit's cached `termination_a`/`termination_z` field, recording the change. + Set or clear a Circuit's cached `termination_a`/`termination_z` fields, recording the change. """ circuit.snapshot() - setattr(circuit, field_name, value) - circuit.save(update_fields=[field_name, 'last_updated']) + for field_name, value in fields.items(): + setattr(circuit, field_name, value) + circuit.save(update_fields=[*fields, 'last_updated']) class CircuitTermination( @@ -416,28 +417,34 @@ class CircuitTermination( pk=self._orig_circuit_id, **{old_termination_name: self.pk} ).first() if circuit is not None: - _set_circuit_termination(circuit, old_termination_name, None) + _set_circuit_terminations(circuit, {old_termination_name: None}) # Update the cache if this is a new termination or circuit/term_side changed if is_new or circuit_changed or term_side_changed: # Update the new circuit's termination reference termination_name = f'termination_{self.term_side.lower()}' - _set_circuit_termination(Circuit.objects.get(pk=self.circuit_id), termination_name, self) + _set_circuit_terminations(Circuit.objects.get(pk=self.circuit_id), {termination_name: self}) # Update cached values for subsequent saves self._orig_circuit_id = self.circuit_id self._orig_term_side = self.term_side - def delete(self, *args, **kwargs): - # Clear the circuit's reference here; on_delete=SET_NULL is not change-logged - termination_name = f'termination_{self.term_side.lower()}' - circuit = Circuit.objects.filter(pk=self.circuit_id, **{termination_name: self.pk}).first() - if circuit is not None: - _set_circuit_termination(circuit, termination_name, None) + @classmethod + def clear_cached_references(cls, instances, collector): + # Called by CustomCollector ahead of the DELETE, for explicit and cascaded deletions alike + pks = {instance.pk for instance in instances} + doomed_circuits = {circuit.pk for circuit in collector.data.get(Circuit, ())} - return super().delete(*args, **kwargs) + circuits = Circuit.objects.filter( + models.Q(termination_a__in=pks) | models.Q(termination_z__in=pks) + ).exclude(pk__in=doomed_circuits) - delete.alters_data = True + for circuit in circuits: + _set_circuit_terminations(circuit, { + name: None + for name in ('termination_a', 'termination_z') + if getattr(circuit, f'{name}_id') in pks + }) def cache_related_objects(self): self._provider_network = self._region = self._site_group = self._site = self._location = None diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 2320b9f8a..4399d9032 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -507,8 +507,9 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertIsNone(changes[0].postchange_data['termination_a']) @tag('regression') # Ref: #23134 - def test_deletion_leaves_another_terminations_pointer_alone(self): - # A pointer referencing a different termination must never be cleared + def test_deletion_resolves_the_pointer_from_the_database(self): + # The pointer cleared is the one which references this termination, not the one named by + # a stale in-memory term_side termination_a = self._tracked(lambda: CircuitTermination.objects.create( circuit=self.circuits[0], term_side='A', termination=self.sites[0], )) @@ -516,12 +517,52 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): circuit=self.circuits[0], term_side='Z', termination=self.sites[1], )) ObjectChange.objects.all().delete() + termination_z_pk = termination_z.pk termination_z.term_side = 'A' self._tracked(termination_z.delete) self.circuits[0].refresh_from_db() self.assertEqual(self.circuits[0].termination_a_id, termination_a.pk) + self.assertIsNone(self.circuits[0].termination_z_id) + + changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes[0].prechange_data['termination_z'], termination_z_pk) + self.assertIsNone(changes[0].postchange_data['termination_z']) + self.assertEqual(changes[0].postchange_data['termination_a'], termination_a.pk) + + @tag('regression') # Ref: #23134 + def test_cascade_deletion_records_circuit_update(self): + # Deleting the terminating Site reaches the termination through the collector, which does + # not call delete(). Both sides go in one record. + termination_a = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + termination_z = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='Z', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + termination_a_pk, termination_z_pk = termination_a.pk, termination_z.pk + + self._tracked(self.sites[0].delete) + + self.circuits[0].refresh_from_db() + self.assertIsNone(self.circuits[0].termination_a_id) + self.assertIsNone(self.circuits[0].termination_z_id) + + changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes[0].prechange_data['termination_a'], termination_a_pk) + self.assertEqual(changes[0].prechange_data['termination_z'], termination_z_pk) + self.assertIsNone(changes[0].postchange_data['termination_a']) + self.assertIsNone(changes[0].postchange_data['termination_z']) + + # The clear must precede the DELETEs which the cascade emits for the terminations + termination_delete = self._termination_change( + termination_a_pk, ObjectChangeActionChoices.ACTION_DELETE + ) + self.assertLess(changes[0].pk, termination_delete.pk) def test_circuit_deletion_records_no_pointer_update(self): self._tracked(lambda: CircuitTermination.objects.create( diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index b0600e9e2..3df671d5e 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -1,7 +1,7 @@ import logging from django.contrib.contenttypes.fields import GenericRelation -from django.db import router +from django.db import router, transaction from django.db.models.deletion import CASCADE, Collector from django.utils.translation import gettext as _ @@ -126,6 +126,18 @@ class CustomCollector(Collector): # Add the model that the generic relation points to as a dependency self.add_dependency(field.related_model, instance, reverse_dependency=True) + def delete(self): + # Clear any cached references to the objects being deleted first, so that each clear is + # recorded and precedes the DELETE. Django nulls a SET_NULL column with a bulk UPDATE, + # which emits no post_save and so is never change-logged. Models opt in by defining + # clear_cached_references(); cascaded objects reach this the same as explicit deletions. + with transaction.atomic(using=self.using): + for model, instances in self.data.items(): + if clear_references := getattr(model, 'clear_cached_references', None): + clear_references(instances, self) + + return super().delete() + class DeleteMixin: """