Просмотр исходного кода

fix(dcim): Handle empty and mutated Cable Termination assignments

Copy cached termination lists on read and materialize iterables on write
so caller-owned mutations cannot bypass change detection.

Mark uncached ends as modified so clearing an end is persisted without
replacing the opposite end's termination rows. Preserve path rebuilding
when termination rows have already been updated during change replay.

Add model, REST API, and cable-path regression coverage.

Fixes #23094
Martin Hauser 19 часов назад
Родитель
Сommit
6318e7d0d3

+ 7 - 2
netbox/dcim/models/cables.py

@@ -241,7 +241,8 @@ class Cable(PrimaryModel):
         attr = f'_{side.lower()}_terminations'
 
         if hasattr(self, attr):
-            return getattr(self, attr)
+            # Return a copy so a caller mutating the result cannot defeat change detection
+            return list(getattr(self, attr))
         if not self.pk:
             return []
         return [
@@ -257,6 +258,9 @@ class Cable(PrimaryModel):
             raise ValueError(f"Unknown cable side: {side}")
         _attr = f'_{side.lower()}_terminations'
 
+        # Materialize first, so the stored list is our own and a single-pass iterable is read only once
+        value = list(value)
+
         # If the provided value is a list of CableTermination IDs, resolve them
         # to their corresponding termination objects.
         if all(isinstance(item, int) for item in value):
@@ -264,7 +268,8 @@ class Cable(PrimaryModel):
                 ct.termination for ct in CableTermination.objects.filter(pk__in=value).prefetch_related('termination')
             ]
 
-        if not self.pk or getattr(self, _attr, []) != list(value):
+        # An uncached end always counts as assigned: replayed rows may already match while paths still need rebuilding
+        if not self.pk or not hasattr(self, _attr) or getattr(self, _attr) != value:
             self._terminations_modified = True
 
         setattr(self, _attr, value)

+ 33 - 0
netbox/dcim/tests/test_api.py

@@ -5098,6 +5098,39 @@ class CableTestCase(APIViewTestCases.APIViewTestCase):
                     self.assertTrue(Interface.objects.get(pk=interface.pk)._path.is_complete)
                 self.assertEqual(CablePath.objects.filter(_nodes__contains=cable).count(), 2)
 
+    @tag('regression')  # Issue #23094
+    def test_patch_clearing_an_end_keeps_the_other_end(self):
+        """
+        A PATCH with an empty termination list must detach that end only and keep the other end's row.
+        """
+        self.add_permissions('dcim.change_cable')
+        for label, attr, cleared_side, kept_side in (
+            ('Cable 1', 'a_terminations', CableEndChoices.SIDE_A, CableEndChoices.SIDE_B),
+            ('Cable 2', 'b_terminations', CableEndChoices.SIDE_B, CableEndChoices.SIDE_A),
+        ):
+            with self.subTest(attr=attr):
+                cable = Cable.objects.get(label=label)
+                cleared = Interface.objects.get(cable=cable, cable_end=cleared_side)
+                kept = Interface.objects.get(cable=cable, cable_end=kept_side)
+                kept_row_pk = CableTermination.objects.get(cable=cable, cable_end=kept_side).pk
+
+                response = self.client.patch(self._get_detail_url(cable), {attr: []}, format='json', **self.header)
+
+                self.assertHttpStatus(response, status.HTTP_200_OK)
+                self.assertEqual(
+                    list(
+                        CableTermination.objects.filter(cable=cable)
+                        .values_list('pk', 'cable_end', 'termination_id')
+                    ),
+                    [(kept_row_pk, kept_side, kept.pk)]
+                )
+                cleared.refresh_from_db()
+                self.assertIsNone(cleared.cable)
+                self.assertIsNone(cleared._path_id)
+                kept.refresh_from_db()
+                self.assertEqual(kept.cable, cable)
+                self.assertFalse(kept._path.is_complete)
+
     def test_graphql_cable_termination_cached_filters(self):
         """
         Validate filtering cables by cached CableTermination relations via GraphQL:

+ 35 - 0
netbox/dcim/tests/test_cablepaths.py

@@ -3452,6 +3452,41 @@ class LegacyCablePathTestCase(BaseCablePathTestCase):
             self.assertCurrentPathExists((interface, cable, interface1), is_complete=True)
         self.assertEqual(CablePath.objects.count(), 4)
 
+    def test_322_replayed_termination_move_rebuilds_paths(self):
+        """
+        [IF1] --C1-- [IF2] becomes [IF1] --C1-- [IF3]
+
+        Assigning a replaced end's serialized terminations to a fresh instance must rebuild the paths from its rows.
+        """
+        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()
+
+        # Replace the B end's row directly, as change replay does before it saves the cable itself
+        CableTermination.objects.get(cable=cable1, cable_end=CableEndChoices.SIDE_B).delete()
+        CableTermination(cable=cable1, cable_end=CableEndChoices.SIDE_B, termination=interface3).save()
+        termination_pks = set(CableTermination.objects.filter(cable=cable1).values_list('pk', flat=True))
+        interface3.refresh_from_db()
+        self.assertPathIsNotSet(interface3)
+
+        data = cable1.serialize_object()
+        cable1 = Cable.objects.get(pk=cable1.pk)
+        cable1.b_terminations = data['b_terminations']
+        cable1.full_clean()
+        cable1.save()
+
+        self.assertCurrentPathExists((interface1, cable1, interface3), is_complete=True, is_active=True)
+        self.assertCurrentPathExists((interface3, cable1, interface1), is_complete=True, is_active=True)
+        self.assertEqual(
+            set(CableTermination.objects.filter(cable=cable1).values_list('pk', flat=True)),
+            termination_pks
+        )
+        interface2.refresh_from_db()
+        self.assertIsNone(interface2.cable)
+        self.assertPathIsNotSet(interface2)
+
     def test_401_exclude_midspan_devices(self):
         """
         [IF1] --C1-- [FP1][Test Device][RP1] --C2-- [RP2][Test Device][FP2] --C3-- [IF2]

+ 157 - 0
netbox/dcim/tests/test_models.py

@@ -2626,6 +2626,163 @@ class CableTestCase(TestCase):
             if ct.termination in original_cts:
                 self.assertEqual(ct.pk, original_cts[ct.termination])
 
+    @tag('regression')  # #23094
+    def test_clearing_an_end_of_a_fresh_instance_removes_its_terminations(self):
+        """
+        Assigning an empty list to an end of a freshly loaded cable must delete that end's terminations only.
+        """
+        interface1 = Interface.objects.get(device__name='TestDevice1', name='eth0')
+        interface2 = Interface.objects.get(device__name='TestDevice2', name='eth0')
+        cable_pk = Cable.objects.first().pk
+
+        for attr, kept_side, cleared, kept in (
+            ('a_terminations', CableEndChoices.SIDE_B, interface1, interface2),
+            ('b_terminations', CableEndChoices.SIDE_A, interface2, interface1),
+        ):
+            with self.subTest(attr=attr):
+                cable = Cable.objects.get(pk=cable_pk)
+                kept_row_pk = CableTermination.objects.get(cable=cable, cable_end=kept_side).pk
+                setattr(cable, attr, [])
+                self.assertTrue(cable._terminations_modified)
+                cable.full_clean()
+                cable.save()
+
+                self.assertEqual(
+                    list(CableTermination.objects.filter(cable=cable).values_list('pk', 'cable_end')),
+                    [(kept_row_pk, kept_side)]
+                )
+                cleared.refresh_from_db()
+                self.assertIsNone(cleared.cable)
+                self.assertIsNone(cleared._path_id)
+                kept.refresh_from_db()
+                self.assertEqual(kept.cable, cable)
+                self.assertFalse(kept._path.is_complete)
+
+                # Reconnect the cleared end so the other side starts from a complete cable
+                cable = Cable.objects.get(pk=cable_pk)
+                setattr(cable, attr, [cleared])
+                cable.save()
+
+        # A profiled cable keeps the other end's rows and connectors
+        cable, a_interfaces, b_interfaces = self._create_multiposition_cable()
+        a_row_pks = set(cable.terminations.filter(cable_end=CableEndChoices.SIDE_A).values_list('pk', flat=True))
+        cable = Cable.objects.get(pk=cable.pk)
+        cable.b_terminations = []
+        cable.full_clean()
+        cable.save()
+
+        self.assertEqual(self._get_connectors(cable, 'B'), [])
+        self.assertEqual(self._get_connectors(cable, 'A'), list(enumerate(a_interfaces, start=1)))
+        self.assertEqual(
+            set(cable.terminations.filter(cable_end=CableEndChoices.SIDE_A).values_list('pk', flat=True)), a_row_pks
+        )
+
+    @tag('regression')  # #23094
+    def test_reassigning_a_mutated_termination_list_flags_a_change(self):
+        """
+        Appending to a retrieved termination list and assigning it back must be applied on save.
+        """
+        cable = Cable.objects.first()
+        interface1 = Interface.objects.get(device__name='TestDevice1', name='eth0')
+        interface3 = Interface.objects.create(device=interface1.device, name='eth1')
+
+        # Assigning warms the A end's cache and the save resets the flag
+        cable.a_terminations = [interface1]
+        cable.save()
+        self.assertFalse(cable._terminations_modified)
+
+        terminations = cable.a_terminations
+        terminations.append(interface3)
+        cable.a_terminations = terminations
+        self.assertTrue(cable._terminations_modified)
+        cable.full_clean()
+        cable.save()
+
+        self.assertEqual(
+            [ct.termination for ct in cable.terminations.filter(cable_end=CableEndChoices.SIDE_A)],
+            [interface1, interface3]
+        )
+
+    @tag('regression')  # #23094
+    def test_assigned_termination_list_is_copied(self):
+        """
+        Mutating a list after assigning it to an end must not change the cable's terminations.
+        """
+        interface1 = Interface.objects.get(device__name='TestDevice1', name='eth0')
+        interface3 = Interface.objects.get(device__name='TestDevice2', name='eth1')
+        cable = Cable.objects.first()
+
+        terminations = [interface1]
+        cable.a_terminations = terminations
+        terminations.append(interface3)
+
+        self.assertEqual(cable.a_terminations, [interface1])
+
+    @tag('regression')  # #23094
+    def test_assigning_an_iterable_stores_a_list_of_its_terminations(self):
+        """
+        Single-pass iterables and querysets assigned to an end must be stored as a list of the resolved terminations.
+        """
+        interface1 = Interface.objects.get(device__name='TestDevice1', name='eth0')
+        interface3 = Interface.objects.get(device__name='TestDevice2', name='eth1')
+        row_pk = CableTermination.objects.get(cable_end=CableEndChoices.SIDE_A).pk
+
+        for label, value, expected in (
+            ('iterator of objects', iter([interface1, interface3]), [interface1, interface3]),
+            ('iterator of IDs', iter([row_pk]), [interface1]),
+            ('queryset', Interface.objects.filter(pk=interface1.pk), [interface1]),
+        ):
+            with self.subTest(value=label):
+                cable = Cable.objects.first()
+                cable.a_terminations = value
+                self.assertIsInstance(cable._a_terminations, list)
+                self.assertEqual(cable.a_terminations, expected)
+
+    def test_assigning_terminations_to_a_fresh_instance_flags_a_change(self):
+        """
+        Assigning an end of a freshly loaded cable must flag a change, even when it repeats the stored terminations.
+        """
+        interface1 = Interface.objects.get(device__name='TestDevice1', name='eth0')
+        interface2 = Interface.objects.get(device__name='TestDevice2', name='eth0')
+        data = Cable.objects.first().serialize_object()
+
+        # Change replay writes the CableTermination rows first and relies on this to retrace the paths
+        for attr, interface in (('a_terminations', interface1), ('b_terminations', interface2)):
+            for label, value in (('objects', [interface]), ('IDs', data[attr])):
+                with self.subTest(attr=attr, value=label):
+                    cable = Cable.objects.first()
+                    setattr(cable, attr, value)
+                    self.assertEqual(getattr(cable, attr), [interface])
+                    self.assertTrue(cable._terminations_modified)
+
+    def test_reassigning_an_unchanged_end_on_a_warm_instance_does_not_flag_a_change(self):
+        """
+        Assigning the value an end already holds in memory must not flag a change or clear a pending one.
+        """
+        interface1 = Interface.objects.get(device__name='TestDevice1', name='eth0')
+        interface3 = Interface.objects.get(device__name='TestDevice2', name='eth1')
+
+        # Assigning warms the A end's cache and the save resets the flag
+        cable = Cable.objects.first()
+        cable.a_terminations = [interface1]
+        cable.save()
+        self.assertFalse(cable._terminations_modified)
+        termination_pks = set(CableTermination.objects.filter(cable=cable).values_list('pk', flat=True))
+        path_pks = set(CablePath.objects.filter(_nodes__contains=cable).values_list('pk', flat=True))
+
+        cable.a_terminations = [interface1]
+        self.assertFalse(cable._terminations_modified)
+        cable.save()
+        self.assertEqual(
+            set(CableTermination.objects.filter(cable=cable).values_list('pk', flat=True)),
+            termination_pks
+        )
+        self.assertEqual(set(CablePath.objects.filter(_nodes__contains=cable).values_list('pk', flat=True)), path_pks)
+
+        cable.b_terminations = [interface3]
+        cable.a_terminations = [interface1]
+        self.assertTrue(cable._terminations_modified)
+
     @tag('regression')  # #21498
     def test_path_refreshes_replaced_cablepath_reference(self):
         """