diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index 93b5f8b21..1c8d6cd38 100644 --- a/docs/models/dcim/modulebaytemplate.md +++ b/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. diff --git a/docs/models/dcim/moduletype.md b/docs/models/dcim/moduletype.md index 993c5cbfe..9f491c8bb 100644 --- a/docs/models/dcim/moduletype.md +++ b/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. diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index 3466d6243..75426f46a 100644 --- a/netbox/dcim/forms/object_import.py +++ b/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', ] diff --git a/netbox/dcim/models/device_component_templates.py b/netbox/dcim/models/device_component_templates.py index f480042a3..8873dd2cf 100644 --- a/netbox/dcim/models/device_component_templates.py +++ b/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()], } diff --git a/netbox/dcim/models/modules.py b/netbox/dcim/models/modules.py index ac6f7b746..4ab0b6ac1 100644 --- a/netbox/dcim/models/modules.py +++ b/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 diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 9556ff86d..bec25fa84 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/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 diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index e7f9de608..ae74a43a6 100644 --- a/netbox/dcim/tests/test_models.py +++ b/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. diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index c474f1ae8..d0a9d9ef7 100644 --- a/netbox/dcim/tests/test_views.py +++ b/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): diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index 8cf95ed2f..ac123a185 100644 --- a/netbox/dcim/views.py +++ b/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):