From 4564f591ba2e30d378176844566d551f5ddc5bd6 Mon Sep 17 00:00:00 2001 From: Arthur Date: Thu, 10 Sep 2026 09:51:10 -0700 Subject: [PATCH 1/3] Fix #23160 - Change-log the disconnect on terminating objects when a Cable is deleted --- netbox/dcim/signals.py | 34 ++++++++-- netbox/dcim/tests/test_models.py | 105 ++++++++++++++++++++++++++++++- 2 files changed, 131 insertions(+), 8 deletions(-) diff --git a/netbox/dcim/signals.py b/netbox/dcim/signals.py index f051fe146..208e9f591 100644 --- a/netbox/dcim/signals.py +++ b/netbox/dcim/signals.py @@ -323,12 +323,34 @@ def nullify_connected_endpoints(instance, **kwargs): Disassociate the Cable from the termination object, and retrace any affected CablePaths. """ model = instance.termination_type.model_class() - model.objects.filter(pk=instance.termination_id).update( - cable=None, - cable_end=None, - cable_connector=None, - cable_positions=None, - ) + + # Deleting a Cable deletes its terminations in bulk, bypassing CableTermination.delete() and the + # change-logged clear it performs on the terminating object; do the same here so the disconnect is + # recorded. `cable` is a SET_NULL FK which the deletion collector has already nulled by now, so restore + # the pre-delete values before snapshotting or the record shows no change. + termination = None + if Cable._is_being_deleted(instance.cable_id): + termination = model.objects.filter(pk=instance.termination_id).first() + + if termination is not None: + termination.cable_id = instance.cable_id + termination.cable_end = instance.cable_end + termination.cable_connector = instance.connector + termination.cable_positions = instance.positions + termination.snapshot() + termination.cable = None + termination.cable_end = None + termination.cable_connector = None + termination.cable_positions = None + termination.save() + else: + # Already recorded by CableTermination.delete(), or the terminating object is going away too. + model.objects.filter(pk=instance.termination_id).update( + cable=None, + cable_end=None, + cable_connector=None, + cable_positions=None, + ) # If the removed termination was a channelized interface, also clear the cable attributes mirrored onto its channel # subinterfaces. This must happen before the retrace below so that each channel's (now dead) path is torn down diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index 036426665..da6041500 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -1,21 +1,25 @@ +import uuid from decimal import Decimal from django.core.exceptions import ValidationError from django.db import connection from django.db.models import ProtectedError from django.db.models.signals import post_save -from django.test import TestCase, tag +from django.test import RequestFactory, TestCase, tag from django.test.utils import CaptureQueriesContext from circuits.models import * -from core.models import ObjectType +from core.choices import ObjectChangeActionChoices +from core.models import ObjectChange, ObjectType from dcim.choices import * from dcim.models import * from extras.events import serialize_for_event from extras.models import CustomField from ipam.models import Prefix from netbox.choices import DiameterUnitChoices, FlowRateUnitChoices, WeightUnitChoices +from netbox.context_managers import event_tracking from tenancy.models import Tenant +from users.models import User from utilities.data import drange from virtualization.models import Cluster, ClusterType @@ -2922,6 +2926,103 @@ class CableTerminationTestCase(TestCase): cable_termination.cache_related_objects() +class CableDisconnectChangeLoggingTestCase(TestCase): + """ + Deleting a Cable must change-log the disconnect on each terminating object. Its CableTerminations are + deleted by the cascade, so CableTermination.delete() -- which records the disconnect when a termination + is removed from a Cable -- never runs. + """ + + @classmethod + def setUpTestData(cls): + site = Site.objects.create(name='Test Site 1', slug='test-site-1') + manufacturer = Manufacturer.objects.create(name='Test Manufacturer 1', slug='test-manufacturer-1') + device_type = DeviceType.objects.create( + manufacturer=manufacturer, model='Test Device Type 1', slug='test-device-type-1' + ) + role = DeviceRole.objects.create(name='Test Device Role 1', slug='test-device-role-1') + cls.device1 = Device.objects.create( + name='Device 1', site=site, device_type=device_type, role=role + ) + cls.device2 = Device.objects.create( + name='Device 2', site=site, device_type=device_type, role=role + ) + cls.interface1 = Interface.objects.create( + device=cls.device1, name='eth0', type=InterfaceTypeChoices.TYPE_1GE_FIXED + ) + cls.interface2 = Interface.objects.create( + device=cls.device2, name='eth0', type=InterfaceTypeChoices.TYPE_1GE_FIXED + ) + cls.interface3 = Interface.objects.create( + device=cls.device2, name='eth1', type=InterfaceTypeChoices.TYPE_1GE_FIXED + ) + cls.user = User.objects.create_user(username='testuser') + + def _request(self): + request = RequestFactory().get('/') + request.id = uuid.uuid4() + request.user = self.user + return request + + def _connect(self, termination_a, termination_b): + with event_tracking(self._request()): + cable = Cable(a_terminations=[termination_a], b_terminations=[termination_b]) + cable.save() + return cable + + def _updates(self, interface): + return ObjectChange.objects.filter( + changed_object_type=ObjectType.objects.get_for_model(Interface), + changed_object_id=interface.pk, + action=ObjectChangeActionChoices.ACTION_UPDATE, + ).order_by('time') + + @tag('regression') # Ref: netbox-branching#631 + def test_cable_deletion_records_disconnect(self): + cable = self._connect(self.interface1, self.interface2) + cable_pk = cable.pk + + with event_tracking(self._request()): + Cable.objects.get(pk=cable_pk).delete() + + for interface in (self.interface1, self.interface2): + # One update for the connect, one for the disconnect + self.assertEqual(self._updates(interface).count(), 2, f'No disconnect recorded for {interface}') + change = self._updates(interface).last() + self.assertEqual(change.prechange_data['cable'], cable_pk) + self.assertIsNone(change.postchange_data['cable']) + self.assertIsNone(change.postchange_data['cable_end']) + self.assertIsNone(Interface.objects.get(pk=interface.pk).cable_id) + + def test_termination_removal_records_disconnect(self): + # Removing a termination from a Cable (rather than deleting the Cable) is recorded by + # CableTermination.delete(); the disconnect must not be logged twice or logged as a no-op. + cable = self._connect(self.interface1, self.interface2) + cable_pk = cable.pk + + request = self._request() + with event_tracking(request): + cable = Cable.objects.get(pk=cable_pk) + cable.b_terminations = [self.interface3] + cable.save() + + changes = self._updates(self.interface2).filter(request_id=request.id) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes[0].prechange_data['cable'], cable_pk) + self.assertIsNone(changes[0].postchange_data['cable']) + + def test_terminating_object_deletion_records_no_update(self): + # The cascade from deleting the terminating object itself reaches the same handler, but the object + # is on its way out: it must be left alone rather than resurrected as an update. + self._connect(self.interface1, self.interface2) + + request = self._request() + with event_tracking(request): + Interface.objects.get(pk=self.interface1.pk).delete() + + self.assertFalse(self._updates(self.interface1).filter(request_id=request.id).exists()) + + class VirtualDeviceContextTestCase(TestCase): @classmethod From ab55a765669a435fdd08856a3a1fc94f29b91a4c Mon Sep 17 00:00:00 2001 From: Arthur Date: Thu, 10 Sep 2026 10:41:19 -0700 Subject: [PATCH 2/3] Fix #23160 - Change-log the disconnect on terminating objects when a Cable is deleted --- netbox/dcim/models/cables.py | 32 ++++++++++----- netbox/dcim/signals.py | 42 ++++++++++++++++---- netbox/dcim/tests/test_models.py | 67 +++++++++++++++++++++++++++++--- 3 files changed, 118 insertions(+), 23 deletions(-) diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index af39946f1..8ab9cafb2 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -385,20 +385,32 @@ class Cable(PrimaryModel): self._terminations_modified = False def delete(self, *args, **kwargs): - # Track this Cable as being deleted so the post_delete signal handler - # for cascaded CableTerminations can skip redundant path retracing; - # retrace_cable_paths() will retrace each affected path once after the - # Cable itself is deleted. Cache the PK locally because super().delete() - # clears self.pk before the finally block runs. The tracking set lives - # on a threading.local() to isolate concurrent deletions across threads. - if not hasattr(Cable._deletion_tracking, 'pks'): - Cable._deletion_tracking.pks = set() + # Cache the PK locally because super().delete() clears self.pk before the finally block runs. The + # tracking itself is done by the pre_delete/post_delete receivers in dcim.signals, which also cover a + # queryset delete; this pairing just guarantees the PK is discarded if the delete raises. pk = self.pk - Cable._deletion_tracking.pks.add(pk) + Cable._track_deletion(pk) try: return super().delete(*args, **kwargs) finally: - Cable._deletion_tracking.pks.discard(pk) + Cable._untrack_deletion(pk) + + @classmethod + def _track_deletion(cls, pk): + """ + Track a Cable as being deleted, so that the post_delete handler for its cascaded CableTerminations can + record the disconnect on each terminating object and skip redundant path retracing (retrace_cable_paths() + retraces each affected path once, after the Cable itself is deleted). The tracking set lives on a + threading.local() to isolate concurrent deletions across threads. + """ + if not hasattr(cls._deletion_tracking, 'pks'): + cls._deletion_tracking.pks = set() + cls._deletion_tracking.pks.add(pk) + + @classmethod + def _untrack_deletion(cls, pk): + if hasattr(cls._deletion_tracking, 'pks'): + cls._deletion_tracking.pks.discard(pk) @classmethod def _is_being_deleted(cls, pk): diff --git a/netbox/dcim/signals.py b/netbox/dcim/signals.py index 208e9f591..449aa7249 100644 --- a/netbox/dcim/signals.py +++ b/netbox/dcim/signals.py @@ -2,7 +2,7 @@ import logging from django.db import transaction from django.db.models import Q -from django.db.models.signals import post_delete, post_save, pre_save +from django.db.models.signals import post_delete, post_save, pre_delete, pre_save from django.dispatch import receiver from dcim.choices import CableEndChoices, LinkStatusChoices @@ -306,6 +306,24 @@ def retrace_cable_paths(instance, **kwargs): cablepath.retrace() +@receiver(pre_delete, sender=Cable) +def track_cable_deletion(instance, **kwargs): + """ + Flag the Cable as being deleted for nullify_connected_endpoints() below, which runs for each of its + cascaded CableTerminations. Tracking here rather than only in Cable.delete() covers a queryset delete, + which never calls the model's delete() method. + """ + Cable._track_deletion(instance.pk) + + +@receiver(post_delete, sender=Cable) +def untrack_cable_deletion(instance, **kwargs): + # Registered after retrace_cable_paths() so that the flag is still set while it runs. A delete that raises + # between the two signals leaves the PK tracked; Cable.delete() clears it in a finally, and for a queryset + # delete the transaction rolls back with only this stale entry left behind. + Cable._untrack_deletion(instance.pk) + + @receiver((post_delete, post_save), sender=PortMapping) def update_passthrough_port_paths(instance, **kwargs): """ @@ -326,8 +344,10 @@ def nullify_connected_endpoints(instance, **kwargs): # Deleting a Cable deletes its terminations in bulk, bypassing CableTermination.delete() and the # change-logged clear it performs on the terminating object; do the same here so the disconnect is - # recorded. `cable` is a SET_NULL FK which the deletion collector has already nulled by now, so restore - # the pre-delete values before snapshotting or the record shows no change. + # recorded. `cable` is a SET_NULL FK which the deletion collector has already nulled by now, so the + # pre-delete values are restored before snapshotting, or the record would show no change. They are + # restored field by field rather than via set_cable_termination(), whose Interface override would + # propagate the cable back onto the channel subinterfaces we are about to clear. termination = None if Cable._is_being_deleted(instance.cable_id): termination = model.objects.filter(pk=instance.termination_id).first() @@ -338,11 +358,17 @@ def nullify_connected_endpoints(instance, **kwargs): termination.cable_connector = instance.connector termination.cable_positions = instance.positions termination.snapshot() - termination.cable = None - termination.cable_end = None - termination.cable_connector = None - termination.cable_positions = None - termination.save() + termination.clear_cable_termination(instance) + update_fields = ['cable', 'cable_end', 'cable_connector', 'cable_positions', 'last_updated'] + + # retrace_cable_paths() tears down the originating path once the Cable itself is deleted, clearing + # _path outside the changelog. Clear it here so the recorded state doesn't outlive the path. + if isinstance(termination, PathEndpoint): + termination._path = None + update_fields.append('_path') + + # A narrow write: this row was read mid-cascade, and its full save() would pull in unrelated work + termination.save(update_fields=update_fields) else: # Already recorded by CableTermination.delete(), or the terminating object is going away too. model.objects.filter(pk=instance.termination_id).update( diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index da6041500..77b555d1b 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -2956,6 +2956,12 @@ class CableDisconnectChangeLoggingTestCase(TestCase): cls.interface3 = Interface.objects.create( device=cls.device2, name='eth1', type=InterfaceTypeChoices.TYPE_1GE_FIXED ) + cls.rear_port = RearPort.objects.create( + device=cls.device2, name='Rear Port 1', type=PortTypeChoices.TYPE_8P8C, positions=4 + ) + cls.front_port = FrontPort.objects.create( + device=cls.device2, name='Front Port 1', type=PortTypeChoices.TYPE_8P8C, positions=4 + ) cls.user = User.objects.create_user(username='testuser') def _request(self): @@ -2964,16 +2970,16 @@ class CableDisconnectChangeLoggingTestCase(TestCase): request.user = self.user return request - def _connect(self, termination_a, termination_b): + def _connect(self, termination_a, termination_b, **kwargs): with event_tracking(self._request()): - cable = Cable(a_terminations=[termination_a], b_terminations=[termination_b]) + cable = Cable(a_terminations=[termination_a], b_terminations=[termination_b], **kwargs) cable.save() return cable - def _updates(self, interface): + def _updates(self, obj): return ObjectChange.objects.filter( - changed_object_type=ObjectType.objects.get_for_model(Interface), - changed_object_id=interface.pk, + changed_object_type=ObjectType.objects.get_for_model(obj), + changed_object_id=obj.pk, action=ObjectChangeActionChoices.ACTION_UPDATE, ).order_by('time') @@ -2989,10 +2995,61 @@ class CableDisconnectChangeLoggingTestCase(TestCase): # One update for the connect, one for the disconnect self.assertEqual(self._updates(interface).count(), 2, f'No disconnect recorded for {interface}') change = self._updates(interface).last() + + # The cable attributes are cleared in the database before this is recorded, so the pre-change + # data is only correct if they were restored before the snapshot was taken self.assertEqual(change.prechange_data['cable'], cable_pk) + self.assertIn(change.prechange_data['cable_end'], (CableEndChoices.SIDE_A, CableEndChoices.SIDE_B)) + self.assertIsNone(change.postchange_data['cable']) self.assertIsNone(change.postchange_data['cable_end']) + # The originating path is deleted along with the Cable, outside the changelog + self.assertIsNone(change.postchange_data['_path']) + self.assertIsNone(Interface.objects.get(pk=interface.pk).cable_id) + self.assertFalse(CablePath.objects.exists()) + + def test_profiled_cable_deletion_records_connector_and_positions(self): + cable = self._connect(self.interface1, self.rear_port, profile=CableProfileChoices.SINGLE_1C4P) + cable_pk = cable.pk + connector = Interface.objects.get(pk=self.interface1.pk).cable_connector + positions = Interface.objects.get(pk=self.interface1.pk).cable_positions + self.assertIsNotNone(connector) + self.assertTrue(positions) + + with event_tracking(self._request()): + Cable.objects.get(pk=cable_pk).delete() + + change = self._updates(self.interface1).last() + self.assertEqual(change.prechange_data['cable_connector'], connector) + self.assertEqual(change.prechange_data['cable_positions'], positions) + self.assertIsNone(change.postchange_data['cable_connector']) + self.assertIsNone(change.postchange_data['cable_positions']) + + def test_queryset_deletion_records_disconnect(self): + # A queryset delete never calls Cable.delete(), so the disconnect is tracked by a pre_delete receiver + cable = self._connect(self.interface1, self.interface2) + cable_pk = cable.pk + + with event_tracking(self._request()): + Cable.objects.filter(pk=cable_pk).delete() + + change = self._updates(self.interface1).last() + self.assertEqual(change.prechange_data['cable'], cable_pk) + self.assertIsNone(change.postchange_data['cable']) + + def test_non_path_endpoint_termination(self): + # A front port carries no _path of its own; the disconnect is recorded the same way + cable = self._connect(self.interface1, self.front_port) + cable_pk = cable.pk + + with event_tracking(self._request()): + Cable.objects.get(pk=cable_pk).delete() + + change = self._updates(self.front_port).last() + self.assertEqual(change.prechange_data['cable'], cable_pk) + self.assertIsNone(change.postchange_data['cable']) + self.assertIsNone(FrontPort.objects.get(pk=self.front_port.pk).cable_id) def test_termination_removal_records_disconnect(self): # Removing a termination from a Cable (rather than deleting the Cable) is recorded by From e864528795f9d938931b1c3143b93e3aecb59bb2 Mon Sep 17 00:00:00 2001 From: Arthur Date: Thu, 10 Sep 2026 11:27:03 -0700 Subject: [PATCH 3/3] Fix #23160 - Change-log the disconnect on terminating objects when a Cable is deleted --- netbox/dcim/models/cables.py | 24 ++++++++++++++++--- netbox/dcim/signals.py | 45 +++++++++++++++--------------------- 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index 8ab9cafb2..5d7646da5 100644 --- a/netbox/dcim/models/cables.py +++ b/netbox/dcim/models/cables.py @@ -74,13 +74,29 @@ class CableBundle(PrimaryModel): # Cables # +class CableQuerySet(RestrictedQuerySet): + + def delete(self): + # Track these Cables as being deleted for the duration, as Cable.delete() does for a single + # instance: a queryset delete never calls it. Between them the two cover every deletion, as a + # Cable is never itself cascade-deleted (nothing points at it with on_delete=CASCADE). + pks = list(self.values_list('pk', flat=True)) + for pk in pks: + Cable._track_deletion(pk) + try: + return super().delete() + finally: + for pk in pks: + Cable._untrack_deletion(pk) + + class Cable(PrimaryModel): """ A physical connection between two endpoints. """ - # Per-thread tracking of Cable PKs currently in delete(); referenced by - # dcim.signals.nullify_connected_endpoints to skip per-CableTermination - # cable path retracing during cascade (retrace_cable_paths handles it once). + # Per-thread tracking of Cable PKs currently being deleted; referenced by + # dcim.signals.nullify_connected_endpoints to record the disconnect on each terminating object and + # to skip per-CableTermination path retracing during the cascade (retrace_cable_paths does it once). _deletion_tracking = threading.local() type = models.CharField( @@ -150,6 +166,8 @@ class Cable(PrimaryModel): clone_fields = ('tenant', 'type', 'profile', 'bundle') + objects = CableQuerySet.as_manager() + class Meta: ordering = ('pk',) verbose_name = _('cable') diff --git a/netbox/dcim/signals.py b/netbox/dcim/signals.py index 449aa7249..682e11721 100644 --- a/netbox/dcim/signals.py +++ b/netbox/dcim/signals.py @@ -2,7 +2,7 @@ import logging from django.db import transaction from django.db.models import Q -from django.db.models.signals import post_delete, post_save, pre_delete, pre_save +from django.db.models.signals import post_delete, post_save, pre_save from django.dispatch import receiver from dcim.choices import CableEndChoices, LinkStatusChoices @@ -306,24 +306,6 @@ def retrace_cable_paths(instance, **kwargs): cablepath.retrace() -@receiver(pre_delete, sender=Cable) -def track_cable_deletion(instance, **kwargs): - """ - Flag the Cable as being deleted for nullify_connected_endpoints() below, which runs for each of its - cascaded CableTerminations. Tracking here rather than only in Cable.delete() covers a queryset delete, - which never calls the model's delete() method. - """ - Cable._track_deletion(instance.pk) - - -@receiver(post_delete, sender=Cable) -def untrack_cable_deletion(instance, **kwargs): - # Registered after retrace_cable_paths() so that the flag is still set while it runs. A delete that raises - # between the two signals leaves the PK tracked; Cable.delete() clears it in a finally, and for a queryset - # delete the transaction rolls back with only this stale entry left behind. - Cable._untrack_deletion(instance.pk) - - @receiver((post_delete, post_save), sender=PortMapping) def update_passthrough_port_paths(instance, **kwargs): """ @@ -350,7 +332,13 @@ def nullify_connected_endpoints(instance, **kwargs): # propagate the cable back onto the channel subinterfaces we are about to clear. termination = None if Cable._is_being_deleted(instance.cable_id): - termination = model.objects.filter(pk=instance.termination_id).first() + # The change record serializes the object twice (before and after), and ComponentModel.save() + # re-caches its denormalized references off the parent device: fetch both up front so neither + # costs a round trip per terminating object. + queryset = model.objects.filter(pk=instance.termination_id).prefetch_related('tags') + if hasattr(model, 'device'): + queryset = queryset.select_related('device__site', 'device__location', 'device__rack') + termination = queryset.first() if termination is not None: termination.cable_id = instance.cable_id @@ -358,6 +346,7 @@ def nullify_connected_endpoints(instance, **kwargs): termination.cable_connector = instance.connector termination.cable_positions = instance.positions termination.snapshot() + # clear_cable_termination() also clears the mirrored attributes on any channel subinterfaces termination.clear_cable_termination(instance) update_fields = ['cable', 'cable_end', 'cable_connector', 'cable_positions', 'last_updated'] @@ -378,13 +367,15 @@ def nullify_connected_endpoints(instance, **kwargs): cable_positions=None, ) - # If the removed termination was a channelized interface, also clear the cable attributes mirrored onto its channel - # subinterfaces. This must happen before the retrace below so that each channel's (now dead) path is torn down - # rather than rebuilt from a stale cable reference. - if model is Interface: - Interface.objects.filter(parent_id=instance.termination_id, channel_id__isnull=False).update( - cable=None, cable_end='', cable_connector=None, cable_positions=None - ) + # If the removed termination was a channelized interface, also clear the cable attributes mirrored onto + # its channel subinterfaces. This must happen before the retrace below so that each channel's (now dead) + # path is torn down rather than rebuilt from a stale cable reference. These writes are deliberately not + # change-logged: propagate_channel_cables() doesn't log the mirrored attributes when it sets them + # either, so a channel subinterface has no recorded cable state for this to contradict. + if model is Interface: + Interface.objects.filter(parent_id=instance.termination_id, channel_id__isnull=False).update( + cable=None, cable_end='', cable_connector=None, cable_positions=None + ) # If the parent Cable is being deleted in this same operation, skip the # per-termination retrace; retrace_cable_paths() will retrace each affected