Merge 2f081f8fb4 into 2f79d987a8

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:

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):
         """

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)

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):
     """

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.

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):