Fixes #23134: Fix changelog record for circuit termination (#23135)

netbox/circuits/models/circuits.py

@@ -1,3 +1,5 @@
+from itertools import batched
+
 from django.apps import apps
 from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation
 from django.core.exceptions import ValidationError
@@ -241,6 +243,23 @@ class CircuitGroupAssignment(CustomFieldsMixin, ExportTemplatesMixin, TagsMixin,
         return reverse('circuits:circuitgroupassignment', args=[self.pk])
 
 
+def _set_circuit_terminations(circuit, fields, using):
+    """
+    Set or clear a Circuit's cached `termination_a`/`termination_z` fields, recording the change.
+    """
+    from extras.models import CustomField
+
+    circuit.snapshot()
+    for field_name, value in fields.items():
+        setattr(circuit, field_name, value)
+
+    # Write custom_field_data only if save() will populate a default, which the change log serializes
+    update_fields = [*fields, 'last_updated']
+    if CustomField.objects.get_defaults_for_model(circuit).keys() - circuit.custom_field_data.keys():
+        update_fields.append('custom_field_data')
+    circuit.save(using=using, update_fields=update_fields)
+
+
 class CircuitTermination(
     CustomFieldsMixin,
     CustomLinksMixin,
@@ -403,18 +422,44 @@ 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.using(self._state.db).filter(
+                pk=self._orig_circuit_id, **{old_termination_name: self.pk}
+            ).prefetch_related('tags').first()
+            if circuit is not None:
+                _set_circuit_terminations(circuit, {old_termination_name: None}, using=self._state.db)
 
         # 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})
+            circuit = Circuit.objects.using(self._state.db).prefetch_related('tags').get(pk=self.circuit_id)
+            _set_circuit_terminations(circuit, {termination_name: self}, using=self._state.db)
 
             # 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
+        doomed_circuits = {circuit.pk for circuit in collector.data.get(Circuit, ())}
+
+        # A circuit only points at its own terminations, so any deleted along with it need no clear
+        pks = {instance.pk for instance in instances if instance.circuit_id not in doomed_circuits}
+        circuit_ids = {instance.circuit_id for instance in instances if instance.pk in pks}
+
+        for batch in batched(circuit_ids, 1000):
+            # Read and write on the deletion's alias, inside its transaction
+            circuits = Circuit.objects.using(collector.using).filter(pk__in=batch).prefetch_related('tags')
+            for circuit in circuits:
+                fields = {
+                    name: None
+                    for name in ('termination_a', 'termination_z')
+                    if getattr(circuit, f'{name}_id') in pks
+                }
+                if fields:
+                    _set_circuit_terminations(circuit, fields, using=collector.using)
+
     def cache_related_objects(self):
         self._provider_network = self._region = self._site_group = self._site = self._location = None
         if self.termination_type:

netbox/circuits/tests/test_models.py

@@ -1,9 +1,18 @@
+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 circuits.models.circuits import _set_circuit_terminations
+from core.choices import ObjectChangeActionChoices
+from core.models import ObjectChange
 from dcim.models import Location, Region, Site, SiteGroup
+from extras.choices import CustomFieldStatusChoices, CustomFieldTypeChoices
+from extras.models import CustomField
+from netbox.context_managers import event_tracking
+from users.models import User
 
 
 class CircuitTerminationTestCase(TestCase):
@@ -270,3 +279,349 @@ 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())
+
+    @tag('regression')  # Ref: #23134
+    def test_pointer_update_persists_populated_custom_field_defaults(self):
+        # The pointer is written with update_fields, but CustomFieldsMixin.save() populates defaults
+        # into custom_field_data, which the change log serializes. Both must reach the database.
+        custom_field = CustomField.objects.create(
+            name='probe_field',
+            type=CustomFieldTypeChoices.TYPE_TEXT,
+            default='default-value',
+            status=CustomFieldStatusChoices.STATUS_ACTIVE,
+        )
+        custom_field.object_types.set([ContentType.objects.get_for_model(Circuit)])
+        CustomField.objects.clear_cache()
+        Circuit.objects.filter(pk=self.circuits[0].pk).update(custom_field_data={})
+        ObjectChange.objects.all().delete()
+
+        self._tracked(lambda: CircuitTermination.objects.create(
+            circuit=self.circuits[0], term_side='A', termination=self.sites[0],
+        ))
+
+        self.circuits[0].refresh_from_db()
+        self.assertEqual(self.circuits[0].custom_field_data, {'probe_field': 'default-value'})
+
+        changes = self._circuit_changes(self.circuits[0])
+        self.assertEqual(changes.count(), 1)
+        self.assertEqual(
+            changes[0].postchange_data['custom_fields'], self.circuits[0].custom_field_data
+        )
+
+    @tag('regression')  # Ref: #23134
+    def test_pointer_update_leaves_complete_custom_field_data_alone(self):
+        # With no default to populate, the pointer write must not overwrite a concurrent edit
+        custom_field = CustomField.objects.create(
+            name='probe_field',
+            type=CustomFieldTypeChoices.TYPE_TEXT,
+            default='default-value',
+            status=CustomFieldStatusChoices.STATUS_ACTIVE,
+        )
+        custom_field.object_types.set([ContentType.objects.get_for_model(Circuit)])
+        CustomField.objects.clear_cache()
+        CircuitTermination.objects.create(circuit=self.circuits[0], term_side='A', termination=self.sites[0])
+        circuit = Circuit.objects.get(pk=self.circuits[0].pk)
+        Circuit.objects.filter(pk=circuit.pk).update(custom_field_data={'probe_field': 'concurrent'})
+
+        _set_circuit_terminations(circuit, {'termination_a': None}, using='default')
+
+        circuit.refresh_from_db()
+        self.assertIsNone(circuit.termination_a_id)
+        self.assertEqual(circuit.custom_field_data, {'probe_field': 'concurrent'})

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,19 @@ 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(); it covers cascaded objects as well as directly deleted ones,
+        # but only on this collector -- a queryset delete uses Django's and bypasses it.
+        with transaction.atomic(using=self.using, savepoint=False):
+            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:
     """