Address review: dead code, ModuleType's own side of the round trip, N+1
- clean_module_bay_types()'s two ValidationErrors were unreachable: ModelMultipleChoiceField.clean() already raises before the clean_<name> hook runs on a non-list or an unresolvable name, per Django's BaseForm._clean_fields(). Simplify to dedupe from cleaned_data (already scoped and validated) via a shared dedupe_module_bay_types_by_manufacturer() helper in dcim/utils.py, used by both ModuleBayTemplateImportForm and the new ModuleTypeImportForm.module_bay_types below. This also drops the self.data access that ignored the form prefix, broke on a QueryDict, and re-queried once per name. - ModuleType.module_bay_types (the module's own side of the bay/module compatibility intersection) was still missing from the YAML round trip. Add it to ModuleType.to_yaml() and ModuleTypeImportForm, mirroring ModuleBayTemplateImportForm's manufacturer-scoping and dedup. - ModuleBayTemplate.to_yaml() emitted enabled but the import form didn't accept it, so it silently reset to False (not the model's default=True) on any dict-bound re-import. Add it with the same clean_enabled()-defaults-to-True pattern already used by ModuleBayImportForm's CSV import. - Prefetch module_bay_types in DeviceTypeListView/ModuleTypeListView's export_yaml() so bulk YAML export doesn't add one query per module bay template across the exported queryset. - Document the manufacturer-preference precedence rule in the model docs, since export emits a bare name and import can resolve a colliding one to either a global or manufacturer-specific type. Adds regression tests for the module_type-scoped path, the enabled round trip, an export/import round trip, export ordering, the new ModuleTypeImportForm coverage, and the prefetch fix.
This commit is contained in:
parent
508e2eaba2
commit
63045d8551
|
|
@ -3,3 +3,5 @@
|
|||
A template for a module bay that will be created on all instantiations of the parent device type. See the [module bay](./modulebay.md) documentation for more detail.
|
||||
|
||||
[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.
|
||||
|
|
|
|||
|
|
@ -91,6 +91,8 @@ 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.
|
||||
|
||||
### Attributes
|
||||
|
||||
Depending on the module type's assigned [profile](./moduletypeprofile.md) (if any), one or more user-defined attributes may be available to configure.
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ from django import forms
|
|||
from django.contrib.contenttypes.models import ContentType
|
||||
from django.contrib.postgres.forms.array import SimpleArrayField
|
||||
from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist
|
||||
from django.db.models import Q
|
||||
from django.utils.functional import lazy
|
||||
from django.utils.html import format_html
|
||||
from django.utils.safestring import SafeString, mark_safe
|
||||
|
|
@ -10,7 +11,7 @@ from django.utils.translation import gettext_lazy as _
|
|||
from dcim.choices import *
|
||||
from dcim.constants import *
|
||||
from dcim.models import *
|
||||
from dcim.utils import reconcile_port_mappings
|
||||
from dcim.utils import dedupe_module_bay_types_by_manufacturer, reconcile_port_mappings
|
||||
from extras.models import ConfigTemplate
|
||||
from ipam.choices import VLANQinQRoleChoices
|
||||
from ipam.models import VLAN, VRF, IPAddress, VLANGroup
|
||||
|
|
@ -550,14 +551,35 @@ class ModuleTypeImportForm(PrimaryModelImportForm):
|
|||
required=False,
|
||||
help_text=_('Attribute values for the assigned profile, passed as a dictionary')
|
||||
)
|
||||
module_bay_types = CSVModelMultipleChoiceField(
|
||||
label=_('Module bay types'),
|
||||
queryset=ModuleBayType.objects.all(),
|
||||
to_field_name='name',
|
||||
required=False,
|
||||
help_text=_('Types of module bays this module type can be installed in (empty = unconstrained)'),
|
||||
)
|
||||
|
||||
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',
|
||||
'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()
|
||||
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ from django.utils.translation import gettext_lazy as _
|
|||
|
||||
from dcim.choices import InterfacePoEModeChoices, InterfacePoETypeChoices, InterfaceTypeChoices, PortTypeChoices
|
||||
from dcim.models import *
|
||||
from dcim.utils import dedupe_module_bay_types_by_manufacturer
|
||||
from wireless.choices import WirelessRoleChoices
|
||||
|
||||
__all__ = (
|
||||
|
|
@ -223,10 +224,22 @@ 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', 'description', 'module_bay_types',
|
||||
'device_type', 'module_type', 'name', 'label', 'position', 'enabled', 'description',
|
||||
'module_bay_types',
|
||||
]
|
||||
|
||||
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.
|
||||
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(
|
||||
|
|
@ -246,29 +259,7 @@ class ModuleBayTemplateImportForm(forms.ModelForm):
|
|||
return module_type
|
||||
|
||||
def clean_module_bay_types(self):
|
||||
"""
|
||||
Resolve each submitted name against the scoped queryset, preferring a manufacturer-
|
||||
specific match over a global one when both exist. ModuleBayType's unique constraint
|
||||
is on (manufacturer, name), not name alone, so a name can legitimately collide between
|
||||
a global type and one scoped to this template's manufacturer; ModelMultipleChoiceField's
|
||||
default name-based lookup would otherwise silently attach both.
|
||||
"""
|
||||
names = self.data.get('module_bay_types') or []
|
||||
if not isinstance(names, (list, tuple)):
|
||||
raise forms.ValidationError(_("Module bay types must be a list."))
|
||||
|
||||
queryset = self.fields['module_bay_types'].queryset
|
||||
resolved = []
|
||||
for name in names:
|
||||
candidates = list(queryset.filter(name=name))
|
||||
if not candidates:
|
||||
raise forms.ValidationError(
|
||||
_("Module bay type not found: {name}").format(name=name)
|
||||
)
|
||||
match = next((c for c in candidates if c.manufacturer_id is not None), candidates[0])
|
||||
resolved.append(match)
|
||||
|
||||
return resolved
|
||||
return dedupe_module_bay_types_by_manufacturer(self.cleaned_data['module_bay_types'])
|
||||
|
||||
|
||||
class DeviceBayTemplateImportForm(forms.ModelForm):
|
||||
|
|
|
|||
|
|
@ -313,6 +313,7 @@ class ModuleType(ImageAttachmentsMixin, PrimaryModel, WeightMixin):
|
|||
'end_of_life': self.end_of_life.isoformat() if self.end_of_life else None,
|
||||
'attribute_data': self.attribute_data,
|
||||
'comments': self.comments,
|
||||
'module_bay_types': [t.name for t in self.module_bay_types.all()],
|
||||
}
|
||||
|
||||
# Component templates
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
from unittest.mock import patch
|
||||
|
||||
import yaml
|
||||
from django import forms
|
||||
from django.test import TestCase
|
||||
|
||||
|
|
@ -273,7 +274,129 @@ class ModuleBayTemplateImportFormTestCase(TestCase):
|
|||
'module_bay_types': ['Nonexistent'],
|
||||
})
|
||||
self.assertFalse(form.is_valid())
|
||||
self.assertIn('module_bay_types', form.errors)
|
||||
self.assertEqual(
|
||||
form.errors['module_bay_types'],
|
||||
['Select a valid choice. Nonexistent is not one of the available choices.'],
|
||||
)
|
||||
|
||||
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).
|
||||
"""
|
||||
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(
|
||||
name='SFP28', slug='sfp28-scoped', manufacturer=manufacturer,
|
||||
)
|
||||
module_type = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1')
|
||||
|
||||
form = ModuleBayTemplateImportForm({
|
||||
'module_type': module_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()), [scoped_type],
|
||||
)
|
||||
self.assertNotIn(global_type, module_bay_template.module_bay_types.all())
|
||||
|
||||
def test_enabled_defaults_true_when_omitted(self):
|
||||
device_type = DeviceType.objects.create(
|
||||
manufacturer=Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1'),
|
||||
model='Device Type 1',
|
||||
slug='device-type-1',
|
||||
)
|
||||
|
||||
form = ModuleBayTemplateImportForm({
|
||||
'device_type': device_type.pk,
|
||||
'name': 'Module Bay 1',
|
||||
})
|
||||
self.assertTrue(form.is_valid(), form.errors)
|
||||
self.assertTrue(form.save().enabled)
|
||||
|
||||
def test_enabled_honors_explicit_false(self):
|
||||
device_type = DeviceType.objects.create(
|
||||
manufacturer=Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1'),
|
||||
model='Device Type 1',
|
||||
slug='device-type-1',
|
||||
)
|
||||
|
||||
form = ModuleBayTemplateImportForm({
|
||||
'device_type': device_type.pk,
|
||||
'name': 'Module Bay 1',
|
||||
'enabled': False,
|
||||
})
|
||||
self.assertTrue(form.is_valid(), form.errors)
|
||||
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.
|
||||
"""
|
||||
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')
|
||||
device_type = DeviceType.objects.create(
|
||||
manufacturer=manufacturer, model='Device Type 1', slug='device-type-1',
|
||||
)
|
||||
original = ModuleBayTemplate.objects.create(device_type=device_type, name='Module Bay 1')
|
||||
original.module_bay_types.set([bay_type_a, bay_type_b])
|
||||
|
||||
exported = original.to_yaml()
|
||||
form = ModuleBayTemplateImportForm({
|
||||
'device_type': device_type.pk,
|
||||
'name': 'Module Bay 2',
|
||||
'module_bay_types': exported['module_bay_types'],
|
||||
})
|
||||
self.assertTrue(form.is_valid(), form.errors)
|
||||
|
||||
reimported = form.save()
|
||||
self.assertEqual(
|
||||
set(reimported.module_bay_types.values_list('name', flat=True)),
|
||||
set(original.module_bay_types.values_list('name', flat=True)),
|
||||
)
|
||||
|
||||
|
||||
class ModuleTypeImportFormTestCase(TestCase):
|
||||
|
||||
def test_module_bay_types_round_trip(self):
|
||||
manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
|
||||
bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28', manufacturer=manufacturer)
|
||||
|
||||
form = ModuleTypeImportForm({
|
||||
'manufacturer': manufacturer.name,
|
||||
'model': 'Module Type 1',
|
||||
'module_bay_types': ['SFP28'],
|
||||
})
|
||||
self.assertTrue(form.is_valid(), form.errors)
|
||||
|
||||
module_type = form.save()
|
||||
self.assertEqual(list(module_type.module_bay_types.all()), [bay_type])
|
||||
self.assertEqual(yaml.safe_load(module_type.to_yaml())['module_bay_types'], ['SFP28'])
|
||||
|
||||
def test_module_bay_types_prefers_manufacturer_specific_match_over_global(self):
|
||||
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(
|
||||
name='SFP28', slug='sfp28-scoped', manufacturer=manufacturer,
|
||||
)
|
||||
|
||||
form = ModuleTypeImportForm({
|
||||
'manufacturer': manufacturer.name,
|
||||
'model': 'Module Type 1',
|
||||
'module_bay_types': ['SFP28'],
|
||||
})
|
||||
self.assertTrue(form.is_valid(), form.errors)
|
||||
|
||||
module_type = form.save()
|
||||
self.assertEqual(list(module_type.module_bay_types.all()), [scoped_type])
|
||||
self.assertNotIn(global_type, module_type.module_bay_types.all())
|
||||
|
||||
|
||||
class ModuleFormTestCase(TestCase):
|
||||
|
|
|
|||
|
|
@ -1,9 +1,11 @@
|
|||
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
|
||||
|
|
@ -152,6 +154,32 @@ 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):
|
||||
|
||||
|
|
@ -195,6 +223,21 @@ class ModuleTypeTestCase(TestCase):
|
|||
data = module_bay_template.to_yaml()
|
||||
self.assertEqual(data['module_bay_types'], ['SFP28'])
|
||||
|
||||
def test_module_bay_template_to_yaml_orders_module_bay_types(self):
|
||||
"""
|
||||
Multiple module bay types should export in ModuleBayType's own ordering
|
||||
(manufacturer, name), independent of assignment order.
|
||||
"""
|
||||
manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1')
|
||||
module_type = ModuleType.objects.create(manufacturer=manufacturer, model='Module Type 1')
|
||||
bay_type_b = ModuleBayType.objects.create(name='QSFP28', slug='qsfp28')
|
||||
bay_type_a = ModuleBayType.objects.create(name='SFP28', slug='sfp28')
|
||||
module_bay_template = ModuleBayTemplate.objects.create(module_type=module_type, name='Module Bay 1')
|
||||
module_bay_template.module_bay_types.set([bay_type_b, bay_type_a])
|
||||
|
||||
data = module_bay_template.to_yaml()
|
||||
self.assertEqual(data['module_bay_types'], ['QSFP28', 'SFP28'])
|
||||
|
||||
def test_attributes(self):
|
||||
"""
|
||||
ModuleType.attributes should normalize iterable values into strings for presentation.
|
||||
|
|
|
|||
|
|
@ -8,6 +8,23 @@ from django.utils.translation import gettext as _
|
|||
from dcim.constants import MODULE_TOKEN
|
||||
|
||||
|
||||
def dedupe_module_bay_types_by_manufacturer(module_bay_types):
|
||||
"""
|
||||
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.
|
||||
|
||||
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.
|
||||
"""
|
||||
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):
|
||||
resolved[module_bay_type.name] = module_bay_type
|
||||
return list(resolved.values())
|
||||
|
||||
|
||||
def inherit_module_token(position, parent_positions):
|
||||
"""
|
||||
Resolve a single {module} token in a bay position by inheriting from the position
|
||||
|
|
|
|||
|
|
@ -1447,6 +1447,13 @@ class DeviceTypeListView(generic.ObjectListView):
|
|||
filterset_form = forms.DeviceTypeFilterForm
|
||||
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 bulk export doesn't issue one query per
|
||||
# module bay template across the whole queryset.
|
||||
self.queryset = self.queryset.prefetch_related('modulebaytemplates__module_bay_types')
|
||||
return super().export_yaml()
|
||||
|
||||
|
||||
@register_model_view(DeviceType)
|
||||
class DeviceTypeView(GetRelatedModelsMixin, generic.ObjectView):
|
||||
|
|
@ -1902,6 +1909,15 @@ class ModuleTypeListView(generic.ObjectListView):
|
|||
filterset_form = forms.ModuleTypeFilterForm
|
||||
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 bulk export doesn't issue one query per
|
||||
# module type/module bay template across the whole queryset.
|
||||
self.queryset = self.queryset.prefetch_related(
|
||||
'module_bay_types', 'modulebaytemplates__module_bay_types',
|
||||
)
|
||||
return super().export_yaml()
|
||||
|
||||
|
||||
@register_model_view(ModuleType)
|
||||
class ModuleTypeView(GetRelatedModelsMixin, generic.ObjectView):
|
||||
|
|
|
|||
Loading…
Reference in New Issue