From 63045d8551aeec0636d3a757444fbd8d9558f8c2 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 09:31:34 -0400 Subject: [PATCH] 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):