diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 2b194e4c3..6fbbce8a5 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -241,6 +241,16 @@ class CircuitGroupAssignment(CustomFieldsMixin, ExportTemplatesMixin, TagsMixin, return reverse('circuits:circuitgroupassignment', args=[self.pk]) +def _set_circuit_terminations(circuit, fields): + """ + Set or clear a Circuit's cached `termination_a`/`termination_z` fields, recording the change. + """ + circuit.snapshot() + for field_name, value in fields.items(): + setattr(circuit, field_name, value) + circuit.save(update_fields=[*fields, 'last_updated']) + + class CircuitTermination( CustomFieldsMixin, CustomLinksMixin, @@ -403,18 +413,39 @@ class CircuitTermination( # Clear the old termination reference if circuit or term_side changed if circuit_changed or term_side_changed: old_termination_name = f'termination_{self._orig_term_side.lower()}' - Circuit.objects.filter(pk=self._orig_circuit_id).update(**{old_termination_name: None}) + circuit = Circuit.objects.filter( + pk=self._orig_circuit_id, **{old_termination_name: self.pk} + ).first() + if circuit is not 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()}' - Circuit.objects.filter(pk=self.circuit_id).update(**{termination_name: self.pk}) + _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 + @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, ())} + + circuits = Circuit.objects.filter( + models.Q(termination_a__in=pks) | models.Q(termination_z__in=pks) + ).exclude(pk__in=doomed_circuits) + + 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 if self.termination_type: diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 2f8c88242..4399d9032 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -1,9 +1,15 @@ +import uuid + from django.contrib.contenttypes.models import ContentType from django.core.exceptions import NON_FIELD_ERRORS, ValidationError -from django.test import TestCase +from django.test import RequestFactory, TestCase, tag from circuits.models import Circuit, CircuitTermination, CircuitType, Provider, ProviderNetwork +from core.choices import ObjectChangeActionChoices +from core.models import ObjectChange from dcim.models import Location, Region, Site, SiteGroup +from netbox.context_managers import event_tracking +from users.models import User class CircuitTerminationTestCase(TestCase): @@ -270,3 +276,300 @@ class CircuitTerminationDenormalizationTriggerTestCase(TestCase): termination.refresh_from_db() self.assertEqual(termination._region, region_b) + + +class CircuitTerminationChangeLoggingTestCase(TestCase): + """ + Circuit.termination_a/termination_z are maintained by CircuitTermination.save(). Writing them + with a queryset update() emitted no post_save, and so no ObjectChange. (#23134) + """ + @classmethod + def setUpTestData(cls): + cls.user = User.objects.create_user(username='testuser', password='pw') + + provider = Provider.objects.create(name='Provider 1', slug='provider-1') + circuit_type = CircuitType.objects.create(name='Circuit Type 1', slug='circuit-type-1') + + cls.sites = ( + Site.objects.create(name='Site 1', slug='site-1'), + Site.objects.create(name='Site 2', slug='site-2'), + ) + cls.circuits = ( + Circuit.objects.create(cid='Circuit 1', provider=provider, type=circuit_type), + Circuit.objects.create(cid='Circuit 2', provider=provider, type=circuit_type), + ) + + def _tracked(self, func): + request = RequestFactory().get('/') + request.id = uuid.uuid4() + request.user = self.user + with event_tracking(request): + return func() + + def _termination_change(self, termination_pk, action): + return ObjectChange.objects.get( + changed_object_type=ContentType.objects.get_for_model(CircuitTermination), + changed_object_id=termination_pk, + action=action, + ) + + def _circuit_changes(self, circuit): + return ObjectChange.objects.filter( + changed_object_type=ContentType.objects.get_for_model(Circuit), + changed_object_id=circuit.pk, + action=ObjectChangeActionChoices.ACTION_UPDATE, + ).order_by('pk') + + @tag('regression') # Ref: #23134 + def test_creation_records_circuit_update(self): + termination = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + + changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(changes.count(), 1) + self.assertIsNone(changes[0].prechange_data['termination_a']) + self.assertEqual(changes[0].postchange_data['termination_a'], termination.pk) + + # The pointer references the termination's PK, so the create must be recorded first + termination_create = self._termination_change( + termination.pk, ObjectChangeActionChoices.ACTION_CREATE + ) + self.assertLess(termination_create.pk, changes[0].pk) + + @tag('regression') # Ref: #23134 + def test_second_termination_snapshots_current_state(self): + # The A pointer is already committed when the Z termination is created + termination_a = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + + termination_z = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='Z', termination=self.sites[1], + )) + + changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes[0].prechange_data['termination_a'], termination_a.pk) + self.assertIsNone(changes[0].prechange_data['termination_z']) + self.assertEqual(changes[0].postchange_data['termination_a'], termination_a.pk) + self.assertEqual(changes[0].postchange_data['termination_z'], termination_z.pk) + + @tag('regression') # Ref: #23134 + def test_circuit_change_records_both_circuits(self): + termination = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + + def _move(): + termination.circuit = self.circuits[1] + termination.save() + + self._tracked(_move) + + # The old circuit's pointer is cleared + old_changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(old_changes.count(), 1) + self.assertEqual(old_changes[0].prechange_data['termination_a'], termination.pk) + self.assertIsNone(old_changes[0].postchange_data['termination_a']) + + # The new circuit's pointer is set + new_changes = self._circuit_changes(self.circuits[1]) + self.assertEqual(new_changes.count(), 1) + self.assertIsNone(new_changes[0].prechange_data['termination_a']) + self.assertEqual(new_changes[0].postchange_data['termination_a'], termination.pk) + + @tag('regression') # Ref: #23134 + def test_term_side_change_records_circuit_updates(self): + termination = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + + def _flip(): + termination.term_side = 'Z' + termination.save() + + self._tracked(_flip) + + # The old pointer is cleared, then the new one is set + changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(changes.count(), 2) + self.assertEqual(changes[0].prechange_data['termination_a'], termination.pk) + self.assertIsNone(changes[0].postchange_data['termination_a']) + self.assertIsNone(changes[1].prechange_data['termination_z']) + self.assertEqual(changes[1].postchange_data['termination_z'], termination.pk) + + @tag('regression') # Ref: #23134 + def test_creation_leaves_another_terminations_pointer_alone(self): + # A pointer referencing a different termination must never be cleared + termination_a = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + + def _create(): + termination = CircuitTermination( + circuit=self.circuits[0], term_side='A', termination=self.sites[1], + ) + termination.term_side = 'Z' + termination.save() + return termination + + termination_z = self._tracked(_create) + + self.circuits[0].refresh_from_db() + self.assertEqual(self.circuits[0].termination_a_id, termination_a.pk) + self.assertEqual(self.circuits[0].termination_z_id, termination_z.pk) + + changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes[0].postchange_data['termination_a'], termination_a.pk) + + @tag('regression') # Ref: #23134 + def test_noop_resave_records_no_circuit_update(self): + termination = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + + self._tracked(termination.save) + + self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) + + @tag('regression') # Ref: #23134 + def test_circuit_change_via_update_fields_records_circuit_update(self): + # save(update_fields=...) takes its own branch when deciding what is being persisted + termination = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + + def _move(): + termination.circuit = self.circuits[1] + termination.save(update_fields=('circuit',)) + + self._tracked(_move) + + old_changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(old_changes.count(), 1) + self.assertIsNone(old_changes[0].postchange_data['termination_a']) + + new_changes = self._circuit_changes(self.circuits[1]) + self.assertEqual(new_changes.count(), 1) + self.assertEqual(new_changes[0].postchange_data['termination_a'], termination.pk) + + @tag('regression') # Ref: #23134 + def test_deletion_records_circuit_update(self): + 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(termination.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.assertIsNone(changes[0].postchange_data['termination_a']) + + # The pointer clear must precede the DELETE, so that a consumer replaying in reverse + # restores the termination before the record which references it + termination_delete = self._termination_change( + termination_pk, ObjectChangeActionChoices.ACTION_DELETE + ) + self.assertLess(changes[0].pk, termination_delete.pk) + + @tag('regression') # Ref: #23134 + def test_bulk_deletion_records_circuit_update(self): + # 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 + + 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']) + + @tag('regression') # Ref: #23134 + 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], + )) + termination_z = self._tracked(lambda: CircuitTermination.objects.create( + 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( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + + self._tracked(self.circuits[0].delete) + + self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) 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: """