diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index b20ef06af..ad96b44e5 100644 --- a/docs/models/dcim/modulebaytemplate.md +++ b/docs/models/dcim/modulebaytemplate.md @@ -4,4 +4,4 @@ A template for a module bay that will be created on all instantiations of the pa [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. +Bay types are importable and exportable as part of a device type's YAML definition (`module-bays[].module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the device type's own may be referenced; this mirrors the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the device type's own manufacturer, then a global (manufacturer-less) bay type, then any remaining candidate. diff --git a/docs/models/dcim/moduletype.md b/docs/models/dcim/moduletype.md index 8084c902d..8b79c6c9f 100644 --- a/docs/models/dcim/moduletype.md +++ b/docs/models/dcim/moduletype.md @@ -91,7 +91,7 @@ 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. +Bay types are importable and exportable as part of a module type's YAML definition (`module_bay_types`), referenced by name. A bay type belonging to a manufacturer other than the module type's own may be referenced -- e.g. a third-party module declaring compatibility with another manufacturer's proprietary bay type -- mirroring the UI and REST API, which likewise place no manufacturer restriction on the assignment. Since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, more than one bay type may share a name; import prefers, in order, an exact match on the module type's own manufacturer, then a global (manufacturer-less) bay type, then any remaining candidate. ### Attributes diff --git a/netbox/dcim/forms/bulk_import.py b/netbox/dcim/forms/bulk_import.py index ba870d876..6b4135535 100644 --- a/netbox/dcim/forms/bulk_import.py +++ b/netbox/dcim/forms/bulk_import.py @@ -561,25 +561,11 @@ class ModuleTypeImportForm(PrimaryModelImportForm): 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', '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() @@ -591,6 +577,11 @@ class ModuleTypeImportForm(PrimaryModelImportForm): if self.cleaned_data.get('profile') and not self.cleaned_data.get('attribute_data'): self.cleaned_data['attribute_data'] = {} + if module_bay_types := self.cleaned_data.get('module_bay_types'): + self.cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( + module_bay_types, self.cleaned_data.get('manufacturer'), + ) + class DeviceRoleImportForm(NestedGroupModelImportForm): parent = CSVModelChoiceField( diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index dd7bd43e1..a048139fa 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 @@ -224,10 +223,6 @@ 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', 'enabled', 'description', 'module_bay_types', @@ -235,31 +230,28 @@ class ModuleBayTemplateImportForm(forms.ModelForm): 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. + # default=True -- match ModuleBayImportForm's equivalent CSV-import behavior. Reads + # self.data directly (no add_prefix()/QueryDict handling) because, like that form, + # this one is only ever bound to a plain dict of import data, never a real HTML + # checkbox POST. 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( - Q(manufacturer__isnull=True) | Q(manufacturer=manufacturer) - ) + def clean(self): + cleaned_data = super().clean() - def clean_device_type(self): - if device_type := self.cleaned_data['device_type']: - self._scope_module_bay_types(device_type.manufacturer) + if module_bay_types := cleaned_data.get('module_bay_types'): + device_type = cleaned_data.get('device_type') + module_type = cleaned_data.get('module_type') + manufacturer = device_type.manufacturer if device_type else ( + module_type.manufacturer if module_type else None + ) + cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( + module_bay_types, 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): - return dedupe_module_bay_types_by_manufacturer(self.cleaned_data['module_bay_types']) + return cleaned_data class DeviceBayTemplateImportForm(forms.ModelForm): diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 42d5185e9..fd02745c3 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -275,8 +275,7 @@ class ModuleBayTemplateImportFormTestCase(TestCase): }) self.assertFalse(form.is_valid()) self.assertEqual( - form.errors['module_bay_types'], - ['Select a valid choice. Nonexistent is not one of the available choices.'], + form.errors.as_data()['module_bay_types'][0].code, 'invalid_choice', ) def test_module_bay_types_prefers_manufacturer_specific_match_over_global_for_module_type(self): @@ -362,6 +361,40 @@ class ModuleBayTemplateImportFormTestCase(TestCase): set(original.module_bay_types.values_list('name', flat=True)), ) + def test_module_bay_types_permits_a_different_manufacturers_type(self): + """ + The UI (ModuleBayTemplateForm) and REST API place no manufacturer restriction on + module_bay_types -- a third-party device may legitimately declare a bay compatible + with another manufacturer's proprietary bay type. Import must permit the same, and a + type assigned this way must survive an export/re-import round trip rather than + becoming permanently unimportable. + """ + juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') + cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') + cisco_bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) + device_type = DeviceType.objects.create( + manufacturer=juniper, model='Juniper Device Type', slug='juniper-device-type', + ) + + 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()), [cisco_bay_type]) + + exported = module_bay_template.to_yaml()['module_bay_types'] + reimport_form = ModuleBayTemplateImportForm({ + 'device_type': device_type.pk, + 'name': 'Module Bay 2', + 'module_bay_types': exported, + }) + self.assertTrue(reimport_form.is_valid(), reimport_form.errors) + self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) + class ModuleTypeImportFormTestCase(TestCase): @@ -398,6 +431,37 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertEqual(list(module_type.module_bay_types.all()), [scoped_type]) self.assertNotIn(global_type, module_type.module_bay_types.all()) + def test_module_bay_types_permits_a_different_manufacturers_type(self): + """ + The UI (ModuleTypeForm) and REST API place no manufacturer restriction on + module_bay_types -- a third-party module may legitimately declare compatibility with + another manufacturer's proprietary bay type. Import must permit the same, and a type + created this way must survive an export/re-import round trip rather than becoming + permanently unimportable. + """ + juniper = Manufacturer.objects.create(name='Juniper', slug='juniper') + cisco = Manufacturer.objects.create(name='Cisco', slug='cisco') + cisco_bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) + + form = ModuleTypeImportForm({ + 'manufacturer': juniper.name, + 'model': 'Juniper Line Card', + 'module_bay_types': ['SFP28'], + }) + self.assertTrue(form.is_valid(), form.errors) + + module_type = form.save() + self.assertEqual(list(module_type.module_bay_types.all()), [cisco_bay_type]) + + exported = yaml.safe_load(module_type.to_yaml())['module_bay_types'] + reimport_form = ModuleTypeImportForm({ + 'manufacturer': juniper.name, + 'model': 'Juniper Line Card 2', + 'module_bay_types': exported, + }) + self.assertTrue(reimport_form.is_valid(), reimport_form.errors) + self.assertEqual(list(reimport_form.save().module_bay_types.all()), [cisco_bay_type]) + class ModuleFormTestCase(TestCase): diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index 03737009a..ae74a43a6 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -1,11 +1,9 @@ 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 @@ -154,32 +152,6 @@ 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): diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 60476dfb3..c01f41887 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -7,8 +7,10 @@ from zoneinfo import ZoneInfo import yaml from django.contrib.contenttypes.models import ContentType +from django.db import connection from django.http import StreamingHttpResponse from django.test import override_settings, tag +from django.test.utils import CaptureQueriesContext from django.urls import reverse from netaddr import EUI @@ -17,6 +19,7 @@ from core.models import ObjectChange, ObjectType from dcim.choices import * from dcim.constants import * from dcim.models import * +from dcim.views import DeviceTypeListView, ModuleTypeListView from extras.models import ConfigTemplate from ipam.models import ASN, RIR, VLAN, VRF from netbox.choices import ( @@ -1139,6 +1142,38 @@ inventory-items: ii1 = InventoryItemTemplate.objects.first() self.assertEqual(ii1.name, 'Inventory Item 1') + def test_bulk_yaml_export_module_bay_types_query_count_is_constant(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 as the number of bays grows. + """ + manufacturer = Manufacturer.objects.create(name='Export Query Manufacturer', slug='export-query-mfr') + bay_type = ModuleBayType.objects.create(name='Export Query SFP28', slug='export-query-sfp28') + + def make_device_type(model_name, bay_count): + device_type = DeviceType.objects.create( + manufacturer=manufacturer, model=model_name, slug=model_name.lower().replace(' ', '-'), + ) + for i in range(bay_count): + bay = ModuleBayTemplate.objects.create(device_type=device_type, name=f'Bay {i}') + bay.module_bay_types.set([bay_type]) + return device_type + + one_bay_device_type = make_device_type('Export Query DT One Bay', 1) + five_bay_device_type = make_device_type('Export Query DT Five Bays', 5) + + view = DeviceTypeListView() + view.queryset = DeviceType.objects.filter(pk=one_bay_device_type.pk) + with CaptureQueriesContext(connection) as one_bay_queries: + view.export_yaml() + + view.queryset = DeviceType.objects.filter(pk=five_bay_device_type.pk) + with CaptureQueriesContext(connection) as five_bay_queries: + view.export_yaml() + + self.assertEqual(len(one_bay_queries), len(five_bay_queries)) + def test_import_error_numbering(self): # Add all required permissions to the test user self.add_permissions( @@ -1761,6 +1796,37 @@ module-bays: self.assertEqual(mb1.position, '1') self.assertEqual(list(mb1.module_bay_types.values_list('name', flat=True)), ['SFP28']) + def test_bulk_yaml_export_module_bay_types_query_count_is_constant(self): + """ + ModuleTypeListView.export_yaml() prefetches both module_bay_types (on the module type + itself) and modulebaytemplates__module_bay_types (on its nested module bay templates), + so neither adds a query per module bay template as the number of bays grows. + """ + manufacturer = Manufacturer.objects.create(name='Export Query Manufacturer', slug='export-query-mfr') + bay_type = ModuleBayType.objects.create(name='Export Query SFP28', slug='export-query-sfp28') + + def make_module_type(model_name, bay_count): + module_type = ModuleType.objects.create(manufacturer=manufacturer, model=model_name) + module_type.module_bay_types.set([bay_type]) + for i in range(bay_count): + bay = ModuleBayTemplate.objects.create(module_type=module_type, name=f'Bay {i}') + bay.module_bay_types.set([bay_type]) + return module_type + + one_bay_module_type = make_module_type('Export Query MT One Bay', 1) + five_bay_module_type = make_module_type('Export Query MT Five Bays', 5) + + view = ModuleTypeListView() + view.queryset = ModuleType.objects.filter(pk=one_bay_module_type.pk) + with CaptureQueriesContext(connection) as one_bay_queries: + view.export_yaml() + + view.queryset = ModuleType.objects.filter(pk=five_bay_module_type.pk) + with CaptureQueriesContext(connection) as five_bay_queries: + view.export_yaml() + + self.assertEqual(len(one_bay_queries), len(five_bay_queries)) + @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 23bfc7618..ea2a102e4 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -8,19 +8,33 @@ from django.utils.translation import gettext as _ from dcim.constants import MODULE_TOKEN -def dedupe_module_bay_types_by_manufacturer(module_bay_types): +def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None): """ - 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. + 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. - 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. + 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 + legitimately share a name. This does not exclude any manufacturer's types: a module or + bay may legitimately declare compatibility with another manufacturer's proprietary bay + 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. """ + manufacturer_id = manufacturer.pk if manufacturer else None + + def preference(module_bay_type): + if module_bay_type.manufacturer_id == manufacturer_id: + return 0 + if module_bay_type.manufacturer_id is None: + return 1 + return 2 + 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): + if existing is None or preference(module_bay_type) < preference(existing): resolved[module_bay_type.name] = module_bay_type return list(resolved.values()) diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index 09d3e87d1..dff5af244 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1449,8 +1449,10 @@ class DeviceTypeListView(generic.ObjectListView): 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. + # module_bay_types -- prefetch both so this one relation doesn't add a query per + # module bay template across the whole queryset. to_yaml()'s other component-template + # relations (interfaces, ports, etc.) are unprefetched here as they were before this + # relation existed, and remain their own N+1 across a large export. self.queryset = self.queryset.prefetch_related('modulebaytemplates__module_bay_types') return super().export_yaml() @@ -1911,8 +1913,11 @@ class ModuleTypeListView(generic.ObjectListView): 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. + # own module_bay_types -- prefetch both so these relations don't add a query per + # module type/module bay template across the whole queryset. to_yaml()'s other + # component-template relations (interfaces, ports, etc.) are unprefetched here as + # they were before these relations existed, and remain their own N+1 across a large + # export. self.queryset = self.queryset.prefetch_related( 'module_bay_types', 'modulebaytemplates__module_bay_types', )