From 2f081f8fb44c31a1d8b2b1b81a29ddd4715af900 Mon Sep 17 00:00:00 2001 From: Martin Hauser Date: Wed, 16 Sep 2026 16:13:51 +0200 Subject: [PATCH] fix(dcim): Retire superseded Cable Paths on Termination changes Delete any path already originating at a node before tracing a fresh one from it, so that replacing, extending or recreating a Cable's Terminations no longer leaves the rows it supersedes behind. Repeated changes now converge on the paths the current terminations trace. Retire from the expanded origins so that a channelized end retires its channels' paths rather than the parent's, which has none, and write the replacements under a transaction so a failed trace cannot leave an origin with neither its old path nor a new one. Add regression coverage for replaced, extended and channelized ends and for repeated identical REST writes. Fixes #23121 --- netbox/dcim/tests/test_api.py | 28 ++++++++ netbox/dcim/tests/test_cablepaths.py | 91 ++++++++++++++++++++++++ netbox/dcim/tests/test_cablepaths2.py | 65 +++++++++++++++++ netbox/dcim/tests/test_channelization.py | 84 ++++++++++++++++++++++ netbox/dcim/tests/utils.py | 15 ++++ netbox/dcim/utils.py | 74 +++++++++++++++---- 6 files changed, 342 insertions(+), 15 deletions(-) diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index 3e7287597..3754edff2 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -4741,6 +4741,34 @@ class CableTestCase(APIViewTestCases.APIViewTestCase): }, ] + def test_repeated_put_does_not_accumulate_paths(self): + """ + Repeating an identical PUT must leave the cable with the two paths its terminations trace. + """ + self.add_permissions('dcim.change_cable') + cable = Cable.objects.get(label='Cable 1') + interface_a = Interface.objects.get(cable=cable, cable_end=CableEndChoices.SIDE_A) + interface_b = Interface.objects.get(cable=cable, cable_end=CableEndChoices.SIDE_B) + data = { + 'status': cable.status, + 'a_terminations': [{'object_type': 'dcim.interface', 'object_id': interface_a.pk}], + 'b_terminations': [{'object_type': 'dcim.interface', 'object_id': interface_b.pk}], + } + + previous = set() + for attempt in range(3): + with self.subTest(attempt=attempt): + response = self.client.put(self._get_detail_url(cable), data, format='json', **self.header) + + self.assertHttpStatus(response, status.HTTP_200_OK) + for interface in (interface_a, interface_b): + self.assertTrue(Interface.objects.get(pk=interface.pk)._path.is_complete) + paths = set(CablePath.objects.filter(_nodes__contains=cable).values_list('pk', flat=True)) + self.assertEqual(len(paths), 2) + # A write that retraced nothing would hold the count at 2 without reaching the retirement + self.assertFalse(paths & previous, msg='the PUT retraced nothing, so the count proves nothing') + previous = paths + def test_graphql_cable_termination_cached_filters(self): """ Validate filtering cables by cached CableTermination relations via GraphQL: diff --git a/netbox/dcim/tests/test_cablepaths.py b/netbox/dcim/tests/test_cablepaths.py index ec7de6020..abe794746 100644 --- a/netbox/dcim/tests/test_cablepaths.py +++ b/netbox/dcim/tests/test_cablepaths.py @@ -1,3 +1,7 @@ +from io import StringIO + +from django.core.management import call_command + from circuits.models import * from dcim.choices import LinkStatusChoices from dcim.models import * @@ -2891,6 +2895,93 @@ class LegacyCablePathTestCase(BaseCablePathTestCase): # Verify _path is cleared on removed interface (#21127) interface3.refresh_from_db() self.assertPathIsNotSet(interface3) + self.assertEqual(CablePath.objects.count(), 2) + + def test_304_replacing_a_termination_retires_superseded_paths(self): + """ + [IF1] --C1-- [IF2] becomes [IF1] --C1-- [IF3], and back again + + Each replacement must leave only the two paths the cable's current terminations trace. + """ + interface1 = Interface.objects.create(device=self.device, name='Interface 1') + interface2 = Interface.objects.create(device=self.device, name='Interface 2') + interface3 = Interface.objects.create(device=self.device, name='Interface 3') + + cable1 = Cable(a_terminations=[interface1], b_terminations=[interface2]) + cable1.save() + self.assertEqual(CablePath.objects.count(), 2) + + for peer, detached in ((interface3, interface2), (interface2, interface3)): + with self.subTest(peer=peer.name): + cable1 = Cable.objects.get(pk=cable1.pk) + cable1.b_terminations = [peer] + cable1.full_clean() + cable1.save() + + self.assertCurrentPathExists((interface1, cable1, peer), is_complete=True, is_active=True) + self.assertCurrentPathExists((peer, cable1, interface1), is_complete=True, is_active=True) + self.assertEqual(CablePath.objects.count(), 2) + detached.refresh_from_db() + self.assertIsNone(detached.cable) + self.assertPathIsNotSet(detached) + + def test_305_adding_a_termination_retires_superseded_paths(self): + """ + [IF1] --C1-- [IF2] gains a second B-side termination [IF3] + + Extending an end must retire the paths whose destinations the extension supersedes. + """ + interface1 = Interface.objects.create(device=self.device, name='Interface 1') + interface2 = Interface.objects.create(device=self.device, name='Interface 2') + interface3 = Interface.objects.create(device=self.device, name='Interface 3') + + cable1 = Cable(a_terminations=[interface1], b_terminations=[interface2]) + cable1.save() + self.assertEqual(CablePath.objects.count(), 2) + + cable1 = Cable.objects.get(pk=cable1.pk) + cable1.b_terminations = [interface2, interface3] + cable1.full_clean() + cable1.save() + + self.assertCurrentPathExists( + (interface1, cable1, [interface2, interface3]), is_complete=True, is_active=True + ) + path2 = self.assertPathExists( + ([interface2, interface3], cable1, interface1), is_complete=True, is_active=True + ) + for interface in (interface2, interface3): + interface.refresh_from_db() + self.assertPathIsSet(interface, path2) + self.assertEqual(CablePath.objects.count(), 2) + + def test_306_retracing_one_joint_origin_keeps_its_sibling_pathed(self): + """ + [IF1] --C1-- [IF2] + [IF3] + + trace_paths retraces one endpoint at a time, so retiring a shared origin hop must restore the + co-origins it leaves behind. + """ + interface1 = Interface.objects.create(device=self.device, name='Interface 1') + interface2 = Interface.objects.create(device=self.device, name='Interface 2') + interface3 = Interface.objects.create(device=self.device, name='Interface 3') + + cable1 = Cable(a_terminations=[interface1], b_terminations=[interface2, interface3]) + cable1.save() + self.assertEqual(CablePath.objects.count(), 2) + + # trace_paths selects on a null _path + Interface.objects.filter(pk=interface2.pk).update(_path=None) + + call_command('trace_paths', no_input=True, stdout=StringIO()) + + for interface in (interface1, interface2, interface3): + interface.refresh_from_db() + self.assertIsNotNone(interface._path_id, msg=f'{interface} left without a path') + self.assertEqual( + CablePath.objects.count(), 3, msg='the joint hop should split into one path per co-origin' + ) def test_401_exclude_midspan_devices(self): """ diff --git a/netbox/dcim/tests/test_cablepaths2.py b/netbox/dcim/tests/test_cablepaths2.py index 5732917a4..8efef11f7 100644 --- a/netbox/dcim/tests/test_cablepaths2.py +++ b/netbox/dcim/tests/test_cablepaths2.py @@ -2785,3 +2785,68 @@ class CablePathTestCase(BaseCablePathTestCase): set(CableTermination.objects.filter(cable=cable1).values_list('pk', flat=True)), termination_pks ) + + def test_311_moving_a_midspan_termination_preserves_far_end_paths(self): + """ + [IF1] --C1-- [FP1][RP1] --C3-- [RP2][FP2] --C2-- [IF2] + becomes + [IF1] --C1-- [FP1][RP1] --C3-- [RP3][FP3] --C4-- [IF3] + + Rear ports originate no path, so moving a mid-span cable's end must retrace the rows which traverse it + rather than delete them: their origins cannot be recovered from the cable's own terminations. + """ + interfaces = [ + Interface.objects.create(device=self.device, name=f'Interface {i}') for i in range(1, 4) + ] + rear_ports = [ + RearPort.objects.create(device=self.device, name=f'Rear Port {i}') for i in range(1, 4) + ] + front_ports = [ + FrontPort.objects.create(device=self.device, name=f'Front Port {i}') for i in range(1, 4) + ] + for front_port, rear_port in zip(front_ports, rear_ports): + PortMapping.objects.create( + device=self.device, + front_port=front_port, + front_port_position=1, + rear_port=rear_port, + rear_port_position=1 + ) + + cable1 = Cable(a_terminations=[interfaces[0]], b_terminations=[front_ports[0]]) + cable1.clean() + cable1.save() + cable2 = Cable(a_terminations=[front_ports[1]], b_terminations=[interfaces[1]]) + cable2.clean() + cable2.save() + cable3 = Cable(a_terminations=[rear_ports[0]], b_terminations=[rear_ports[1]]) + cable3.clean() + cable3.save() + cable4 = Cable(a_terminations=[front_ports[2]], b_terminations=[interfaces[2]]) + cable4.clean() + cable4.save() + + before = ( + interfaces[0], cable1, front_ports[0], rear_ports[0], cable3, rear_ports[1], front_ports[1], cable2, + interfaces[1], + ) + self.assertCurrentPathExists(before, is_complete=True, is_active=True) + self.assertCurrentPathExists(tuple(reversed(before)), is_complete=True, is_active=True) + # Two for the completed link, one for the third interface stopping at its own rear port + self.assertEqual(CablePath.objects.count(), 3) + + cable3 = Cable.objects.get(pk=cable3.pk) + cable3.b_terminations = [rear_ports[2]] + cable3.full_clean() + cable3.save() + + after = ( + interfaces[0], cable1, front_ports[0], rear_ports[0], cable3, rear_ports[2], front_ports[2], cable4, + interfaces[2], + ) + self.assertCurrentPathExists(after, is_complete=True, is_active=True) + self.assertCurrentPathExists(tuple(reversed(after)), is_complete=True, is_active=True) + self.assertCurrentPathExists( + (interfaces[1], cable2, front_ports[1], rear_ports[1]), is_complete=False + ) + self.assertEqual(CablePath.objects.count(), 3) diff --git a/netbox/dcim/tests/test_channelization.py b/netbox/dcim/tests/test_channelization.py index 86a1d26dc..b76d67055 100644 --- a/netbox/dcim/tests/test_channelization.py +++ b/netbox/dcim/tests/test_channelization.py @@ -12,6 +12,7 @@ from rest_framework.test import APIClient from core.choices import ObjectChangeActionChoices from core.models import ObjectChange from dcim.choices import CableProfileChoices, InterfaceTypeChoices +from dcim.exceptions import UnsupportedCablePath from dcim.filtersets import InterfaceFilterSet from dcim.models import ( Cable, @@ -29,6 +30,7 @@ from dcim.svg.cables import Connector from dcim.tests.utils import BaseCablePathTestCase from users.constants import TOKEN_PREFIX from users.models import Token, User +from utilities.exceptions import AbortRequest from utilities.ordering import naturalize_interface from utilities.testing import TestCase as ViewTestCase @@ -468,6 +470,88 @@ class ChannelizedCablePathTestCase(BaseCablePathTestCase): self.assertIsNone(channel.cable_positions) self.assertPathIsNotSet(channel) + def test_114_replacing_a_far_end_retires_the_channels_superseded_paths(self): + """ + Replacing one far-end interface of a breakout cable must leave one path per channel in each direction. The + channels are the real origins, so the rows the retrace supersedes can only be found after the expansion. + """ + parent, channels = self._create_channelized_interface('et0', 4) + far = [ + Interface.objects.create(device=self.device, name=f'xe{i}', type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS) + for i in range(4) + ] + replacement = Interface.objects.create( + device=self.device, name='xe4', type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS + ) + + cable = Cable( + profile=CableProfileChoices.BREAKOUT_1C4P_4C1P, + a_terminations=[parent], + b_terminations=far, + ) + cable.clean() + cable.save() + self.assertEqual(CablePath.objects.count(), 8) + + cable = Cable.objects.get(pk=cable.pk) + cable.b_terminations = [*far[:3], replacement] + cable.clean() + cable.save() + + self.assertEqual(CablePath.objects.count(), 8) + for channel, far_iface in zip(channels, [*far[:3], replacement]): + channel.refresh_from_db() + far_iface.refresh_from_db() + forward = self.assertPathExists((channel, cable, far_iface), is_complete=True, is_active=True) + reverse = self.assertPathExists((far_iface, cable, channel), is_complete=True, is_active=True) + self.assertPathIsSet(channel, forward) + self.assertPathIsSet(far_iface, reverse) + far[3].refresh_from_db() + self.assertIsNone(far[3].cable) + self.assertPathIsNotSet(far[3]) + + def test_115_failed_channel_trace_restores_the_replaced_paths(self): + """ + A trace that raises partway through a channelized end must leave every channel's stored path in place. + """ + parent, channels = self._create_channelized_interface('et0', 2) + far = [ + Interface.objects.create(device=self.device, name=f'xe{i}', type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS) + for i in range(2) + ] + cable = Cable( + profile=CableProfileChoices.BREAKOUT_1C2P_2C1P, + a_terminations=[parent], + b_terminations=far, + ) + cable.clean() + cable.save() + self.assertEqual(CablePath.objects.count(), 4) + + stored_paths = {} + for channel in channels: + channel.refresh_from_db() + stored_paths[channel.pk] = channel._path + + traced = CablePath.from_origin + + def failing_from_origin(terminations): + if terminations and terminations[0] == channels[1]: + raise UnsupportedCablePath('Simulated trace error') + return traced(terminations) + + cable = Cable.objects.get(pk=cable.pk) + cable.b_terminations = far + with mock.patch.object(CablePath, 'from_origin', side_effect=failing_from_origin): + with self.assertRaises(AbortRequest): + cable.save() + + # Reaching a query at all proves the block kept its savepoint + self.assertEqual(CablePath.objects.count(), 4) + for channel in channels: + channel.refresh_from_db() + self.assertPathIsSet(channel, stored_paths[channel.pk]) + class ChannelizedInterfaceTestCase(TestCase): """ diff --git a/netbox/dcim/tests/utils.py b/netbox/dcim/tests/utils.py index 5fe270b34..aa929a8c0 100644 --- a/netbox/dcim/tests/utils.py +++ b/netbox/dcim/tests/utils.py @@ -64,6 +64,21 @@ class BaseCablePathTestCase(TestCase): cablepath = self._get_cablepath(nodes, **kwargs) self.assertIsNone(cablepath, msg='Unexpected CablePath found') + def assertCurrentPathExists(self, nodes, **kwargs): + """ + Assert that the first node references a CablePath with the given route via _path, and return it. + + :param nodes: Iterable of steps, the first being the originating path endpoint object + """ + origin = type(nodes[0]).objects.get(pk=nodes[0].pk) + self.assertIsNotNone(origin._path_id, msg=f'No path set on originating endpoint {origin}') + # Matched on the route alone, so a flag mismatch does not report itself as a wrong route + cablepath = self._get_cablepath(nodes, pk=origin._path_id) + self.assertIsNotNone(cablepath, msg=f'Path #{origin._path_id} on {origin} does not match the expected route') + for attr, expected in kwargs.items(): + self.assertEqual(getattr(cablepath, attr), expected, msg=f'Path #{cablepath.pk} on {origin}: {attr}') + return cablepath + def assertPathIsSet(self, origin, cablepath, msg=None): """ Assert that a specific CablePath instance is set as the path on the origin. diff --git a/netbox/dcim/utils.py b/netbox/dcim/utils.py index 097774d7a..9db8cc02a 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -125,13 +125,36 @@ def path_node_to_object(repr): return ct.model_class().objects.filter(pk=object_id).first() +def retire_superseded_paths(objects): + """ + Delete any CablePath which originates at one of the given nodes, so that a fresh trace from them + does not leave the row it replaces behind. Returns the origin hops those deletions left unpathed. + """ + from dcim.models import CablePath + + nodes = {object_to_path_node(obj) for obj in objects} + orphaned = [] + + # `overlap` takes the encoded nodes directly, and matches nothing for an empty set + for cp in CablePath.objects.filter(_nodes__overlap=list(nodes)): + # `_nodes` matches a node anywhere in a path, including as another path's destination + if cp.path and nodes.intersection(cp.path[0]): + # CablePath.delete() clears `_path` on every origin in the hop + if leftover := [node for node in cp.path[0] if node not in nodes]: + orphaned.append(leftover) + cp.delete() + + return orphaned + + def create_cablepaths(objects): """ - Create CablePaths for all paths originating from the specified set of nodes. + Create CablePaths for all paths originating from the specified set of nodes, retiring any path which + already originates there. :param objects: Iterable of cabled objects (e.g. Interfaces) """ - from dcim.models import CablePath, Interface + from dcim.models import CablePath, Interface, PathEndpoint # Expand any channelized interface into its channel subinterfaces. A channelized parent originates no path of its # own; instead, each channel subinterface traces independently from the single connector position it occupies. @@ -144,20 +167,41 @@ def create_cablepaths(objects): else: expanded.append(obj) - # Arrange objects by cable connector. All objects with a null connector are grouped together. Channel - # subinterfaces must each originate their own path, as sharing a connector would otherwise collapse a group of - # siblings into a single malformed path. - origins = defaultdict(list) - for obj in expanded: - if isinstance(obj, Interface) and obj.channel_id: - if cp := CablePath.from_origin([obj]): - cp.save() - else: - origins[obj.cable_connector].append(obj) + # The savepoint must stay: Cable.save() turns UnsupportedCablePath into AbortRequest and callers keep querying + with transaction.atomic(using=router.db_for_write(CablePath)): - for connector, objects in origins.items(): - if cp := CablePath.from_origin(objects): - cp.save() + # Arrange objects by cable connector. All objects with a null connector are grouped together. Channel + # subinterfaces must each originate their own path, as sharing a connector would otherwise collapse a + # group of siblings into a single malformed path. + origins = defaultdict(list) + orphaned = [] + for obj in expanded: + if isinstance(obj, Interface) and obj.channel_id: + # Trace first, so an unsupported topology raises before anything is deleted + cp = CablePath.from_origin([obj]) + orphaned += retire_superseded_paths([obj]) + if cp: + cp.save() + else: + origins[obj.cable_connector].append(obj) + + for connector, objects in origins.items(): + cp = CablePath.from_origin(objects) + orphaned += retire_superseded_paths(objects) + if cp: + cp.save() + + # A caller may hold only part of an origin hop + # A regroup would lose the stored node order and the channel separation + for hop in orphaned: + recovered = [] + for node in hop: + obj = path_node_to_object(node) + # Only a PathEndpoint carries the back-reference that marks an origin already recovered + if isinstance(obj, PathEndpoint) and obj.link and obj._path_id is None: + recovered.append(obj) + if cp := CablePath.from_origin(recovered): + cp.save() def rebuild_paths(terminations):