diff --git a/docs/models/dcim/modulebay.md b/docs/models/dcim/modulebay.md index 103a95632..4df243ca1 100644 --- a/docs/models/dcim/modulebay.md +++ b/docs/models/dcim/modulebay.md @@ -34,8 +34,6 @@ 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. 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 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 63bdd7050..3b4d2b14b 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. 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. +Bay types are importable and exportable as part of a device type's YAML definition (`module-bays[].module_bay_types`), referenced by name. A referenced name is resolved against bay types belonging to the device type's own manufacturer or with no manufacturer set (global); a name may match both, since a bay type's uniqueness is scoped to `(manufacturer, name)` rather than name alone, in which case the manufacturer-specific type takes precedence. diff --git a/docs/models/dcim/moduletype.md b/docs/models/dcim/moduletype.md index 161e98790..993c5cbfe 100644 --- a/docs/models/dcim/moduletype.md +++ b/docs/models/dcim/moduletype.md @@ -91,8 +91,6 @@ 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. 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 Depending on the module type's assigned [profile](./moduletypeprofile.md) (if any), one or more user-defined attributes may be available to configure. diff --git a/netbox/dcim/forms/bulk_import.py b/netbox/dcim/forms/bulk_import.py index 901e057be..469257ae6 100644 --- a/netbox/dcim/forms/bulk_import.py +++ b/netbox/dcim/forms/bulk_import.py @@ -2,7 +2,6 @@ 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 @@ -11,7 +10,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 dedupe_module_bay_types_by_manufacturer, reconcile_port_mappings +from dcim.utils import reconcile_port_mappings from extras.models import ConfigTemplate from ipam.choices import VLANQinQRoleChoices from ipam.models import VLAN, VRF, IPAddress, VLANGroup @@ -551,19 +550,12 @@ 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 fields = [ 'manufacturer', 'model', 'part_number', 'description', 'cooling_method', 'airflow', 'weight', 'weight_unit', - 'end_of_life', 'profile', 'attribute_data', 'owner', 'comments', 'tags', 'module_bay_types', + 'end_of_life', 'profile', 'attribute_data', 'owner', 'comments', 'tags', ] def clean(self): @@ -577,11 +569,6 @@ 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( @@ -1439,19 +1426,10 @@ class ModuleBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): queryset=Device.objects.all(), to_field_name='name' ) - 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 bay accepts (empty = unconstrained)'), - ) class Meta: model = ModuleBay - fields = ( - 'device', 'name', 'label', 'position', 'enabled', 'description', 'owner', 'tags', 'module_bay_types', - ) + fields = ('device', 'name', 'label', 'position', 'enabled', 'description', 'owner', 'tags') def clean_enabled(self): # Make sure enabled is True when it's not included in the uploaded data @@ -1459,16 +1437,6 @@ class ModuleBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): return True return self.cleaned_data['enabled'] - def clean(self): - super().clean() - - if module_bay_types := self.cleaned_data.get('module_bay_types'): - device = self.cleaned_data.get('device') - manufacturer = device.device_type.manufacturer if device else None - self.cleaned_data['module_bay_types'] = dedupe_module_bay_types_by_manufacturer( - module_bay_types, manufacturer, - ) - class DeviceBayImportForm(OwnerCSVMixin, NetBoxModelImportForm): device = CSVModelChoiceField( diff --git a/netbox/dcim/forms/object_import.py b/netbox/dcim/forms/object_import.py index b6da78dc6..b2899baa2 100644 --- a/netbox/dcim/forms/object_import.py +++ b/netbox/dcim/forms/object_import.py @@ -1,10 +1,9 @@ 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 from dcim.models import * -from dcim.utils import dedupe_module_bay_types_by_manufacturer -from utilities.forms.fields import CSVModelMultipleChoiceField from wireless.choices import WirelessRoleChoices __all__ = ( @@ -215,10 +214,7 @@ class PortTemplateMappingImportForm(forms.ModelForm): class ModuleBayTemplateImportForm(forms.ModelForm): - # 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( + module_bay_types = forms.ModelMultipleChoiceField( label=_('Module bay types'), queryset=ModuleBayType.objects.all(), to_field_name='name', @@ -227,6 +223,8 @@ 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. fields = [ 'device_type', 'module_type', 'name', 'label', 'position', 'enabled', 'description', 'module_bay_types', @@ -239,20 +237,42 @@ class ModuleBayTemplateImportForm(forms.ModelForm): return True return self.cleaned_data['enabled'] - def clean(self): - cleaned_data = super().clean() + 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) + ) - 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, - ) + def clean_device_type(self): + if device_type := self.cleaned_data['device_type']: + self._scope_module_bay_types(device_type.manufacturer) - return cleaned_data + 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): + """ + Collapse to one match per name, preferring a manufacturer-specific match over a global + one. 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 + own manufacturer (narrowed by clean_device_type/clean_module_type above); the field's + default name-based lookup resolves both matches into cleaned_data rather than picking + one, since it has no way to know which is meant. + """ + module_bay_types = self.cleaned_data['module_bay_types'] + + by_name = {} + for module_bay_type in module_bay_types: + existing = by_name.get(module_bay_type.name) + if existing is None or module_bay_type.manufacturer_id is not None: + by_name[module_bay_type.name] = module_bay_type + + return list(by_name.values()) class DeviceBayTemplateImportForm(forms.ModelForm): diff --git a/netbox/dcim/tables/devices.py b/netbox/dcim/tables/devices.py index 2fcda5cd1..ac10e8bd2 100644 --- a/netbox/dcim/tables/devices.py +++ b/netbox/dcim/tables/devices.py @@ -1028,8 +1028,6 @@ class ModuleBayTable(ModularDeviceComponentTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. - export_transform=lambda obj: obj.name, ) class Meta(ModularDeviceComponentTable.Meta): diff --git a/netbox/dcim/tables/devicetypes.py b/netbox/dcim/tables/devicetypes.py index 20bbb38c7..3e4682d0d 100644 --- a/netbox/dcim/tables/devicetypes.py +++ b/netbox/dcim/tables/devicetypes.py @@ -305,8 +305,6 @@ class ModuleBayTemplateTable(ComponentTemplateTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. - export_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 e3706cac3..a8aebf873 100644 --- a/netbox/dcim/tables/modules.py +++ b/netbox/dcim/tables/modules.py @@ -78,8 +78,6 @@ class ModuleTypeTable(PrimaryModelTable): module_bay_types = columns.ManyToManyColumn( verbose_name=_('Bay Types'), linkify_item=True, - # Export as bare name (import resolves by name alone); UI keeps str()'s manufacturer prefix. - export_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 dacf7024b..6b1246153 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -1,6 +1,5 @@ from unittest.mock import patch -import yaml from django import forms from django.test import TestCase @@ -17,7 +16,6 @@ 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 @@ -350,252 +348,29 @@ 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/API place no manufacturer restriction on module_bay_types; import must match.""" + def test_module_bay_types_name_belonging_only_to_other_manufacturers_is_unresolvable(self): + """ + A name that exists only for manufacturers other than the device type's own (and isn't + global) must not resolve at all -- module_bay_types is scoped to the device type's own + manufacturer plus global types, with no cross-manufacturer fallback. + """ 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) + 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]) - - 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()) - # 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): - """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') - bay_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28') - - form = ModuleBayImportForm({ - 'device': device.name, - 'name': 'Bay 1', - 'module_bay_types': 'SFP28', - }) - self.assertTrue(form.is_valid(), form.errors) - module_bay = form.save() - self.assertEqual(list(module_bay.module_bay_types.all()), [bay_type]) - - def test_module_bay_types_prefers_devices_own_manufacturer(self): - device = create_test_device('Module Bay Import Device') - own_manufacturer = device.device_type.manufacturer - other_manufacturer = Manufacturer.objects.create(name='Other Mfr', slug='other-mfr') - own_type = ModuleBayType.objects.create(name='SFP28', slug='sfp28-own', manufacturer=own_manufacturer) - ModuleBayType.objects.create(name='SFP28', slug='sfp28-other', manufacturer=other_manufacturer) - - form = ModuleBayImportForm({ - 'device': device.name, - 'name': 'Bay 1', - 'module_bay_types': 'SFP28', - }) - 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()) - # 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): - - 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()) - - def test_module_bay_types_accepts_csv_comma_separated_string(self): - """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') - - form = ModuleTypeImportForm({ - 'manufacturer': manufacturer.name, - 'model': 'Module Type 1', - 'module_bay_types': 'SFP28,QSFP28', - }) - self.assertTrue(form.is_valid(), form.errors) - - module_type = form.save() self.assertEqual( - set(module_type.module_bay_types.values_list('name', flat=True)), - {bay_type_a.name, bay_type_b.name}, + form.errors.as_data()['module_bay_types'][0].code, 'invalid_choice', ) - def test_module_bay_types_accepts_empty_csv_string(self): - manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') - - form = ModuleTypeImportForm({ - 'manufacturer': manufacturer.name, - 'model': 'Module Type 1', - 'module_bay_types': '', - }) - 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): - """The table's CSV export (multi-value separator, name-only transform) must be re-importable, - without changing the rendered UI column, which should keep str()'s manufacturer prefix.""" - manufacturer = Manufacturer.objects.create(name='Manufacturer 1', slug='manufacturer-1') - 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]) - - column = ModuleTypeTable([original]).columns['module_bay_types'].column - exported_value = column.value(original.module_bay_types.all()) - self.assertEqual(exported_value, 'QSFP28, SFP28') - self.assertIn(str(manufacturer), str(column.render(original.module_bay_types.all()))) - - 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 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({ - '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/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) - - 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]) - - def test_module_bay_types_rejects_ambiguous_name_across_two_foreign_manufacturers(self): - """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') - ModuleBayType.objects.create(name='SFP28', slug='sfp28-cisco', manufacturer=cisco) - ModuleBayType.objects.create(name='SFP28', slug='sfp28-arista', manufacturer=arista) - - form = ModuleTypeImportForm({ - 'manufacturer': juniper.name, - 'model': 'Juniper Line Card', - 'module_bay_types': ['SFP28'], - }) - self.assertFalse(form.is_valid()) - # 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/utils.py b/netbox/dcim/utils.py index 313b67d3e..097774d7a 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -2,57 +2,12 @@ from collections import defaultdict from django.apps import apps from django.contrib.contenttypes.models import ContentType -from django.core.exceptions import ValidationError from django.db import router, transaction from django.utils.translation import gettext as _ from dcim.constants import MODULE_TOKEN -def dedupe_module_bay_types_by_manufacturer(module_bay_types, manufacturer=None): - """ - 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. - - 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 - - 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 - - # 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: - by_name[module_bay_type.name][module_bay_type.pk] = module_bay_type - - resolved = [] - 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: - manufacturers = ', '.join(sorted(c.manufacturer.name for c in best)) - raise ValidationError({ - 'module_bay_types': _( - "Module bay type \"{name}\" is ambiguous: it belongs to more than one " - "manufacturer ({manufacturers}), none of which is this type's own " - "manufacturer." - ).format(name=name, manufacturers=manufacturers) - }) - resolved.append(best[0]) - - return resolved - - def inherit_module_token(position, parent_positions): """ Resolve a single {module} token in a bay position by inheriting from the position diff --git a/netbox/netbox/tables/columns.py b/netbox/netbox/tables/columns.py index 5ab71f8d3..314bb7128 100644 --- a/netbox/netbox/tables/columns.py +++ b/netbox/netbox/tables/columns.py @@ -131,17 +131,9 @@ class DurationColumn(tables.Column): class ManyToManyColumn(tables.ManyToManyColumn): """ Overrides django-tables2's stock ManyToManyColumn to ensure that value() returns only plaintext data. - - export_transform: optional callable used only for value() (CSV/table export), letting export use a - different representation than the rendered column (e.g. a bare name where the UI shows str(obj)). - Defaults to transform, matching the stock behavior of exporting the same text that's rendered. """ - def __init__(self, *args, export_transform=None, **kwargs): - super().__init__(*args, **kwargs) - self.export_transform = export_transform or self.transform - def value(self, value): - items = [self.export_transform(item) for item in self.filter(value)] + items = [self.transform(item) for item in self.filter(value)] return self.separator.join(items) diff --git a/netbox/utilities/forms/fields/csv.py b/netbox/utilities/forms/fields/csv.py index 03f384417..497a1816e 100644 --- a/netbox/utilities/forms/fields/csv.py +++ b/netbox/utilities/forms/fields/csv.py @@ -100,9 +100,7 @@ class CSVModelMultipleChoiceField(forms.ModelMultipleChoiceField): def clean(self, value): if not isinstance(value, list): - # 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 [] + value = value.split(',') if value else [] return super().clean(value)