From 3031430523b4ec256dbf03b39b4b92c6b9931a59 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 14 Aug 2026 14:40:18 -0400 Subject: [PATCH] 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):