Closes #22146: Avoid renumbering MPTT trees when creating module bays

netbox/dcim/models/device_components.py

@@ -1313,6 +1313,29 @@ class RearPort(ModularComponentModel, CabledObjectModel, TrackingModelMixin):
 # Bays
 #
 
+class ModuleBayManager(TreeManager):
+    """
+    Order ModuleBays by the natural-sort name of each tree's root, then by MPTT
+    left value within the tree. Combined with the root-insert bypass in
+    ModuleBay.save(), this lets us keep MPTTMeta.order_insertion_by for cheap
+    intra-tree sibling placement while skipping the global tree_id renumbering
+    it would otherwise perform on every root insert.
+    """
+    def get_queryset(self, *args, **kwargs):
+        # Use the raw manager to avoid recursing through this get_queryset() when
+        # building the annotation subquery.
+        root_name = self.model._objects_raw.filter(
+            tree_id=models.OuterRef('tree_id'),
+            level=0,
+        ).values('name')[:1]
+        return super().get_queryset(*args, **kwargs).annotate(
+            _root_name=models.Subquery(
+                root_name,
+                output_field=models.CharField(db_collation='natural_sort'),
+            )
+        ).order_by('_root_name', 'lft')
+
+
 class ModuleBay(ModularComponentModel, TrackingModelMixin, MPTTModel):
     """
     An empty space within a Device which can house a child device
@@ -1337,7 +1360,10 @@ class ModuleBay(ModularComponentModel, TrackingModelMixin, MPTTModel):
         default=True,
     )
 
-    objects = TreeManager()
+    objects = ModuleBayManager()
+    # Plain TreeManager used by ModuleBayManager to build the _root_name subquery
+    # without recursing through our annotated get_queryset().
+    _objects_raw = TreeManager()
 
     clone_fields = ('device', 'enabled')
 
@@ -1355,6 +1381,9 @@ class ModuleBay(ModularComponentModel, TrackingModelMixin, MPTTModel):
         verbose_name_plural = _('module bays')
 
     class MPTTMeta:
+        # Used for placing siblings within a single tree at insert time. Costs
+        # are bounded to that tree's rows. Cross-tree (root) insertion is
+        # handled in save() to avoid the O(N) tree_id shift this would trigger.
         order_insertion_by = ('name',)
 
     def clean(self):
@@ -1376,6 +1405,36 @@ class ModuleBay(ModularComponentModel, TrackingModelMixin, MPTTModel):
             self.parent = self.module.module_bay
         else:
             self.parent = None
+        opts = self._mptt_meta
+        # For new root nodes, allocate the next tree_id and pre-set MPTT fields
+        # so super().save() skips MPTT's order_insertion_by-driven insertion
+        # path. That path would otherwise UPDATE every later tree_id on each
+        # root insert (NB-2800). Children still go through MPTT, which keeps
+        # siblings in name order via the same order_insertion_by setting.
+        if self._state.adding and self.parent_id is None and not self.lft and not self.rght:
+            max_tree_id = ModuleBay._objects_raw.aggregate(
+                models.Max('tree_id')
+            )['tree_id__max'] or 0
+            self.tree_id = max_tree_id + 1
+            self.lft = 1
+            self.rght = 2
+            self.level = 0
+        elif (
+            not self._state.adding
+            and self.parent_id is None
+            and self._mptt_cached_fields.get(opts.parent_attr) is None
+        ):
+            # Existing root being updated. Spoof the cached order_insertion_by
+            # values so MPTT's same_order check passes and it skips its reorder
+            # path, which would UPDATE every later tree_id on each root rename.
+            # ModuleBayManager._root_name recovers display order at query time,
+            # so the tree_id reshuffling would be wasted work. Child renames
+            # and root<->child transitions intentionally fall through to MPTT.
+            for field_name in opts.order_insertion_by:
+                field_name = field_name.lstrip('-')
+                self._mptt_cached_fields[field_name] = opts.get_raw_field_value(
+                    self, field_name
+                )
         super().save(*args, **kwargs)
 
     @property

netbox/dcim/tests/test_models.py

@@ -915,6 +915,214 @@ class ModuleBayTestCase(TestCase):
             module_1.clean()
             module_1.save()
 
+    @tag('regression')  # #22146
+    def test_module_bay_ordering_after_recreate(self):
+        """
+        Module bays must remain in name order after a delete-and-recreate cycle,
+        even though MPTT no longer renumbers tree_ids on root insertion.
+        """
+        device_type = DeviceType.objects.first()
+        device_role = DeviceRole.objects.first()
+        site = Site.objects.first()
+        device = Device.objects.create(
+            name='Ordering Test Device',
+            device_type=device_type,
+            role=device_role,
+            site=site,
+        )
+        for name in ('Bay 1', 'Bay 2', 'Bay 3', 'Bay 4'):
+            ModuleBay.objects.create(device=device, name=name)
+
+        ModuleBay.objects.get(device=device, name='Bay 3').delete()
+        ModuleBay.objects.create(device=device, name='Bay 3')
+
+        names = list(ModuleBay.objects.filter(device=device).values_list('name', flat=True))
+        self.assertEqual(names, ['Bay 1', 'Bay 2', 'Bay 3', 'Bay 4'])
+
+    @tag('regression')  # #22146
+    def test_module_bay_natural_ordering(self):
+        """
+        Module bays must be returned in natural (numeric-aware) order, e.g.
+        "Bay 2" before "Bay 10".
+        """
+        device_type = DeviceType.objects.first()
+        device_role = DeviceRole.objects.first()
+        site = Site.objects.first()
+        device = Device.objects.create(
+            name='Natural Sort Device',
+            device_type=device_type,
+            role=device_role,
+            site=site,
+        )
+        # Insert in non-natural order to confirm sort is not insertion-driven.
+        for name in ('Bay 10', 'Bay 1', 'Bay 2', 'Bay 11'):
+            ModuleBay.objects.create(device=device, name=name)
+
+        names = list(ModuleBay.objects.filter(device=device).values_list('name', flat=True))
+        self.assertEqual(names, ['Bay 1', 'Bay 2', 'Bay 10', 'Bay 11'])
+
+    @tag('regression')  # #22146
+    def test_child_module_bay_ordering(self):
+        """
+        Child module bays inside a module must be returned in name order even
+        when inserted out of order.
+        """
+        device_type = DeviceType.objects.first()
+        device_role = DeviceRole.objects.first()
+        site = Site.objects.first()
+        device = Device.objects.create(
+            name='Child Ordering Device',
+            device_type=device_type,
+            role=device_role,
+            site=site,
+        )
+        root_bay = ModuleBay.objects.create(device=device, name='Bay 1')
+        manufacturer = Manufacturer.objects.first()
+        module_type = ModuleType.objects.create(
+            manufacturer=manufacturer, model='Child Ordering Type'
+        )
+        module = Module.objects.create(
+            device=device, module_bay=root_bay, module_type=module_type
+        )
+        # Insert children out of name order.
+        for name in ('Bay 1.1', 'Bay 1.3', 'Bay 1.2'):
+            ModuleBay.objects.create(device=device, module=module, name=name)
+
+        names = list(ModuleBay.objects.filter(device=device).values_list('name', flat=True))
+        self.assertEqual(names, ['Bay 1', 'Bay 1.1', 'Bay 1.2', 'Bay 1.3'])
+
+    @tag('regression')  # #22146
+    def test_root_module_bay_rename_preserves_tree_ids(self):
+        """
+        Renaming a root module bay must not renumber any other root tree's
+        tree_id. The renamed bay's own tree_id is also expected to remain
+        stable, but the load-bearing assertion is that the *other* bays are
+        not shifted.
+        """
+        device_type = DeviceType.objects.first()
+        device_role = DeviceRole.objects.first()
+        site = Site.objects.first()
+        device = Device.objects.create(
+            name='Rename TreeID Device',
+            device_type=device_type,
+            role=device_role,
+            site=site,
+        )
+        for name in ('Bay 1', 'Bay 2', 'Bay 3', 'Bay 4'):
+            ModuleBay.objects.create(device=device, name=name)
+
+        tree_ids_before = {
+            bay.name: bay.tree_id
+            for bay in ModuleBay.objects.filter(device=device)
+        }
+
+        bay = ModuleBay.objects.get(device=device, name='Bay 2')
+        bay.name = 'Bay 99'
+        bay.save()
+
+        tree_ids_after = {
+            bay.name: bay.tree_id
+            for bay in ModuleBay.objects.filter(device=device)
+        }
+        for name in ('Bay 1', 'Bay 3', 'Bay 4'):
+            self.assertEqual(tree_ids_after[name], tree_ids_before[name])
+        self.assertEqual(tree_ids_after['Bay 99'], tree_ids_before['Bay 2'])
+
+    @tag('regression')  # #22146
+    def test_root_module_bay_rename_updates_display_order(self):
+        """
+        Even though renaming a root module bay does not renumber tree_ids,
+        the manager's _root_name annotation must reflect the new name so the
+        display ordering is correct.
+        """
+        device_type = DeviceType.objects.first()
+        device_role = DeviceRole.objects.first()
+        site = Site.objects.first()
+        device = Device.objects.create(
+            name='Rename Order Device',
+            device_type=device_type,
+            role=device_role,
+            site=site,
+        )
+        for name in ('Bay 1', 'Bay 2', 'Bay 3'):
+            ModuleBay.objects.create(device=device, name=name)
+
+        bay = ModuleBay.objects.get(device=device, name='Bay 1')
+        bay.name = 'Bay 4'
+        bay.save()
+
+        names = list(ModuleBay.objects.filter(device=device).values_list('name', flat=True))
+        self.assertEqual(names, ['Bay 2', 'Bay 3', 'Bay 4'])
+
+    @tag('regression')  # #22146
+    def test_child_module_bay_rename_preserves_intra_tree_ordering(self):
+        """
+        Renaming a *child* module bay must still trigger MPTT's intra-tree
+        reorder, so siblings appear in name order after the rename. The
+        rename-bypass only covers root bays.
+        """
+        device_type = DeviceType.objects.first()
+        device_role = DeviceRole.objects.first()
+        site = Site.objects.first()
+        device = Device.objects.create(
+            name='Child Rename Device',
+            device_type=device_type,
+            role=device_role,
+            site=site,
+        )
+        root_bay = ModuleBay.objects.create(device=device, name='Bay 1')
+        manufacturer = Manufacturer.objects.first()
+        module_type = ModuleType.objects.create(
+            manufacturer=manufacturer, model='Child Rename Type'
+        )
+        module = Module.objects.create(
+            device=device, module_bay=root_bay, module_type=module_type
+        )
+        for name in ('Bay 1.1', 'Bay 1.2', 'Bay 1.3'):
+            ModuleBay.objects.create(device=device, module=module, name=name)
+
+        child = ModuleBay.objects.get(device=device, name='Bay 1.1')
+        child.name = 'Bay 1.4'
+        child.save()
+
+        names = list(ModuleBay.objects.filter(device=device).values_list('name', flat=True))
+        self.assertEqual(names, ['Bay 1', 'Bay 1.2', 'Bay 1.3', 'Bay 1.4'])
+
+    @tag('regression')  # #22146
+    def test_root_to_child_transition_still_relocates(self):
+        """
+        Promoting an existing root module bay to a child (by assigning a
+        module) must still flow through MPTT's normal move logic. The
+        rename-bypass must not suppress legitimate parent changes.
+        """
+        device_type = DeviceType.objects.first()
+        device_role = DeviceRole.objects.first()
+        site = Site.objects.first()
+        device = Device.objects.create(
+            name='Root To Child Device',
+            device_type=device_type,
+            role=device_role,
+            site=site,
+        )
+        host_bay = ModuleBay.objects.create(device=device, name='Host Bay')
+        movable_bay = ModuleBay.objects.create(device=device, name='Movable Bay')
+
+        manufacturer = Manufacturer.objects.first()
+        module_type = ModuleType.objects.create(
+            manufacturer=manufacturer, model='Root To Child Type'
+        )
+        host_module = Module.objects.create(
+            device=device, module_bay=host_bay, module_type=module_type
+        )
+
+        movable_bay.module = host_module
+        movable_bay.save()
+
+        movable_bay.refresh_from_db()
+        host_bay.refresh_from_db()
+        self.assertEqual(movable_bay.parent_id, host_bay.pk)
+        self.assertEqual(movable_bay.tree_id, host_bay.tree_id)
+
     def test_single_module_token(self):
         device_type = DeviceType.objects.first()
         device_role = DeviceRole.objects.first()