diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 2128fc81a..f422d9cd4 100644 --- a/netbox/circuits/models/circuits.py +++ b/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 diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index c319d0c48..8370a5657 100644 --- a/netbox/circuits/tests/test_models.py +++ b/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())