From 157a30ecd752b32c2f941edadb0f27cba6bb4743 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 07:50:59 -0400 Subject: [PATCH 01/14] #19731: Support module_bay_types in device/module type YAML import and export Follow-up QA for the ModuleBayType feature added in #22648. ModuleBayTemplate.to_yaml() omitted module_bay_types, and ModuleBayTemplateImportForm (used by the DeviceType/ModuleType YAML "Import Components" flow) didn't expose the field either, so bay-type constraints could never be defined as part of a device type's YAML definition -- only assigned by hand, one bay at a time, after import. Add module_bay_types (by name) to the import form, scoped to the parent device/module type's manufacturer (or global types) via clean_device_type/ clean_module_type, mirroring the existing scoping pattern used elsewhere in this form for power_port/cooling_intake. Add it to to_yaml()'s output symmetrically. --- netbox/dcim/forms/object_import.py | 27 ++++++++++++++++++- .../dcim/models/device_component_templates.py | 1 + netbox/dcim/tests/test_models.py | 13 +++++++++ netbox/dcim/tests/test_views.py | 8 ++++++ 4 files changed, 48 insertions(+), 1 deletion(-) diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index 3466d6243..2acc00824 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -1,4 +1,5 @@ from django import forms +from django.db.models import Q from django.utils.translation import gettext_lazy as _ from dcim.choices import InterfacePoEModeChoices, InterfacePoETypeChoices, InterfaceTypeChoices, PortTypeChoices @@ -213,13 +214,37 @@ 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', 'description', 'module_bay_types', ] + def clean_device_type(self): + if device_type := self.cleaned_data['device_type']: + module_bay_types = self.fields['module_bay_types'] + module_bay_types.queryset = module_bay_types.queryset.filter( + Q(manufacturer__isnull=True) | Q(manufacturer=device_type.manufacturer) + ) + + return device_type + + def clean_module_type(self): + if module_type := self.cleaned_data['module_type']: + module_bay_types = self.fields['module_bay_types'] + module_bay_types.queryset = module_bay_types.queryset.filter( + Q(manufacturer__isnull=True) | Q(manufacturer=module_type.manufacturer) + ) + + return module_type + class DeviceBayTemplateImportForm(forms.ModelForm): diff --git a/netbox/dcim/models/device_component_templates.py b/netbox/dcim/models/device_component_templates.py index dfcdd8159..97ced7d9b 100644 --- a/netbox/dcim/models/device_component_templates.py +++ b/netbox/dcim/models/device_component_templates.py @@ -988,6 +988,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/tests/test_models.py b/netbox/dcim/tests/test_models.py index e7f9de608..cd95fc66e 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -182,6 +182,19 @@ 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_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..60476dfb3 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -998,6 +998,8 @@ port-mappings: rear_port: Rear Port 3 module-bays: - name: Module Bay 1 + module_bay_types: + - SFP28 - name: Module Bay 2 - name: Module Bay 3 device-bays: @@ -1018,6 +1020,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,6 +1129,7 @@ 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.assertEqual(device_type.devicebaytemplates.count(), 3) db1 = DeviceBayTemplate.objects.first() @@ -1648,6 +1652,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 +1663,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 +1759,7 @@ 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']) @override_settings(STREAMING_EXPORTS=True) def test_export_objects(self): From 508e2eaba2f1b693d3783e1838eabc3ec83fafd9 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 08:21:47 -0400 Subject: [PATCH 02/14] Resolve module_bay_types by name deterministically, not via blind filter ModuleBayType's unique constraint is on (manufacturer, name), not name alone, so a global type and a manufacturer-scoped type can legally share the same name. The manufacturer-or-null queryset scoping added for ModuleBayTemplateImportForm.module_bay_types left both rows in the filtered queryset in that case, and ModelMultipleChoiceField's default name-based lookup silently attached both instead of just the one referenced -- confirmed by reproducing it directly against the form. Add clean_module_bay_types() to resolve each submitted name explicitly, preferring a manufacturer-specific match over a global one, and raising a clear error for an unresolvable name instead of silently under- or over-matching. Also factor clean_device_type/clean_module_type's duplicated scoping logic into a shared helper. --- netbox/dcim/forms/object_import.py | 41 +++++++++++++++++++++----- netbox/dcim/tests/test_forms.py | 47 ++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 8 deletions(-) diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index 2acc00824..457fcfae2 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -227,24 +227,49 @@ class ModuleBayTemplateImportForm(forms.ModelForm): 'device_type', 'module_type', 'name', 'label', 'position', 'description', 'module_bay_types', ] + def _scope_module_bay_types(self, manufacturer): + module_bay_types = self.fields['module_bay_types'] + module_bay_types.queryset = module_bay_types.queryset.filter( + Q(manufacturer__isnull=True) | Q(manufacturer=manufacturer) + ) + def clean_device_type(self): if device_type := self.cleaned_data['device_type']: - module_bay_types = self.fields['module_bay_types'] - module_bay_types.queryset = module_bay_types.queryset.filter( - Q(manufacturer__isnull=True) | Q(manufacturer=device_type.manufacturer) - ) + self._scope_module_bay_types(device_type.manufacturer) return device_type def clean_module_type(self): if module_type := self.cleaned_data['module_type']: - module_bay_types = self.fields['module_bay_types'] - module_bay_types.queryset = module_bay_types.queryset.filter( - Q(manufacturer__isnull=True) | Q(manufacturer=module_type.manufacturer) - ) + self._scope_module_bay_types(module_type.manufacturer) return module_type + def clean_module_bay_types(self): + """ + Resolve each submitted name against the scoped queryset, preferring a manufacturer- + specific match over a global one when both exist. ModuleBayType's unique constraint + is on (manufacturer, name), not name alone, so a name can legitimately collide between + a global type and one scoped to this template's manufacturer; ModelMultipleChoiceField's + default name-based lookup would otherwise silently attach both. + """ + names = self.data.get('module_bay_types') or [] + if not isinstance(names, (list, tuple)): + raise forms.ValidationError(_("Module bay types must be a list.")) + + queryset = self.fields['module_bay_types'].queryset + resolved = [] + for name in names: + candidates = list(queryset.filter(name=name)) + if not candidates: + raise forms.ValidationError( + _("Module bay type not found: {name}").format(name=name) + ) + match = next((c for c in candidates if c.manufacturer_id is not None), candidates[0]) + resolved.append(match) + + return resolved + class DeviceBayTemplateImportForm(forms.ModelForm): diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 9556ff86d..5241c1b05 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -229,6 +229,53 @@ 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): + """ + ModuleBayType's unique constraint is on (manufacturer, name), not name alone, so a + global type and a manufacturer-scoped type can legally share the same name. Referencing + that name by import should resolve to the manufacturer-specific match only, not attach + both. + """ + 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.assertIn('module_bay_types', form.errors) + + class ModuleFormTestCase(TestCase): @classmethod From 63045d8551aeec0636d3a757444fbd8d9558f8c2 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 09:31:34 -0400 Subject: [PATCH 03/14] Address review: dead code, ModuleType's own side of the round trip, N+1 - clean_module_bay_types()'s two ValidationErrors were unreachable: ModelMultipleChoiceField.clean() already raises before the clean_ hook runs on a non-list or an unresolvable name, per Django's BaseForm._clean_fields(). Simplify to dedupe from cleaned_data (already scoped and validated) via a shared dedupe_module_bay_types_by_manufacturer() helper in dcim/utils.py, used by both ModuleBayTemplateImportForm and the new ModuleTypeImportForm.module_bay_types below. This also drops the self.data access that ignored the form prefix, broke on a QueryDict, and re-queried once per name. - ModuleType.module_bay_types (the module's own side of the bay/module compatibility intersection) was still missing from the YAML round trip. Add it to ModuleType.to_yaml() and ModuleTypeImportForm, mirroring ModuleBayTemplateImportForm's manufacturer-scoping and dedup. - ModuleBayTemplate.to_yaml() emitted enabled but the import form didn't accept it, so it silently reset to False (not the model's default=True) on any dict-bound re-import. Add it with the same clean_enabled()-defaults-to-True pattern already used by ModuleBayImportForm's CSV import. - Prefetch module_bay_types in DeviceTypeListView/ModuleTypeListView's export_yaml() so bulk YAML export doesn't add one query per module bay template across the exported queryset. - Document the manufacturer-preference precedence rule in the model docs, since export emits a bare name and import can resolve a colliding one to either a global or manufacturer-specific type. Adds regression tests for the module_type-scoped path, the enabled round trip, an export/import round trip, export ordering, the new ModuleTypeImportForm coverage, and the prefetch fix. --- docs/models/dcim/modulebaytemplate.md | 2 + docs/models/dcim/moduletype.md | 2 + netbox/dcim/forms/bulk_import.py | 26 +++++- netbox/dcim/forms/object_import.py | 39 ++++---- netbox/dcim/models/modules.py | 1 + netbox/dcim/tests/test_forms.py | 125 +++++++++++++++++++++++++- netbox/dcim/tests/test_models.py | 43 +++++++++ netbox/dcim/utils.py | 17 ++++ netbox/dcim/views.py | 16 ++++ 9 files changed, 244 insertions(+), 27 deletions(-) diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index 93b5f8b21..b20ef06af 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 and exportable as part of a device type's YAML definition, referenced by name. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, a global bay type and one scoped to the device type's own manufacturer may share a name; import resolves such a name to the manufacturer-specific bay type. diff --git a/docs/models/dcim/moduletype.md b/docs/models/dcim/moduletype.md index 993c5cbfe..8084c902d 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 importable and exportable as part of a module type's YAML definition, referenced by name. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, a global bay type and one scoped to the module type's own manufacturer may share a name; import resolves such a name to the manufacturer-specific bay type. + ### 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/bulk_import.py b/netbox/dcim/forms/bulk_import.py index 469257ae6..ba870d876 100644 --- a/netbox/dcim/forms/bulk_import.py +++ b/netbox/dcim/forms/bulk_import.py @@ -2,6 +2,7 @@ from django import forms from django.contrib.contenttypes.models import ContentType from django.contrib.postgres.forms.array import SimpleArrayField from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist +from django.db.models import Q from django.utils.functional import lazy from django.utils.html import format_html from django.utils.safestring import SafeString, mark_safe @@ -10,7 +11,7 @@ from django.utils.translation import gettext_lazy as _ from dcim.choices import * from dcim.constants import * from dcim.models import * -from dcim.utils import reconcile_port_mappings +from dcim.utils import dedupe_module_bay_types_by_manufacturer, reconcile_port_mappings from extras.models import ConfigTemplate from ipam.choices import VLANQinQRoleChoices from ipam.models import VLAN, VRF, IPAddress, VLANGroup @@ -550,14 +551,35 @@ class ModuleTypeImportForm(PrimaryModelImportForm): required=False, help_text=_('Attribute values for the assigned profile, passed as a dictionary') ) + module_bay_types = CSVModelMultipleChoiceField( + label=_('Module bay types'), + queryset=ModuleBayType.objects.all(), + to_field_name='name', + required=False, + help_text=_('Types of module bays this module type can be installed in (empty = unconstrained)'), + ) class Meta: model = ModuleType + # module_bay_types must stay last: clean_manufacturer() narrows its queryset by + # manufacturer before it is itself cleaned, and Django cleans fields in this order. fields = [ 'manufacturer', 'model', 'part_number', 'description', 'cooling_method', 'airflow', 'weight', 'weight_unit', - 'end_of_life', 'profile', 'attribute_data', 'owner', 'comments', 'tags', + 'end_of_life', 'profile', 'attribute_data', 'owner', 'comments', 'tags', 'module_bay_types', ] + def clean_manufacturer(self): + if manufacturer := self.cleaned_data['manufacturer']: + module_bay_types = self.fields['module_bay_types'] + module_bay_types.queryset = module_bay_types.queryset.filter( + Q(manufacturer__isnull=True) | Q(manufacturer=manufacturer) + ) + + return manufacturer + + def clean_module_bay_types(self): + return dedupe_module_bay_types_by_manufacturer(self.cleaned_data['module_bay_types']) + def clean(self): super().clean() diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index 457fcfae2..dd7bd43e1 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -4,6 +4,7 @@ from django.utils.translation import gettext_lazy as _ from dcim.choices import InterfacePoEModeChoices, InterfacePoETypeChoices, InterfaceTypeChoices, PortTypeChoices from dcim.models import * +from dcim.utils import dedupe_module_bay_types_by_manufacturer from wireless.choices import WirelessRoleChoices __all__ = ( @@ -223,10 +224,22 @@ class ModuleBayTemplateImportForm(forms.ModelForm): class Meta: model = ModuleBayTemplate + # module_bay_types must stay last: clean_device_type/clean_module_type narrow its + # queryset by manufacturer before it is itself cleaned, and Django cleans fields in + # this order. Without that narrowing, dedupe_module_bay_types_by_manufacturer() could + # pick an arbitrary manufacturer's type for a name shared across several. fields = [ - 'device_type', 'module_type', 'name', 'label', 'position', 'description', 'module_bay_types', + 'device_type', 'module_type', 'name', 'label', 'position', 'enabled', 'description', + 'module_bay_types', ] + def clean_enabled(self): + # A dict-bound BooleanField resolves a missing key to False, not the model's own + # default=True -- match ModuleBayImportForm's equivalent CSV-import behavior. + if 'enabled' not in self.data: + return True + return self.cleaned_data['enabled'] + def _scope_module_bay_types(self, manufacturer): module_bay_types = self.fields['module_bay_types'] module_bay_types.queryset = module_bay_types.queryset.filter( @@ -246,29 +259,7 @@ class ModuleBayTemplateImportForm(forms.ModelForm): return module_type def clean_module_bay_types(self): - """ - Resolve each submitted name against the scoped queryset, preferring a manufacturer- - specific match over a global one when both exist. ModuleBayType's unique constraint - is on (manufacturer, name), not name alone, so a name can legitimately collide between - a global type and one scoped to this template's manufacturer; ModelMultipleChoiceField's - default name-based lookup would otherwise silently attach both. - """ - names = self.data.get('module_bay_types') or [] - if not isinstance(names, (list, tuple)): - raise forms.ValidationError(_("Module bay types must be a list.")) - - queryset = self.fields['module_bay_types'].queryset - resolved = [] - for name in names: - candidates = list(queryset.filter(name=name)) - if not candidates: - raise forms.ValidationError( - _("Module bay type not found: {name}").format(name=name) - ) - match = next((c for c in candidates if c.manufacturer_id is not None), candidates[0]) - resolved.append(match) - - return resolved + return dedupe_module_bay_types_by_manufacturer(self.cleaned_data['module_bay_types']) class DeviceBayTemplateImportForm(forms.ModelForm): 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 5241c1b05..42d5185e9 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -1,5 +1,6 @@ from unittest.mock import patch +import yaml from django import forms from django.test import TestCase @@ -273,7 +274,129 @@ class ModuleBayTemplateImportFormTestCase(TestCase): 'module_bay_types': ['Nonexistent'], }) self.assertFalse(form.is_valid()) - self.assertIn('module_bay_types', form.errors) + self.assertEqual( + form.errors['module_bay_types'], + ['Select a valid choice. Nonexistent is not one of the available choices.'], + ) + + def test_module_bay_types_prefers_manufacturer_specific_match_over_global_for_module_type(self): + """ + Same disambiguation as the device_type-scoped case, but through the module_type path + (a module bay template nested within a ModuleType rather than a DeviceType). + """ + 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_defaults_true_when_omitted(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', + }) + self.assertTrue(form.is_valid(), form.errors) + self.assertTrue(form.save().enabled) + + 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): + """ + A ModuleBayTemplate exported via to_yaml() and re-imported through this form should + end up with the same module bay types, closing the exact export/import loop this + feature exists for. + """ + 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)), + ) + + +class ModuleTypeImportFormTestCase(TestCase): + + def test_module_bay_types_round_trip(self): + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28', manufacturer=manufacturer) + + form = ModuleTypeImportForm({ + 'manufacturer': manufacturer.name, + 'model': 'Module Type 1', + 'module_bay_types': ['SFP28'], + }) + self.assertTrue(form.is_valid(), form.errors) + + module_type = form.save() + self.assertEqual(list(module_type.module_bay_types.all()), [bay_type]) + self.assertEqual(yaml.safe_load(module_type.to_yaml())['module_bay_types'], ['SFP28']) + + def test_module_bay_types_prefers_manufacturer_specific_match_over_global(self): + 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, + ) + + form = ModuleTypeImportForm({ + 'manufacturer': manufacturer.name, + 'model': 'Module Type 1', + 'module_bay_types': ['SFP28'], + }) + self.assertTrue(form.is_valid(), form.errors) + + module_type = form.save() + self.assertEqual(list(module_type.module_bay_types.all()), [scoped_type]) + self.assertNotIn(global_type, module_type.module_bay_types.all()) class ModuleFormTestCase(TestCase): diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index cd95fc66e..03737009a 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -1,9 +1,11 @@ from decimal import Decimal from django.core.exceptions import ValidationError +from django.db import connection from django.db.models import ProtectedError from django.db.models.signals import post_save from django.test import TestCase, tag +from django.test.utils import CaptureQueriesContext from circuits.models import * from core.models import ObjectType @@ -152,6 +154,32 @@ class DeviceTypeTestCase(TestCase): device_type.refresh_from_db() self.assertEqual(device_type.interface_template_count, 1) + def test_bulk_yaml_export_prefetches_module_bay_types(self): + """ + DeviceTypeListView.export_yaml() prefetches modulebaytemplates__module_bay_types so + that to_yaml()'s per-bay module_bay_types lookup doesn't add one query per module bay + template across the exported queryset. + """ + from dcim.views import DeviceTypeListView + + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28') + device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Device Type 1', slug='dt1') + for i in range(3): + bay = ModuleBayTemplate.objects.create(device_type=device_type, name=f'Bay {i}') + bay.module_bay_types.set([bay_type]) + + with CaptureQueriesContext(connection) as unprefetched: + [obj.to_yaml() for obj in DeviceType.objects.filter(pk=device_type.pk)] + + view = DeviceTypeListView() + view.queryset = DeviceType.objects.filter(pk=device_type.pk) + with CaptureQueriesContext(connection) as prefetched: + view.export_yaml() + + # Without the prefetch, each of the 3 bays issues its own module_bay_types query. + self.assertLess(len(prefetched), len(unprefetched)) + class ModuleTypeTestCase(TestCase): @@ -195,6 +223,21 @@ class ModuleTypeTestCase(TestCase): 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/utils.py b/netbox/dcim/utils.py index 097774d7a..23bfc7618 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -8,6 +8,23 @@ from django.utils.translation import gettext as _ from dcim.constants import MODULE_TOKEN +def dedupe_module_bay_types_by_manufacturer(module_bay_types): + """ + Given an iterable of ModuleBayType instances resolved by name, collapse any sharing a + name to a single entry, preferring a manufacturer-specific match over a global one. + + ModuleBayType's unique constraint is on (manufacturer, name), not name alone, so a + global type and a manufacturer-scoped type can legitimately share the same name; a + plain name-based queryset lookup would otherwise resolve to both. + """ + resolved = {} + for module_bay_type in module_bay_types: + existing = resolved.get(module_bay_type.name) + if existing is None or (existing.manufacturer_id is None and module_bay_type.manufacturer_id is not None): + resolved[module_bay_type.name] = module_bay_type + return list(resolved.values()) + + def inherit_module_token(position, parent_positions): """ Resolve a single {module} token in a bay position by inheriting from the position diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index 8cf95ed2f..09d3e87d1 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1447,6 +1447,13 @@ class DeviceTypeListView(generic.ObjectListView): filterset_form = forms.DeviceTypeFilterForm table = tables.DeviceTypeTable + def export_yaml(self): + # to_yaml() walks each device type's module bay templates and, for each, its + # module_bay_types -- prefetch both so bulk export doesn't issue one query per + # module bay template across the whole queryset. + 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 +1909,15 @@ class ModuleTypeListView(generic.ObjectListView): filterset_form = forms.ModuleTypeFilterForm table = tables.ModuleTypeTable + def export_yaml(self): + # to_yaml() reads module_bay_types directly, plus each nested module bay template's + # own module_bay_types -- prefetch both so bulk export doesn't issue one query per + # module type/module bay template across the whole queryset. + self.queryset = self.queryset.prefetch_related( + 'module_bay_types', 'modulebaytemplates__module_bay_types', + ) + return super().export_yaml() + @register_model_view(ModuleType) class ModuleTypeView(GetRelatedModelsMixin, generic.ObjectView): From 6f3c53791b699678347d6627d7447c9e34a737c5 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 11:04:47 -0400 Subject: [PATCH 04/14] Fix regression: manufacturer scoping made cross-manufacturer bay types unimportable The manufacturer-or-null queryset scoping added to disambiguate a name shared by a global and a manufacturer-scoped ModuleBayType went further than intended: it also excluded a *different* manufacturer's bay type entirely. The UI (ModuleTypeForm/ModuleBayTemplateForm) and REST API place no such restriction -- a third-party module may legitimately declare compatibility with another manufacturer's proprietary bay type. Confirmed the regression directly: creating that assignment via ModuleTypeForm succeeds, but exporting it and re-importing the same YAML failed with "Object not found: SFP28", making valid existing data unimportable -- worse than the bug this feature exists to fix. Remove the queryset scoping entirely and instead make dedupe_module_bay_types_by_manufacturer() manufacturer-aware: given the target manufacturer, it now prefers (in order) an exact match, then a global type, then any remaining candidate, resolved from an unscoped queryset in clean() rather than a sibling clean_() mutating the field's queryset. This also drops the Meta.fields-ordering dependency those methods required. Also, from the same review round: - Test asserting Django's literal English error string now asserts the error code instead, so it survives wording changes/translation. - The prefetch query-count test moved from test_models.py (which doesn't otherwise touch views) to test_views.py, and strengthened from "prefetch saves at least one query" to "query count is constant regardless of bay count" -- the actual invariant. Added equivalent coverage for ModuleTypeListView, which the prior version didn't test at all. - Corrected the export_yaml() prefetch comments to not imply the other ~11 relations to_yaml() touches are also covered -- they aren't, and weren't before this feature either. - Updated the model docs to describe the new (permissive, cross-manufacturer allowed) behavior instead of the old (restrictive) one they described a commit ago. Adds regression tests importing a bay type belonging to a different manufacturer than the importing device/module type, through both ModuleBayTemplateImportForm and ModuleTypeImportForm. --- docs/models/dcim/modulebaytemplate.md | 2 +- docs/models/dcim/moduletype.md | 2 +- netbox/dcim/forms/bulk_import.py | 19 ++------ netbox/dcim/forms/object_import.py | 40 +++++++--------- netbox/dcim/tests/test_forms.py | 68 ++++++++++++++++++++++++++- netbox/dcim/tests/test_models.py | 28 ----------- netbox/dcim/tests/test_views.py | 66 ++++++++++++++++++++++++++ netbox/dcim/utils.py | 28 ++++++++--- netbox/dcim/views.py | 13 +++-- 9 files changed, 185 insertions(+), 81 deletions(-) diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index b20ef06af..ad96b44e5 100644 --- a/docs/models/dcim/modulebaytemplate.md +++ b/docs/models/dcim/modulebaytemplate.md @@ -4,4 +4,4 @@ A template for a module bay that will be created on all instantiations of the pa [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 and exportable as part of a device type's YAML definition, referenced by name. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, a global bay type and one scoped to the device type's own manufacturer may share a name; import resolves such a name to the manufacturer-specific bay type. +Bay types are importable and exportable as part of a device type's YAML definition (`module-bays[].module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the device type's own may be referenced; this mirrors the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the device type's own manufacturer, then a global (manufacturer-less) bay type, then any remaining candidate. diff --git a/docs/models/dcim/moduletype.md b/docs/models/dcim/moduletype.md index 8084c902d..8b79c6c9f 100644 --- a/docs/models/dcim/moduletype.md +++ b/docs/models/dcim/moduletype.md @@ -91,7 +91,7 @@ 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 importable and exportable as part of a module type's YAML definition, referenced by name. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, a global bay type and one scoped to the module type's own manufacturer may share a name; import resolves such a name to the manufacturer-specific bay type. +Bay types are importable and exportable as part of a module type's YAML definition (`module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the module type's own may be referenced -- e.g. a third-party module declaring compatibility with another manufacturer's proprietary bay type -- mirroring the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the module type's own manufacturer, then a global (manufacturer-less) bay type, then any remaining candidate. ### Attributes diff --git a/netbox/dcim/forms/bulk_import.py b/netbox/dcim/forms/bulk_import.py index ba870d876..6b4135535 100644 --- a/netbox/dcim/forms/bulk_import.py +++ b/netbox/dcim/forms/bulk_import.py @@ -561,25 +561,11 @@ class ModuleTypeImportForm(PrimaryModelImportForm): class Meta: model = ModuleType - # module_bay_types must stay last: clean_manufacturer() narrows its queryset by - # manufacturer before it is itself cleaned, and Django cleans fields in this order. fields = [ 'manufacturer', 'model', 'part_number', 'description', 'cooling_method', 'airflow', 'weight', 'weight_unit', 'end_of_life', 'profile', 'attribute_data', 'owner', 'comments', 'tags', 'module_bay_types', ] - def clean_manufacturer(self): - if manufacturer := self.cleaned_data['manufacturer']: - module_bay_types = self.fields['module_bay_types'] - module_bay_types.queryset = module_bay_types.queryset.filter( - Q(manufacturer__isnull=True) | Q(manufacturer=manufacturer) - ) - - return manufacturer - - def clean_module_bay_types(self): - return dedupe_module_bay_types_by_manufacturer(self.cleaned_data['module_bay_types']) - def clean(self): super().clean() @@ -591,6 +577,11 @@ class ModuleTypeImportForm(PrimaryModelImportForm): if self.cleaned_data.get('profile') and not self.cleaned_data.get('attribute_data'): self.cleaned_data['attribute_data'] = {} + if module_bay_types := self.cleaned_data.get('module_bay_types'): + self.cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( + module_bay_types, self.cleaned_data.get('manufacturer'), + ) + class DeviceRoleImportForm(NestedGroupModelImportForm): parent = CSVModelChoiceField( diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index dd7bd43e1..a048139fa 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -1,5 +1,4 @@ from django import forms -from django.db.models import Q from django.utils.translation import gettext_lazy as _ from dcim.choices import InterfacePoEModeChoices, InterfacePoETypeChoices, InterfaceTypeChoices, PortTypeChoices @@ -224,10 +223,6 @@ class ModuleBayTemplateImportForm(forms.ModelForm): class Meta: model = ModuleBayTemplate - # module_bay_types must stay last: clean_device_type/clean_module_type narrow its - # queryset by manufacturer before it is itself cleaned, and Django cleans fields in - # this order. Without that narrowing, dedupe_module_bay_types_by_manufacturer() could - # pick an arbitrary manufacturer's type for a name shared across several. fields = [ 'device_type', 'module_type', 'name', 'label', 'position', 'enabled', 'description', 'module_bay_types', @@ -235,31 +230,28 @@ class ModuleBayTemplateImportForm(forms.ModelForm): def clean_enabled(self): # A dict-bound BooleanField resolves a missing key to False, not the model's own - # default=True -- match ModuleBayImportForm's equivalent CSV-import behavior. + # default=True -- match ModuleBayImportForm's equivalent CSV-import behavior. Reads + # self.data directly (no add_prefix()/QueryDict handling) because, like that form, + # this one is only ever bound to a plain dict of import data, never a real HTML + # checkbox POST. if 'enabled' not in self.data: return True return self.cleaned_data['enabled'] - def _scope_module_bay_types(self, manufacturer): - module_bay_types = self.fields['module_bay_types'] - module_bay_types.queryset = module_bay_types.queryset.filter( - Q(manufacturer__isnull=True) | Q(manufacturer=manufacturer) - ) + def clean(self): + cleaned_data = super().clean() - def clean_device_type(self): - if device_type := self.cleaned_data['device_type']: - self._scope_module_bay_types(device_type.manufacturer) + if module_bay_types := cleaned_data.get('module_bay_types'): + device_type = cleaned_data.get('device_type') + module_type = cleaned_data.get('module_type') + manufacturer = device_type.manufacturer if device_type else ( + module_type.manufacturer if module_type else None + ) + cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( + module_bay_types, manufacturer, + ) - return device_type - - def clean_module_type(self): - if module_type := self.cleaned_data['module_type']: - self._scope_module_bay_types(module_type.manufacturer) - - return module_type - - def clean_module_bay_types(self): - return dedupe_module_bay_types_by_manufacturer(self.cleaned_data['module_bay_types']) + return cleaned_data class DeviceBayTemplateImportForm(forms.ModelForm): diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 42d5185e9..fd02745c3 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -275,8 +275,7 @@ class ModuleBayTemplateImportFormTestCase(TestCase): }) self.assertFalse(form.is_valid()) self.assertEqual( - form.errors['module_bay_types'], - ['Select a valid choice. Nonexistent is not one of the available choices.'], + 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): @@ -362,6 +361,40 @@ class ModuleBayTemplateImportFormTestCase(TestCase): set(original.module_bay_types.values_list('name', flat=True)), ) + def test_module_bay_types_permits_a_different_manufacturers_type(self): + """ + The UI (ModuleBayTemplateForm) and REST API place no manufacturer restriction on + module_bay_types -- a third-party device may legitimately declare a bay compatible + with another manufacturer's proprietary bay type. Import must permit the same, and a + type assigned this way must survive an export/re-import round trip rather than + becoming permanently unimportable. + """ + juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') + cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') + cisco_bay_type = 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.assertTrue(form.is_valid(), form.errors) + + module_bay_template = form.save() + self.assertEqual(list(module_bay_template.module_bay_types.all()), [cisco_bay_type]) + + exported = module_bay_template.to_yaml()['module_bay_types'] + reimport_form = ModuleBayTemplateImportForm({ + 'device_type': device_type.pk, + 'name': 'Module Bay 2', + 'module_bay_types': exported, + }) + self.assertTrue(reimport_form.is_valid(), reimport_form.errors) + self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) + class ModuleTypeImportFormTestCase(TestCase): @@ -398,6 +431,37 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertEqual(list(module_type.module_bay_types.all()), [scoped_type]) self.assertNotIn(global_type, module_type.module_bay_types.all()) + def test_module_bay_types_permits_a_different_manufacturers_type(self): + """ + The UI (ModuleTypeForm) and REST API place no manufacturer restriction on + module_bay_types -- a third-party module may legitimately declare compatibility with + another manufacturer's proprietary bay type. Import must permit the same, and a type + created this way must survive an export/re-import round trip rather than becoming + permanently unimportable. + """ + juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') + cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') + cisco_bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) + + form = ModuleTypeImportForm({ + 'manufacturer': juniper.name, + 'model': 'Juniper Line Card', + 'module_bay_types': ['SFP28'], + }) + self.assertTrue(form.is_valid(), form.errors) + + module_type = form.save() + self.assertEqual(list(module_type.module_bay_types.all()), [cisco_bay_type]) + + exported = yaml.safe_load(module_type.to_yaml())['module_bay_types'] + reimport_form = ModuleTypeImportForm({ + 'manufacturer': juniper.name, + 'model': 'Juniper Line Card 2', + 'module_bay_types': exported, + }) + self.assertTrue(reimport_form.is_valid(), reimport_form.errors) + self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) + class ModuleFormTestCase(TestCase): diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index 03737009a..ae74a43a6 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -1,11 +1,9 @@ from decimal import Decimal from django.core.exceptions import ValidationError -from django.db import connection from django.db.models import ProtectedError from django.db.models.signals import post_save from django.test import TestCase, tag -from django.test.utils import CaptureQueriesContext from circuits.models import * from core.models import ObjectType @@ -154,32 +152,6 @@ class DeviceTypeTestCase(TestCase): device_type.refresh_from_db() self.assertEqual(device_type.interface_template_count, 1) - def test_bulk_yaml_export_prefetches_module_bay_types(self): - """ - DeviceTypeListView.export_yaml() prefetches modulebaytemplates__module_bay_types so - that to_yaml()'s per-bay module_bay_types lookup doesn't add one query per module bay - template across the exported queryset. - """ - from dcim.views import DeviceTypeListView - - manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') - bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28') - device_type = DeviceType.objects.create(manufacturer=manufacturer, model='Device Type 1', slug='dt1') - for i in range(3): - bay = ModuleBayTemplate.objects.create(device_type=device_type, name=f'Bay {i}') - bay.module_bay_types.set([bay_type]) - - with CaptureQueriesContext(connection) as unprefetched: - [obj.to_yaml() for obj in DeviceType.objects.filter(pk=device_type.pk)] - - view = DeviceTypeListView() - view.queryset = DeviceType.objects.filter(pk=device_type.pk) - with CaptureQueriesContext(connection) as prefetched: - view.export_yaml() - - # Without the prefetch, each of the 3 bays issues its own module_bay_types query. - self.assertLess(len(prefetched), len(unprefetched)) - class ModuleTypeTestCase(TestCase): diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 60476dfb3..c01f41887 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 ( @@ -1139,6 +1142,38 @@ inventory-items: ii1 = InventoryItemTemplate.objects.first() self.assertEqual(ii1.name, 'Inventory Item 1') + def test_bulk_yaml_export_module_bay_types_query_count_is_constant(self): + """ + DeviceTypeListView.export_yaml() prefetches modulebaytemplates__module_bay_types so + that to_yaml()'s per-bay module_bay_types lookup doesn't add one query per module bay + template as the number of bays grows. + """ + 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( @@ -1761,6 +1796,37 @@ module-bays: self.assertEqual(mb1.position, '1') self.assertEqual(list(mb1.module_bay_types.values_list('name', flat=True)), ['SFP28']) + def test_bulk_yaml_export_module_bay_types_query_count_is_constant(self): + """ + ModuleTypeListView.export_yaml() prefetches both module_bay_types (on the module type + itself) and modulebaytemplates__module_bay_types (on its nested module bay templates), + so neither adds a query per module bay template as the number of bays grows. + """ + 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_module_type(model_name, bay_count): + module_type = ModuleType.objects.create(manufacturer=manufacturer, model=model_name) + module_type.module_bay_types.set([bay_type]) + for i in range(bay_count): + bay = ModuleBayTemplate.objects.create(module_type=module_type, name=f'Bay {i}') + bay.module_bay_types.set([bay_type]) + return module_type + + one_bay_module_type = make_module_type('Export Query MT One Bay', 1) + five_bay_module_type = make_module_type('Export Query MT Five Bays', 5) + + view = ModuleTypeListView() + view.queryset = ModuleType.objects.filter(pk=one_bay_module_type.pk) + with CaptureQueriesContext(connection) as one_bay_queries: + view.export_yaml() + + view.queryset = ModuleType.objects.filter(pk=five_bay_module_type.pk) + with CaptureQueriesContext(connection) as five_bay_queries: + view.export_yaml() + + self.assertEqual(len(one_bay_queries), len(five_bay_queries)) + @override_settings(STREAMING_EXPORTS=True) def test_export_objects(self): url = reverse('dcim:moduletype_list') diff --git a/netbox/dcim/utils.py b/netbox/dcim/utils.py index 23bfc7618..ea2a102e4 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -8,19 +8,33 @@ from django.utils.translation import gettext as _ from dcim.constants import MODULE_TOKEN -def dedupe_module_bay_types_by_manufacturer(module_bay_types): +def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None): """ - Given an iterable of ModuleBayType instances resolved by name, collapse any sharing a - name to a single entry, preferring a manufacturer-specific match over a global one. + Collapse an iterable of ModuleBayType instances resolved by name to one entry per name, + preferring (in order) an exact match on *manufacturer*, then a global (manufacturer-less) + type, then any remaining candidate. - ModuleBayType's unique constraint is on (manufacturer, name), not name alone, so a - global type and a manufacturer-scoped type can legitimately share the same name; a - plain name-based queryset lookup would otherwise resolve to both. + ModuleBayType's uniqueness is scoped to (manufacturer, name), not name alone, so two + different manufacturers -- or a global type and a manufacturer-scoped one -- can + legitimately share a name. This does not exclude any manufacturer's types: a module or + bay may legitimately declare compatibility with another manufacturer's proprietary bay + type (e.g. a third-party line card), so callers must not scope the underlying queryset + by manufacturer -- only this preference order, for disambiguating an otherwise-ambiguous + name, is manufacturer-aware. """ + manufacturer_id = manufacturer.pk if manufacturer else None + + def preference(module_bay_type): + if module_bay_type.manufacturer_id == manufacturer_id: + return 0 + if module_bay_type.manufacturer_id is None: + return 1 + return 2 + resolved = {} for module_bay_type in module_bay_types: existing = resolved.get(module_bay_type.name) - if existing is None or (existing.manufacturer_id is None and module_bay_type.manufacturer_id is not None): + if existing is None or preference(module_bay_type) < preference(existing): resolved[module_bay_type.name] = module_bay_type return list(resolved.values()) diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index 09d3e87d1..dff5af244 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1449,8 +1449,10 @@ class DeviceTypeListView(generic.ObjectListView): def export_yaml(self): # to_yaml() walks each device type's module bay templates and, for each, its - # module_bay_types -- prefetch both so bulk export doesn't issue one query per - # module bay template across the whole queryset. + # module_bay_types -- prefetch both so this one relation doesn't add a query per + # module bay template across the whole queryset. to_yaml()'s other component-template + # relations (interfaces, ports, etc.) are unprefetched here as they were before this + # relation existed, and remain their own N+1 across a large export. self.queryset = self.queryset.prefetch_related('modulebaytemplates__module_bay_types') return super().export_yaml() @@ -1911,8 +1913,11 @@ class ModuleTypeListView(generic.ObjectListView): def export_yaml(self): # to_yaml() reads module_bay_types directly, plus each nested module bay template's - # own module_bay_types -- prefetch both so bulk export doesn't issue one query per - # module type/module bay template across the whole queryset. + # own module_bay_types -- prefetch both so these relations don't add a query per + # module type/module bay template across the whole queryset. to_yaml()'s other + # component-template relations (interfaces, ports, etc.) are unprefetched here as + # they were before these relations existed, and remain their own N+1 across a large + # export. self.queryset = self.queryset.prefetch_related( 'module_bay_types', 'modulebaytemplates__module_bay_types', ) From ec98245ebdb148be6e6dbe97be8d7adad0e62401 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 11:34:57 -0400 Subject: [PATCH 05/14] Add regression coverage for the CSV (comma-separated string) import path ModuleTypeImportForm.module_bay_types uses CSVModelMultipleChoiceField specifically because this form also serves plain CSV bulk import, where the cell value arrives as a string rather than a list -- unlike ModuleBayTemplateImportForm.module_bay_types, which only ever binds from a YAML-parsed list. Every existing test exercised the list-binding path only; verified the comma-separated-string path directly before adding permanent coverage for it, including the empty-string case. --- netbox/dcim/tests/test_forms.py | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index fd02745c3..b6dc96406 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -431,6 +431,41 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertEqual(list(module_type.module_bay_types.all()), [scoped_type]) self.assertNotIn(global_type, module_type.module_bay_types.all()) + def test_module_bay_types_accepts_csv_comma_separated_string(self): + """ + Unlike ModuleBayTemplateImportForm.module_bay_types (a plain ModelMultipleChoiceField, + bound only from YAML-parsed lists), this form's module_bay_types is a + CSVModelMultipleChoiceField because ModuleTypeImportForm also serves plain CSV bulk + import, where the cell value arrives as a comma-separated string rather than a list. + """ + 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') + + form = ModuleTypeImportForm({ + 'manufacturer': manufacturer.name, + 'model': 'Module Type 1', + 'module_bay_types': 'SFP28,QSFP28', + }) + self.assertTrue(form.is_valid(), form.errors) + + module_type = form.save() + self.assertEqual( + set(module_type.module_bay_types.values_list('name', flat=True)), + {bay_type_a.name, bay_type_b.name}, + ) + + def test_module_bay_types_accepts_empty_csv_string(self): + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + + form = ModuleTypeImportForm({ + 'manufacturer': manufacturer.name, + 'model': 'Module Type 1', + 'module_bay_types': '', + }) + self.assertTrue(form.is_valid(), form.errors) + self.assertFalse(form.save().module_bay_types.exists()) + def test_module_bay_types_permits_a_different_manufacturers_type(self): """ The UI (ModuleTypeForm) and REST API place no manufacturer restriction on From a3b5e4b30d0cf284bd630fb5c829c6b20ead1fc3 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 13:29:53 -0400 Subject: [PATCH 06/14] Refuse genuinely ambiguous bay-type names; close the ModuleBay CSV gap - dedupe_module_bay_types_by_manufacturer()'s lowest preference tier (a bay type belonging to some manufacturer other than the importing type's own) previously picked whichever candidate happened to sort first when two or more *different* foreign manufacturers shared a name. Verified directly: importing 'SFP28' for a Juniper module type, with only Cisco's and Arista's same-named types in the database (no Juniper or global one), silently linked to Arista's -- a wrong FK with no signal to the importer. The permissive fix from the last round only needs this tier to be reachable for the single-candidate case, not tolerant of a genuine tie; now raises ValidationError, attributed to module_bay_types, naming the competing manufacturers. - ModuleBayTemplateImportForm.module_bay_types was a plain ModelMultipleChoiceField (list only), while ModuleTypeImportForm's otherwise-identical field is a CSVModelMultipleChoiceField (list or comma-separated string), so `module_bay_types: SFP28` was accepted at the module-type level and rejected under `module-bays:` within the same YAML document. Switched to CSVModelMultipleChoiceField in both, which costs nothing here since it passes lists through unchanged. - ModuleBayImportForm (CSV import for real ModuleBay instances, as opposed to ModuleBayTemplateImportForm's templates) still had no module_bay_types support -- the same class of round-trip gap this PR exists to close, on the instance side rather than the template side. Added it, scoped via the importing device's own device_type.manufacturer. - The ModuleType prefetch query-count test only varied bay count (module type count fixed at 1), so it couldn't detect a regression in the module_bay_types prefetch on ModuleType itself -- confirmed directly: the test stayed green with that prefetch removed entirely. Varying module type count instead doesn't work either, since to_yaml() touches several other per-instance relations (manufacturer, port_mappings, ...) that legitimately scale with row count regardless of this fix and swamp an exact-equality comparison -- hit this myself on the first attempt. Replaced with a with/without-prefetch comparison on the identical queryset, which isolates the saving without that confound; verified it fails when the prefetch is removed and passes when it's present. --- netbox/dcim/forms/bulk_import.py | 21 ++++++++++- netbox/dcim/forms/object_import.py | 9 ++++- netbox/dcim/tests/test_forms.py | 57 ++++++++++++++++++++++++++++++ netbox/dcim/tests/test_views.py | 38 ++++++++++++++++++++ netbox/dcim/utils.py | 32 +++++++++++++---- 5 files changed, 149 insertions(+), 8 deletions(-) diff --git a/netbox/dcim/forms/bulk_import.py b/netbox/dcim/forms/bulk_import.py index 6b4135535..901e057be 100644 --- a/netbox/dcim/forms/bulk_import.py +++ b/netbox/dcim/forms/bulk_import.py @@ -1439,10 +1439,19 @@ class ModuleBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): queryset=Device.objects.all(), to_field_name='name' ) + module_bay_types = CSVModelMultipleChoiceField( + label=_('Module bay types'), + queryset=ModuleBayType.objects.all(), + to_field_name='name', + required=False, + help_text=_('Types of module bays this bay accepts (empty = unconstrained)'), + ) class Meta: model = ModuleBay - fields = ('device', 'name', 'label', 'position', 'enabled', 'description', 'owner', 'tags') + fields = ( + 'device', 'name', 'label', 'position', 'enabled', 'description', 'owner', 'tags', 'module_bay_types', + ) def clean_enabled(self): # Make sure enabled is True when it's not included in the uploaded data @@ -1450,6 +1459,16 @@ class ModuleBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): return True return self.cleaned_data['enabled'] + def clean(self): + super().clean() + + if module_bay_types := self.cleaned_data.get('module_bay_types'): + device = self.cleaned_data.get('device') + manufacturer = device.device_type.manufacturer if device else None + self.cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( + module_bay_types, manufacturer, + ) + class DeviceBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): device = CSVModelChoiceField( diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index a048139fa..54f005699 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -4,6 +4,7 @@ from django.utils.translation import gettext_lazy as _ from dcim.choices import InterfacePoEModeChoices, InterfacePoETypeChoices, InterfaceTypeChoices, PortTypeChoices from dcim.models import * from dcim.utils import dedupe_module_bay_types_by_manufacturer +from utilities.forms.fields import CSVModelMultipleChoiceField from wireless.choices import WirelessRoleChoices __all__ = ( @@ -214,7 +215,13 @@ class PortTemplateMappingImportForm(forms.ModelForm): class ModuleBayTemplateImportForm(forms.ModelForm): - module_bay_types = forms.ModelMultipleChoiceField( + # CSVModelMultipleChoiceField (not the plain ModelMultipleChoiceField used elsewhere in + # this file) so a scalar name string is accepted alongside a list -- this form is + # YAML-only, but ModuleTypeImportForm's equivalent field also serves plain CSV import and + # therefore must accept both; keeping the two consistent means `module_bay_types: SFP28` + # behaves the same whether it appears at the module-type level or under `module-bays:` + # within the same YAML document. + module_bay_types = CSVModelMultipleChoiceField( label=_('Module bay types'), queryset=ModuleBayType.objects.all(), to_field_name='name', diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index b6dc96406..1ece0d149 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -396,6 +396,42 @@ class ModuleBayTemplateImportFormTestCase(TestCase): self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) +class ModuleBayImportFormTestCase(TestCase): + """ + ModuleBayImportForm covers real ModuleBay instances created directly via CSV (as + opposed to ModuleBayTemplateImportForm, which covers templates nested under a device or + module type's YAML definition) -- the same class of round-trip gap, on the instance side. + """ + + def test_module_bay_types_csv_import(self): + device = create_test_device('Module Bay Import Device') + bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28') + + form = ModuleBayImportForm({ + 'device': device.name, + 'name': 'Bay 1', + 'module_bay_types': 'SFP28', + }) + self.assertTrue(form.is_valid(), form.errors) + module_bay = form.save() + self.assertEqual(list(module_bay.module_bay_types.all()), [bay_type]) + + def test_module_bay_types_prefers_devices_own_manufacturer(self): + device = create_test_device('Module Bay Import Device') + own_manufacturer = device.device_type.manufacturer + other_manufacturer = Manufacturer.objects.create(name='Other Mfr', slug='other-mfr') + own_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-own', manufacturer=own_manufacturer) + ModuleBayType.objects.create(name='SFP28', slug='sfp28-other', manufacturer=other_manufacturer) + + form = ModuleBayImportForm({ + 'device': device.name, + 'name': 'Bay 1', + 'module_bay_types': 'SFP28', + }) + self.assertTrue(form.is_valid(), form.errors) + self.assertEqual(list(form.save().module_bay_types.all()), [own_type]) + + class ModuleTypeImportFormTestCase(TestCase): def test_module_bay_types_round_trip(self): @@ -497,6 +533,27 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertTrue(reimport_form.is_valid(), reimport_form.errors) self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) + def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): + """ + A single other-manufacturer match is permitted (see above), but if the name matches + two or more *different* foreign manufacturers, there's no principled way to choose + one -- silently picking whichever sorts first would create a wrong FK link with no + signal to the importer. This must be refused rather than resolved arbitrarily. + """ + juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') + cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') + arista = Manufacturer.objects.create(name='Arista', slug='arista') + ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) + ModuleBayType.objects.create(name='SFP28', slug='sfp28-arista', manufacturer=arista) + + form = ModuleTypeImportForm({ + 'manufacturer': juniper.name, + 'model': 'Juniper Line Card', + 'module_bay_types': ['SFP28'], + }) + self.assertFalse(form.is_valid()) + self.assertIn('module_bay_types', form.errors) + class ModuleFormTestCase(TestCase): diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index c01f41887..0d226a5a2 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1827,6 +1827,44 @@ module-bays: self.assertEqual(len(one_bay_queries), len(five_bay_queries)) + def test_bulk_yaml_export_prefetches_module_bay_types_on_the_module_type_itself(self): + """ + Companion to test_..._is_constant above: that test holds the module type count fixed + at 1 and varies bay count, so it can't detect a regression in the module_bay_types + prefetch on ModuleType itself (which saves one query per module TYPE row, not per + bay) -- a 1-row queryset can't show a per-row saving. Comparing module-type COUNTS + (e.g. 1 vs. 5) doesn't work either: to_yaml() touches several other per-instance + relations (manufacturer, port_mappings, ...) that legitimately scale with row count + regardless of this fix, which would swamp the signal. Instead, compare the *same* + 5-row queryset with and without the module_bay_types prefetch, isolating exactly what + it saves. + """ + 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. The exact delta isn't asserted -- prefetching modulebaytemplates (even when + # empty, as here) also lets to_yaml()'s .exists() check on that relation short-circuit + # from the prefetch cache instead of querying, so the totals reflect more than just + # module_bay_types -- but dropping the module_bay_types prefetch can only narrow this + # gap, never widen it, so a strict inequality still catches that regression. + self.assertGreater(len(unprefetched), len(prefetched)) + @override_settings(STREAMING_EXPORTS=True) def test_export_objects(self): url = reverse('dcim:moduletype_list') diff --git a/netbox/dcim/utils.py b/netbox/dcim/utils.py index ea2a102e4..f9efa07c8 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -2,6 +2,7 @@ from collections import defaultdict from django.apps import apps from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import ValidationError from django.db import router, transaction from django.utils.translation import gettext as _ @@ -12,7 +13,7 @@ def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None) """ Collapse an iterable of ModuleBayType instances resolved by name to one entry per name, preferring (in order) an exact match on *manufacturer*, then a global (manufacturer-less) - type, then any remaining candidate. + type. ModuleBayType's uniqueness is scoped to (manufacturer, name), not name alone, so two different manufacturers -- or a global type and a manufacturer-scoped one -- can @@ -21,6 +22,11 @@ def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None) type (e.g. a third-party line card), so callers must not scope the underlying queryset by manufacturer -- only this preference order, for disambiguating an otherwise-ambiguous name, is manufacturer-aware. + + Raises ValidationError if a name resolves to more than one candidate that ties for the + best preference tier (e.g. two different manufacturers, neither *manufacturer* nor + unset, share the name) -- there's no principled way to pick a winner there, so the + import is refused rather than silently linked to an arbitrary one. """ manufacturer_id = manufacturer.pk if manufacturer else None @@ -31,12 +37,26 @@ def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None) return 1 return 2 - resolved = {} + by_name = defaultdict(list) for module_bay_type in module_bay_types: - existing = resolved.get(module_bay_type.name) - if existing is None or preference(module_bay_type) < preference(existing): - resolved[module_bay_type.name] = module_bay_type - return list(resolved.values()) + by_name[module_bay_type.name].append(module_bay_type) + + resolved = [] + for name, candidates in by_name.items(): + best_rank = min(preference(c) for c in candidates) + best = [c for c in candidates if preference(c) == best_rank] + if len(best) > 1: + manufacturers = ', '.join(sorted(c.manufacturer.name for c in best)) + raise ValidationError({ + 'module_bay_types': _( + "Module bay type \"{name}\" is ambiguous: it belongs to more than one " + "manufacturer ({manufacturers}), none of which is this type's own " + "manufacturer." + ).format(name=name, manufacturers=manufacturers) + }) + resolved.append(best[0]) + + return resolved def inherit_module_token(position, parent_positions): From dfde52df05c1a47d8011a43346ac18337c56376f Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 14:25:36 -0400 Subject: [PATCH 07/14] Fix CSVModelMultipleChoiceField's own export/import round trip; docs; hardening - CSVModelMultipleChoiceField.clean() split on a bare comma with no whitespace stripping, but ManyToManyColumn's default CSV export separator is ", " (comma + space) -- so re-importing NetBox's own CSV export of any multi-value column using this field (module_bay_types among others, since this is a shared utility field) failed with "Object not found: " on every value after the first. Verified directly against ModuleTypeTable's actual export value before fixing. Also cast to str() before splitting: a YAML-bound caller (as opposed to a CSV cell, always a string) can pass a non-string scalar, which previously raised an unhandled AttributeError instead of a form error. - Docs for module bay type resolution still described the pre-a3b5e4b fallback ("then any remaining candidate"); updated to describe the refusal behavior that replaced it. Added a matching note to modulebay.md, which had none. - dedupe_module_bay_types_by_manufacturer() collapses candidates by pk within each name group before computing preference, so a caller passing a duplicate row in a raw list -- the signature accepts "an iterable," not just a queryset -- can't manufacture a same-manufacturer tie that would then crash on None.manufacturer.name. Unreachable via the three current callers today (each resolves from a queryset, which can't contain a row twice), but cheap to make the helper safe standalone. - Fixed a stale test docstring contrasting the two import forms' field types by a distinction (plain vs. CSV multiple-choice field) that no longer exists since both were aligned to CSVModelMultipleChoiceField. - Added ambiguity-refusal coverage at the other two call sites (ModuleBayTemplateImportForm, ModuleBayImportForm) -- previously only ModuleTypeImportForm was covered for this path. Also found independently while verifying the above: ModuleTypeListView .export_yaml() prefetched modulebaytemplates__module_bay_types, but ModuleType.to_yaml() -- unlike DeviceType.to_yaml() -- never reads self.modulebaytemplates at all (a separate, pre-existing, out-of-scope gap: ModuleType.to_yaml() doesn't export a nested module-bays section). That prefetch was dead weight, adding a query with no corresponding saving. Removed it, and with it the now-meaningless "bay count doesn't affect query count" test (nothing in ModuleType.to_yaml() ever varied with bay count to begin with), replacing it with an exact-delta assertion isolating what the one relevant prefetch (module_bay_types on the module type itself) actually saves. --- docs/models/dcim/modulebay.md | 2 + docs/models/dcim/modulebaytemplate.md | 2 +- docs/models/dcim/moduletype.md | 2 +- netbox/dcim/tests/test_forms.py | 86 +++++++++++++++++++++++++-- netbox/dcim/tests/test_views.py | 58 ++++-------------- netbox/dcim/utils.py | 10 +++- netbox/dcim/views.py | 16 +++-- netbox/utilities/forms/fields/csv.py | 8 ++- 8 files changed, 119 insertions(+), 65 deletions(-) diff --git a/docs/models/dcim/modulebay.md b/docs/models/dcim/modulebay.md index 4df243ca1..ffb3945ed 100644 --- a/docs/models/dcim/modulebay.md +++ b/docs/models/dcim/modulebay.md @@ -34,6 +34,8 @@ The numeric position in which this module bay is situated. For example, this wou Zero or more [module bay types](./modulebaytype.md) assigned to this bay. When at least one bay type is set, only module types that share a common bay type may be installed. Leave empty to allow any module type. +Bay types are importable via CSV, referenced by name, with the same manufacturer-based resolution described for [module bay templates](./modulebaytemplate.md) -- scoped to the manufacturer of the module bay's own device. + ### Enabled Whether this module bay is enabled. Disabled module bays are not available for installation. diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index ad96b44e5..63bdd7050 100644 --- a/docs/models/dcim/modulebaytemplate.md +++ b/docs/models/dcim/modulebaytemplate.md @@ -4,4 +4,4 @@ A template for a module bay that will be created on all instantiations of the pa [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 and exportable as part of a device type's YAML definition (`module-bays[].module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the device type's own may be referenced; this mirrors the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the device type's own manufacturer, then a global (manufacturer-less) bay type, then any remaining candidate. +Bay types are importable and exportable as part of a device type's YAML definition (`module-bays[].module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the device type's own may be referenced; this mirrors the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the device type's own manufacturer, then a global (manufacturer-less) bay type. If a name instead matches two or more bay types belonging to *other* manufacturers, with neither the device type's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. diff --git a/docs/models/dcim/moduletype.md b/docs/models/dcim/moduletype.md index 8b79c6c9f..161e98790 100644 --- a/docs/models/dcim/moduletype.md +++ b/docs/models/dcim/moduletype.md @@ -91,7 +91,7 @@ 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 importable and exportable as part of a module type's YAML definition (`module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the module type's own may be referenced -- e.g. a third-party module declaring compatibility with another manufacturer's proprietary bay type -- mirroring the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the module type's own manufacturer, then a global (manufacturer-less) bay type, then any remaining candidate. +Bay types are importable and exportable as part of a module type's YAML definition (`module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the module type's own may be referenced -- e.g. a third-party module declaring compatibility with another manufacturer's proprietary bay type -- mirroring the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the module type's own manufacturer, then a global (manufacturer-less) bay type. If a name instead matches two or more bay types belonging to *other* manufacturers, with neither the module type's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. ### Attributes diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 1ece0d149..22a26719c 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -17,6 +17,7 @@ from dcim.choices import ( ) from dcim.forms import * from dcim.models import * +from dcim.tables.modules import ModuleTypeTable from dcim.tests.test_module_moves import fail_after from ipam.models import ASN, RIR, VLAN from utilities.exceptions import AbortRequest @@ -395,6 +396,24 @@ class ModuleBayTemplateImportFormTestCase(TestCase): self.assertTrue(reimport_form.is_valid(), reimport_form.errors) self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) + def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): + juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') + cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') + arista = Manufacturer.objects.create(name='Arista', slug='arista') + ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) + ModuleBayType.objects.create(name='SFP28', slug='sfp28-arista', manufacturer=arista) + device_type = DeviceType.objects.create( + manufacturer=juniper, model='Juniper Device Type 2', slug='juniper-device-type-2', + ) + + form = ModuleBayTemplateImportForm({ + 'device_type': device_type.pk, + 'name': 'Module Bay 1', + 'module_bay_types': ['SFP28'], + }) + self.assertFalse(form.is_valid()) + self.assertIn('module_bay_types', form.errors) + class ModuleBayImportFormTestCase(TestCase): """ @@ -431,6 +450,21 @@ class ModuleBayImportFormTestCase(TestCase): self.assertTrue(form.is_valid(), form.errors) self.assertEqual(list(form.save().module_bay_types.all()), [own_type]) + def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): + device = create_test_device('Module Bay Import Device') + cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') + arista = Manufacturer.objects.create(name='Arista', slug='arista') + ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) + ModuleBayType.objects.create(name='SFP28', slug='sfp28-arista', manufacturer=arista) + + form = ModuleBayImportForm({ + 'device': device.name, + 'name': 'Bay 1', + 'module_bay_types': 'SFP28', + }) + self.assertFalse(form.is_valid()) + self.assertIn('module_bay_types', form.errors) + class ModuleTypeImportFormTestCase(TestCase): @@ -469,10 +503,9 @@ class ModuleTypeImportFormTestCase(TestCase): def test_module_bay_types_accepts_csv_comma_separated_string(self): """ - Unlike ModuleBayTemplateImportForm.module_bay_types (a plain ModelMultipleChoiceField, - bound only from YAML-parsed lists), this form's module_bay_types is a - CSVModelMultipleChoiceField because ModuleTypeImportForm also serves plain CSV bulk - import, where the cell value arrives as a comma-separated string rather than a list. + module_bay_types is a CSVModelMultipleChoiceField because ModuleTypeImportForm also + serves plain CSV bulk import, where the cell value arrives as a comma-separated + string rather than a list. """ manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28') @@ -502,6 +535,51 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertTrue(form.is_valid(), form.errors) self.assertFalse(form.save().module_bay_types.exists()) + def test_module_bay_types_round_trips_through_the_table_column_export_value(self): + """ + ManyToManyColumn's export separator is ", " (comma + space, django-tables2's + default), not the bare "," this field's CSVModelMultipleChoiceField splits on -- so + re-importing NetBox's own CSV export of a multi-value module_bay_types column must + not fail on the leading space of every value after the first. + """ + 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') + original = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1') + original.module_bay_types.set([bay_type_a, bay_type_b]) + + table = ModuleTypeTable([original]) + exported_value = table.columns['module_bay_types'].column.value(original.module_bay_types.all()) + self.assertEqual(exported_value, 'QSFP28, SFP28') + + form = ModuleTypeImportForm({ + 'manufacturer': manufacturer.name, + 'model': 'Module Type 2', + 'module_bay_types': exported_value, + }) + self.assertTrue(form.is_valid(), form.errors) + self.assertEqual( + set(form.save().module_bay_types.values_list('name', flat=True)), + {bay_type_a.name, bay_type_b.name}, + ) + + def test_module_bay_types_non_string_scalar_is_a_validation_error_not_a_crash(self): + """ + A CSV cell is always a string, but this field is also bound from YAML-parsed data + (e.g. via ModuleBayTemplateImportForm), where a scalar column can arrive as a + non-string (an int or bool). .split() on that would raise AttributeError -- an + unhandled 500 rather than a form error. + """ + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + + form = ModuleTypeImportForm({ + 'manufacturer': manufacturer.name, + 'model': 'Module Type 1', + 'module_bay_types': 100, + }) + self.assertFalse(form.is_valid()) + self.assertIn('module_bay_types', form.errors) + def test_module_bay_types_permits_a_different_manufacturers_type(self): """ The UI (ModuleTypeForm) and REST API place no manufacturer restriction on diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 0d226a5a2..65fd5a2d9 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1796,48 +1796,18 @@ module-bays: self.assertEqual(mb1.position, '1') self.assertEqual(list(mb1.module_bay_types.values_list('name', flat=True)), ['SFP28']) - def test_bulk_yaml_export_module_bay_types_query_count_is_constant(self): - """ - ModuleTypeListView.export_yaml() prefetches both module_bay_types (on the module type - itself) and modulebaytemplates__module_bay_types (on its nested module bay templates), - so neither adds a query per module bay template as the number of bays grows. - """ - 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_module_type(model_name, bay_count): - module_type = ModuleType.objects.create(manufacturer=manufacturer, model=model_name) - module_type.module_bay_types.set([bay_type]) - for i in range(bay_count): - bay = ModuleBayTemplate.objects.create(module_type=module_type, name=f'Bay {i}') - bay.module_bay_types.set([bay_type]) - return module_type - - one_bay_module_type = make_module_type('Export Query MT One Bay', 1) - five_bay_module_type = make_module_type('Export Query MT Five Bays', 5) - - view = ModuleTypeListView() - view.queryset = ModuleType.objects.filter(pk=one_bay_module_type.pk) - with CaptureQueriesContext(connection) as one_bay_queries: - view.export_yaml() - - view.queryset = ModuleType.objects.filter(pk=five_bay_module_type.pk) - with CaptureQueriesContext(connection) as five_bay_queries: - view.export_yaml() - - self.assertEqual(len(one_bay_queries), len(five_bay_queries)) - def test_bulk_yaml_export_prefetches_module_bay_types_on_the_module_type_itself(self): """ - Companion to test_..._is_constant above: that test holds the module type count fixed - at 1 and varies bay count, so it can't detect a regression in the module_bay_types - prefetch on ModuleType itself (which saves one query per module TYPE row, not per - bay) -- a 1-row queryset can't show a per-row saving. Comparing module-type COUNTS - (e.g. 1 vs. 5) doesn't work either: to_yaml() touches several other per-instance - relations (manufacturer, port_mappings, ...) that legitimately scale with row count - regardless of this fix, which would swamp the signal. Instead, compare the *same* - 5-row queryset with and without the module_bay_types prefetch, isolating exactly what - it saves. + Comparing module-type COUNTS (e.g. 1 vs. 5) to detect a per-row prefetch saving + doesn't work: to_yaml() touches several other per-instance relations (manufacturer, + port_mappings, ...) that legitimately scale with row count regardless of this fix, + which would swamp the signal. Instead, compare the *same* 5-row queryset with and + without the module_bay_types prefetch, isolating exactly what it saves. + + Unlike DeviceType.to_yaml(), ModuleType.to_yaml() does not export a nested + module-bays section at all (a separate, pre-existing gap, out of scope here), so + module_bay_types is the only relation ModuleTypeListView.export_yaml() needs to + prefetch -- confirmed by this test's exact-delta assertion below. """ 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') @@ -1858,12 +1828,8 @@ module-bays: view.export_yaml() # Without the prefetch, each of the 5 module types issues its own module_bay_types - # query. The exact delta isn't asserted -- prefetching modulebaytemplates (even when - # empty, as here) also lets to_yaml()'s .exists() check on that relation short-circuit - # from the prefetch cache instead of querying, so the totals reflect more than just - # module_bay_types -- but dropping the module_bay_types prefetch can only narrow this - # gap, never widen it, so a strict inequality still catches that regression. - self.assertGreater(len(unprefetched), len(prefetched)) + # 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/utils.py b/netbox/dcim/utils.py index f9efa07c8..bc1ec3e90 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -37,12 +37,16 @@ def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None) return 1 return 2 - by_name = defaultdict(list) + by_name = defaultdict(dict) for module_bay_type in module_bay_types: - by_name[module_bay_type.name].append(module_bay_type) + # Keyed by pk within each name group so a caller passing the same row twice (the + # three current callers never do, since each resolves from a queryset, but the + # signature accepts any iterable) can't manufacture a same-manufacturer "tie" below. + by_name[module_bay_type.name][module_bay_type.pk] = module_bay_type resolved = [] - for name, candidates in by_name.items(): + for name, candidates_by_pk in by_name.items(): + candidates = list(candidates_by_pk.values()) best_rank = min(preference(c) for c in candidates) best = [c for c in candidates if preference(c) == best_rank] if len(best) > 1: diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index dff5af244..65b17dcd6 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1912,15 +1912,13 @@ class ModuleTypeListView(generic.ObjectListView): table = tables.ModuleTypeTable def export_yaml(self): - # to_yaml() reads module_bay_types directly, plus each nested module bay template's - # own module_bay_types -- prefetch both so these relations don't add a query per - # module type/module bay template across the whole queryset. to_yaml()'s other - # component-template relations (interfaces, ports, etc.) are unprefetched here as - # they were before these relations existed, and remain their own N+1 across a large - # export. - self.queryset = self.queryset.prefetch_related( - 'module_bay_types', 'modulebaytemplates__module_bay_types', - ) + # to_yaml() reads module_bay_types directly -- unlike DeviceType.to_yaml(), it does + # not export a nested module-bays section at all (a separate, pre-existing gap, out + # of scope here), so there is no modulebaytemplates relation to prefetch alongside + # it. to_yaml()'s other component-template relations (interfaces, ports, etc.) are + # unprefetched here as they were before this relation existed, and remain their own + # N+1 across a large export. + self.queryset = self.queryset.prefetch_related('module_bay_types') return super().export_yaml() diff --git a/netbox/utilities/forms/fields/csv.py b/netbox/utilities/forms/fields/csv.py index 497a1816e..0316068af 100644 --- a/netbox/utilities/forms/fields/csv.py +++ b/netbox/utilities/forms/fields/csv.py @@ -100,7 +100,13 @@ class CSVModelMultipleChoiceField(forms.ModelMultipleChoiceField): def clean(self, value): if not isinstance(value, list): - value = value.split(',') if value else [] + # str(value): a caller may bind this field from parsed YAML/JSON rather than a + # CSV cell, where a scalar column can arrive as a non-string (e.g. an int or + # bool) -- .split() would raise AttributeError otherwise. .strip() each piece: + # a table's default ManyToManyColumn export separator is ", " (comma space), so + # re-importing NetBox's own CSV export of a multi-value column would otherwise + # fail to match on the leading space. + value = [v.strip() for v in str(value).split(',')] if value else [] return super().clean(value) From faafb1a11fbee7191eb21eb4a45fa67b01ae54e7 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 15:21:59 -0400 Subject: [PATCH 08/14] Fix manufacturer-scoped bay type CSV export; tighten ambiguity tests; trim comments - ModuleBayType.__str__() includes the manufacturer (e.g. "Cisco SFP28"), but the three module_bay_types ManyToManyColumn declarations had no transform, so django-tables2 defaulted to str() for CSV export while the import forms resolve by name alone. Verified directly: exporting a manufacturer-scoped bay type produced "Cisco SFP28", which then failed to re-import with "Object not found: Cisco SFP28" -- broken for exactly the case (manufacturer-scoped types) the preference/ ambiguity machinery exists to serve. Set transform=lambda obj: obj.name on all three columns to match to_yaml(), and rewrote the existing round-trip test to use a manufacturer-scoped bay type instead of a global one, which is the only case that exercised str(). - The three ambiguity-refusal tests asserted only that the field errored, which a plain invalid_choice (e.g. from a queryset that excluded both candidates) would also satisfy -- masking a regression of the manufacturer scoping removed two commits ago. Tightened each to assert the error names both competing manufacturers. - Corrected modulebay.md, which still described module_bay_types resolution as "scoped to" the device's manufacturer -- the behavior the prior commit removed as a bug; it's a preference, not a scope. - Trimmed comments and docstrings introduced across this branch to a more proportionate length. Deliberately out of scope for this PR (tracked as follow-up considerations, not fixed here): an escape hatch for a bay type name that's genuinely ambiguous across manufacturers with no local match (would require a new wire-format convention), and ModuleType.to_yaml() not exporting a module-bays section at all (a separate, pre-existing asymmetry, larger than this PR's scope). --- docs/models/dcim/modulebay.md | 2 +- netbox/dcim/forms/object_import.py | 14 ++--- netbox/dcim/tables/devices.py | 2 + netbox/dcim/tables/devicetypes.py | 2 + netbox/dcim/tables/modules.py | 2 + netbox/dcim/tests/test_forms.py | 86 ++++++++-------------------- netbox/dcim/tests/test_views.py | 20 +------ netbox/dcim/utils.py | 26 +++------ netbox/dcim/views.py | 15 ++--- netbox/utilities/forms/fields/csv.py | 8 +-- 10 files changed, 52 insertions(+), 125 deletions(-) diff --git a/docs/models/dcim/modulebay.md b/docs/models/dcim/modulebay.md index ffb3945ed..103a95632 100644 --- a/docs/models/dcim/modulebay.md +++ b/docs/models/dcim/modulebay.md @@ -34,7 +34,7 @@ The numeric position in which this module bay is situated. For example, this wou Zero or more [module bay types](./modulebaytype.md) assigned to this bay. When at least one bay type is set, only module types that share a common bay type may be installed. Leave empty to allow any module type. -Bay types are importable via CSV, referenced by name, with the same manufacturer-based resolution described for [module bay templates](./modulebaytemplate.md) -- scoped to the manufacturer of the module bay's own device. +Bay types are importable via CSV, referenced by name. A bay type belonging to a manufacturer other than the module bay's own device may be referenced; this mirrors the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the device's own manufacturer, then a global (manufacturer-less) bay type. If a name instead matches two or more bay types belonging to *other* manufacturers, with neither the device's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. ### Enabled diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index 54f005699..b6da78dc6 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -215,12 +215,9 @@ class PortTemplateMappingImportForm(forms.ModelForm): class ModuleBayTemplateImportForm(forms.ModelForm): - # CSVModelMultipleChoiceField (not the plain ModelMultipleChoiceField used elsewhere in - # this file) so a scalar name string is accepted alongside a list -- this form is - # YAML-only, but ModuleTypeImportForm's equivalent field also serves plain CSV import and - # therefore must accept both; keeping the two consistent means `module_bay_types: SFP28` - # behaves the same whether it appears at the module-type level or under `module-bays:` - # within the same YAML document. + # CSVModelMultipleChoiceField, not the plain ModelMultipleChoiceField used elsewhere in + # this file, so a scalar name is accepted alongside a list -- matches ModuleTypeImportForm's + # equivalent field, which also serves plain CSV import. module_bay_types = CSVModelMultipleChoiceField( label=_('Module bay types'), queryset=ModuleBayType.objects.all(), @@ -237,10 +234,7 @@ class ModuleBayTemplateImportForm(forms.ModelForm): def clean_enabled(self): # A dict-bound BooleanField resolves a missing key to False, not the model's own - # default=True -- match ModuleBayImportForm's equivalent CSV-import behavior. Reads - # self.data directly (no add_prefix()/QueryDict handling) because, like that form, - # this one is only ever bound to a plain dict of import data, never a real HTML - # checkbox POST. + # default=True -- match ModuleBayImportForm's equivalent CSV-import behavior. if 'enabled' not in self.data: return True return self.cleaned_data['enabled'] diff --git a/netbox/dcim/tables/devices.py b/netbox/dcim/tables/devices.py index ac10e8bd2..6c5ad9249 100644 --- a/netbox/dcim/tables/devices.py +++ b/netbox/dcim/tables/devices.py @@ -1028,6 +1028,8 @@ class ModuleBayTable(ModularDeviceComponentTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, + # __str__() includes the manufacturer, but import resolves by name alone. + transform=lambda obj: obj.name, ) class Meta(ModularDeviceComponentTable.Meta): diff --git a/netbox/dcim/tables/devicetypes.py b/netbox/dcim/tables/devicetypes.py index 3e4682d0d..427ca0ee4 100644 --- a/netbox/dcim/tables/devicetypes.py +++ b/netbox/dcim/tables/devicetypes.py @@ -305,6 +305,8 @@ class ModuleBayTemplateTable(ComponentTemplateTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, + # __str__() includes the manufacturer, but import resolves by name alone. + transform=lambda obj: obj.name, ) actions = columns.ActionsColumn( actions=('edit', 'delete') diff --git a/netbox/dcim/tables/modules.py b/netbox/dcim/tables/modules.py index a8aebf873..2586b1155 100644 --- a/netbox/dcim/tables/modules.py +++ b/netbox/dcim/tables/modules.py @@ -78,6 +78,8 @@ class ModuleTypeTable(PrimaryModelTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, + # __str__() includes the manufacturer, but import resolves by name alone. + transform=lambda obj: obj.name, ) model = tables.Column( linkify=True, diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 22a26719c..559b099d3 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -234,12 +234,7 @@ class ModuleTypeFormTestCase(TestCase): class ModuleBayTemplateImportFormTestCase(TestCase): def test_module_bay_types_prefers_manufacturer_specific_match_over_global(self): - """ - ModuleBayType's unique constraint is on (manufacturer, name), not name alone, so a - global type and a manufacturer-scoped type can legally share the same name. Referencing - that name by import should resolve to the manufacturer-specific match only, not attach - both. - """ + """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( @@ -280,10 +275,7 @@ class ModuleBayTemplateImportFormTestCase(TestCase): ) def test_module_bay_types_prefers_manufacturer_specific_match_over_global_for_module_type(self): - """ - Same disambiguation as the device_type-scoped case, but through the module_type path - (a module bay template nested within a ModuleType rather than a DeviceType). - """ + """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( @@ -334,11 +326,7 @@ class ModuleBayTemplateImportFormTestCase(TestCase): self.assertFalse(form.save().enabled) def test_import_export_round_trip_preserves_module_bay_types(self): - """ - A ModuleBayTemplate exported via to_yaml() and re-imported through this form should - end up with the same module bay types, closing the exact export/import loop this - feature exists for. - """ + """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') @@ -363,13 +351,7 @@ class ModuleBayTemplateImportFormTestCase(TestCase): ) def test_module_bay_types_permits_a_different_manufacturers_type(self): - """ - The UI (ModuleBayTemplateForm) and REST API place no manufacturer restriction on - module_bay_types -- a third-party device may legitimately declare a bay compatible - with another manufacturer's proprietary bay type. Import must permit the same, and a - type assigned this way must survive an export/re-import round trip rather than - becoming permanently unimportable. - """ + """The UI/API place no manufacturer restriction on module_bay_types; import must match.""" juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') cisco_bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) @@ -412,15 +394,14 @@ class ModuleBayTemplateImportFormTestCase(TestCase): 'module_bay_types': ['SFP28'], }) self.assertFalse(form.is_valid()) - self.assertIn('module_bay_types', form.errors) + # Must name both manufacturers, not just error -- a plain invalid_choice would also + # pass assertIn() and mask a regression of the scoping removed in 6f3c537. + errors = form.errors.get('module_bay_types', []) + self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) class ModuleBayImportFormTestCase(TestCase): - """ - ModuleBayImportForm covers real ModuleBay instances created directly via CSV (as - opposed to ModuleBayTemplateImportForm, which covers templates nested under a device or - module type's YAML definition) -- the same class of round-trip gap, on the instance side. - """ + """Covers real ModuleBay instances via CSV, as opposed to templates via ModuleBayTemplateImportForm.""" def test_module_bay_types_csv_import(self): device = create_test_device('Module Bay Import Device') @@ -463,7 +444,10 @@ class ModuleBayImportFormTestCase(TestCase): 'module_bay_types': 'SFP28', }) self.assertFalse(form.is_valid()) - self.assertIn('module_bay_types', form.errors) + # Must name both manufacturers, not just error -- a plain invalid_choice would also + # pass assertIn() and mask a regression of the scoping removed in 6f3c537. + errors = form.errors.get('module_bay_types', []) + self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) class ModuleTypeImportFormTestCase(TestCase): @@ -502,11 +486,7 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertNotIn(global_type, module_type.module_bay_types.all()) def test_module_bay_types_accepts_csv_comma_separated_string(self): - """ - module_bay_types is a CSVModelMultipleChoiceField because ModuleTypeImportForm also - serves plain CSV bulk import, where the cell value arrives as a comma-separated - string rather than a list. - """ + """This form also serves plain CSV import, where the value is a string, not a list.""" 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') @@ -536,15 +516,10 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertFalse(form.save().module_bay_types.exists()) def test_module_bay_types_round_trips_through_the_table_column_export_value(self): - """ - ManyToManyColumn's export separator is ", " (comma + space, django-tables2's - default), not the bare "," this field's CSVModelMultipleChoiceField splits on -- so - re-importing NetBox's own CSV export of a multi-value module_bay_types column must - not fail on the leading space of every value after the first. - """ + """The table's CSV export (multi-value separator, name-only transform) must be re-importable.""" 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') + bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28', manufacturer=manufacturer) + bay_type_b = ModuleBayType.objects.create(name='QSFP28', slug='qsfp28', manufacturer=manufacturer) original = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1') original.module_bay_types.set([bay_type_a, bay_type_b]) @@ -564,12 +539,7 @@ class ModuleTypeImportFormTestCase(TestCase): ) def test_module_bay_types_non_string_scalar_is_a_validation_error_not_a_crash(self): - """ - A CSV cell is always a string, but this field is also bound from YAML-parsed data - (e.g. via ModuleBayTemplateImportForm), where a scalar column can arrive as a - non-string (an int or bool). .split() on that would raise AttributeError -- an - unhandled 500 rather than a form error. - """ + """A non-string scalar (e.g. from YAML) must produce a form error, not a crash.""" manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') form = ModuleTypeImportForm({ @@ -581,13 +551,7 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertIn('module_bay_types', form.errors) def test_module_bay_types_permits_a_different_manufacturers_type(self): - """ - The UI (ModuleTypeForm) and REST API place no manufacturer restriction on - module_bay_types -- a third-party module may legitimately declare compatibility with - another manufacturer's proprietary bay type. Import must permit the same, and a type - created this way must survive an export/re-import round trip rather than becoming - permanently unimportable. - """ + """The UI/API place no manufacturer restriction on module_bay_types; import must match.""" juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') cisco_bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) @@ -612,12 +576,7 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): - """ - A single other-manufacturer match is permitted (see above), but if the name matches - two or more *different* foreign manufacturers, there's no principled way to choose - one -- silently picking whichever sorts first would create a wrong FK link with no - signal to the importer. This must be refused rather than resolved arbitrarily. - """ + """A name matching two different foreign manufacturers must be refused, not resolved arbitrarily.""" juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') arista = Manufacturer.objects.create(name='Arista', slug='arista') @@ -630,7 +589,10 @@ class ModuleTypeImportFormTestCase(TestCase): 'module_bay_types': ['SFP28'], }) self.assertFalse(form.is_valid()) - self.assertIn('module_bay_types', form.errors) + # Must name both manufacturers, not just error -- a plain invalid_choice would also + # pass assertIn() and mask a regression of the scoping removed in 6f3c537. + errors = form.errors.get('module_bay_types', []) + self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) class ModuleFormTestCase(TestCase): diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 65fd5a2d9..81da915c8 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1143,11 +1143,7 @@ inventory-items: self.assertEqual(ii1.name, 'Inventory Item 1') def test_bulk_yaml_export_module_bay_types_query_count_is_constant(self): - """ - DeviceTypeListView.export_yaml() prefetches modulebaytemplates__module_bay_types so - that to_yaml()'s per-bay module_bay_types lookup doesn't add one query per module bay - template as the number of bays grows. - """ + """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') @@ -1797,18 +1793,8 @@ module-bays: 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): - """ - Comparing module-type COUNTS (e.g. 1 vs. 5) to detect a per-row prefetch saving - doesn't work: to_yaml() touches several other per-instance relations (manufacturer, - port_mappings, ...) that legitimately scale with row count regardless of this fix, - which would swamp the signal. Instead, compare the *same* 5-row queryset with and - without the module_bay_types prefetch, isolating exactly what it saves. - - Unlike DeviceType.to_yaml(), ModuleType.to_yaml() does not export a nested - module-bays section at all (a separate, pre-existing gap, out of scope here), so - module_bay_types is the only relation ModuleTypeListView.export_yaml() needs to - prefetch -- confirmed by this test's exact-delta assertion below. - """ + """Compares the same queryset with/without the prefetch, since row-count comparisons + would be swamped by other per-instance relations that legitimately scale with it.""" 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') diff --git a/netbox/dcim/utils.py b/netbox/dcim/utils.py index bc1ec3e90..313b67d3e 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -11,22 +11,14 @@ from dcim.constants import MODULE_TOKEN def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None): """ - Collapse an iterable of ModuleBayType instances resolved by name to one entry per name, - preferring (in order) an exact match on *manufacturer*, then a global (manufacturer-less) - type. + Collapse ModuleBayType instances resolved by name to one per name, preferring an exact + match on *manufacturer*, then a global (manufacturer-less) type. Names aren't globally + unique -- uniqueness is scoped to (manufacturer, name) -- and callers must not scope the + queryset by manufacturer themselves, since a type may legitimately belong to another + manufacturer entirely; only this preference order is manufacturer-aware. - ModuleBayType's uniqueness is scoped to (manufacturer, name), not name alone, so two - different manufacturers -- or a global type and a manufacturer-scoped one -- can - legitimately share a name. This does not exclude any manufacturer's types: a module or - bay may legitimately declare compatibility with another manufacturer's proprietary bay - type (e.g. a third-party line card), so callers must not scope the underlying queryset - by manufacturer -- only this preference order, for disambiguating an otherwise-ambiguous - name, is manufacturer-aware. - - Raises ValidationError if a name resolves to more than one candidate that ties for the - best preference tier (e.g. two different manufacturers, neither *manufacturer* nor - unset, share the name) -- there's no principled way to pick a winner there, so the - import is refused rather than silently linked to an arbitrary one. + Raises ValidationError if a name ties across two or more non-preferred manufacturers, + rather than picking one arbitrarily. """ manufacturer_id = manufacturer.pk if manufacturer else None @@ -37,11 +29,9 @@ def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None) return 1 return 2 + # Keyed by pk so a caller passing the same row twice can't manufacture a false tie below. by_name = defaultdict(dict) for module_bay_type in module_bay_types: - # Keyed by pk within each name group so a caller passing the same row twice (the - # three current callers never do, since each resolves from a queryset, but the - # signature accepts any iterable) can't manufacture a same-manufacturer "tie" below. by_name[module_bay_type.name][module_bay_type.pk] = module_bay_type resolved = [] diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index 65b17dcd6..ac123a185 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1448,11 +1448,7 @@ class DeviceTypeListView(generic.ObjectListView): table = tables.DeviceTypeTable def export_yaml(self): - # to_yaml() walks each device type's module bay templates and, for each, its - # module_bay_types -- prefetch both so this one relation doesn't add a query per - # module bay template across the whole queryset. to_yaml()'s other component-template - # relations (interfaces, ports, etc.) are unprefetched here as they were before this - # relation existed, and remain their own N+1 across a large export. + # 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() @@ -1912,12 +1908,9 @@ class ModuleTypeListView(generic.ObjectListView): table = tables.ModuleTypeTable def export_yaml(self): - # to_yaml() reads module_bay_types directly -- unlike DeviceType.to_yaml(), it does - # not export a nested module-bays section at all (a separate, pre-existing gap, out - # of scope here), so there is no modulebaytemplates relation to prefetch alongside - # it. to_yaml()'s other component-template relations (interfaces, ports, etc.) are - # unprefetched here as they were before this relation existed, and remain their own - # N+1 across a large export. + # 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() diff --git a/netbox/utilities/forms/fields/csv.py b/netbox/utilities/forms/fields/csv.py index 0316068af..03f384417 100644 --- a/netbox/utilities/forms/fields/csv.py +++ b/netbox/utilities/forms/fields/csv.py @@ -100,12 +100,8 @@ class CSVModelMultipleChoiceField(forms.ModelMultipleChoiceField): def clean(self, value): if not isinstance(value, list): - # str(value): a caller may bind this field from parsed YAML/JSON rather than a - # CSV cell, where a scalar column can arrive as a non-string (e.g. an int or - # bool) -- .split() would raise AttributeError otherwise. .strip() each piece: - # a table's default ManyToManyColumn export separator is ", " (comma space), so - # re-importing NetBox's own CSV export of a multi-value column would otherwise - # fail to match on the leading space. + # str(): a non-CSV caller (e.g. YAML) may pass a non-string scalar. strip(): a + # table's default ManyToManyColumn export separator is ", ", not ",". value = [v.strip() for v in str(value).split(',')] if value else [] return super().clean(value) From e5c0d60d09077ec7a892298add9ed6190da5f1f1 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 16:08:31 -0400 Subject: [PATCH 09/14] Fix UI regression: transform= changed the rendered column, not just CSV export django-tables2's ManyToManyColumn.render() and NetBox's own value() override both call self.transform() for each item -- there's no built-in way to give CSV export a different representation than the rendered column. Setting transform=lambda obj: obj.name on the three module_bay_types columns to fix CSV export therefore also dropped the manufacturer prefix from the Bay Types column in the Module Bays, Module Bay Templates, and Module Types list views -- the opposite of what ModuleBayType.__str__() adds that prefix for. Verified directly: with the old transform=, two same-named bay types from different manufacturers render as visually identical "SFP28" list items. Add export_transform to NetBox's ManyToManyColumn subclass, defaulting to transform so existing columns are unaffected, and used only by value() (export) rather than render() (UI). Switch the three columns to export_transform=lambda obj: obj.name, leaving transform unset so render() keeps str()'s manufacturer prefix. Extended the existing round-trip test to also assert the rendered column still includes the manufacturer name; confirmed it fails against the old transform= approach and passes with export_transform=. --- netbox/dcim/tables/devices.py | 4 ++-- netbox/dcim/tables/devicetypes.py | 4 ++-- netbox/dcim/tables/modules.py | 4 ++-- netbox/dcim/tests/test_forms.py | 8 +++++--- netbox/netbox/tables/columns.py | 10 +++++++++- 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/netbox/dcim/tables/devices.py b/netbox/dcim/tables/devices.py index 6c5ad9249..2fcda5cd1 100644 --- a/netbox/dcim/tables/devices.py +++ b/netbox/dcim/tables/devices.py @@ -1028,8 +1028,8 @@ class ModuleBayTable(ModularDeviceComponentTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # __str__() includes the manufacturer, but import resolves by name alone. - transform=lambda obj: obj.name, + # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. + export_transform=lambda obj: obj.name, ) class Meta(ModularDeviceComponentTable.Meta): diff --git a/netbox/dcim/tables/devicetypes.py b/netbox/dcim/tables/devicetypes.py index 427ca0ee4..20bbb38c7 100644 --- a/netbox/dcim/tables/devicetypes.py +++ b/netbox/dcim/tables/devicetypes.py @@ -305,8 +305,8 @@ class ModuleBayTemplateTable(ComponentTemplateTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # __str__() includes the manufacturer, but import resolves by name alone. - transform=lambda obj: obj.name, + # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. + export_transform=lambda obj: obj.name, ) actions = columns.ActionsColumn( actions=('edit', 'delete') diff --git a/netbox/dcim/tables/modules.py b/netbox/dcim/tables/modules.py index 2586b1155..e3706cac3 100644 --- a/netbox/dcim/tables/modules.py +++ b/netbox/dcim/tables/modules.py @@ -78,8 +78,8 @@ class ModuleTypeTable(PrimaryModelTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # __str__() includes the manufacturer, but import resolves by name alone. - transform=lambda obj: obj.name, + # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. + export_transform=lambda obj: obj.name, ) model = tables.Column( linkify=True, diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 559b099d3..dacf7024b 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -516,16 +516,18 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertFalse(form.save().module_bay_types.exists()) def test_module_bay_types_round_trips_through_the_table_column_export_value(self): - """The table's CSV export (multi-value separator, name-only transform) must be re-importable.""" + """The table's CSV export (multi-value separator, name-only transform) must be re-importable, + without changing the rendered UI column, which should keep str()'s manufacturer prefix.""" manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28', manufacturer=manufacturer) bay_type_b = ModuleBayType.objects.create(name='QSFP28', slug='qsfp28', manufacturer=manufacturer) original = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1') original.module_bay_types.set([bay_type_a, bay_type_b]) - table = ModuleTypeTable([original]) - exported_value = table.columns['module_bay_types'].column.value(original.module_bay_types.all()) + column = ModuleTypeTable([original]).columns['module_bay_types'].column + exported_value = column.value(original.module_bay_types.all()) self.assertEqual(exported_value, 'QSFP28, SFP28') + self.assertIn(str(manufacturer), str(column.render(original.module_bay_types.all()))) form = ModuleTypeImportForm({ 'manufacturer': manufacturer.name, diff --git a/netbox/netbox/tables/columns.py b/netbox/netbox/tables/columns.py index 314bb7128..5ab71f8d3 100644 --- a/netbox/netbox/tables/columns.py +++ b/netbox/netbox/tables/columns.py @@ -131,9 +131,17 @@ class DurationColumn(tables.Column): class ManyToManyColumn(tables.ManyToManyColumn): """ Overrides django-tables2's stock ManyToManyColumn to ensure that value() returns only plaintext data. + + export_transform: optional callable used only for value() (CSV/table export), letting export use a + different representation than the rendered column (e.g. a bare name where the UI shows str(obj)). + Defaults to transform, matching the stock behavior of exporting the same text that's rendered. """ + def __init__(self, *args, export_transform=None, **kwargs): + super().__init__(*args, **kwargs) + self.export_transform = export_transform or self.transform + def value(self, value): - items = [self.transform(item) for item in self.filter(value)] + items = [self.export_transform(item) for item in self.filter(value)] return self.separator.join(items) From f65c72da9ce63c1f298d7133caecac2f30e6399c Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 14 Aug 2026 11:00:25 -0400 Subject: [PATCH 10/14] Address review: drop module_bay_types CSV import and cross-manufacturer resolution Per review, ditch dedupe_module_bay_types_by_manufacturer() and any import logic that resolves module_bay_types by name alone across manufacturers. ModuleBayType's unique constraint is (manufacturer, name), not name alone, so resolving a bare name against an unscoped, potentially cross-manufacturer pool -- guessing via a preference order, rejecting only on a genuine tie -- is not a reliable way to identify a specific object. CSV import forms have no way to qualify an M2M reference beyond a bare name, so module_bay_types is no longer exposed there at all (ModuleTypeImportForm, ModuleBayImportForm in bulk_import.py): it's acceptable not to support this rather than resolve it unreliably. This also reverts the netbox/tables/columns.py export_transform API addition and the three tables' use of it, which existed only to make the CSV round trip work. The one import path that survives is ModuleBayTemplateImportForm (the YAML device/module type "Import Components" flow), because it can reliably scope module_bay_types' queryset to the parent device/module type's own manufacturer plus global (manufacturer-less) types *before* resolving by name -- so a name collision is never cross-manufacturer, only "this manufacturer's own type vs. a global one of the same name," which ModuleBayType's own uniqueness constraint makes unambiguous. A name matching only some other manufacturer's type doesn't resolve at all, rather than being coerced to an arbitrary guess. Kept: the ModuleBayTemplateImportForm.enabled field/clean_enabled() fix (default=True was previously lost on YAML re-import; unrelated to the above), and to_yaml()'s export of module_bay_types on both ModuleType and ModuleBayTemplate, plus the export_yaml() prefetch optimizations -- none of these involve resolving an object's identity from an ambiguous attribute. Trimmed the model docs to match: the modulebay.md and moduletype.md paragraphs described capabilities (CSV import, cross-manufacturer YAML import) that no longer exist and are removed; modulebaytemplate.md's note is rewritten to describe the actual (manufacturer-or-global-scoped) resolution behavior. --- docs/models/dcim/modulebay.md | 2 - docs/models/dcim/modulebaytemplate.md | 2 +- docs/models/dcim/moduletype.md | 2 - netbox/dcim/forms/bulk_import.py | 38 +--- netbox/dcim/forms/object_import.py | 56 ++++-- netbox/dcim/tables/devices.py | 2 - netbox/dcim/tables/devicetypes.py | 2 - netbox/dcim/tables/modules.py | 2 - netbox/dcim/tests/test_forms.py | 241 +------------------------- netbox/dcim/utils.py | 45 ----- netbox/netbox/tables/columns.py | 10 +- netbox/utilities/forms/fields/csv.py | 4 +- 12 files changed, 52 insertions(+), 354 deletions(-) diff --git a/docs/models/dcim/modulebay.md b/docs/models/dcim/modulebay.md index 103a95632..4df243ca1 100644 --- a/docs/models/dcim/modulebay.md +++ b/docs/models/dcim/modulebay.md @@ -34,8 +34,6 @@ The numeric position in which this module bay is situated. For example, this wou Zero or more [module bay types](./modulebaytype.md) assigned to this bay. When at least one bay type is set, only module types that share a common bay type may be installed. Leave empty to allow any module type. -Bay types are importable via CSV, referenced by name. A bay type belonging to a manufacturer other than the module bay's own device may be referenced; this mirrors the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the device's own manufacturer, then a global (manufacturer-less) bay type. If a name instead matches two or more bay types belonging to *other* manufacturers, with neither the device's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. - ### Enabled Whether this module bay is enabled. Disabled module bays are not available for installation. diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index 63bdd7050..3b4d2b14b 100644 --- a/docs/models/dcim/modulebaytemplate.md +++ b/docs/models/dcim/modulebaytemplate.md @@ -4,4 +4,4 @@ A template for a module bay that will be created on all instantiations of the pa [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 and exportable as part of a device type's YAML definition (`module-bays[].module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the device type's own may be referenced; this mirrors the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the device type's own manufacturer, then a global (manufacturer-less) bay type. If a name instead matches two or more bay types belonging to *other* manufacturers, with neither the device type's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. +Bay types are importable and exportable as part of a device type's YAML definition (`module-bays[].module_bay_types`), referenced by name. A referenced name is resolved against bay types belonging to the device 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. diff --git a/docs/models/dcim/moduletype.md b/docs/models/dcim/moduletype.md index 161e98790..993c5cbfe 100644 --- a/docs/models/dcim/moduletype.md +++ b/docs/models/dcim/moduletype.md @@ -91,8 +91,6 @@ 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 importable and exportable as part of a module type's YAML definition (`module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the module type's own may be referenced -- e.g. a third-party module declaring compatibility with another manufacturer's proprietary bay type -- mirroring the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the module type's own manufacturer, then a global (manufacturer-less) bay type. If a name instead matches two or more bay types belonging to *other* manufacturers, with neither the module type's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. - ### 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/bulk_import.py b/netbox/dcim/forms/bulk_import.py index 901e057be..469257ae6 100644 --- a/netbox/dcim/forms/bulk_import.py +++ b/netbox/dcim/forms/bulk_import.py @@ -2,7 +2,6 @@ from django import forms from django.contrib.contenttypes.models import ContentType from django.contrib.postgres.forms.array import SimpleArrayField from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist -from django.db.models import Q from django.utils.functional import lazy from django.utils.html import format_html from django.utils.safestring import SafeString, mark_safe @@ -11,7 +10,7 @@ from django.utils.translation import gettext_lazy as _ from dcim.choices import * from dcim.constants import * from dcim.models import * -from dcim.utils import dedupe_module_bay_types_by_manufacturer, reconcile_port_mappings +from dcim.utils import reconcile_port_mappings from extras.models import ConfigTemplate from ipam.choices import VLANQinQRoleChoices from ipam.models import VLAN, VRF, IPAddress, VLANGroup @@ -551,19 +550,12 @@ class ModuleTypeImportForm(PrimaryModelImportForm): required=False, help_text=_('Attribute values for the assigned profile, passed as a dictionary') ) - module_bay_types = CSVModelMultipleChoiceField( - label=_('Module bay types'), - queryset=ModuleBayType.objects.all(), - to_field_name='name', - required=False, - help_text=_('Types of module bays this module type can be installed in (empty = unconstrained)'), - ) class Meta: model = ModuleType fields = [ 'manufacturer', 'model', 'part_number', 'description', 'cooling_method', 'airflow', 'weight', 'weight_unit', - 'end_of_life', 'profile', 'attribute_data', 'owner', 'comments', 'tags', 'module_bay_types', + 'end_of_life', 'profile', 'attribute_data', 'owner', 'comments', 'tags', ] def clean(self): @@ -577,11 +569,6 @@ class ModuleTypeImportForm(PrimaryModelImportForm): if self.cleaned_data.get('profile') and not self.cleaned_data.get('attribute_data'): self.cleaned_data['attribute_data'] = {} - if module_bay_types := self.cleaned_data.get('module_bay_types'): - self.cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( - module_bay_types, self.cleaned_data.get('manufacturer'), - ) - class DeviceRoleImportForm(NestedGroupModelImportForm): parent = CSVModelChoiceField( @@ -1439,19 +1426,10 @@ class ModuleBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): queryset=Device.objects.all(), to_field_name='name' ) - module_bay_types = CSVModelMultipleChoiceField( - label=_('Module bay types'), - queryset=ModuleBayType.objects.all(), - to_field_name='name', - required=False, - help_text=_('Types of module bays this bay accepts (empty = unconstrained)'), - ) class Meta: model = ModuleBay - fields = ( - 'device', 'name', 'label', 'position', 'enabled', 'description', 'owner', 'tags', 'module_bay_types', - ) + fields = ('device', 'name', 'label', 'position', 'enabled', 'description', 'owner', 'tags') def clean_enabled(self): # Make sure enabled is True when it's not included in the uploaded data @@ -1459,16 +1437,6 @@ class ModuleBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): return True return self.cleaned_data['enabled'] - def clean(self): - super().clean() - - if module_bay_types := self.cleaned_data.get('module_bay_types'): - device = self.cleaned_data.get('device') - manufacturer = device.device_type.manufacturer if device else None - self.cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( - module_bay_types, manufacturer, - ) - class DeviceBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): device = CSVModelChoiceField( diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index b6da78dc6..b2899baa2 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -1,10 +1,9 @@ from django import forms +from django.db.models import Q from django.utils.translation import gettext_lazy as _ from dcim.choices import InterfacePoEModeChoices, InterfacePoETypeChoices, InterfaceTypeChoices, PortTypeChoices from dcim.models import * -from dcim.utils import dedupe_module_bay_types_by_manufacturer -from utilities.forms.fields import CSVModelMultipleChoiceField from wireless.choices import WirelessRoleChoices __all__ = ( @@ -215,10 +214,7 @@ class PortTemplateMappingImportForm(forms.ModelForm): class ModuleBayTemplateImportForm(forms.ModelForm): - # CSVModelMultipleChoiceField, not the plain ModelMultipleChoiceField used elsewhere in - # this file, so a scalar name is accepted alongside a list -- matches ModuleTypeImportForm's - # equivalent field, which also serves plain CSV import. - module_bay_types = CSVModelMultipleChoiceField( + module_bay_types = forms.ModelMultipleChoiceField( label=_('Module bay types'), queryset=ModuleBayType.objects.all(), to_field_name='name', @@ -227,6 +223,8 @@ class ModuleBayTemplateImportForm(forms.ModelForm): class Meta: model = ModuleBayTemplate + # module_bay_types must stay last: clean_device_type/clean_module_type narrow its queryset by + # manufacturer before it is itself cleaned, and Django cleans fields in this order. fields = [ 'device_type', 'module_type', 'name', 'label', 'position', 'enabled', 'description', 'module_bay_types', @@ -239,20 +237,42 @@ class ModuleBayTemplateImportForm(forms.ModelForm): return True return self.cleaned_data['enabled'] - def clean(self): - cleaned_data = super().clean() + def _scope_module_bay_types(self, manufacturer): + module_bay_types = self.fields['module_bay_types'] + module_bay_types.queryset = module_bay_types.queryset.filter( + Q(manufacturer__isnull=True) | Q(manufacturer=manufacturer) + ) - if module_bay_types := cleaned_data.get('module_bay_types'): - device_type = cleaned_data.get('device_type') - module_type = cleaned_data.get('module_type') - manufacturer = device_type.manufacturer if device_type else ( - module_type.manufacturer if module_type else None - ) - cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( - module_bay_types, manufacturer, - ) + def clean_device_type(self): + if device_type := self.cleaned_data['device_type']: + self._scope_module_bay_types(device_type.manufacturer) - return cleaned_data + return device_type + + def clean_module_type(self): + if module_type := self.cleaned_data['module_type']: + self._scope_module_bay_types(module_type.manufacturer) + + return module_type + + def clean_module_bay_types(self): + """ + Collapse to one match per name, 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 collide between a global type and one scoped to this template's + own manufacturer (narrowed by clean_device_type/clean_module_type above); the field's + default name-based lookup resolves both matches into cleaned_data rather than picking + one, since it has no way to know which is meant. + """ + module_bay_types = self.cleaned_data['module_bay_types'] + + by_name = {} + for module_bay_type in module_bay_types: + 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 + + return list(by_name.values()) class DeviceBayTemplateImportForm(forms.ModelForm): diff --git a/netbox/dcim/tables/devices.py b/netbox/dcim/tables/devices.py index 2fcda5cd1..ac10e8bd2 100644 --- a/netbox/dcim/tables/devices.py +++ b/netbox/dcim/tables/devices.py @@ -1028,8 +1028,6 @@ class ModuleBayTable(ModularDeviceComponentTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. - export_transform=lambda obj: obj.name, ) class Meta(ModularDeviceComponentTable.Meta): diff --git a/netbox/dcim/tables/devicetypes.py b/netbox/dcim/tables/devicetypes.py index 20bbb38c7..3e4682d0d 100644 --- a/netbox/dcim/tables/devicetypes.py +++ b/netbox/dcim/tables/devicetypes.py @@ -305,8 +305,6 @@ class ModuleBayTemplateTable(ComponentTemplateTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. - export_transform=lambda obj: obj.name, ) actions = columns.ActionsColumn( actions=('edit', 'delete') diff --git a/netbox/dcim/tables/modules.py b/netbox/dcim/tables/modules.py index e3706cac3..a8aebf873 100644 --- a/netbox/dcim/tables/modules.py +++ b/netbox/dcim/tables/modules.py @@ -78,8 +78,6 @@ class ModuleTypeTable(PrimaryModelTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. - export_transform=lambda obj: obj.name, ) model = tables.Column( linkify=True, diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index dacf7024b..6b1246153 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -1,6 +1,5 @@ from unittest.mock import patch -import yaml from django import forms from django.test import TestCase @@ -17,7 +16,6 @@ from dcim.choices import ( ) from dcim.forms import * from dcim.models import * -from dcim.tables.modules import ModuleTypeTable from dcim.tests.test_module_moves import fail_after from ipam.models import ASN, RIR, VLAN from utilities.exceptions import AbortRequest @@ -350,252 +348,29 @@ class ModuleBayTemplateImportFormTestCase(TestCase): set(original.module_bay_types.values_list('name', flat=True)), ) - def test_module_bay_types_permits_a_different_manufacturers_type(self): - """The UI/API place no manufacturer restriction on module_bay_types; import must match.""" + 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') - cisco_bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=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.assertTrue(form.is_valid(), form.errors) - - module_bay_template = form.save() - self.assertEqual(list(module_bay_template.module_bay_types.all()), [cisco_bay_type]) - - exported = module_bay_template.to_yaml()['module_bay_types'] - reimport_form = ModuleBayTemplateImportForm({ - 'device_type': device_type.pk, - 'name': 'Module Bay 2', - 'module_bay_types': exported, - }) - self.assertTrue(reimport_form.is_valid(), reimport_form.errors) - self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) - - def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): - juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') - cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') - arista = Manufacturer.objects.create(name='Arista', slug='arista') - ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) - ModuleBayType.objects.create(name='SFP28', slug='sfp28-arista', manufacturer=arista) - device_type = DeviceType.objects.create( - manufacturer=juniper, model='Juniper Device Type 2', slug='juniper-device-type-2', - ) - form = ModuleBayTemplateImportForm({ 'device_type': device_type.pk, 'name': 'Module Bay 1', 'module_bay_types': ['SFP28'], }) self.assertFalse(form.is_valid()) - # Must name both manufacturers, not just error -- a plain invalid_choice would also - # pass assertIn() and mask a regression of the scoping removed in 6f3c537. - errors = form.errors.get('module_bay_types', []) - self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) - - -class ModuleBayImportFormTestCase(TestCase): - """Covers real ModuleBay instances via CSV, as opposed to templates via ModuleBayTemplateImportForm.""" - - def test_module_bay_types_csv_import(self): - device = create_test_device('Module Bay Import Device') - bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28') - - form = ModuleBayImportForm({ - 'device': device.name, - 'name': 'Bay 1', - 'module_bay_types': 'SFP28', - }) - self.assertTrue(form.is_valid(), form.errors) - module_bay = form.save() - self.assertEqual(list(module_bay.module_bay_types.all()), [bay_type]) - - def test_module_bay_types_prefers_devices_own_manufacturer(self): - device = create_test_device('Module Bay Import Device') - own_manufacturer = device.device_type.manufacturer - other_manufacturer = Manufacturer.objects.create(name='Other Mfr', slug='other-mfr') - own_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-own', manufacturer=own_manufacturer) - ModuleBayType.objects.create(name='SFP28', slug='sfp28-other', manufacturer=other_manufacturer) - - form = ModuleBayImportForm({ - 'device': device.name, - 'name': 'Bay 1', - 'module_bay_types': 'SFP28', - }) - self.assertTrue(form.is_valid(), form.errors) - self.assertEqual(list(form.save().module_bay_types.all()), [own_type]) - - def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): - device = create_test_device('Module Bay Import Device') - cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') - arista = Manufacturer.objects.create(name='Arista', slug='arista') - ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) - ModuleBayType.objects.create(name='SFP28', slug='sfp28-arista', manufacturer=arista) - - form = ModuleBayImportForm({ - 'device': device.name, - 'name': 'Bay 1', - 'module_bay_types': 'SFP28', - }) - self.assertFalse(form.is_valid()) - # Must name both manufacturers, not just error -- a plain invalid_choice would also - # pass assertIn() and mask a regression of the scoping removed in 6f3c537. - errors = form.errors.get('module_bay_types', []) - self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) - - -class ModuleTypeImportFormTestCase(TestCase): - - def test_module_bay_types_round_trip(self): - manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') - bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28', manufacturer=manufacturer) - - form = ModuleTypeImportForm({ - 'manufacturer': manufacturer.name, - 'model': 'Module Type 1', - 'module_bay_types': ['SFP28'], - }) - self.assertTrue(form.is_valid(), form.errors) - - module_type = form.save() - self.assertEqual(list(module_type.module_bay_types.all()), [bay_type]) - self.assertEqual(yaml.safe_load(module_type.to_yaml())['module_bay_types'], ['SFP28']) - - def test_module_bay_types_prefers_manufacturer_specific_match_over_global(self): - 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, - ) - - form = ModuleTypeImportForm({ - 'manufacturer': manufacturer.name, - 'model': 'Module Type 1', - 'module_bay_types': ['SFP28'], - }) - self.assertTrue(form.is_valid(), form.errors) - - module_type = form.save() - self.assertEqual(list(module_type.module_bay_types.all()), [scoped_type]) - self.assertNotIn(global_type, module_type.module_bay_types.all()) - - def test_module_bay_types_accepts_csv_comma_separated_string(self): - """This form also serves plain CSV import, where the value is a string, not a list.""" - 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') - - form = ModuleTypeImportForm({ - 'manufacturer': manufacturer.name, - 'model': 'Module Type 1', - 'module_bay_types': 'SFP28,QSFP28', - }) - self.assertTrue(form.is_valid(), form.errors) - - module_type = form.save() self.assertEqual( - set(module_type.module_bay_types.values_list('name', flat=True)), - {bay_type_a.name, bay_type_b.name}, + form.errors.as_data()['module_bay_types'][0].code, 'invalid_choice', ) - def test_module_bay_types_accepts_empty_csv_string(self): - manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') - - form = ModuleTypeImportForm({ - 'manufacturer': manufacturer.name, - 'model': 'Module Type 1', - 'module_bay_types': '', - }) - self.assertTrue(form.is_valid(), form.errors) - self.assertFalse(form.save().module_bay_types.exists()) - - def test_module_bay_types_round_trips_through_the_table_column_export_value(self): - """The table's CSV export (multi-value separator, name-only transform) must be re-importable, - without changing the rendered UI column, which should keep str()'s manufacturer prefix.""" - manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') - bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28', manufacturer=manufacturer) - bay_type_b = ModuleBayType.objects.create(name='QSFP28', slug='qsfp28', manufacturer=manufacturer) - original = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1') - original.module_bay_types.set([bay_type_a, bay_type_b]) - - column = ModuleTypeTable([original]).columns['module_bay_types'].column - exported_value = column.value(original.module_bay_types.all()) - self.assertEqual(exported_value, 'QSFP28, SFP28') - self.assertIn(str(manufacturer), str(column.render(original.module_bay_types.all()))) - - form = ModuleTypeImportForm({ - 'manufacturer': manufacturer.name, - 'model': 'Module Type 2', - 'module_bay_types': exported_value, - }) - self.assertTrue(form.is_valid(), form.errors) - self.assertEqual( - set(form.save().module_bay_types.values_list('name', flat=True)), - {bay_type_a.name, bay_type_b.name}, - ) - - def test_module_bay_types_non_string_scalar_is_a_validation_error_not_a_crash(self): - """A non-string scalar (e.g. from YAML) must produce a form error, not a crash.""" - manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') - - form = ModuleTypeImportForm({ - 'manufacturer': manufacturer.name, - 'model': 'Module Type 1', - 'module_bay_types': 100, - }) - self.assertFalse(form.is_valid()) - self.assertIn('module_bay_types', form.errors) - - def test_module_bay_types_permits_a_different_manufacturers_type(self): - """The UI/API place no manufacturer restriction on module_bay_types; import must match.""" - juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') - cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') - cisco_bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) - - form = ModuleTypeImportForm({ - 'manufacturer': juniper.name, - 'model': 'Juniper Line Card', - 'module_bay_types': ['SFP28'], - }) - self.assertTrue(form.is_valid(), form.errors) - - module_type = form.save() - self.assertEqual(list(module_type.module_bay_types.all()), [cisco_bay_type]) - - exported = yaml.safe_load(module_type.to_yaml())['module_bay_types'] - reimport_form = ModuleTypeImportForm({ - 'manufacturer': juniper.name, - 'model': 'Juniper Line Card 2', - 'module_bay_types': exported, - }) - self.assertTrue(reimport_form.is_valid(), reimport_form.errors) - self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) - - def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): - """A name matching two different foreign manufacturers must be refused, not resolved arbitrarily.""" - juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') - cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') - arista = Manufacturer.objects.create(name='Arista', slug='arista') - ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) - ModuleBayType.objects.create(name='SFP28', slug='sfp28-arista', manufacturer=arista) - - form = ModuleTypeImportForm({ - 'manufacturer': juniper.name, - 'model': 'Juniper Line Card', - 'module_bay_types': ['SFP28'], - }) - self.assertFalse(form.is_valid()) - # Must name both manufacturers, not just error -- a plain invalid_choice would also - # pass assertIn() and mask a regression of the scoping removed in 6f3c537. - errors = form.errors.get('module_bay_types', []) - self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) - class ModuleFormTestCase(TestCase): diff --git a/netbox/dcim/utils.py b/netbox/dcim/utils.py index 313b67d3e..097774d7a 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -2,57 +2,12 @@ from collections import defaultdict from django.apps import apps from django.contrib.contenttypes.models import ContentType -from django.core.exceptions import ValidationError from django.db import router, transaction from django.utils.translation import gettext as _ from dcim.constants import MODULE_TOKEN -def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None): - """ - Collapse ModuleBayType instances resolved by name to one per name, preferring an exact - match on *manufacturer*, then a global (manufacturer-less) type. Names aren't globally - unique -- uniqueness is scoped to (manufacturer, name) -- and callers must not scope the - queryset by manufacturer themselves, since a type may legitimately belong to another - manufacturer entirely; only this preference order is manufacturer-aware. - - Raises ValidationError if a name ties across two or more non-preferred manufacturers, - rather than picking one arbitrarily. - """ - manufacturer_id = manufacturer.pk if manufacturer else None - - def preference(module_bay_type): - if module_bay_type.manufacturer_id == manufacturer_id: - return 0 - if module_bay_type.manufacturer_id is None: - return 1 - return 2 - - # Keyed by pk so a caller passing the same row twice can't manufacture a false tie below. - by_name = defaultdict(dict) - for module_bay_type in module_bay_types: - by_name[module_bay_type.name][module_bay_type.pk] = module_bay_type - - resolved = [] - for name, candidates_by_pk in by_name.items(): - candidates = list(candidates_by_pk.values()) - best_rank = min(preference(c) for c in candidates) - best = [c for c in candidates if preference(c) == best_rank] - if len(best) > 1: - manufacturers = ', '.join(sorted(c.manufacturer.name for c in best)) - raise ValidationError({ - 'module_bay_types': _( - "Module bay type \"{name}\" is ambiguous: it belongs to more than one " - "manufacturer ({manufacturers}), none of which is this type's own " - "manufacturer." - ).format(name=name, manufacturers=manufacturers) - }) - resolved.append(best[0]) - - return resolved - - def inherit_module_token(position, parent_positions): """ Resolve a single {module} token in a bay position by inheriting from the position diff --git a/netbox/netbox/tables/columns.py b/netbox/netbox/tables/columns.py index 5ab71f8d3..314bb7128 100644 --- a/netbox/netbox/tables/columns.py +++ b/netbox/netbox/tables/columns.py @@ -131,17 +131,9 @@ class DurationColumn(tables.Column): class ManyToManyColumn(tables.ManyToManyColumn): """ Overrides django-tables2's stock ManyToManyColumn to ensure that value() returns only plaintext data. - - export_transform: optional callable used only for value() (CSV/table export), letting export use a - different representation than the rendered column (e.g. a bare name where the UI shows str(obj)). - Defaults to transform, matching the stock behavior of exporting the same text that's rendered. """ - def __init__(self, *args, export_transform=None, **kwargs): - super().__init__(*args, **kwargs) - self.export_transform = export_transform or self.transform - def value(self, value): - items = [self.export_transform(item) for item in self.filter(value)] + items = [self.transform(item) for item in self.filter(value)] return self.separator.join(items) diff --git a/netbox/utilities/forms/fields/csv.py b/netbox/utilities/forms/fields/csv.py index 03f384417..497a1816e 100644 --- a/netbox/utilities/forms/fields/csv.py +++ b/netbox/utilities/forms/fields/csv.py @@ -100,9 +100,7 @@ class CSVModelMultipleChoiceField(forms.ModelMultipleChoiceField): def clean(self, value): if not isinstance(value, list): - # str(): a non-CSV caller (e.g. YAML) may pass a non-string scalar. strip(): a - # table's default ManyToManyColumn export separator is ", ", not ",". - value = [v.strip() for v in str(value).split(',')] if value else [] + value = value.split(',') if value else [] return super().clean(value) From 6cfda2b49c6ae2650d35c9d5932d246475f86b8e Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 14 Aug 2026 11:39:19 -0400 Subject: [PATCH 11/14] Address automated review: documentation clarifications for module_bay_types - Document that ModuleType.to_yaml() exports module_bay_types by name but the field isn't currently importable back through it (no ModuleTypeImportForm field survived the CSV-import revert). - modulebaytemplate.md's note covered only the device-type-parented import path; ModuleBayTemplateImportForm is registered for both DeviceTypeImportView and ModuleTypeImportView, scoping to whichever parent type's manufacturer applies. Reworded to cover both, and added the "rejected rather than resolved" clause for a name matching only some other manufacturer's type. - Clarified clean_module_bay_types()'s docstring: the "never a cross-manufacturer collision" guarantee holds only because ModularComponentTemplateModel.clean() rejects a template with neither device_type nor module_type before this method's result would ever be saved. - Fixed a test docstring overstating symmetry between its two comparison arms. Co-Authored-By: Claude Sonnet 5 --- docs/models/dcim/modulebaytemplate.md | 2 +- docs/models/dcim/moduletype.md | 2 ++ netbox/dcim/forms/object_import.py | 6 ++++++ netbox/dcim/tests/test_views.py | 6 ++++-- 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index 3b4d2b14b..70c3c3c61 100644 --- a/docs/models/dcim/modulebaytemplate.md +++ b/docs/models/dcim/modulebaytemplate.md @@ -4,4 +4,4 @@ A template for a module bay that will be created on all instantiations of the pa [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 and exportable as part of a device type's YAML definition (`module-bays[].module_bay_types`), referenced by name. A referenced name is resolved against bay types belonging to the device 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. +Bay types are importable and exportable as part of a device type's or module type's YAML definition (`module-bays[].module_bay_types`), referenced by name. 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 b2899baa2..0cf73dad5 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -263,6 +263,12 @@ class ModuleBayTemplateImportForm(forms.ModelForm): own manufacturer (narrowed by clean_device_type/clean_module_type above); the field's default name-based lookup resolves both matches into cleaned_data rather than picking one, since it has no way to know which is meant. + + If neither device_type nor module_type resolved (so the queryset above was never + narrowed), a name could in principle collide across two unrelated manufacturers here + too. That's not reachable with valid data: ModularComponentTemplateModel.clean() + rejects a template with neither parent, so the form fails in _post_clean() before this + method's result would ever be saved. """ module_bay_types = self.cleaned_data['module_bay_types'] diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 81da915c8..e33b93048 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1793,8 +1793,10 @@ module-bays: 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 the same queryset with/without the prefetch, since row-count comparisons - would be swamped by other per-instance relations that legitimately scale with it.""" + """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') From 8bffd79360ffc7d453e99438106c7945a1e9decf Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 14 Aug 2026 14:17:04 -0400 Subject: [PATCH 12/14] Correct claim in documentation --- docs/models/dcim/modulebaytemplate.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index 70c3c3c61..1c8d6cd38 100644 --- a/docs/models/dcim/modulebaytemplate.md +++ b/docs/models/dcim/modulebaytemplate.md @@ -4,4 +4,4 @@ A template for a module bay that will be created on all instantiations of the pa [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 and exportable as part of a device type's or module type's YAML definition (`module-bays[].module_bay_types`), referenced by name. 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. +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. From e57ab760df54e36c451410432d58edfe528a8cd1 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 14 Aug 2026 14:27:49 -0400 Subject: [PATCH 13/14] Fix support for enable=false under DeviceBayTemplateImportForm --- netbox/dcim/forms/object_import.py | 9 +-------- netbox/dcim/tests/test_forms.py | 14 -------------- netbox/dcim/tests/test_views.py | 10 ++++++++++ 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index 0cf73dad5..ea16633e9 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -230,13 +230,6 @@ class ModuleBayTemplateImportForm(forms.ModelForm): 'module_bay_types', ] - def clean_enabled(self): - # A dict-bound BooleanField resolves a missing key to False, not the model's own - # default=True -- match ModuleBayImportForm's equivalent CSV-import behavior. - if 'enabled' not in self.data: - return True - return self.cleaned_data['enabled'] - def _scope_module_bay_types(self, manufacturer): module_bay_types = self.fields['module_bay_types'] module_bay_types.queryset = module_bay_types.queryset.filter( @@ -286,7 +279,7 @@ 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/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 6b1246153..b1711997b 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -294,20 +294,6 @@ class ModuleBayTemplateImportFormTestCase(TestCase): ) self.assertNotIn(global_type, module_bay_template.module_bay_types.all()) - def test_enabled_defaults_true_when_omitted(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', - }) - self.assertTrue(form.is_valid(), form.errors) - self.assertTrue(form.save().enabled) - def test_enabled_honors_explicit_false(self): device_type = DeviceType.objects.create( manufacturer=Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1'), diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index e33b93048..d0a9d9ef7 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1004,10 +1004,12 @@ module-bays: 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 @@ -1133,10 +1135,18 @@ inventory-items: 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() From 3031430523b4ec256dbf03b39b4b92c6b9931a59 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 14 Aug 2026 14:40:18 -0400 Subject: [PATCH 14/14] Consolidate various helper methods on ModuleBayTemplateImportForm into clean() --- netbox/dcim/forms/object_import.py | 71 +++++++++++++++--------------- netbox/dcim/tests/test_forms.py | 45 +++++++++++++++++++ 2 files changed, 81 insertions(+), 35 deletions(-) diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index ea16633e9..75426f46a 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -1,5 +1,4 @@ from django import forms -from django.db.models import Q from django.utils.translation import gettext_lazy as _ from dcim.choices import InterfacePoEModeChoices, InterfacePoETypeChoices, InterfaceTypeChoices, PortTypeChoices @@ -223,55 +222,57 @@ class ModuleBayTemplateImportForm(forms.ModelForm): class Meta: model = ModuleBayTemplate - # module_bay_types must stay last: clean_device_type/clean_module_type narrow its queryset by - # manufacturer before it is itself cleaned, and Django cleans fields in this order. fields = [ 'device_type', 'module_type', 'name', 'label', 'position', 'enabled', 'description', 'module_bay_types', ] - def _scope_module_bay_types(self, manufacturer): - module_bay_types = self.fields['module_bay_types'] - module_bay_types.queryset = module_bay_types.queryset.filter( - Q(manufacturer__isnull=True) | Q(manufacturer=manufacturer) - ) - - def clean_device_type(self): - if device_type := self.cleaned_data['device_type']: - self._scope_module_bay_types(device_type.manufacturer) - - return device_type - - def clean_module_type(self): - if module_type := self.cleaned_data['module_type']: - self._scope_module_bay_types(module_type.manufacturer) - - return module_type - - def clean_module_bay_types(self): + def clean(self): """ - Collapse to one match per name, 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 collide between a global type and one scoped to this template's - own manufacturer (narrowed by clean_device_type/clean_module_type above); the field's - default name-based lookup resolves both matches into cleaned_data rather than picking - one, since it has no way to know which is meant. + 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. - If neither device_type nor module_type resolved (so the queryset above was never - narrowed), a name could in principle collide across two unrelated manufacturers here - too. That's not reachable with valid data: ModularComponentTemplateModel.clean() - rejects a template with neither parent, so the form fails in _post_clean() before this - method's result would ever be saved. + 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. """ - module_bay_types = self.cleaned_data['module_bay_types'] + 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 - return list(by_name.values()) + # 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): diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index b1711997b..bec25fa84 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -357,6 +357,51 @@ class ModuleBayTemplateImportFormTestCase(TestCase): 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):