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