From 594d9a12767b66b178aad3690ae1a9e4ce4fad29 Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 4 Sep 2026 11:57:00 -0700 Subject: [PATCH] cleanup --- netbox/circuits/models/circuits.py | 68 ++++++++++++++------------- netbox/circuits/tests/test_models.py | 70 ++++++++++++++++++---------- 2 files changed, 81 insertions(+), 57 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index f422d9cd4..ceb73d04b 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -1,7 +1,7 @@ from django.apps import apps from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation from django.core.exceptions import ValidationError -from django.db import models +from django.db import models, router, transaction from django.urls import reverse from django.utils.translation import gettext_lazy as _ @@ -394,15 +394,13 @@ class CircuitTermination( circuit_changed = tracking_relevant and self._orig_circuit_id and self._orig_circuit_id != self.circuit_id term_side_changed = tracking_relevant and self._orig_term_side and self._orig_term_side != self.term_side + pointer_moved = is_new or circuit_changed or term_side_changed # Cache objects associated with the terminating object (for filtering) self.cache_related_objects() - 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 + # Collect the pointer writes per circuit, so that a term_side change within one + # circuit clears the old side and sets the new one in a single write updates = {} # Clear the old termination reference if circuit or term_side changed @@ -410,57 +408,61 @@ class CircuitTermination( old_termination_name = f'termination_{self._orig_term_side.lower()}' 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()}' - updates.setdefault(self.circuit_id, {})[termination_name] = self.pk + # Write the termination row and the pointers which reference it together + with transaction.atomic(using=router.db_for_write(type(self))): + super().save(*args, **kwargs) - # Update cached values for subsequent saves - self._orig_circuit_id = self.circuit_id - self._orig_term_side = self.term_side + # Update the cache if this is a new termination or circuit/term_side changed + if pointer_moved: + # Update the new circuit's termination reference + termination_name = f'termination_{self.term_side.lower()}' + updates.setdefault(self.circuit_id, {})[termination_name] = self.pk - for circuit_id, fields in updates.items(): - self._set_circuit_terminations(circuit_id, fields) + for circuit_id, fields in updates.items(): + circuit = self._set_circuit_terminations(circuit_id, fields) + # Keep the instance's cached Circuit in step with the write + if circuit is not None and circuit.pk == self.circuit_id: + self.circuit = circuit + + # Update cached values for subsequent saves, only once the pointer writes have + # succeeded, so that a failed save is still pending on retry + if pointer_moved: + self._orig_circuit_id = self.circuit_id + self._orig_term_side = self.term_side @staticmethod def _set_circuit_terminations(circuit_id, fields): """ - 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. + Set or clear a Circuit's cached `termination_a`/`termination_z` fields. `fields` maps + field name to CircuitTermination PK (or None). - 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, - so consumers which replay ObjectChange records have no record of the write and silently - drop the association. + Written via snapshot() + save() rather than a queryset update(), which emits no post_save + and so records nothing in the changelog. The Circuit is re-fetched so that sequential A/Z + saves snapshot current state; concurrent writers under READ COMMITTED still cannot see + each other's uncommitted writes. - The Circuit is re-fetched rather than reusing a cached `self.circuit` so that saving the A - 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 the write to the pointer fields - this termination owns (plus last_updated, which is last-writer-wins). + Returns the Circuit, or None if no such row exists. """ circuit = Circuit.objects.filter(pk=circuit_id).first() if circuit is None: - return + return None - # Skip fields which already hold the intended value, so that a redundant write neither - # touches the row nor dispatches an event + # Skip fields which already hold the intended value fields = { field_name: value for field_name, value in fields.items() if getattr(circuit, f'{field_name}_id') != value } if not fields: - return + return circuit circuit.snapshot() for field_name, value in fields.items(): setattr(circuit, f'{field_name}_id', value) circuit.save(update_fields=[*fields, 'last_updated']) + return circuit + 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 8370a5657..9bb6827c3 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -1,4 +1,5 @@ import uuid +from unittest.mock import patch from django.contrib.contenttypes.models import ContentType from django.core.exceptions import NON_FIELD_ERRORS, ValidationError @@ -280,9 +281,8 @@ class CircuitTerminationDenormalizationTriggerTestCase(TestCase): class CircuitTerminationChangeLoggingTestCase(TestCase): """ - The Circuit.termination_a/termination_z pointers are maintained by CircuitTermination.save(). - They were previously written with a queryset update(), which emits no post_save and therefore - no ObjectChange, so consumers which replay the changelog never saw the association. (#23134) + 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): @@ -327,8 +327,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): @tag('regression') # Ref: #23134 def test_second_termination_snapshots_current_state(self): - # The A pointer is already committed when the Z termination is created; its prechange - # snapshot must reflect that rather than a Circuit cached before the A write. + # 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], )) @@ -358,13 +357,13 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self._tracked(_move) - # The old circuit's pointer is cleared... + # 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']) - # ...and the new circuit's pointer is set. + # 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']) @@ -383,8 +382,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self._tracked(_flip) - # 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. + # Both pointers move within one circuit, so the clear and the set are coalesced changes = self._circuit_changes(self.circuits[0]) self.assertEqual(changes.count(), 1) self.assertEqual(changes[0].prechange_data['termination_a'], termination.pk) @@ -394,8 +392,8 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): @tag('regression') # Ref: #23134 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. + # bulk_create() bypasses save(), leaving the pointer unwritten; moving the termination + # afterwards reaches the clear path with it already null CircuitTermination.objects.bulk_create([ CircuitTermination(circuit=self.circuits[0], term_side='A', termination=self.sites[0]), ]) @@ -409,13 +407,13 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self._tracked(_move) - # The old circuit's pointer was already null, so it is not written to at all... + # The old circuit's pointer was already null, so it is not written to self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) self.assertEqual( Circuit.objects.get(pk=self.circuits[0].pk).last_updated, old_circuit_last_updated ) - # ...while the new circuit's pointer is set as usual. + # The new circuit's pointer is set as usual 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) @@ -433,8 +431,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): @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. + # 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], )) @@ -454,20 +451,45 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): 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. + def test_deletion_clears_pointer(self): + # on_delete=SET_NULL clears the pointer without a post_save, and related_name='+' keeps + # these relations out of _meta.related_objects, so handle_deleted_object() misses them + # too. Only the resulting database state is asserted. 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()) + + @tag('regression') # Ref: #23134 + def test_failed_pointer_write_leaves_the_change_pending(self): + # The cached originals must not advance until the pointer writes have succeeded + termination = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + + def _move(): + termination.circuit = self.circuits[1] + termination.save() + + with patch.object( + CircuitTermination, '_set_circuit_terminations', side_effect=OSError('boom') + ): + with self.assertRaises(OSError): + self._tracked(_move) + + # The termination row was rolled back along with the pointer writes + termination.refresh_from_db() + self.assertEqual(termination.circuit, self.circuits[0]) + + # A retry still sees the move as pending, so both pointers end up correct + termination.circuit = self.circuits[1] + self._tracked(termination.save) + + self.circuits[0].refresh_from_db() + self.circuits[1].refresh_from_db() + self.assertIsNone(self.circuits[0].termination_a_id) + self.assertEqual(self.circuits[1].termination_a_id, termination.pk)