From faafb1a11fbee7191eb21eb4a45fa67b01ae54e7 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 15:21:59 -0400 Subject: [PATCH] Fix manufacturer-scoped bay type CSV export; tighten ambiguity tests; trim comments - ModuleBayType.__str__() includes the manufacturer (e.g. "Cisco SFP28"), but the three module_bay_types ManyToManyColumn declarations had no transform, so django-tables2 defaulted to str() for CSV export while the import forms resolve by name alone. Verified directly: exporting a manufacturer-scoped bay type produced "Cisco SFP28", which then failed to re-import with "Object not found: Cisco SFP28" -- broken for exactly the case (manufacturer-scoped types) the preference/ ambiguity machinery exists to serve. Set transform=lambda obj: obj.name on all three columns to match to_yaml(), and rewrote the existing round-trip test to use a manufacturer-scoped bay type instead of a global one, which is the only case that exercised str(). - The three ambiguity-refusal tests asserted only that the field errored, which a plain invalid_choice (e.g. from a queryset that excluded both candidates) would also satisfy -- masking a regression of the manufacturer scoping removed two commits ago. Tightened each to assert the error names both competing manufacturers. - Corrected modulebay.md, which still described module_bay_types resolution as "scoped to" the device's manufacturer -- the behavior the prior commit removed as a bug; it's a preference, not a scope. - Trimmed comments and docstrings introduced across this branch to a more proportionate length. Deliberately out of scope for this PR (tracked as follow-up considerations, not fixed here): an escape hatch for a bay type name that's genuinely ambiguous across manufacturers with no local match (would require a new wire-format convention), and ModuleType.to_yaml() not exporting a module-bays section at all (a separate, pre-existing asymmetry, larger than this PR's scope). --- docs/models/dcim/modulebay.md | 2 +- netbox/dcim/forms/object_import.py | 14 ++--- netbox/dcim/tables/devices.py | 2 + netbox/dcim/tables/devicetypes.py | 2 + netbox/dcim/tables/modules.py | 2 + netbox/dcim/tests/test_forms.py | 86 ++++++++-------------------- netbox/dcim/tests/test_views.py | 20 +------ netbox/dcim/utils.py | 26 +++------ netbox/dcim/views.py | 15 ++--- netbox/utilities/forms/fields/csv.py | 8 +-- 10 files changed, 52 insertions(+), 125 deletions(-) diff --git a/docs/models/dcim/modulebay.md b/docs/models/dcim/modulebay.md index ffb3945ed..103a95632 100644 --- a/docs/models/dcim/modulebay.md +++ b/docs/models/dcim/modulebay.md @@ -34,7 +34,7 @@ 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. +Bay types are importable via CSV, referenced by name. A bay type belonging to a manufacturer other than the module bay's own device 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'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's own manufacturer nor a global type available to break the tie, the import is rejected rather than resolved to an arbitrary one. ### Enabled diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index 54f005699..b6da78dc6 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -215,12 +215,9 @@ class PortTemplateMappingImportForm(forms.ModelForm): class ModuleBayTemplateImportForm(forms.ModelForm): - # 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. + # CSVModelMultipleChoiceField, not the plain ModelMultipleChoiceField used elsewhere in + # this file, so a scalar name is accepted alongside a list -- matches ModuleTypeImportForm's + # equivalent field, which also serves plain CSV import. module_bay_types = CSVModelMultipleChoiceField( label=_('Module bay types'), queryset=ModuleBayType.objects.all(), @@ -237,10 +234,7 @@ 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. 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. + # default=True -- match ModuleBayImportForm's equivalent CSV-import behavior. if 'enabled' not in self.data: return True return self.cleaned_data['enabled'] diff --git a/netbox/dcim/tables/devices.py b/netbox/dcim/tables/devices.py index ac10e8bd2..6c5ad9249 100644 --- a/netbox/dcim/tables/devices.py +++ b/netbox/dcim/tables/devices.py @@ -1028,6 +1028,8 @@ class ModuleBayTable(ModularDeviceComponentTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, + # __str__() includes the manufacturer, but import resolves by name alone. + transform=lambda obj: obj.name, ) class Meta(ModularDeviceComponentTable.Meta): diff --git a/netbox/dcim/tables/devicetypes.py b/netbox/dcim/tables/devicetypes.py index 3e4682d0d..427ca0ee4 100644 --- a/netbox/dcim/tables/devicetypes.py +++ b/netbox/dcim/tables/devicetypes.py @@ -305,6 +305,8 @@ class ModuleBayTemplateTable(ComponentTemplateTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, + # __str__() includes the manufacturer, but import resolves by name alone. + transform=lambda obj: obj.name, ) actions = columns.ActionsColumn( actions=('edit', 'delete') diff --git a/netbox/dcim/tables/modules.py b/netbox/dcim/tables/modules.py index a8aebf873..2586b1155 100644 --- a/netbox/dcim/tables/modules.py +++ b/netbox/dcim/tables/modules.py @@ -78,6 +78,8 @@ class ModuleTypeTable(PrimaryModelTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, + # __str__() includes the manufacturer, but import resolves by name alone. + transform=lambda obj: obj.name, ) model = tables.Column( linkify=True, diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 22a26719c..559b099d3 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -234,12 +234,7 @@ class ModuleTypeFormTestCase(TestCase): class ModuleBayTemplateImportFormTestCase(TestCase): def test_module_bay_types_prefers_manufacturer_specific_match_over_global(self): - """ - ModuleBayType's unique constraint is on (manufacturer, name), not name alone, so a - global type and a manufacturer-scoped type can legally share the same name. Referencing - that name by import should resolve to the manufacturer-specific match only, not attach - both. - """ + """A name shared by a global and a manufacturer-scoped type resolves to the scoped one.""" 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( @@ -280,10 +275,7 @@ class ModuleBayTemplateImportFormTestCase(TestCase): ) 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). - """ + """Same disambiguation, but for a module bay template nested under a ModuleType.""" 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( @@ -334,11 +326,7 @@ class ModuleBayTemplateImportFormTestCase(TestCase): 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. - """ + """to_yaml() then re-import through this form preserves module bay types.""" 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') @@ -363,13 +351,7 @@ class ModuleBayTemplateImportFormTestCase(TestCase): ) 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. - """ + """The UI/API place no manufacturer restriction on module_bay_types; import must match.""" 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) @@ -412,15 +394,14 @@ class ModuleBayTemplateImportFormTestCase(TestCase): 'module_bay_types': ['SFP28'], }) self.assertFalse(form.is_valid()) - self.assertIn('module_bay_types', form.errors) + # Must name both manufacturers, not just error -- a plain invalid_choice would also + # pass assertIn() and mask a regression of the scoping removed in 6f3c537. + errors = form.errors.get('module_bay_types', []) + self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) 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. - """ + """Covers real ModuleBay instances via CSV, as opposed to templates via ModuleBayTemplateImportForm.""" def test_module_bay_types_csv_import(self): device = create_test_device('Module Bay Import Device') @@ -463,7 +444,10 @@ class ModuleBayImportFormTestCase(TestCase): 'module_bay_types': 'SFP28', }) self.assertFalse(form.is_valid()) - self.assertIn('module_bay_types', form.errors) + # Must name both manufacturers, not just error -- a plain invalid_choice would also + # pass assertIn() and mask a regression of the scoping removed in 6f3c537. + errors = form.errors.get('module_bay_types', []) + self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) class ModuleTypeImportFormTestCase(TestCase): @@ -502,11 +486,7 @@ class ModuleTypeImportFormTestCase(TestCase): self.assertNotIn(global_type, module_type.module_bay_types.all()) def test_module_bay_types_accepts_csv_comma_separated_string(self): - """ - 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. - """ + """This form also serves plain CSV import, where the value is a string, not a list.""" 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') @@ -536,15 +516,10 @@ class ModuleTypeImportFormTestCase(TestCase): 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. - """ + """The table's CSV export (multi-value separator, name-only transform) must be re-importable.""" 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') + bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28', manufacturer=manufacturer) + bay_type_b = ModuleBayType.objects.create(name='QSFP28', slug='qsfp28', manufacturer=manufacturer) original = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1') original.module_bay_types.set([bay_type_a, bay_type_b]) @@ -564,12 +539,7 @@ class ModuleTypeImportFormTestCase(TestCase): ) 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. - """ + """A non-string scalar (e.g. from YAML) must produce a form error, not a crash.""" manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') form = ModuleTypeImportForm({ @@ -581,13 +551,7 @@ class ModuleTypeImportFormTestCase(TestCase): 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 - 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. - """ + """The UI/API place no manufacturer restriction on module_bay_types; import must match.""" 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) @@ -612,12 +576,7 @@ class ModuleTypeImportFormTestCase(TestCase): 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. - """ + """A name matching two different foreign manufacturers must be refused, not 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') @@ -630,7 +589,10 @@ class ModuleTypeImportFormTestCase(TestCase): 'module_bay_types': ['SFP28'], }) self.assertFalse(form.is_valid()) - self.assertIn('module_bay_types', form.errors) + # Must name both manufacturers, not just error -- a plain invalid_choice would also + # pass assertIn() and mask a regression of the scoping removed in 6f3c537. + errors = form.errors.get('module_bay_types', []) + self.assertTrue(any('Cisco' in e and 'Arista' in e for e in errors), errors) class ModuleFormTestCase(TestCase): diff --git a/netbox/dcim/tests/test_views.py b/netbox/dcim/tests/test_views.py index 65fd5a2d9..81da915c8 100644 --- a/netbox/dcim/tests/test_views.py +++ b/netbox/dcim/tests/test_views.py @@ -1143,11 +1143,7 @@ inventory-items: 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. - """ + """Query count shouldn't scale with bay count -- module_bay_types is prefetched.""" 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') @@ -1797,18 +1793,8 @@ module-bays: self.assertEqual(list(mb1.module_bay_types.values_list('name', flat=True)), ['SFP28']) def test_bulk_yaml_export_prefetches_module_bay_types_on_the_module_type_itself(self): - """ - 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. - """ + """Compares the same queryset with/without the prefetch, since row-count comparisons + would be swamped by other per-instance relations that legitimately scale with it.""" 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') diff --git a/netbox/dcim/utils.py b/netbox/dcim/utils.py index bc1ec3e90..313b67d3e 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -11,22 +11,14 @@ from dcim.constants import MODULE_TOKEN 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. + Collapse ModuleBayType instances resolved by name to one per name, preferring an exact + match on *manufacturer*, then a global (manufacturer-less) type. Names aren't globally + unique -- uniqueness is scoped to (manufacturer, name) -- and callers must not scope the + queryset by manufacturer themselves, since a type may legitimately belong to another + manufacturer entirely; only this preference order is manufacturer-aware. - 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. - - 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. + Raises ValidationError if a name ties across two or more non-preferred manufacturers, + rather than picking one arbitrarily. """ manufacturer_id = manufacturer.pk if manufacturer else None @@ -37,11 +29,9 @@ def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None) return 1 return 2 + # Keyed by pk so a caller passing the same row twice can't manufacture a false tie below. by_name = defaultdict(dict) for module_bay_type in module_bay_types: - # 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 = [] diff --git a/netbox/dcim/views.py b/netbox/dcim/views.py index 65b17dcd6..ac123a185 100644 --- a/netbox/dcim/views.py +++ b/netbox/dcim/views.py @@ -1448,11 +1448,7 @@ class DeviceTypeListView(generic.ObjectListView): 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 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. + # Avoid one module_bay_types query per module bay template across the export. self.queryset = self.queryset.prefetch_related('modulebaytemplates__module_bay_types') return super().export_yaml() @@ -1912,12 +1908,9 @@ class ModuleTypeListView(generic.ObjectListView): table = tables.ModuleTypeTable def export_yaml(self): - # 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. + # Avoid one module_bay_types query per module type across the export. (Unlike + # DeviceType.to_yaml(), ModuleType.to_yaml() doesn't export module bay templates at + # all, so there's nothing to prefetch alongside it.) 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 0316068af..03f384417 100644 --- a/netbox/utilities/forms/fields/csv.py +++ b/netbox/utilities/forms/fields/csv.py @@ -100,12 +100,8 @@ class CSVModelMultipleChoiceField(forms.ModelMultipleChoiceField): def clean(self, value): if not isinstance(value, list): - # 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. + # str(): a non-CSV caller (e.g. YAML) may pass a non-string scalar. strip(): a + # table's default ManyToManyColumn export separator is ", ", not ",". value = [v.strip() for v in str(value).split(',')] if value else [] return super().clean(value)