Merge pull request #22919 from netbox-community/19731-cleanup

#19731: Pre-release QA

docs/models/dcim/modulebaytemplate.md

@@ -3,3 +3,5 @@
 A template for a module bay that will be created on all instantiations of the parent device type. See the [module bay](./modulebay.md) documentation for more detail.
 
 [Bay types](./modulebaytype.md) assigned to a module bay template are copied to each instantiated module bay, so constraints defined on the device type propagate automatically to all devices of that type.
+
+Bay types are importable as part of a device type's or module type's YAML definition (`module-bays[].module_bay_types`), referenced by name. They are included when a device type is exported; a module type's exported definition omits its module bays entirely, so bay types are not carried through it. A referenced name is resolved against bay types belonging to the parent type's own manufacturer or with no manufacturer set (global); a name may match both, since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, in which case the manufacturer-specific type takes precedence. A name matching only some other manufacturer's bay type is rejected rather than resolved to it.

docs/models/dcim/moduletype.md

@@ -91,6 +91,8 @@ The assigned [profile](./moduletypeprofile.md) for the type of module. Profiles
 
 Zero or more [module bay types](./modulebaytype.md) that this module type is compatible with. When at least one bay type is set, the module type may only be installed into bays that share a common type. Leave empty to allow installation into any bay.
 
+Bay types are included, by name, in a module type's exported YAML definition, but are not currently importable back through it; re-importing an exported definition leaves this field unset.
+
 ### Attributes
 
 Depending on the module type's assigned [profile](./moduletypeprofile.md) (if any), one or more user-defined attributes may be available to configure.

netbox/dcim/forms/object_import.py

@@ -213,20 +213,74 @@ class PortTemplateMappingImportForm(forms.ModelForm):
 
 
 class ModuleBayTemplateImportForm(forms.ModelForm):
+    module_bay_types = forms.ModelMultipleChoiceField(
+        label=_('Module bay types'),
+        queryset=ModuleBayType.objects.all(),
+        to_field_name='name',
+        required=False,
+    )
 
     class Meta:
         model = ModuleBayTemplate
         fields = [
-            'device_type', 'module_type', 'name', 'label', 'position', 'description',
+            'device_type', 'module_type', 'name', 'label', 'position', 'enabled', 'description',
+            'module_bay_types',
         ]
 
+    def clean(self):
+        """
+        Resolve each referenced bay type name against the parent type's own manufacturer, plus
+        bay types having no manufacturer (global), preferring a manufacturer-specific match
+        over a global one. ModuleBayType's unique constraint is on (manufacturer, name), not
+        name alone, so a name can legitimately match both, and the field's name-based lookup
+        resolves every match into cleaned_data rather than picking one.
+
+        This runs in clean() rather than in clean_module_bay_types() so that it does not depend
+        on the parent having been cleaned first, which would make it sensitive to the order of
+        Meta.fields.
+        """
+        super().clean()
+
+        module_bay_types = self.cleaned_data.get('module_bay_types')
+        if not module_bay_types:
+            return
+
+        # If neither parent resolved, leave the field alone: ModularComponentTemplateModel.clean()
+        # rejects a parentless template in _post_clean(), and reporting unresolvable names on top
+        # of that would just be noise.
+        parent = self.cleaned_data.get('device_type') or self.cleaned_data.get('module_type')
+        if parent is None:
+            return
+
+        by_name = {}
+        for module_bay_type in module_bay_types:
+            if module_bay_type.manufacturer_id not in (None, parent.manufacturer_id):
+                continue
+            existing = by_name.get(module_bay_type.name)
+            if existing is None or module_bay_type.manufacturer_id is not None:
+                by_name[module_bay_type.name] = module_bay_type
+
+        # A name matching only some other manufacturer's bay type is rejected rather than
+        # resolved to it.
+        for module_bay_type in module_bay_types:
+            if module_bay_type.name not in by_name:
+                raise forms.ValidationError({
+                    'module_bay_types': forms.ValidationError(
+                        self.fields['module_bay_types'].error_messages['invalid_choice'],
+                        code='invalid_choice',
+                        params={'value': module_bay_type.name},
+                    )
+                })
+
+        self.cleaned_data['module_bay_types'] = list(by_name.values())
+
 
 class DeviceBayTemplateImportForm(forms.ModelForm):
 
     class Meta:
         model = DeviceBayTemplate
         fields = [
-            'device_type', 'name', 'label', 'description',
+            'device_type', 'name', 'label', 'enabled', 'description',
         ]
 
 

netbox/dcim/models/device_component_templates.py

@@ -981,6 +981,7 @@ class ModuleBayTemplate(ModularComponentTemplateModel):
             'position': self.position,
             'enabled': self.enabled,
             'description': self.description,
+            'module_bay_types': [t.name for t in self.module_bay_types.all()],
         }
 
 

netbox/dcim/models/modules.py

@@ -313,6 +313,7 @@ class ModuleType(ImageAttachmentsMixin, PrimaryModel, WeightMixin):
             'end_of_life': self.end_of_life.isoformat() if self.end_of_life else None,
             'attribute_data': self.attribute_data,
             'comments': self.comments,
+            'module_bay_types': [t.name for t in self.module_bay_types.all()],
         }
 
         # Component templates

netbox/dcim/tests/test_forms.py

@@ -229,6 +229,180 @@ class ModuleTypeFormTestCase(TestCase):
             self.assertEqual(module_type.attribute_data, {'media': ['copper', 'qsfp28']})
 
 
+class ModuleBayTemplateImportFormTestCase(TestCase):
+
+    def test_module_bay_types_prefers_manufacturer_specific_match_over_global(self):
+        """A name shared by a global and a manufacturer-scoped type resolves to the scoped one."""
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        global_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-global')
+        scoped_type = ModuleBayType.objects.create(
+            name='SFP28', slug='sfp28-scoped', manufacturer=manufacturer,
+        )
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer, model='Device Type 1', slug='device-type-1',
+        )
+
+        form = ModuleBayTemplateImportForm({
+            'device_type': device_type.pk,
+            'name': 'Module Bay 1',
+            'module_bay_types': ['SFP28'],
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+
+        module_bay_template = form.save()
+        self.assertEqual(
+            list(module_bay_template.module_bay_types.all()), [scoped_type],
+        )
+        self.assertNotIn(global_type, module_bay_template.module_bay_types.all())
+
+    def test_module_bay_types_unknown_name_raises_error(self):
+        device_type = DeviceType.objects.create(
+            manufacturer=Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1'),
+            model='Device Type 1',
+            slug='device-type-1',
+        )
+
+        form = ModuleBayTemplateImportForm({
+            'device_type': device_type.pk,
+            'name': 'Module Bay 1',
+            'module_bay_types': ['Nonexistent'],
+        })
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            form.errors.as_data()['module_bay_types'][0].code, 'invalid_choice',
+        )
+
+    def test_module_bay_types_prefers_manufacturer_specific_match_over_global_for_module_type(self):
+        """Same disambiguation, but for a module bay template nested under a ModuleType."""
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        global_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-global')
+        scoped_type = ModuleBayType.objects.create(
+            name='SFP28', slug='sfp28-scoped', manufacturer=manufacturer,
+        )
+        module_type = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1')
+
+        form = ModuleBayTemplateImportForm({
+            'module_type': module_type.pk,
+            'name': 'Module Bay 1',
+            'module_bay_types': ['SFP28'],
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+
+        module_bay_template = form.save()
+        self.assertEqual(
+            list(module_bay_template.module_bay_types.all()), [scoped_type],
+        )
+        self.assertNotIn(global_type, module_bay_template.module_bay_types.all())
+
+    def test_enabled_honors_explicit_false(self):
+        device_type = DeviceType.objects.create(
+            manufacturer=Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1'),
+            model='Device Type 1',
+            slug='device-type-1',
+        )
+
+        form = ModuleBayTemplateImportForm({
+            'device_type': device_type.pk,
+            'name': 'Module Bay 1',
+            'enabled': False,
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        self.assertFalse(form.save().enabled)
+
+    def test_import_export_round_trip_preserves_module_bay_types(self):
+        """to_yaml() then re-import through this form preserves module bay types."""
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28')
+        bay_type_b = ModuleBayType.objects.create(name='QSFP28', slug='qsfp28')
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer, model='Device Type 1', slug='device-type-1',
+        )
+        original = ModuleBayTemplate.objects.create(device_type=device_type, name='Module Bay 1')
+        original.module_bay_types.set([bay_type_a, bay_type_b])
+
+        exported = original.to_yaml()
+        form = ModuleBayTemplateImportForm({
+            'device_type': device_type.pk,
+            'name': 'Module Bay 2',
+            'module_bay_types': exported['module_bay_types'],
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+
+        reimported = form.save()
+        self.assertEqual(
+            set(reimported.module_bay_types.values_list('name', flat=True)),
+            set(original.module_bay_types.values_list('name', flat=True)),
+        )
+
+    def test_module_bay_types_name_belonging_only_to_other_manufacturers_is_unresolvable(self):
+        """
+        A name that exists only for manufacturers other than the device type's own (and isn't
+        global) must not resolve at all -- module_bay_types is scoped to the device type's own
+        manufacturer plus global types, with no cross-manufacturer fallback.
+        """
+        juniper = Manufacturer.objects.create(name='Juniper', slug='juniper')
+        cisco = Manufacturer.objects.create(name='Cisco', slug='cisco')
+        ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco)
+        device_type = DeviceType.objects.create(
+            manufacturer=juniper, model='Juniper Device Type', slug='juniper-device-type',
+        )
+
+        form = ModuleBayTemplateImportForm({
+            'device_type': device_type.pk,
+            'name': 'Module Bay 1',
+            'module_bay_types': ['SFP28'],
+        })
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            form.errors.as_data()['module_bay_types'][0].code, 'invalid_choice',
+        )
+
+    def test_module_bay_types_resolution_is_independent_of_field_order(self):
+        """
+        Resolution must not depend on the parent type having been cleaned first, so declaring
+        module_bay_types ahead of device_type/module_type must not change the outcome.
+        """
+        class ReorderedImportForm(ModuleBayTemplateImportForm):
+            class Meta(ModuleBayTemplateImportForm.Meta):
+                fields = [
+                    'module_bay_types', 'device_type', 'module_type', 'name', 'label', 'position',
+                    'enabled', 'description',
+                ]
+
+        self.assertEqual(list(ReorderedImportForm().fields)[0], 'module_bay_types')
+
+        juniper = Manufacturer.objects.create(name='Juniper', slug='juniper')
+        cisco = Manufacturer.objects.create(name='Cisco', slug='cisco')
+        global_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-global')
+        juniper_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-juniper', manufacturer=juniper)
+        cisco_type = ModuleBayType.objects.create(name='QSFP28', slug='qsfp28-cisco', manufacturer=cisco)
+        device_type = DeviceType.objects.create(
+            manufacturer=juniper, model='Juniper Device Type', slug='juniper-device-type',
+        )
+
+        # The device type's own manufacturer still wins over the global type of the same name
+        form = ReorderedImportForm({
+            'device_type': device_type.pk,
+            'name': 'Module Bay 1',
+            'module_bay_types': ['SFP28'],
+        })
+        self.assertTrue(form.is_valid(), form.errors)
+        module_bay_template = form.save()
+        self.assertEqual(list(module_bay_template.module_bay_types.all()), [juniper_type])
+        self.assertNotIn(global_type, module_bay_template.module_bay_types.all())
+
+        # ...and another manufacturer's bay type is still rejected rather than resolved to
+        form = ReorderedImportForm({
+            'device_type': device_type.pk,
+            'name': 'Module Bay 2',
+            'module_bay_types': [cisco_type.name],
+        })
+        self.assertFalse(form.is_valid())
+        self.assertEqual(
+            form.errors.as_data()['module_bay_types'][0].code, 'invalid_choice',
+        )
+
+
 class ModuleFormTestCase(TestCase):
 
     @classmethod

netbox/dcim/tests/test_models.py

@@ -182,6 +182,34 @@ class ModuleTypeTestCase(TestCase):
         module_type.refresh_from_db()
         self.assertEqual(module_type.interface_template_count, 1)
 
+    def test_module_bay_template_to_yaml_includes_module_bay_types(self):
+        """
+        ModuleBayTemplate.to_yaml() should export its assigned module bay types by name.
+        """
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        module_type = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1')
+        bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28')
+        module_bay_template = ModuleBayTemplate.objects.create(module_type=module_type, name='Module Bay 1')
+        module_bay_template.module_bay_types.set([bay_type])
+
+        data = module_bay_template.to_yaml()
+        self.assertEqual(data['module_bay_types'], ['SFP28'])
+
+    def test_module_bay_template_to_yaml_orders_module_bay_types(self):
+        """
+        Multiple module bay types should export in ModuleBayType's own ordering
+        (manufacturer, name), independent of assignment order.
+        """
+        manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
+        module_type = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1')
+        bay_type_b = ModuleBayType.objects.create(name='QSFP28', slug='qsfp28')
+        bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28')
+        module_bay_template = ModuleBayTemplate.objects.create(module_type=module_type, name='Module Bay 1')
+        module_bay_template.module_bay_types.set([bay_type_b, bay_type_a])
+
+        data = module_bay_template.to_yaml()
+        self.assertEqual(data['module_bay_types'], ['QSFP28', 'SFP28'])
+
     def test_attributes(self):
         """
         ModuleType.attributes should normalize iterable values into strings for presentation.

netbox/dcim/tests/test_views.py

@@ -7,8 +7,10 @@ from zoneinfo import ZoneInfo
 
 import yaml
 from django.contrib.contenttypes.models import ContentType
+from django.db import connection
 from django.http import StreamingHttpResponse
 from django.test import override_settings, tag
+from django.test.utils import CaptureQueriesContext
 from django.urls import reverse
 from netaddr import EUI
 
@@ -17,6 +19,7 @@ from core.models import ObjectChange, ObjectType
 from dcim.choices import *
 from dcim.constants import *
 from dcim.models import *
+from dcim.views import DeviceTypeListView, ModuleTypeListView
 from extras.models import ConfigTemplate
 from ipam.models import ASN, RIR, VLAN, VRF
 from netbox.choices import (
@@ -998,11 +1001,15 @@ port-mappings:
     rear_port: Rear Port 3
 module-bays:
   - name: Module Bay 1
+    module_bay_types:
+      - SFP28
   - name: Module Bay 2
+    enabled: false
   - name: Module Bay 3
 device-bays:
   - name: Device Bay 1
   - name: Device Bay 2
+    enabled: false
   - name: Device Bay 3
 inventory-items:
   - name: Inventory Item 1
@@ -1018,6 +1025,7 @@ inventory-items:
         manufacturer.save()
         platform = Platform(name='Platform', slug='test-platform', manufacturer=manufacturer)
         platform.save()
+        ModuleBayType.objects.create(name='SFP28', slug='sfp28')
 
         # Add all required permissions to the test user
         self.add_permissions(
@@ -1126,15 +1134,52 @@ inventory-items:
         self.assertEqual(device_type.modulebaytemplates.count(), 3)
         mb1 = ModuleBayTemplate.objects.first()
         self.assertEqual(mb1.name, 'Module Bay 1')
+        self.assertEqual(list(mb1.module_bay_types.values_list('name', flat=True)), ['SFP28'])
+        self.assertTrue(mb1.enabled)
+
+        mb2 = ModuleBayTemplate.objects.filter(name='Module Bay 2').first()
+        self.assertFalse(mb2.enabled)
 
         self.assertEqual(device_type.devicebaytemplates.count(), 3)
         db1 = DeviceBayTemplate.objects.first()
         self.assertEqual(db1.name, 'Device Bay 1')
+        self.assertTrue(db1.enabled)
+
+        db2 = DeviceBayTemplate.objects.filter(name='Device Bay 2').first()
+        self.assertFalse(db2.enabled)
 
         self.assertEqual(device_type.inventoryitemtemplates.count(), 3)
         ii1 = InventoryItemTemplate.objects.first()
         self.assertEqual(ii1.name, 'Inventory Item 1')
 
+    def test_bulk_yaml_export_module_bay_types_query_count_is_constant(self):
+        """Query count shouldn't scale with bay count -- module_bay_types is prefetched."""
+        manufacturer = Manufacturer.objects.create(name='Export Query Manufacturer', slug='export-query-mfr')
+        bay_type = ModuleBayType.objects.create(name='Export Query SFP28', slug='export-query-sfp28')
+
+        def make_device_type(model_name, bay_count):
+            device_type = DeviceType.objects.create(
+                manufacturer=manufacturer, model=model_name, slug=model_name.lower().replace(' ', '-'),
+            )
+            for i in range(bay_count):
+                bay = ModuleBayTemplate.objects.create(device_type=device_type, name=f'Bay {i}')
+                bay.module_bay_types.set([bay_type])
+            return device_type
+
+        one_bay_device_type = make_device_type('Export Query DT One Bay', 1)
+        five_bay_device_type = make_device_type('Export Query DT Five Bays', 5)
+
+        view = DeviceTypeListView()
+        view.queryset = DeviceType.objects.filter(pk=one_bay_device_type.pk)
+        with CaptureQueriesContext(connection) as one_bay_queries:
+            view.export_yaml()
+
+        view.queryset = DeviceType.objects.filter(pk=five_bay_device_type.pk)
+        with CaptureQueriesContext(connection) as five_bay_queries:
+            view.export_yaml()
+
+        self.assertEqual(len(one_bay_queries), len(five_bay_queries))
+
     def test_import_error_numbering(self):
         # Add all required permissions to the test user
         self.add_permissions(
@@ -1648,6 +1693,8 @@ port-mappings:
 module-bays:
   - name: Module Bay 1
     position: 1
+    module_bay_types:
+      - SFP28
   - name: Module Bay 2
     position: 2
   - name: Module Bay 3
@@ -1657,6 +1704,7 @@ module-bays:
         # Create the manufacturer
         manufacturer = Manufacturer(name='Generic', slug='generic')
         manufacturer.save()
+        ModuleBayType.objects.create(name='SFP28', slug='sfp28')
 
         # Add all required permissions to the test user
         self.add_permissions(
@@ -1752,6 +1800,34 @@ module-bays:
         mb1 = ModuleBayTemplate.objects.first()
         self.assertEqual(mb1.name, 'Module Bay 1')
         self.assertEqual(mb1.position, '1')
+        self.assertEqual(list(mb1.module_bay_types.values_list('name', flat=True)), ['SFP28'])
+
+    def test_bulk_yaml_export_prefetches_module_bay_types_on_the_module_type_itself(self):
+        """Compares an unprefetched to_yaml() call per instance against export_yaml() (which
+        prefetches and issues no queries of its own beyond that), rather than a row-count
+        comparison, which other per-instance relations that legitimately scale with it would
+        swamp."""
+        manufacturer = Manufacturer.objects.create(name='Export Query MT Manufacturer', slug='export-query-mt-mfr')
+        bay_type = ModuleBayType.objects.create(name='Export Query MT SFP28', slug='export-query-mt-sfp28')
+
+        module_types = []
+        for i in range(5):
+            module_type = ModuleType.objects.create(manufacturer=manufacturer, model=f'Export Query MT {i}')
+            module_type.module_bay_types.set([bay_type])
+            module_types.append(module_type)
+        pks = [mt.pk for mt in module_types]
+
+        with CaptureQueriesContext(connection) as unprefetched:
+            [obj.to_yaml() for obj in ModuleType.objects.filter(pk__in=pks)]
+
+        view = ModuleTypeListView()
+        view.queryset = ModuleType.objects.filter(pk__in=pks)
+        with CaptureQueriesContext(connection) as prefetched:
+            view.export_yaml()
+
+        # Without the prefetch, each of the 5 module types issues its own module_bay_types
+        # query; with it, exactly one query serves all 5.
+        self.assertEqual(len(unprefetched) - len(prefetched), 4)
 
     @override_settings(STREAMING_EXPORTS=True)
     def test_export_objects(self):

netbox/dcim/views.py

@@ -1447,6 +1447,11 @@ class DeviceTypeListView(generic.ObjectListView):
     filterset_form = forms.DeviceTypeFilterForm
     table = tables.DeviceTypeTable
 
+    def export_yaml(self):
+        # Avoid one module_bay_types query per module bay template across the export.
+        self.queryset = self.queryset.prefetch_related('modulebaytemplates__module_bay_types')
+        return super().export_yaml()
+
 
 @register_model_view(DeviceType)
 class DeviceTypeView(GetRelatedModelsMixin, generic.ObjectView):
@@ -1902,6 +1907,13 @@ class ModuleTypeListView(generic.ObjectListView):
     filterset_form = forms.ModuleTypeFilterForm
     table = tables.ModuleTypeTable
 
+    def export_yaml(self):
+        # Avoid one module_bay_types query per module type across the export. (Unlike
+        # DeviceType.to_yaml(), ModuleType.to_yaml() doesn't export module bay templates at
+        # all, so there's nothing to prefetch alongside it.)
+        self.queryset = self.queryset.prefetch_related('module_bay_types')
+        return super().export_yaml()
+
 
 @register_model_view(ModuleType)
 class ModuleTypeView(GetRelatedModelsMixin, generic.ObjectView):