diff --git a/netbox/dcim/models/cables.py b/netbox/dcim/models/cables.py index af39946f1..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') @@ -385,20 +403,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 f051fe146..682e11721 100644 --- a/netbox/dcim/signals.py +++ b/netbox/dcim/signals.py @@ -323,21 +323,60 @@ 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, - ) - # 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 + # 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 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): + # 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 + termination.cable_end = instance.cable_end + 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'] + + # 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( + 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 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 # path once after the Cable is deleted. diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index 036426665..77b555d1b 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,160 @@ 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.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): + request = RequestFactory().get('/') + request.id = uuid.uuid4() + request.user = self.user + return request + + def _connect(self, termination_a, termination_b, **kwargs): + with event_tracking(self._request()): + cable = Cable(a_terminations=[termination_a], b_terminations=[termination_b], **kwargs) + cable.save() + return cable + + def _updates(self, obj): + return ObjectChange.objects.filter( + changed_object_type=ObjectType.objects.get_for_model(obj), + changed_object_id=obj.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() + + # 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 + # 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