From 508e2eaba2f1b693d3783e1838eabc3ec83fafd9 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 08:21:47 -0400 Subject: [PATCH] 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