From b7b61ab16526a9acee2802e91b27fc892b901d26 Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 11:14:47 -0700 Subject: [PATCH] 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