cleanup

netbox/circuits/models/circuits.py

@@ -400,26 +400,35 @@ class CircuitTermination(
 
         super().save(*args, **kwargs)
 
+        # Collect the pointer writes per circuit, so that a term_side change within a single
+        # circuit clears the old side and sets the new one in one write rather than passing
+        # through a state with neither side set
+        updates = {}
+
         # 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()}'
-            self._set_circuit_termination(self._orig_circuit_id, old_termination_name, None)
+            updates.setdefault(self._orig_circuit_id, {})[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()}'
-            self._set_circuit_termination(self.circuit_id, termination_name, self.pk)
+            updates.setdefault(self.circuit_id, {})[termination_name] = self.pk
 
             # Update cached values for subsequent saves
             self._orig_circuit_id = self.circuit_id
             self._orig_term_side = self.term_side
 
+        for circuit_id, fields in updates.items():
+            self._set_circuit_terminations(circuit_id, fields)
+
     @staticmethod
-    def _set_circuit_termination(circuit_id, field_name, value):
+    def _set_circuit_terminations(circuit_id, fields):
         """
-        Point a Circuit's cached `termination_a`/`termination_z` field at the given
-        CircuitTermination PK, or clear it.
+        Point a Circuit's cached `termination_a`/`termination_z` fields at the given
+        CircuitTermination PKs, or clear them. `fields` maps field name to PK (or None); both
+        sides are passed together where they change at once, to keep it to a single write.
 
         This is written via snapshot() + save() rather than a queryset update() so that the write
         passes through post_save and is recorded in the changelog. A raw update() emits no signal,
@@ -430,15 +439,27 @@ class CircuitTermination(
         and Z terminations in sequence does not snapshot a Circuit loaded before the A pointer was
         written. That only holds within a single sequential flow: under READ COMMITTED, concurrent
         writers can each snapshot a Circuit which does not yet reflect the other's uncommitted
-        write. The row itself is safe, as update_fields limits each write to one column.
+        write. The row itself is safe, as update_fields limits the write to the pointer fields
+        this termination owns (plus last_updated, which is last-writer-wins).
         """
         circuit = Circuit.objects.filter(pk=circuit_id).first()
-        if circuit is None or getattr(circuit, f'{field_name}_id') == value:
+        if circuit is None:
+            return
+
+        # Skip fields which already hold the intended value, so that a redundant write neither
+        # touches the row nor dispatches an event
+        fields = {
+            field_name: value
+            for field_name, value in fields.items()
+            if getattr(circuit, f'{field_name}_id') != value
+        }
+        if not fields:
             return
 
         circuit.snapshot()
-        setattr(circuit, f'{field_name}_id', value)
-        circuit.save(update_fields=[field_name, 'last_updated'])
+        for field_name, value in fields.items():
+            setattr(circuit, f'{field_name}_id', value)
+        circuit.save(update_fields=[*fields, 'last_updated'])
 
     def cache_related_objects(self):
         self._provider_network = self._region = self._site_group = self._site = self._location = None

netbox/circuits/tests/test_models.py

@@ -383,14 +383,17 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
 
         self._tracked(_flip)
 
-        # Both pointers move within one circuit, so the clear and the set are recorded separately.
+        # Both pointers move within one circuit, so the clear and the set are coalesced into one
+        # write rather than passing through a state with neither side set.
         changes = self._circuit_changes(self.circuits[0])
-        self.assertEqual(changes.count(), 2)
+        self.assertEqual(changes.count(), 1)
+        self.assertEqual(changes[0].prechange_data['termination_a'], termination.pk)
+        self.assertIsNone(changes[0].prechange_data['termination_z'])
         self.assertIsNone(changes[0].postchange_data['termination_a'])
-        self.assertEqual(changes[1].postchange_data['termination_z'], termination.pk)
+        self.assertEqual(changes[0].postchange_data['termination_z'], termination.pk)
 
     @tag('regression')  # Ref: #23134
-    def test_pointer_already_set_records_no_circuit_update(self):
+    def test_redundant_pointer_write_is_skipped(self):
         # bulk_create() bypasses save(), so the circuit's pointer is never written. Moving the
         # termination afterwards reaches the clear path with the pointer already null.
         CircuitTermination.objects.bulk_create([
@@ -417,6 +420,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
         self.assertEqual(new_changes.count(), 1)
         self.assertEqual(new_changes[0].postchange_data['termination_a'], termination.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],
@@ -426,3 +430,44 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
         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 whether the circuit or
+        # term_side is being persisted; the pointer writes must be recorded there too.
+        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)
+
+    def test_deletion_clears_pointer_without_recording_a_change(self):
+        # Deleting a termination clears the pointer via on_delete=SET_NULL, which emits no
+        # post_save and so is not change-logged. handle_deleted_object() does not cover it
+        # either: termination_a/termination_z declare related_name='+', so they are hidden
+        # relations and absent from CircuitTermination._meta.related_objects. The changelog is
+        # therefore still asymmetric here; documented rather than fixed, as replaying the
+        # termination's DELETE re-applies SET_NULL on the target side.
+        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.delete)
+
+        self.circuits[0].refresh_from_db()
+        self.assertIsNone(self.circuits[0].termination_a_id)
+        self.assertFalse(self._circuit_changes(self.circuits[0]).exists())