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).
This commit is contained in:
Brian Tiemann 2026-08-13 15:21:59 -04:00
parent dfde52df05
commit faafb1a11f
10 changed files with 52 additions and 125 deletions

View File

@ -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

View File

@ -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']

View File

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

View File

@ -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')

View File

@ -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,

View File

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

View File

@ -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')

View File

@ -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 = []

View File

@ -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()

View File

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