From c0f50eb64c1a93d9bc2e84f7ec8fdbb0fa16b2b6 Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 4 Sep 2026 10:39:53 -0700 Subject: [PATCH 01/17] fix changelog record for circuit termination --- netbox/circuits/models/circuits.py | 28 +++++- netbox/circuits/tests/test_models.py | 130 ++++++++++++++++++++++++++- 2 files changed, 155 insertions(+), 3 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 2b194e4c3..e13eeb955 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -403,18 +403,42 @@ 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}) + self._set_circuit_termination(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()}' - Circuit.objects.filter(pk=self.circuit_id).update(**{termination_name: self.pk}) + self._set_circuit_termination(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 + @staticmethod + def _set_circuit_termination(circuit_id, field_name, value): + """ + Point a Circuit's cached `termination_a`/`termination_z` field at the given + CircuitTermination PK, or clear it. + + 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 -- notably the branching plugin, which + applies a CREATE via a raw save that never runs this method -- have no record of the write + and silently drop the association. + + The Circuit is always re-fetched rather than reusing a cached `self.circuit`: creating the + A and Z terminations in sequence would otherwise snapshot a Circuit loaded before the A + pointer was set, recording a prechange value that no longer matches the database. + """ + circuit = Circuit.objects.filter(pk=circuit_id).first() + if circuit is None or getattr(circuit, f'{field_name}_id') == value: + return + + circuit.snapshot() + setattr(circuit, f'{field_name}_id', value) + circuit.save(update_fields=[field_name, 'last_updated']) + 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..a0c70e3e2 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,125 @@ class CircuitTerminationDenormalizationTriggerTestCase(TestCase): termination.refresh_from_db() self.assertEqual(termination._region, region_b) + + +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. (#22651) + """ + @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 _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: #22651 + 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) + + @tag('regression') # Ref: #22651 + 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. + 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: #22651 + 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']) + + # ...and 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: #22651 + def test_term_side_change_records_single_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() + + def _flip(): + termination.term_side = 'Z' + termination.save() + + self._tracked(_flip) + + # Both pointers move within one circuit, so the clear and the set are recorded separately. + changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(changes.count(), 2) + self.assertIsNone(changes[0].postchange_data['termination_a']) + self.assertEqual(changes[1].postchange_data['termination_z'], termination.pk) + + 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()) From ab05234faf340a03cade4ae5145c20b11991403e Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 4 Sep 2026 11:15:00 -0700 Subject: [PATCH 02/17] fixes --- netbox/circuits/models/circuits.py | 13 +++++----- netbox/circuits/tests/test_models.py | 38 ++++++++++++++++++++++++---- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index e13eeb955..2128fc81a 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -423,13 +423,14 @@ class CircuitTermination( 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 -- notably the branching plugin, which - applies a CREATE via a raw save that never runs this method -- have no record of the write - and silently drop the association. + so consumers which replay ObjectChange records have no record of the write and silently + drop the association. - The Circuit is always re-fetched rather than reusing a cached `self.circuit`: creating the - A and Z terminations in sequence would otherwise snapshot a Circuit loaded before the A - pointer was set, recording a prechange value that no longer matches the database. + 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 each write to one column. """ circuit = Circuit.objects.filter(pk=circuit_id).first() if circuit is None or getattr(circuit, f'{field_name}_id') == value: diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index a0c70e3e2..c319d0c48 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -282,7 +282,7 @@ 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. (#22651) + no ObjectChange, so consumers which replay the changelog never saw the association. (#23134) """ @classmethod def setUpTestData(cls): @@ -314,7 +314,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): action=ObjectChangeActionChoices.ACTION_UPDATE, ).order_by('pk') - @tag('regression') # Ref: #22651 + @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], @@ -325,7 +325,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertIsNone(changes[0].prechange_data['termination_a']) self.assertEqual(changes[0].postchange_data['termination_a'], termination.pk) - @tag('regression') # Ref: #22651 + @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. @@ -345,7 +345,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): 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: #22651 + @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], @@ -370,7 +370,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertIsNone(new_changes[0].prechange_data['termination_a']) self.assertEqual(new_changes[0].postchange_data['termination_a'], termination.pk) - @tag('regression') # Ref: #22651 + @tag('regression') # Ref: #23134 def test_term_side_change_records_single_circuit_update(self): termination = self._tracked(lambda: CircuitTermination.objects.create( circuit=self.circuits[0], term_side='A', termination=self.sites[0], @@ -389,6 +389,34 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertIsNone(changes[0].postchange_data['termination_a']) self.assertEqual(changes[1].postchange_data['termination_z'], termination.pk) + @tag('regression') # Ref: #23134 + def test_pointer_already_set_records_no_circuit_update(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([ + CircuitTermination(circuit=self.circuits[0], term_side='A', termination=self.sites[0]), + ]) + termination = CircuitTermination.objects.get(circuit=self.circuits[0], term_side='A') + + def _move(): + termination.circuit = self.circuits[1] + termination.save() + + old_circuit_last_updated = Circuit.objects.get(pk=self.circuits[0].pk).last_updated + + self._tracked(_move) + + # The old circuit's pointer was already null, so it is not written to at all... + 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. + 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_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], From d5e55bec06d301cd75cef5d2d686f37217779c28 Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 4 Sep 2026 11:38:19 -0700 Subject: [PATCH 03/17] cleanup --- netbox/circuits/models/circuits.py | 39 +++++++++++++++----- netbox/circuits/tests/test_models.py | 53 +++++++++++++++++++++++++--- 2 files changed, 79 insertions(+), 13 deletions(-) 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()) From 594d9a12767b66b178aad3690ae1a9e4ce4fad29 Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 4 Sep 2026 11:57:00 -0700 Subject: [PATCH 04/17] 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) From 8ec91108f2927e1095a17ada98753809482cacc8 Mon Sep 17 00:00:00 2001 From: Arthur Date: Fri, 4 Sep 2026 14:37:10 -0700 Subject: [PATCH 05/17] cleanup --- netbox/circuits/models/circuits.py | 37 ++++++++++++++++++------------ 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index ceb73d04b..1ec25ab6b 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -399,6 +399,10 @@ class CircuitTermination( # Cache objects associated with the terminating object (for filtering) self.cache_related_objects() + if not pointer_moved: + super().save(*args, **kwargs) + return + # 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 = {} @@ -412,23 +416,24 @@ class CircuitTermination( with transaction.atomic(using=router.db_for_write(type(self))): super().save(*args, **kwargs) - # 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 + # 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(): - circuit = self._set_circuit_terminations(circuit_id, fields) - # Keep the instance's cached Circuit in step with the write + # Ordered by PK, so that two terminations moving between the same pair of circuits + # take the two locks in the same order and cannot deadlock + for circuit_id in sorted(updates, key=lambda pk: pk or 0): + circuit = self._set_circuit_terminations(circuit_id, updates[circuit_id]) + # Adopt the fetched Circuit, so that to_objectchange() and the caller see the + # new pointers. Note this replaces the instance's cached parent, discarding any + # prefetch or annotation set up on it. 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 + self._orig_circuit_id = self.circuit_id + self._orig_term_side = self.term_side @staticmethod def _set_circuit_terminations(circuit_id, fields): @@ -437,13 +442,15 @@ class CircuitTermination( field name to CircuitTermination PK (or None). 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. + and so records nothing in the changelog. The Circuit is re-fetched under a lock so that + the snapshot reflects a sibling pointer written concurrently; without it, a second writer + could record the first writer's pointer as null. no_key avoids blocking the foreign key + inserts which reference this circuit. Returns the Circuit, or None if no such row exists. """ - circuit = Circuit.objects.filter(pk=circuit_id).first() + # order_by() clears the default ordering, whose JOIN would leave the row unlockable + circuit = Circuit.objects.filter(pk=circuit_id).order_by().select_for_update(no_key=True).first() if circuit is None: return None From b7b61ab16526a9acee2802e91b27fc892b901d26 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 11:14:47 -0700 Subject: [PATCH 06/17] cleanup --- netbox/circuits/models/circuits.py | 32 +++++++------- netbox/circuits/signals.py | 22 +++++++++- netbox/circuits/tests/test_models.py | 64 ++++++++++++++++++++++++++-- 3 files changed, 97 insertions(+), 21 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 1ec25ab6b..f4d845169 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -413,7 +413,8 @@ class CircuitTermination( updates.setdefault(self._orig_circuit_id, {})[old_termination_name] = None # Write the termination row and the pointers which reference it together - with transaction.atomic(using=router.db_for_write(type(self))): + using = kwargs.get('using') or router.db_for_write(type(self)) + with transaction.atomic(using=using): super().save(*args, **kwargs) # Update the new circuit's termination reference @@ -422,13 +423,8 @@ class CircuitTermination( # Ordered by PK, so that two terminations moving between the same pair of circuits # take the two locks in the same order and cannot deadlock - for circuit_id in sorted(updates, key=lambda pk: pk or 0): - circuit = self._set_circuit_terminations(circuit_id, updates[circuit_id]) - # Adopt the fetched Circuit, so that to_objectchange() and the caller see the - # new pointers. Note this replaces the instance's cached parent, discarding any - # prefetch or annotation set up on it. - if circuit is not None and circuit.pk == self.circuit_id: - self.circuit = circuit + for circuit_id in sorted(updates): + self._set_circuit_terminations(circuit_id, updates[circuit_id], using=using) # Update cached values for subsequent saves, only once the pointer writes have # succeeded, so that a failed save is still pending on retry @@ -436,7 +432,7 @@ class CircuitTermination( self._orig_term_side = self.term_side @staticmethod - def _set_circuit_terminations(circuit_id, fields): + def _set_circuit_terminations(circuit_id, fields, using=None): """ Set or clear a Circuit's cached `termination_a`/`termination_z` fields. `fields` maps field name to CircuitTermination PK (or None). @@ -447,12 +443,17 @@ class CircuitTermination( could record the first writer's pointer as null. no_key avoids blocking the foreign key inserts which reference this circuit. - Returns the Circuit, or None if no such row exists. + Does nothing if the Circuit no longer exists, or if every field already holds its + intended value. """ + using = using or router.db_for_write(Circuit) + # order_by() clears the default ordering, whose JOIN would leave the row unlockable - circuit = Circuit.objects.filter(pk=circuit_id).order_by().select_for_update(no_key=True).first() + circuit = Circuit.objects.using(using).filter( + pk=circuit_id + ).order_by().select_for_update(no_key=True).first() if circuit is None: - return None + return # Skip fields which already hold the intended value fields = { @@ -461,14 +462,15 @@ class CircuitTermination( if getattr(circuit, f'{field_name}_id') != value } if not fields: - return circuit + return 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 + # update_fields excludes _abs_distance, which DistanceMixin.save() recomputes; safe only + # because the Circuit was just re-fetched + circuit.save(using=using, 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/signals.py b/netbox/circuits/signals.py index 4db765c80..3060fc47f 100644 --- a/netbox/circuits/signals.py +++ b/netbox/circuits/signals.py @@ -1,9 +1,9 @@ -from django.db.models.signals import post_delete, post_save +from django.db.models.signals import post_delete, post_save, pre_delete from django.dispatch import receiver from dcim.signals import rebuild_paths -from .models import CircuitTermination +from .models import Circuit, CircuitTermination @receiver((post_save, post_delete), sender=CircuitTermination) @@ -15,3 +15,21 @@ def rebuild_cablepaths(instance, raw=False, **kwargs): peer_termination = instance.get_peer_termination() if peer_termination: rebuild_paths([peer_termination]) + + +@receiver(pre_delete, sender=CircuitTermination) +def clear_circuit_termination_pointer(instance, using=None, origin=None, **kwargs): + """ + Clear the parent Circuit's cached `termination_a`/`termination_z` pointer with a change-logged + save. on_delete=SET_NULL clears it via a bulk UPDATE, and related_name='+' hides the relation + from Circuit._meta.related_objects, so neither path records an ObjectChange. (#23134) + """ + if not instance.term_side: + return + + # The pointer goes away with the circuit, so a change record for it would be spurious + if isinstance(origin, Circuit) or getattr(origin, 'model', None) is Circuit: + return + + field_name = f'termination_{instance.term_side.lower()}' + CircuitTermination._set_circuit_terminations(instance.circuit_id, {field_name: None}, using=using) diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 9bb6827c3..26ac070fe 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -325,6 +325,14 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): 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 = ObjectChange.objects.get( + changed_object_type=ContentType.objects.get_for_model(CircuitTermination), + changed_object_id=termination.pk, + action=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 @@ -451,19 +459,67 @@ 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(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. + @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']) + + @tag('regression') # Ref: #23134 + def test_bulk_deletion_records_circuit_update(self): + # A queryset delete() passes the queryset as the signal's origin rather than an instance + 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(CircuitTermination.objects.filter(pk=termination_pk).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']) + + def test_cascade_from_termination_parent_records_circuit_update(self): + # The circuit survives the cascade, so the pointer clear still has to be recorded + 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(self.sites[0].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) + + 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_failed_pointer_write_leaves_the_change_pending(self): # The cached originals must not advance until the pointer writes have succeeded From 00ed38771a002f4c45279a3b0cbe3c9d3dbabd4b Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 13:18:06 -0700 Subject: [PATCH 07/17] delete handling --- netbox/circuits/models/circuits.py | 25 +++++++++++------ netbox/circuits/signals.py | 6 ++++- netbox/circuits/tests/test_models.py | 40 +++++++++++++++++++++++++--- 3 files changed, 58 insertions(+), 13 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index f4d845169..2cbb68543 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -421,21 +421,23 @@ class CircuitTermination( termination_name = f'termination_{self.term_side.lower()}' updates.setdefault(self.circuit_id, {})[termination_name] = self.pk - # Ordered by PK, so that two terminations moving between the same pair of circuits - # take the two locks in the same order and cannot deadlock + # Ordered by PK so concurrent saves take the circuit locks in the same order. The + # delete path is unordered (see circuits.signals), so a bulk delete racing a save + # can still deadlock. for circuit_id in sorted(updates): self._set_circuit_terminations(circuit_id, updates[circuit_id], using=using) - # Update cached values for subsequent saves, only once the pointer writes have - # succeeded, so that a failed save is still pending on retry - self._orig_circuit_id = self.circuit_id - self._orig_term_side = self.term_side + # Advanced only once the writes have left the block, so that a rolled-back save is still + # pending on retry + self._orig_circuit_id = self.circuit_id + self._orig_term_side = self.term_side @staticmethod - def _set_circuit_terminations(circuit_id, fields, using=None): + def _set_circuit_terminations(circuit_id, fields, using=None, only_if_references=None): """ Set or clear a Circuit's cached `termination_a`/`termination_z` fields. `fields` maps - field name to CircuitTermination PK (or None). + field name to CircuitTermination PK (or None). `only_if_references` restricts the write + to fields which currently hold that PK. 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 under a lock so that @@ -455,6 +457,13 @@ class CircuitTermination( if circuit is None: return + if only_if_references is not None: + fields = { + field_name: value + for field_name, value in fields.items() + if getattr(circuit, f'{field_name}_id') == only_if_references + } + # Skip fields which already hold the intended value fields = { field_name: value diff --git a/netbox/circuits/signals.py b/netbox/circuits/signals.py index 3060fc47f..bf772bd8f 100644 --- a/netbox/circuits/signals.py +++ b/netbox/circuits/signals.py @@ -31,5 +31,9 @@ def clear_circuit_termination_pointer(instance, using=None, origin=None, **kwarg if isinstance(origin, Circuit) or getattr(origin, 'model', None) is Circuit: return + # only_if_references matches what on_delete=SET_NULL would have cleared: the in-memory + # term_side may not be what the pointer actually references field_name = f'termination_{instance.term_side.lower()}' - CircuitTermination._set_circuit_terminations(instance.circuit_id, {field_name: None}, using=using) + CircuitTermination._set_circuit_terminations( + instance.circuit_id, {field_name: None}, using=using, only_if_references=instance.pk + ) diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 26ac070fe..2fbbd7e57 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -307,6 +307,13 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): 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), @@ -326,10 +333,8 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): 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 = ObjectChange.objects.get( - changed_object_type=ContentType.objects.get_for_model(CircuitTermination), - changed_object_id=termination.pk, - action=ObjectChangeActionChoices.ACTION_CREATE, + termination_create = self._termination_change( + termination.pk, ObjectChangeActionChoices.ACTION_CREATE ) self.assertLess(termination_create.pk, changes[0].pk) @@ -477,6 +482,15 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk) self.assertIsNone(changes[0].postchange_data['termination_a']) + # core.signals.handle_deleted_object is connected before this app's receiver, so the + # DELETE precedes the pointer clear. Replaying in this order relies on the consumer + # applying the DELETE through the ORM, where on_delete=SET_NULL clears the pointer, or + # on the FK being DEFERRABLE INITIALLY DEFERRED within one transaction. + termination_delete = self._termination_change( + termination_pk, ObjectChangeActionChoices.ACTION_DELETE + ) + self.assertLess(termination_delete.pk, changes[0].pk) + @tag('regression') # Ref: #23134 def test_bulk_deletion_records_circuit_update(self): # A queryset delete() passes the queryset as the signal's origin rather than an instance @@ -510,6 +524,24 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertEqual(changes.count(), 1) self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk) + def test_deletion_leaves_pointer_for_another_termination(self): + # An in-memory term_side which diverges from the persisted one must not clear a pointer + # belonging to a different termination + 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.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.assertFalse(self._circuit_changes(self.circuits[0]).exists()) + 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], From 325959fe36f4a1a98d99b772e2befa43c190dc4f Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 14:21:15 -0700 Subject: [PATCH 08/17] delete handling --- netbox/circuits/apps.py | 16 ++++++++++++++++ netbox/circuits/signals.py | 5 +++-- netbox/circuits/tests/test_models.py | 8 +++----- 3 files changed, 22 insertions(+), 7 deletions(-) diff --git a/netbox/circuits/apps.py b/netbox/circuits/apps.py index d1d4fe9e7..1cc517700 100644 --- a/netbox/circuits/apps.py +++ b/netbox/circuits/apps.py @@ -1,4 +1,20 @@ from django.apps import AppConfig +from django.db.models.signals import pre_delete + + +def _clear_circuit_termination_pointer(sender, **kwargs): + from .models import CircuitTermination + from .signals import clear_circuit_termination_pointer + + if sender is CircuitTermination: + clear_circuit_termination_pointer(**kwargs) + + +# This module is imported in populate() phase 1, ahead of the models phase which connects +# core.signals.handle_deleted_object. Connecting here records the Circuit pointer clear before the +# termination's own DELETE; branch revert replays newest-first and needs the termination restored +# before the pointer referencing it. (#23134) +pre_delete.connect(_clear_circuit_termination_pointer) class CircuitsConfig(AppConfig): diff --git a/netbox/circuits/signals.py b/netbox/circuits/signals.py index bf772bd8f..c9d9609fb 100644 --- a/netbox/circuits/signals.py +++ b/netbox/circuits/signals.py @@ -1,4 +1,4 @@ -from django.db.models.signals import post_delete, post_save, pre_delete +from django.db.models.signals import post_delete, post_save from django.dispatch import receiver from dcim.signals import rebuild_paths @@ -17,12 +17,13 @@ def rebuild_cablepaths(instance, raw=False, **kwargs): rebuild_paths([peer_termination]) -@receiver(pre_delete, sender=CircuitTermination) def clear_circuit_termination_pointer(instance, using=None, origin=None, **kwargs): """ Clear the parent Circuit's cached `termination_a`/`termination_z` pointer with a change-logged save. on_delete=SET_NULL clears it via a bulk UPDATE, and related_name='+' hides the relation from Circuit._meta.related_objects, so neither path records an ObjectChange. (#23134) + + Connected in CircuitsConfig, not here, so that it precedes handle_deleted_object. """ if not instance.term_side: return diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 2fbbd7e57..162ddde3e 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -482,14 +482,12 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk) self.assertIsNone(changes[0].postchange_data['termination_a']) - # core.signals.handle_deleted_object is connected before this app's receiver, so the - # DELETE precedes the pointer clear. Replaying in this order relies on the consumer - # applying the DELETE through the ORM, where on_delete=SET_NULL clears the pointer, or - # on the FK being DEFERRABLE INITIALLY DEFERRED within one transaction. + # 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(termination_delete.pk, changes[0].pk) + self.assertLess(changes[0].pk, termination_delete.pk) @tag('regression') # Ref: #23134 def test_bulk_deletion_records_circuit_update(self): From 85ff7b0af365b8fd8df9693d9e551846678eb2ce Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 14:56:42 -0700 Subject: [PATCH 09/17] delete handling --- netbox/circuits/apps.py | 16 -------------- netbox/circuits/models/circuits.py | 31 +++++++++++++++++++++++++--- netbox/circuits/signals.py | 25 +--------------------- netbox/circuits/tests/test_models.py | 21 ++++++++++--------- 4 files changed, 40 insertions(+), 53 deletions(-) diff --git a/netbox/circuits/apps.py b/netbox/circuits/apps.py index 1cc517700..d1d4fe9e7 100644 --- a/netbox/circuits/apps.py +++ b/netbox/circuits/apps.py @@ -1,20 +1,4 @@ from django.apps import AppConfig -from django.db.models.signals import pre_delete - - -def _clear_circuit_termination_pointer(sender, **kwargs): - from .models import CircuitTermination - from .signals import clear_circuit_termination_pointer - - if sender is CircuitTermination: - clear_circuit_termination_pointer(**kwargs) - - -# This module is imported in populate() phase 1, ahead of the models phase which connects -# core.signals.handle_deleted_object. Connecting here records the Circuit pointer clear before the -# termination's own DELETE; branch revert replays newest-first and needs the termination restored -# before the pointer referencing it. (#23134) -pre_delete.connect(_clear_circuit_termination_pointer) class CircuitsConfig(AppConfig): diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 2cbb68543..e5e0847dc 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -421,9 +421,9 @@ class CircuitTermination( termination_name = f'termination_{self.term_side.lower()}' updates.setdefault(self.circuit_id, {})[termination_name] = self.pk - # Ordered by PK so concurrent saves take the circuit locks in the same order. The - # delete path is unordered (see circuits.signals), so a bulk delete racing a save - # can still deadlock. + # Ordered by PK so concurrent saves take the circuit locks in the same order. + # delete() locks in queryset order, so a bulk delete under an enclosing transaction + # racing a save can still deadlock. for circuit_id in sorted(updates): self._set_circuit_terminations(circuit_id, updates[circuit_id], using=using) @@ -481,6 +481,31 @@ class CircuitTermination( # because the Circuit was just re-fetched circuit.save(using=using, update_fields=[*fields, 'last_updated']) + def delete(self, *args, **kwargs): + # Clear the parent Circuit's cached pointer before the deletion starts, so that its change + # record precedes this row's DELETE. on_delete=SET_NULL clears the column with a bulk + # UPDATE, and related_name='+' hides the relation from Circuit._meta.related_objects, so + # neither path records an ObjectChange. (#23134) + # + # Not a pre_delete receiver: core.signals.handle_deleted_object connects during the models + # import phase, ahead of any app's ready(), and Django dispatches in connection order, so a + # receiver here would run only after the DELETE had been recorded. + # + # Cascades (e.g. deleting the terminating Site, or the Circuit itself) reach the row through + # the collector rather than here, and remain unrecorded. + using = kwargs.get('using') or router.db_for_write(type(self)) + with transaction.atomic(using=using): + if self.term_side: + self._set_circuit_terminations( + self.circuit_id, + {f'termination_{self.term_side.lower()}': None}, + using=using, + only_if_references=self.pk, + ) + return super().delete(*args, **kwargs) + + delete.alters_data = True + 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/signals.py b/netbox/circuits/signals.py index c9d9609fb..4db765c80 100644 --- a/netbox/circuits/signals.py +++ b/netbox/circuits/signals.py @@ -3,7 +3,7 @@ from django.dispatch import receiver from dcim.signals import rebuild_paths -from .models import Circuit, CircuitTermination +from .models import CircuitTermination @receiver((post_save, post_delete), sender=CircuitTermination) @@ -15,26 +15,3 @@ def rebuild_cablepaths(instance, raw=False, **kwargs): peer_termination = instance.get_peer_termination() if peer_termination: rebuild_paths([peer_termination]) - - -def clear_circuit_termination_pointer(instance, using=None, origin=None, **kwargs): - """ - Clear the parent Circuit's cached `termination_a`/`termination_z` pointer with a change-logged - save. on_delete=SET_NULL clears it via a bulk UPDATE, and related_name='+' hides the relation - from Circuit._meta.related_objects, so neither path records an ObjectChange. (#23134) - - Connected in CircuitsConfig, not here, so that it precedes handle_deleted_object. - """ - if not instance.term_side: - return - - # The pointer goes away with the circuit, so a change record for it would be spurious - if isinstance(origin, Circuit) or getattr(origin, 'model', None) is Circuit: - return - - # only_if_references matches what on_delete=SET_NULL would have cleared: the in-memory - # term_side may not be what the pointer actually references - field_name = f'termination_{instance.term_side.lower()}' - CircuitTermination._set_circuit_terminations( - instance.circuit_id, {field_name: None}, using=using, only_if_references=instance.pk - ) diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 162ddde3e..d439207a4 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -491,36 +491,37 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): @tag('regression') # Ref: #23134 def test_bulk_deletion_records_circuit_update(self): - # A queryset delete() passes the queryset as the signal's origin rather than an instance + # 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 - self._tracked(CircuitTermination.objects.filter(pk=termination_pk).delete) + 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']) - def test_cascade_from_termination_parent_records_circuit_update(self): - # The circuit survives the cascade, so the pointer clear still has to be recorded - termination = self._tracked(lambda: CircuitTermination.objects.create( + def test_cascade_deletion_leaves_pointer_unrecorded(self): + # Deleting the terminating Site reaches the termination through the collector, which does + # not call delete(). on_delete=SET_NULL still clears the column, but nothing records it. + 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(self.sites[0].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.assertFalse(self._circuit_changes(self.circuits[0]).exists()) def test_deletion_leaves_pointer_for_another_termination(self): # An in-memory term_side which diverges from the persisted one must not clear a pointer From 6f1fc02d426ca5785a31927a2c6c45574cfeb036 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 15:48:22 -0700 Subject: [PATCH 10/17] delete handling --- netbox/circuits/models/circuits.py | 26 ++++++++++++++++++++------ netbox/circuits/tests/test_models.py | 27 +++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index e5e0847dc..8b2bae0c0 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -407,8 +407,11 @@ class CircuitTermination( # 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 - if circuit_changed or term_side_changed: + # Clear the old termination reference if circuit or term_side changed. Skipped while + # inserting: nothing references the row yet, and the originals captured in __init__ + # describe whatever was passed to the constructor, which may be another termination's + # pointer. + if not is_new and (circuit_changed or term_side_changed): old_termination_name = f'termination_{self._orig_term_side.lower()}' updates.setdefault(self._orig_circuit_id, {})[old_termination_name] = None @@ -477,9 +480,13 @@ class CircuitTermination( for field_name, value in fields.items(): setattr(circuit, f'{field_name}_id', value) - # update_fields excludes _abs_distance, which DistanceMixin.save() recomputes; safe only - # because the Circuit was just re-fetched - circuit.save(using=using, update_fields=[*fields, 'last_updated']) + # Saved in full rather than with update_fields, so that postchange_data describes the + # row as written. The mixin save() chain mutates fields beyond the pointers + # (custom_field_data, distance_unit, _abs_distance); excluding them from the write left + # them in the record but not the database, and a replaying consumer applies the + # difference. Writing every column is safe because the row was fetched under the lock + # held for this transaction, so no concurrent write can interleave. + circuit.save(using=using) def delete(self, *args, **kwargs): # Clear the parent Circuit's cached pointer before the deletion starts, so that its change @@ -493,8 +500,15 @@ class CircuitTermination( # # Cascades (e.g. deleting the terminating Site, or the Circuit itself) reach the row through # the collector rather than here, and remain unrecorded. - using = kwargs.get('using') or router.db_for_write(type(self)) + # Model.delete() still accepts `using` positionally + using = kwargs.get('using') or (args[0] if args else None) or router.db_for_write(type(self)) with transaction.atomic(using=using): + # Lock this row before the circuit. super().save() locks it first too, so without + # this a concurrent save and delete of the same termination could deadlock. + CircuitTermination.objects.using(using).filter( + pk=self.pk + ).order_by().select_for_update().first() + if self.term_side: self._set_circuit_terminations( self.circuit_id, diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index d439207a4..9a669f342 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -431,6 +431,33 @@ 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_new_termination_does_not_clear_sibling_pointer(self): + # __init__ captures the originals from the constructor kwargs, so mutating term_side + # before the first save reaches the clear path with originals naming a live sibling + 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( From 6c4a46902915f87f04e82876c383e6a2dcfd4e48 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 16:01:08 -0700 Subject: [PATCH 11/17] cleanup --- netbox/circuits/models/circuits.py | 35 +++++++----------------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 8b2bae0c0..1a1f43b63 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -407,10 +407,8 @@ class CircuitTermination( # 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. Skipped while - # inserting: nothing references the row yet, and the originals captured in __init__ - # describe whatever was passed to the constructor, which may be another termination's - # pointer. + # Clear the old termination reference if circuit or term_side changed. Never on insert: + # __init__ captured the constructor's values, which may name a live sibling's pointer. if not is_new and (circuit_changed or term_side_changed): old_termination_name = f'termination_{self._orig_term_side.lower()}' updates.setdefault(self._orig_circuit_id, {})[old_termination_name] = None @@ -424,9 +422,7 @@ class CircuitTermination( termination_name = f'termination_{self.term_side.lower()}' updates.setdefault(self.circuit_id, {})[termination_name] = self.pk - # Ordered by PK so concurrent saves take the circuit locks in the same order. - # delete() locks in queryset order, so a bulk delete under an enclosing transaction - # racing a save can still deadlock. + # Ordered by PK so concurrent saves take the circuit locks in the same order for circuit_id in sorted(updates): self._set_circuit_terminations(circuit_id, updates[circuit_id], using=using) @@ -480,31 +476,16 @@ class CircuitTermination( for field_name, value in fields.items(): setattr(circuit, f'{field_name}_id', value) - # Saved in full rather than with update_fields, so that postchange_data describes the - # row as written. The mixin save() chain mutates fields beyond the pointers - # (custom_field_data, distance_unit, _abs_distance); excluding them from the write left - # them in the record but not the database, and a replaying consumer applies the - # difference. Writing every column is safe because the row was fetched under the lock - # held for this transaction, so no concurrent write can interleave. + # Saved in full, not with update_fields: the mixin chain also mutates custom_field_data + # and the distance fields, which would reach postchange_data but not the database. circuit.save(using=using) def delete(self, *args, **kwargs): - # Clear the parent Circuit's cached pointer before the deletion starts, so that its change - # record precedes this row's DELETE. on_delete=SET_NULL clears the column with a bulk - # UPDATE, and related_name='+' hides the relation from Circuit._meta.related_objects, so - # neither path records an ObjectChange. (#23134) - # - # Not a pre_delete receiver: core.signals.handle_deleted_object connects during the models - # import phase, ahead of any app's ready(), and Django dispatches in connection order, so a - # receiver here would run only after the DELETE had been recorded. - # - # Cascades (e.g. deleting the terminating Site, or the Circuit itself) reach the row through - # the collector rather than here, and remain unrecorded. - # Model.delete() still accepts `using` positionally + # Clear the pointer first, so its record precedes this DELETE. Not a pre_delete receiver: + # handle_deleted_object connects earlier and would record the DELETE first. (#23134) using = kwargs.get('using') or (args[0] if args else None) or router.db_for_write(type(self)) with transaction.atomic(using=using): - # Lock this row before the circuit. super().save() locks it first too, so without - # this a concurrent save and delete of the same termination could deadlock. + # Locked before the circuit, matching the order super().save() takes them in CircuitTermination.objects.using(using).filter( pk=self.pk ).order_by().select_for_update().first() From deeb6f14897512dec99137ece655f0a0a95c062b Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 16:05:02 -0700 Subject: [PATCH 12/17] cleanup --- netbox/circuits/models/circuits.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 1a1f43b63..b0a420571 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -456,19 +456,14 @@ class CircuitTermination( if circuit is None: return - if only_if_references is not None: - fields = { - field_name: value - for field_name, value in fields.items() - if getattr(circuit, f'{field_name}_id') == only_if_references - } + def needs_write(field_name, value): + current = getattr(circuit, f'{field_name}_id') + if current == value: + return False + # Match what on_delete=SET_NULL would have cleared + return only_if_references is None or current == only_if_references - # 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 - } + fields = {name: value for name, value in fields.items() if needs_write(name, value)} if not fields: return From 5fd7bbe0aed11c43478a2967691d1bf61cdb83b8 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 16 Sep 2026 09:32:50 -0700 Subject: [PATCH 13/17] simplify --- netbox/circuits/models/circuits.py | 119 ++++++++++----------------- netbox/circuits/tests/test_models.py | 58 ++++--------- 2 files changed, 60 insertions(+), 117 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index b0a420571..b8d7dce2b 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, router, transaction +from django.db import models from django.urls import reverse from django.utils.translation import gettext_lazy as _ @@ -394,107 +394,72 @@ 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() - if not pointer_moved: - super().save(*args, **kwargs) - return + super().save(*args, **kwargs) - # 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. Never on insert: - # __init__ captured the constructor's values, which may name a live sibling's pointer. - if not is_new and (circuit_changed or term_side_changed): + # 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()}' - updates.setdefault(self._orig_circuit_id, {})[old_termination_name] = None - - # Write the termination row and the pointers which reference it together - using = kwargs.get('using') or router.db_for_write(type(self)) - with transaction.atomic(using=using): - super().save(*args, **kwargs) + self._set_circuit_terminations( + self._orig_circuit_id, {old_termination_name: None}, only_if_references=self.pk + ) + # 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 + self._set_circuit_terminations(self.circuit_id, {termination_name: self.pk}) - # Ordered by PK so concurrent saves take the circuit locks in the same order - for circuit_id in sorted(updates): - self._set_circuit_terminations(circuit_id, updates[circuit_id], using=using) + # Update cached values for subsequent saves + self._orig_circuit_id = self.circuit_id + self._orig_term_side = self.term_side - # Advanced only once the writes have left the block, so that a rolled-back save is still - # pending on retry - self._orig_circuit_id = self.circuit_id - self._orig_term_side = self.term_side + def delete(self, *args, **kwargs): + # Clear the circuit's reference before the row goes away, so that the change is recorded + # and precedes the DELETE. on_delete=SET_NULL would clear it with an unlogged bulk update. + self._set_circuit_terminations( + self.circuit_id, {'termination_a': None, 'termination_z': None}, only_if_references=self.pk + ) + + return super().delete(*args, **kwargs) + + delete.alters_data = True @staticmethod - def _set_circuit_terminations(circuit_id, fields, using=None, only_if_references=None): + def _set_circuit_terminations(circuit_id, fields, only_if_references=None): """ - Set or clear a Circuit's cached `termination_a`/`termination_z` fields. `fields` maps - field name to CircuitTermination PK (or None). `only_if_references` restricts the write - to fields which currently hold that PK. + Set or clear a Circuit's cached `termination_a`/`termination_z` fields, recording the + change. `fields` maps field name to CircuitTermination PK (or None). A queryset update() + emits no post_save, and so records nothing in the changelog (#23134). - 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 under a lock so that - the snapshot reflects a sibling pointer written concurrently; without it, a second writer - could record the first writer's pointer as null. no_key avoids blocking the foreign key - inserts which reference this circuit. - - Does nothing if the Circuit no longer exists, or if every field already holds its - intended value. + Args: + circuit_id: PK of the Circuit to update + fields: Mapping of field name to the value to assign + only_if_references: If set, restricts the write to fields which currently hold this PK """ - using = using or router.db_for_write(Circuit) - - # order_by() clears the default ordering, whose JOIN would leave the row unlockable - circuit = Circuit.objects.using(using).filter( - pk=circuit_id - ).order_by().select_for_update(no_key=True).first() + # Re-fetched rather than reusing a cached circuit, whose pointers may predate a sibling write + circuit = Circuit.objects.filter(pk=circuit_id).first() if circuit is None: return - def needs_write(field_name, value): + updates = {} + for field_name, value in fields.items(): current = getattr(circuit, f'{field_name}_id') if current == value: - return False - # Match what on_delete=SET_NULL would have cleared - return only_if_references is None or current == only_if_references - - fields = {name: value for name, value in fields.items() if needs_write(name, value)} - if not fields: + continue + if only_if_references is not None and current != only_if_references: + continue + updates[field_name] = value + if not updates: return circuit.snapshot() - for field_name, value in fields.items(): + for field_name, value in updates.items(): setattr(circuit, f'{field_name}_id', value) - - # Saved in full, not with update_fields: the mixin chain also mutates custom_field_data - # and the distance fields, which would reach postchange_data but not the database. - circuit.save(using=using) - - def delete(self, *args, **kwargs): - # Clear the pointer first, so its record precedes this DELETE. Not a pre_delete receiver: - # handle_deleted_object connects earlier and would record the DELETE first. (#23134) - using = kwargs.get('using') or (args[0] if args else None) or router.db_for_write(type(self)) - with transaction.atomic(using=using): - # Locked before the circuit, matching the order super().save() takes them in - CircuitTermination.objects.using(using).filter( - pk=self.pk - ).order_by().select_for_update().first() - - if self.term_side: - self._set_circuit_terminations( - self.circuit_id, - {f'termination_{self.term_side.lower()}': None}, - using=using, - only_if_references=self.pk, - ) - return super().delete(*args, **kwargs) - - delete.alters_data = True + circuit.save(update_fields=[*updates, '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 9a669f342..0c3c27193 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -1,5 +1,4 @@ import uuid -from unittest.mock import patch from django.contrib.contenttypes.models import ContentType from django.core.exceptions import NON_FIELD_ERRORS, ValidationError @@ -383,7 +382,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertEqual(new_changes[0].postchange_data['termination_a'], termination.pk) @tag('regression') # Ref: #23134 - def test_term_side_change_records_single_circuit_update(self): + 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], )) @@ -395,13 +394,13 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self._tracked(_flip) - # Both pointers move within one circuit, so the clear and the set are coalesced + # The old pointer is cleared, then the new one is set changes = self._circuit_changes(self.circuits[0]) - self.assertEqual(changes.count(), 1) + self.assertEqual(changes.count(), 2) 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[0].postchange_data['termination_z'], termination.pk) + self.assertIsNone(changes[1].prechange_data['termination_z']) + self.assertEqual(changes[1].postchange_data['termination_z'], termination.pk) @tag('regression') # Ref: #23134 def test_redundant_pointer_write_is_skipped(self): @@ -550,9 +549,10 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertIsNone(self.circuits[0].termination_a_id) self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) - def test_deletion_leaves_pointer_for_another_termination(self): - # An in-memory term_side which diverges from the persisted one must not clear a pointer - # belonging to a different termination + @tag('regression') # Ref: #23134 + def test_deletion_clears_the_pointer_which_references_it(self): + # An in-memory term_side which diverges from the persisted one must clear this + # termination's own pointer, and leave the one belonging to its sibling alone termination_a = self._tracked(lambda: CircuitTermination.objects.create( circuit=self.circuits[0], term_side='A', termination=self.sites[0], )) @@ -560,13 +560,21 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): 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.assertFalse(self._circuit_changes(self.circuits[0]).exists()) + 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].postchange_data['termination_a'], termination_a.pk) + self.assertEqual(changes[0].prechange_data['termination_z'], termination_z_pk) + self.assertIsNone(changes[0].postchange_data['termination_z']) def test_circuit_deletion_records_no_pointer_update(self): self._tracked(lambda: CircuitTermination.objects.create( @@ -577,33 +585,3 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self._tracked(self.circuits[0].delete) 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) From 5cc33a60c9e372549248f49dca271a8662c358cd Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 16 Sep 2026 09:45:22 -0700 Subject: [PATCH 14/17] simplify --- netbox/circuits/models/circuits.py | 67 ++++++++++------------------ netbox/circuits/tests/test_models.py | 18 +++----- 2 files changed, 29 insertions(+), 56 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index b8d7dce2b..67d88ff48 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -400,67 +400,48 @@ class CircuitTermination( super().save(*args, **kwargs) - # Clear the old termination reference if circuit or term_side changed + # Clear the old termination reference if circuit or term_side changed. Written via + # snapshot() + save() rather than a queryset update(), which emits no post_save and so + # records nothing in the changelog (#23134). Matching on the pointer's current value + # skips the write unless it actually references this termination. if circuit_changed or term_side_changed: old_termination_name = f'termination_{self._orig_term_side.lower()}' - self._set_circuit_terminations( - self._orig_circuit_id, {old_termination_name: None}, only_if_references=self.pk - ) + circuit = Circuit.objects.filter( + pk=self._orig_circuit_id, **{old_termination_name: self.pk} + ).first() + if circuit is not None: + circuit.snapshot() + setattr(circuit, old_termination_name, None) + circuit.save(update_fields=[old_termination_name, 'last_updated']) # 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_terminations(self.circuit_id, {termination_name: self.pk}) + # Re-fetched rather than reusing self.circuit, whose pointers may predate a sibling write + circuit = Circuit.objects.get(pk=self.circuit_id) + circuit.snapshot() + setattr(circuit, termination_name, self) + circuit.save(update_fields=[termination_name, 'last_updated']) # Update cached values for subsequent saves self._orig_circuit_id = self.circuit_id self._orig_term_side = self.term_side def delete(self, *args, **kwargs): - # Clear the circuit's reference before the row goes away, so that the change is recorded - # and precedes the DELETE. on_delete=SET_NULL would clear it with an unlogged bulk update. - self._set_circuit_terminations( - self.circuit_id, {'termination_a': None, 'termination_z': None}, only_if_references=self.pk - ) + # on_delete=SET_NULL would clear the circuit's reference with an unlogged bulk update. + # Clearing it here instead also puts the record ahead of the DELETE. + termination_name = f'termination_{self.term_side.lower()}' + circuit = Circuit.objects.filter(pk=self.circuit_id, **{termination_name: self.pk}).first() + if circuit is not None: + circuit.snapshot() + setattr(circuit, termination_name, None) + circuit.save(update_fields=[termination_name, 'last_updated']) return super().delete(*args, **kwargs) delete.alters_data = True - @staticmethod - def _set_circuit_terminations(circuit_id, fields, only_if_references=None): - """ - Set or clear a Circuit's cached `termination_a`/`termination_z` fields, recording the - change. `fields` maps field name to CircuitTermination PK (or None). A queryset update() - emits no post_save, and so records nothing in the changelog (#23134). - - Args: - circuit_id: PK of the Circuit to update - fields: Mapping of field name to the value to assign - only_if_references: If set, restricts the write to fields which currently hold this PK - """ - # Re-fetched rather than reusing a cached circuit, whose pointers may predate a sibling write - circuit = Circuit.objects.filter(pk=circuit_id).first() - if circuit is None: - return - - updates = {} - for field_name, value in fields.items(): - current = getattr(circuit, f'{field_name}_id') - if current == value: - continue - if only_if_references is not None and current != only_if_references: - continue - updates[field_name] = value - if not updates: - return - - circuit.snapshot() - for field_name, value in updates.items(): - setattr(circuit, f'{field_name}_id', value) - circuit.save(update_fields=[*updates, 'last_updated']) - 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 0c3c27193..a33bd588f 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -549,10 +549,10 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertIsNone(self.circuits[0].termination_a_id) self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) - @tag('regression') # Ref: #23134 - def test_deletion_clears_the_pointer_which_references_it(self): - # An in-memory term_side which diverges from the persisted one must clear this - # termination's own pointer, and leave the one belonging to its sibling alone + def test_deletion_leaves_pointer_for_another_termination(self): + # An in-memory term_side which diverges from the persisted one must not clear a pointer + # belonging to a different termination. The termination's own pointer is then left to + # on_delete=SET_NULL, and goes unrecorded. termination_a = self._tracked(lambda: CircuitTermination.objects.create( circuit=self.circuits[0], term_side='A', termination=self.sites[0], )) @@ -560,21 +560,13 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): 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_a'], termination_a.pk) - self.assertEqual(changes[0].postchange_data['termination_a'], termination_a.pk) - self.assertEqual(changes[0].prechange_data['termination_z'], termination_z_pk) - self.assertIsNone(changes[0].postchange_data['termination_z']) + self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) def test_circuit_deletion_records_no_pointer_update(self): self._tracked(lambda: CircuitTermination.objects.create( From f49d7be00f9daf602d1bf7c9a7235f153c97759d Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 16 Sep 2026 09:48:45 -0700 Subject: [PATCH 15/17] simplify --- netbox/circuits/models/circuits.py | 31 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 67d88ff48..6c095de9b 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -241,6 +241,15 @@ class CircuitGroupAssignment(CustomFieldsMixin, ExportTemplatesMixin, TagsMixin, return reverse('circuits:circuitgroupassignment', args=[self.pk]) +def _set_circuit_termination(circuit, field_name, value): + """ + Set or clear a Circuit's cached `termination_a`/`termination_z` field, recording the change. + """ + circuit.snapshot() + setattr(circuit, field_name, value) + circuit.save(update_fields=[field_name, 'last_updated']) + + class CircuitTermination( CustomFieldsMixin, CustomLinksMixin, @@ -400,43 +409,31 @@ class CircuitTermination( super().save(*args, **kwargs) - # Clear the old termination reference if circuit or term_side changed. Written via - # snapshot() + save() rather than a queryset update(), which emits no post_save and so - # records nothing in the changelog (#23134). Matching on the pointer's current value - # skips the write unless it actually references this termination. + # 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 = Circuit.objects.filter( pk=self._orig_circuit_id, **{old_termination_name: self.pk} ).first() if circuit is not None: - circuit.snapshot() - setattr(circuit, old_termination_name, None) - circuit.save(update_fields=[old_termination_name, 'last_updated']) + _set_circuit_termination(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()}' - # Re-fetched rather than reusing self.circuit, whose pointers may predate a sibling write - circuit = Circuit.objects.get(pk=self.circuit_id) - circuit.snapshot() - setattr(circuit, termination_name, self) - circuit.save(update_fields=[termination_name, 'last_updated']) + _set_circuit_termination(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 def delete(self, *args, **kwargs): - # on_delete=SET_NULL would clear the circuit's reference with an unlogged bulk update. - # Clearing it here instead also puts the record ahead of the DELETE. + # Clear the circuit's reference here; on_delete=SET_NULL is not change-logged termination_name = f'termination_{self.term_side.lower()}' circuit = Circuit.objects.filter(pk=self.circuit_id, **{termination_name: self.pk}).first() if circuit is not None: - circuit.snapshot() - setattr(circuit, termination_name, None) - circuit.save(update_fields=[termination_name, 'last_updated']) + _set_circuit_termination(circuit, termination_name, None) return super().delete(*args, **kwargs) From 5cdd9c6cdd32cab9cbb93cdd8311e8c42d84d91c Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 16 Sep 2026 10:01:50 -0700 Subject: [PATCH 16/17] simplify tests --- netbox/circuits/tests/test_models.py | 55 +++------------------------- 1 file changed, 5 insertions(+), 50 deletions(-) diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index a33bd588f..2320b9f8a 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -403,37 +403,8 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertEqual(changes[1].postchange_data['termination_z'], termination.pk) @tag('regression') # Ref: #23134 - def test_redundant_pointer_write_is_skipped(self): - # 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]), - ]) - termination = CircuitTermination.objects.get(circuit=self.circuits[0], term_side='A') - - def _move(): - termination.circuit = self.circuits[1] - termination.save() - - old_circuit_last_updated = Circuit.objects.get(pk=self.circuits[0].pk).last_updated - - self._tracked(_move) - - # 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 - ) - - # 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) - - @tag('regression') # Ref: #23134 - def test_new_termination_does_not_clear_sibling_pointer(self): - # __init__ captures the originals from the constructor kwargs, so mutating term_side - # before the first save reaches the clear path with originals naming a live sibling + 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], )) @@ -535,24 +506,9 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk) self.assertIsNone(changes[0].postchange_data['termination_a']) - def test_cascade_deletion_leaves_pointer_unrecorded(self): - # Deleting the terminating Site reaches the termination through the collector, which does - # not call delete(). on_delete=SET_NULL still clears the column, but nothing records it. - self._tracked(lambda: CircuitTermination.objects.create( - circuit=self.circuits[0], term_side='A', termination=self.sites[0], - )) - ObjectChange.objects.all().delete() - - self._tracked(self.sites[0].delete) - - self.circuits[0].refresh_from_db() - self.assertIsNone(self.circuits[0].termination_a_id) - self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) - - def test_deletion_leaves_pointer_for_another_termination(self): - # An in-memory term_side which diverges from the persisted one must not clear a pointer - # belonging to a different termination. The termination's own pointer is then left to - # on_delete=SET_NULL, and goes unrecorded. + @tag('regression') # Ref: #23134 + def test_deletion_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], )) @@ -566,7 +522,6 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.circuits[0].refresh_from_db() self.assertEqual(self.circuits[0].termination_a_id, termination_a.pk) - self.assertFalse(self._circuit_changes(self.circuits[0]).exists()) def test_circuit_deletion_records_no_pointer_update(self): self._tracked(lambda: CircuitTermination.objects.create( From ed9069449f858965a87f163dcb84058227dcd914 Mon Sep 17 00:00:00 2001 From: Arthur Date: Wed, 16 Sep 2026 10:22:14 -0700 Subject: [PATCH 17/17] handle cascade delete --- netbox/circuits/models/circuits.py | 35 +++++++++++++--------- netbox/circuits/tests/test_models.py | 45 ++++++++++++++++++++++++++-- netbox/netbox/models/deletion.py | 14 ++++++++- 3 files changed, 77 insertions(+), 17 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index 6c095de9b..6fbbce8a5 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -241,13 +241,14 @@ class CircuitGroupAssignment(CustomFieldsMixin, ExportTemplatesMixin, TagsMixin, return reverse('circuits:circuitgroupassignment', args=[self.pk]) -def _set_circuit_termination(circuit, field_name, value): +def _set_circuit_terminations(circuit, fields): """ - Set or clear a Circuit's cached `termination_a`/`termination_z` field, recording the change. + Set or clear a Circuit's cached `termination_a`/`termination_z` fields, recording the change. """ circuit.snapshot() - setattr(circuit, field_name, value) - circuit.save(update_fields=[field_name, 'last_updated']) + for field_name, value in fields.items(): + setattr(circuit, field_name, value) + circuit.save(update_fields=[*fields, 'last_updated']) class CircuitTermination( @@ -416,28 +417,34 @@ class CircuitTermination( pk=self._orig_circuit_id, **{old_termination_name: self.pk} ).first() if circuit is not None: - _set_circuit_termination(circuit, old_termination_name, 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()}' - _set_circuit_termination(Circuit.objects.get(pk=self.circuit_id), termination_name, self) + _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 - def delete(self, *args, **kwargs): - # Clear the circuit's reference here; on_delete=SET_NULL is not change-logged - termination_name = f'termination_{self.term_side.lower()}' - circuit = Circuit.objects.filter(pk=self.circuit_id, **{termination_name: self.pk}).first() - if circuit is not None: - _set_circuit_termination(circuit, termination_name, None) + @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, ())} - return super().delete(*args, **kwargs) + circuits = Circuit.objects.filter( + models.Q(termination_a__in=pks) | models.Q(termination_z__in=pks) + ).exclude(pk__in=doomed_circuits) - delete.alters_data = True + 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 diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 2320b9f8a..4399d9032 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -507,8 +507,9 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertIsNone(changes[0].postchange_data['termination_a']) @tag('regression') # Ref: #23134 - def test_deletion_leaves_another_terminations_pointer_alone(self): - # A pointer referencing a different termination must never be cleared + 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], )) @@ -516,12 +517,52 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): 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( 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: """