Consolidate various helper methods on ModuleBayTemplateImportForm into clean()

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.
-
-        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.
+        Resolve each referenced bay type name against the parent type's own manufacturer, plus
+        bay types having no manufacturer (global), preferring a manufacturer-specific match
+        over a global one. ModuleBayType's unique constraint is on (manufacturer, name), not
+        name alone, so a name can legitimately match both, and the field's name-based lookup
+        resolves every match into cleaned_data rather than picking one.
+
+        This runs in clean() rather than in clean_module_bay_types() so that it does not depend
+        on the parent having been cleaned first, which would make it sensitive to the order of
+        Meta.fields.
         """
-        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):

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):