diff --git a/netbox/dcim/models/device_components.py b/netbox/dcim/models/device_components.py index e3691fd6c..cf80b94c4 100644 --- a/netbox/dcim/models/device_components.py +++ b/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 diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index 3e3d1c526..5462257e1 100644 --- a/netbox/dcim/tests/test_models.py +++ b/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()