diff --git a/docs/models/dcim/modulebay.md b/docs/models/dcim/modulebay.md index 4df243ca1..ffb3945ed 100644 --- a/docs/models/dcim/modulebay.md +++ b/docs/models/dcim/modulebay.md @@ -34,6 +34,8 @@ The numeric position in which this module bay is situated. For example, this wou Zero or more [module bay types](./modulebaytype.md) assigned to this bay. When at least one bay type is set, only module types that share a common bay type may be installed. Leave empty to allow any module type. +Bay types are importable via CSV, referenced by name, with the same manufacturer-based resolution described for [module bay templates](./modulebaytemplate.md) -- scoped to the manufacturer of the module bay's own device. + ### Enabled Whether this module bay is enabled. Disabled module bays are not available for installation. diff --git a/docs/models/dcim/modulebaytemplate.md b/docs/models/dcim/modulebaytemplate.md index ad96b44e5..63bdd7050 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 (`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. +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. If a name instead matches two or more bay types belonging to *other* manufacturers, with neither the device type's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. diff --git a/docs/models/dcim/moduletype.md b/docs/models/dcim/moduletype.md index 8b79c6c9f..161e98790 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 (`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. +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. If a name instead matches two or more bay types belonging to *other* manufacturers, with neither the module type's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. ### Attributes diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 1ece0d149..22a26719c 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -17,6 +17,7 @@ from dcim.choices import ( ) from dcim.forms import * from dcim.models import * +from dcim.tables.modules import ModuleTypeTable from dcim.tests.test_module_moves import fail_after from ipam.models import ASN, RIR, VLAN from utilities.exceptions import AbortRequest @@ -395,6 +396,24 @@ class ModuleBayTemplateImportFormTestCase(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): + 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) + device_type = DeviceType.objects.create( + manufacturer=juniper, model='Juniper Device Type 2', slug='juniper-device-type-2', + ) + + form = ModuleBayTemplateImportForm({ + 'device_type': device_type.pk, + 'name': 'Module Bay 1', + 'module_bay_types': ['SFP28'], + }) + self.assertFalse(form.is_valid()) + self.assertIn('module_bay_types', form.errors) + class ModuleBayImportFormTestCase(TestCase): """ @@ -431,6 +450,21 @@ class ModuleBayImportFormTestCase(TestCase): self.assertTrue(form.is_valid(), form.errors) self.assertEqual(list(form.save().module_bay_types.all()), [own_type]) + def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): + device = create_test_device('Module Bay Import Device') + 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 = ModuleBayImportForm({ + 'device': device.name, + 'name': 'Bay 1', + 'module_bay_types': 'SFP28', + }) + self.assertFalse(form.is_valid()) + self.assertIn('module_bay_types', form.errors) + class ModuleTypeImportFormTestCase(TestCase): @@ -469,10 +503,9 @@ class ModuleTypeImportFormTestCase(TestCase): def test_module_bay_types_accepts_csv_comma_separated_string(self): """ - Unlike ModuleBayTemplateImportForm.module_bay_types (a plain ModelMultipleChoiceField, - bound only from YAML-parsed lists), this form's module_bay_types is a - CSVModelMultipleChoiceField because ModuleTypeImportForm also serves plain CSV bulk - import, where the cell value arrives as a comma-separated string rather than a list. + module_bay_types is a CSVModelMultipleChoiceField because ModuleTypeImportForm also + serves plain CSV bulk import, where the cell value arrives as a comma-separated + string rather than a list. """ manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28') @@ -502,6 +535,51 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertTrue(form.is_valid(), form.errors) self.assertFalse(form.save().module_bay_types.exists()) + def test_module_bay_types_round_trips_through_the_table_column_export_value(self): + """ + ManyToManyColumn's export separator is ", " (comma + space, django-tables2's + default), not the bare "," this field's CSVModelMultipleChoiceField splits on -- so + re-importing NetBox's own CSV export of a multi-value module_bay_types column must + not fail on the leading space of every value after the first. + """ + 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') + original = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1') + original.module_bay_types.set([bay_type_a, bay_type_b]) + + table = ModuleTypeTable([original]) + exported_value = table.columns['module_bay_types'].column.value(original.module_bay_types.all()) + self.assertEqual(exported_value, 'QSFP28, SFP28') + + form = ModuleTypeImportForm({ + 'manufacturer': manufacturer.name, + 'model': 'Module Type 2', + 'module_bay_types': exported_value, + }) + self.assertTrue(form.is_valid(), form.errors) + self.assertEqual( + set(form.save().module_bay_types.values_list('name', flat=True)), + {bay_type_a.name, bay_type_b.name}, + ) + + def test_module_bay_types_non_string_scalar_is_a_validation_error_not_a_crash(self): + """ + A CSV cell is always a string, but this field is also bound from YAML-parsed data + (e.g. via ModuleBayTemplateImportForm), where a scalar column can arrive as a + non-string (an int or bool). .split() on that would raise AttributeError -- an + unhandled 500 rather than a form error. + """ + manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') + + form = ModuleTypeImportForm({ + 'manufacturer': manufacturer.name, + 'model': 'Module Type 1', + 'module_bay_types': 100, + }) + self.assertFalse(form.is_valid()) + self.assertIn('module_bay_types', form.errors) + def test_module_bay_types_permits_a_different_manufacturers_type(self): """ The UI (ModuleTypeForm) and REST API place no manufacturer restriction on diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 0d226a5a2..65fd5a2d9 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1796,48 +1796,18 @@ 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)) - 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. + Comparing module-type COUNTS (e.g. 1 vs. 5) to detect a per-row prefetch saving + doesn't work: 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. + + Unlike DeviceType.to_yaml(), ModuleType.to_yaml() does not export a nested + module-bays section at all (a separate, pre-existing gap, out of scope here), so + module_bay_types is the only relation ModuleTypeListView.export_yaml() needs to + prefetch -- confirmed by this test's exact-delta assertion below. """ 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') @@ -1858,12 +1828,8 @@ module-bays: 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)) + # query; with it, exactly one query serves all 5. + self.assertEqual(len(unprefetched) - len(prefetched), 4) @override_settings(STREAMING_EXPORTS=True) def test_export_objects(self): diff --git a/netbox/dcim/utils.py b/netbox/dcim/utils.py index f9efa07c8..bc1ec3e90 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -37,12 +37,16 @@ def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None) return 1 return 2 - by_name = defaultdict(list) + by_name = defaultdict(dict) for module_bay_type in module_bay_types: - by_name[module_bay_type.name].append(module_bay_type) + # Keyed by pk within each name group so a caller passing the same row twice (the + # three current callers never do, since each resolves from a queryset, but the + # signature accepts any iterable) can't manufacture a same-manufacturer "tie" below. + by_name[module_bay_type.name][module_bay_type.pk] = module_bay_type resolved = [] - for name, candidates in by_name.items(): + for name, candidates_by_pk in by_name.items(): + candidates = list(candidates_by_pk.values()) best_rank = min(preference(c) for c in candidates) best = [c for c in candidates if preference(c) == best_rank] if len(best) > 1: diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index dff5af244..65b17dcd6 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1912,15 +1912,13 @@ class ModuleTypeListView(generic.ObjectListView): 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 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', - ) + # to_yaml() reads module_bay_types directly -- unlike DeviceType.to_yaml(), it does + # not export a nested module-bays section at all (a separate, pre-existing gap, out + # of scope here), so there is no modulebaytemplates relation to prefetch alongside + # it. 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('module_bay_types') return super().export_yaml() diff --git a/netbox/utilities/forms/fields/csv.py b/netbox/utilities/forms/fields/csv.py index 497a1816e..0316068af 100644 --- a/netbox/utilities/forms/fields/csv.py +++ b/netbox/utilities/forms/fields/csv.py @@ -100,7 +100,13 @@ class CSVModelMultipleChoiceField(forms.ModelMultipleChoiceField): def clean(self, value): if not isinstance(value, list): - value = value.split(',') if value else [] + # str(value): a caller may bind this field from parsed YAML/JSON rather than a + # CSV cell, where a scalar column can arrive as a non-string (e.g. an int or + # bool) -- .split() would raise AttributeError otherwise. .strip() each piece: + # a table's default ManyToManyColumn export separator is ", " (comma space), so + # re-importing NetBox's own CSV export of a multi-value column would otherwise + # fail to match on the leading space. + value = [v.strip() for v in str(value).split(',')] if value else [] return super().clean(value)