Fix regression: manufacturer scoping made cross-manufacturer bay types unimportable

The manufacturer-or-null queryset scoping added to disambiguate a name shared
by a global and a manufacturer-scoped ModuleBayType went further than
intended: it also excluded a *different* manufacturer's bay type entirely.
The UI (ModuleTypeForm/ModuleBayTemplateForm) and REST API place no such
restriction -- a third-party module may legitimately declare compatibility
with another manufacturer's proprietary bay type. Confirmed the regression
directly: creating that assignment via ModuleTypeForm succeeds, but
exporting it and re-importing the same YAML failed with
"Object not found: SFP28", making valid existing data unimportable -- worse
than the bug this feature exists to fix.

Remove the queryset scoping entirely and instead make
dedupe_module_bay_types_by_manufacturer() manufacturer-aware: given the
target manufacturer, it now prefers (in order) an exact match, then a global
type, then any remaining candidate, resolved from an unscoped queryset in
clean() rather than a sibling clean_<field>() mutating the field's queryset.
This also drops the Meta.fields-ordering dependency those methods required.

Also, from the same review round:
- Test asserting Django's literal English error string now asserts the
  error code instead, so it survives wording changes/translation.
- The prefetch query-count test moved from test_models.py (which doesn't
  otherwise touch views) to test_views.py, and strengthened from "prefetch
  saves at least one query" to "query count is constant regardless of bay
  count" -- the actual invariant. Added equivalent coverage for
  ModuleTypeListView, which the prior version didn't test at all.
- Corrected the export_yaml() prefetch comments to not imply the other
  ~11 relations to_yaml() touches are also covered -- they aren't, and
  weren't before this feature either.
- Updated the model docs to describe the new (permissive, cross-manufacturer
  allowed) behavior instead of the old (restrictive) one they described a
  commit ago.

Adds regression tests importing a bay type belonging to a different
manufacturer than the importing device/module type, through both
ModuleBayTemplateImportForm and ModuleTypeImportForm.
This commit is contained in:
Brian Tiemann 2026-08-13 11:04:47 -04:00
parent 63045d8551
commit 6f3c53791b
9 changed files with 185 additions and 81 deletions

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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