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

fix(dcim): Prevent path rebuild when Cable Terminations unchanged

Compare Cable Terminations against stored values instead of the empty
cache when checking for modifications, so a freshly loaded Cable that is
resaved with the same terminations no longer rebuilds its paths. Raise
the flag whenever update_terminations() force-recreates an end, since
the edit form warms the cache that gated it and a profile change then
tore every path down without rebuilding it. Add regression tests for
both.

Fixes #23097
Martin Hauser 2 месяцев назад
Родитель
Сommit
141e74c103

+ 6 - 4
netbox/dcim/models/cables.py

@@ -244,7 +244,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):
+        # Compare against the cached terminations, falling back to the stored rows on a freshly loaded cable
+        if not self.pk or self._get_x_terminations(side) != list(value):
             self._terminations_modified = True
 
         setattr(self, _attr, value)
@@ -510,6 +511,10 @@ class Cable(PrimaryModel):
         force_a = force or self._connectors_reassigned(a_terminations, self.a_terminations)
         force_b = force or self._connectors_reassigned(b_terminations, self.b_terminations)
 
+        # Recreating either end's terminations invalidates its paths, even when the endpoints are unchanged
+        if force_a or force_b:
+            self._terminations_modified = True
+
         # When force-recreating terminations (e.g. after a profile change), cache the termination objects
         # from the database before deleting, so they are available for recreation. Without this, the
         # a_terminations/b_terminations properties would query the DB after deletion and return empty lists.
@@ -518,9 +523,6 @@ class Cable(PrimaryModel):
         if force_b and not hasattr(self, '_b_terminations'):
             self._b_terminations = list(b_terminations.keys())
 
-            # Recreating terminations invalidates existing paths, even when the endpoints are unchanged
-            self._terminations_modified = True
-
         # Delete any stale CableTerminations
         for termination, ct in a_terminations.items():
             if force_a or (termination.pk and termination not in self.a_terminations):

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

@@ -2892,6 +2892,52 @@ class LegacyCablePathTestCase(BaseCablePathTestCase):
         interface3.refresh_from_db()
         self.assertPathIsNotSet(interface3)
 
+    def test_304_resave_cable_with_unchanged_terminations(self):
+        """
+        [IF1] --C1-- [IF2]
+        """
+        interface1 = Interface.objects.create(device=self.device, name='Interface 1')
+        interface2 = Interface.objects.create(device=self.device, name='Interface 2')
+
+        cable1 = Cable(
+            a_terminations=[interface1],
+            b_terminations=[interface2]
+        )
+        cable1.save()
+
+        path_pks = set(CablePath.objects.values_list('pk', flat=True))
+        termination_pks = set(CableTermination.objects.filter(cable=cable1).values_list('pk', flat=True))
+        self.assertEqual(len(path_pks), 2)
+        self.assertEqual(len(termination_pks), 2)
+
+        # Reassign the same terminations, as the edit form does on every submission
+        cable1 = Cable.objects.get(pk=cable1.pk)
+        cable1.a_terminations = [interface1]
+        cable1.b_terminations = [interface2]
+        cable1.label = 'Renamed'
+        cable1.save()
+
+        self.assertEqual(set(CablePath.objects.values_list('pk', flat=True)), path_pks)
+        self.assertEqual(
+            set(CableTermination.objects.filter(cable=cable1).values_list('pk', flat=True)),
+            termination_pks
+        )
+
+        path1 = self.assertPathExists(
+            (interface1, cable1, interface2),
+            is_complete=True,
+            is_active=True
+        )
+        path2 = self.assertPathExists(
+            (interface2, cable1, interface1),
+            is_complete=True,
+            is_active=True
+        )
+        interface1.refresh_from_db()
+        interface2.refresh_from_db()
+        self.assertPathIsSet(interface1, path1)
+        self.assertPathIsSet(interface2, path2)
+
     def test_401_exclude_midspan_devices(self):
         """
         [IF1] --C1-- [FP1][Test Device][RP1] --C2-- [RP2][Test Device][FP2] --C3-- [IF2]

+ 46 - 0
netbox/dcim/tests/test_cablepaths2.py

@@ -2785,3 +2785,49 @@ class CablePathTestCase(BaseCablePathTestCase):
             set(CableTermination.objects.filter(cable=cable1).values_list('pk', flat=True)),
             termination_pks
         )
+
+    def test_311_change_cable_profile_after_reassigning_unchanged_terminations(self):
+        """
+        [IF1] --C1-- [IF2]
+
+        Applying a profile after the edit form re-assigned the unchanged terminations rebuilds the paths.
+        """
+        interfaces = [
+            Interface.objects.create(device=self.device, name='Interface 1'),
+            Interface.objects.create(device=self.device, name='Interface 2'),
+        ]
+
+        # Create cable 1 without a profile
+        cable1 = Cable(
+            a_terminations=[interfaces[0]],
+            b_terminations=[interfaces[1]],
+        )
+        cable1.clean()
+        cable1.save()
+        self.assertEqual(CablePath.objects.count(), 2)
+
+        # Reload, then re-assign the stored terminations as the edit form does on every submission
+        cable1 = Cable.objects.get(pk=cable1.pk)
+        cable1.a_terminations = [interfaces[0]]
+        cable1.b_terminations = [interfaces[1]]
+        self.assertFalse(cable1._terminations_modified)
+
+        cable1.profile = CableProfileChoices.SINGLE_1C1P
+        cable1.full_clean()
+        cable1.save()
+
+        path1 = self.assertPathExists(
+            (interfaces[0], cable1, interfaces[1]),
+            is_complete=True,
+            is_active=True
+        )
+        path2 = self.assertPathExists(
+            (interfaces[1], cable1, interfaces[0]),
+            is_complete=True,
+            is_active=True
+        )
+        self.assertEqual(CablePath.objects.count(), 2)
+        interfaces[0].refresh_from_db()
+        interfaces[1].refresh_from_db()
+        self.assertPathIsSet(interfaces[0], path1)
+        self.assertPathIsSet(interfaces[1], path2)

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

@@ -2413,6 +2413,33 @@ class CableTestCase(TestCase):
         with self.assertRaises(ValidationError):
             cable.clean()
 
+    def test_reassigning_unchanged_terminations_does_not_flag_a_change(self):
+        """
+        Assigning the stored terminations to a freshly loaded cable must leave them unflagged.
+        """
+        interface1 = Interface.objects.get(device__name='TestDevice1', name='eth0')
+        interface2 = Interface.objects.get(device__name='TestDevice2', name='eth0')
+
+        # A cable loaded from the database has no cached terminations
+        cable = Cable.objects.first()
+        cable.a_terminations = [interface1]
+        cable.b_terminations = [interface2]
+
+        self.assertFalse(cable._terminations_modified)
+
+    def test_reassigning_different_terminations_flags_a_change(self):
+        """
+        Assigning a different termination to a freshly loaded cable must flag the change.
+        """
+        interface1 = Interface.objects.get(device__name='TestDevice1', name='eth0')
+        interface3 = Interface.objects.get(device__name='TestDevice2', name='eth1')
+
+        cable = Cable.objects.first()
+        cable.a_terminations = [interface1]
+        cable.b_terminations = [interface3]
+
+        self.assertTrue(cable._terminations_modified)
+
     def test_partial_save_does_not_apply_an_unwritten_profile(self):
         """
         A save excluding profile must leave the terminations alone but keep the change pending.

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

@@ -5271,6 +5271,41 @@ class CableTestCase(
             [(1, interfaces[1]), (2, interfaces[0])]
         )
 
+    @tag('regression')  # Issue #23097
+    def test_edit_with_unchanged_terminations_preserves_paths(self):
+        """Editing a cable without changing its terminations must leave its paths in place."""
+        # The form's termination fields are restricted by view permission
+        self.add_permissions('dcim.change_cable', 'dcim.view_interface')
+
+        interface_a = Interface.objects.get(
+            device__name='Device 1', device__site__name='Site 1', name='Interface 1'
+        )
+        cable = interface_a.cable
+        interface_b = cable.b_terminations[0]
+        path_pks = set(CablePath.objects.filter(_nodes__contains=cable).values_list('pk', flat=True))
+        self.assertEqual(len(path_pks), 2)
+
+        data = {
+            'a_terminations': [interface_a.pk],
+            'b_terminations': [interface_b.pk],
+            'type': CableTypeChoices.TYPE_CAT6,
+            'status': LinkStatusChoices.STATUS_CONNECTED,
+            'label': 'Renamed',
+            'color': 'c0c0c0',
+        }
+        request = {
+            'path': self._get_url('edit', cable),
+            'data': post_data(data),
+        }
+        self.assertHttpStatus(self.client.post(**request), 302)
+
+        cable.refresh_from_db()
+        self.assertEqual(cable.label, 'Renamed')
+        self.assertEqual(
+            set(CablePath.objects.filter(_nodes__contains=cable).values_list('pk', flat=True)),
+            path_pks
+        )
+
 
 #
 # Connections