From c7bbfb24c5f6ad1891d7e261a05a72f97c5e320c Mon Sep 17 00:00:00 2001 From: Mark Robert Coleman Date: Thu, 2 Apr 2026 01:19:43 +0200 Subject: [PATCH] Fix single {module} token rejection at nested module bay depth (#21740) * Fix single {module} token rejection at nested depth (#20474) A module type with a single {module} placeholder in component template names could not be installed in a nested module bay (depth > 1) because the form validation required an exact match between the token count and the tree depth. This resolves the issue by treating a single {module} token as a reference to the immediate parent bay's position, regardless of nesting depth. Multi-token behavior is unchanged. Refactors resolve_name() and resolve_label() into a shared _resolve_module_placeholder() helper to eliminate duplication. Fixes: #20474 * Address review feedback for PR #21740 (fixes #20474) - Rebase on latest main to resolve merge conflicts - Extract shared module bay traversal and {module} token resolution into dcim/utils.py (get_module_bay_positions, resolve_module_placeholder) - Update ModuleCommonForm, ModularComponentTemplateModel, and ModuleBayTemplate to use shared utility functions - Add {module} token validation to ModuleSerializer.validate() so the API enforces the same rules as the UI form - Remove duplicated _get_module_bay_tree (form) and _get_module_tree (model) methods in favor of the shared routine --- netbox/dcim/api/serializers_/devices.py | 57 ++++++++++++++- netbox/dcim/forms/common.py | 32 ++------- .../dcim/models/device_component_templates.py | 33 +++------ netbox/dcim/tests/test_models.py | 71 +++++++++++++++++++ netbox/dcim/utils.py | 48 +++++++++++++ 5 files changed, 191 insertions(+), 50 deletions(-) diff --git a/netbox/dcim/api/serializers_/devices.py b/netbox/dcim/api/serializers_/devices.py index 7be8e42c9..1154374c1 100644 --- a/netbox/dcim/api/serializers_/devices.py +++ b/netbox/dcim/api/serializers_/devices.py @@ -6,8 +6,9 @@ from drf_spectacular.utils import extend_schema_field from rest_framework import serializers from dcim.choices import * -from dcim.constants import MACADDRESS_ASSIGNMENT_MODELS +from dcim.constants import MACADDRESS_ASSIGNMENT_MODELS, MODULE_TOKEN from dcim.models import Device, DeviceBay, MACAddress, Module, VirtualDeviceContext +from dcim.utils import get_module_bay_positions, resolve_module_placeholder from extras.api.serializers_.configtemplates import ConfigTemplateSerializer from ipam.api.serializers_.ip import IPAddressSerializer from netbox.api.fields import ChoiceField, ContentTypeField, RelatedObjectCountField @@ -159,6 +160,60 @@ class ModuleSerializer(PrimaryModelSerializer): ] brief_fields = ('id', 'url', 'display', 'device', 'module_bay', 'module_type', 'description') + def validate(self, data): + data = super().validate(data) + + if self.nested: + return data + + # Skip validation for existing modules (updates) + if self.instance is not None: + return data + + module_bay = data.get('module_bay') + module_type = data.get('module_type') + device = data.get('device') + + if not all((module_bay, module_type, device)): + return data + + positions = get_module_bay_positions(module_bay) + + for templates, component_attribute in [ + ("consoleporttemplates", "consoleports"), + ("consoleserverporttemplates", "consoleserverports"), + ("interfacetemplates", "interfaces"), + ("powerporttemplates", "powerports"), + ("poweroutlettemplates", "poweroutlets"), + ("rearporttemplates", "rearports"), + ("frontporttemplates", "frontports"), + ]: + installed_components = { + component.name: component for component in getattr(device, component_attribute).all() + } + + for template in getattr(module_type, templates).all(): + resolved_name = template.name + if MODULE_TOKEN in template.name: + if not module_bay.position: + raise serializers.ValidationError( + _("Cannot install module with placeholder values in a module bay with no position defined.") + ) + try: + resolved_name = resolve_module_placeholder(template.name, positions) + except ValueError as e: + raise serializers.ValidationError(str(e)) + + if resolved_name in installed_components: + raise serializers.ValidationError( + _("A {model} named {name} already exists").format( + model=template.component_model.__name__, + name=resolved_name + ) + ) + + return data + class MACAddressSerializer(PrimaryModelSerializer): assigned_object_type = ContentTypeField( diff --git a/netbox/dcim/forms/common.py b/netbox/dcim/forms/common.py index a3a781be5..162fd6213 100644 --- a/netbox/dcim/forms/common.py +++ b/netbox/dcim/forms/common.py @@ -3,6 +3,7 @@ from django.utils.translation import gettext_lazy as _ from dcim.choices import * from dcim.constants import * +from dcim.utils import get_module_bay_positions, resolve_module_placeholder from utilities.forms import get_field_value __all__ = ( @@ -70,18 +71,6 @@ class InterfaceCommonForm(forms.Form): class ModuleCommonForm(forms.Form): - def _get_module_bay_tree(self, module_bay): - module_bays = [] - while module_bay: - module_bays.append(module_bay) - if module_bay.module: - module_bay = module_bay.module.module_bay - else: - module_bay = None - - module_bays.reverse() - return module_bays - def clean(self): super().clean() @@ -100,7 +89,7 @@ class ModuleCommonForm(forms.Form): self.instance._disable_replication = True return - module_bays = self._get_module_bay_tree(module_bay) + positions = get_module_bay_positions(module_bay) for templates, component_attribute in [ ("consoleporttemplates", "consoleports"), @@ -119,25 +108,16 @@ class ModuleCommonForm(forms.Form): # Get the templates for the module type. for template in getattr(module_type, templates).all(): resolved_name = template.name - # Installing modules with placeholders require that the bay has a position value if MODULE_TOKEN in template.name: if not module_bay.position: raise forms.ValidationError( _("Cannot install module with placeholder values in a module bay with no position defined.") ) - if len(module_bays) != template.name.count(MODULE_TOKEN): - raise forms.ValidationError( - _( - "Cannot install module with placeholder values in a module bay tree {level} in tree " - "but {tokens} placeholders given." - ).format( - level=len(module_bays), tokens=template.name.count(MODULE_TOKEN) - ) - ) - - for module_bay in module_bays: - resolved_name = resolved_name.replace(MODULE_TOKEN, module_bay.position, 1) + try: + resolved_name = resolve_module_placeholder(template.name, positions) + except ValueError as e: + raise forms.ValidationError(str(e)) existing_item = installed_components.get(resolved_name) diff --git a/netbox/dcim/models/device_component_templates.py b/netbox/dcim/models/device_component_templates.py index 08d52d6eb..ff5df9721 100644 --- a/netbox/dcim/models/device_component_templates.py +++ b/netbox/dcim/models/device_component_templates.py @@ -9,6 +9,7 @@ from dcim.choices import * from dcim.constants import * from dcim.models.base import PortMappingBase from dcim.models.mixins import InterfaceValidationMixin +from dcim.utils import get_module_bay_positions, resolve_module_placeholder from netbox.models import ChangeLoggedModel from utilities.fields import ColorField, NaturalOrderingField from utilities.mptt import TreeManager @@ -165,31 +166,15 @@ class ModularComponentTemplateModel(ComponentTemplateModel): _("A component template must be associated with either a device type or a module type.") ) - def _get_module_tree(self, module): - modules = [] - while module: - modules.append(module) - if module.module_bay: - module = module.module_bay.module - else: - module = None - - modules.reverse() - return modules - - def _resolve_module_placeholder(self, value, module): - if MODULE_TOKEN not in value or not module: - return value - modules = self._get_module_tree(module) - for m in modules: - value = value.replace(MODULE_TOKEN, m.module_bay.position, 1) - return value - def resolve_name(self, module): - return self._resolve_module_placeholder(self.name, module) + if MODULE_TOKEN not in self.name or not module: + return self.name + return resolve_module_placeholder(self.name, get_module_bay_positions(module.module_bay)) def resolve_label(self, module): - return self._resolve_module_placeholder(self.label, module) + if MODULE_TOKEN not in self.label or not module: + return self.label + return resolve_module_placeholder(self.label, get_module_bay_positions(module.module_bay)) class ConsolePortTemplate(ModularComponentTemplateModel): @@ -720,7 +705,9 @@ class ModuleBayTemplate(ModularComponentTemplateModel): verbose_name_plural = _('module bay templates') def resolve_position(self, module): - return self._resolve_module_placeholder(self.position, module) + if MODULE_TOKEN not in self.position or not module: + return self.position + return resolve_module_placeholder(self.position, get_module_bay_positions(module.module_bay)) def instantiate(self, **kwargs): return self.component_model( diff --git a/netbox/dcim/tests/test_models.py b/netbox/dcim/tests/test_models.py index efb2a356e..f676ae5df 100644 --- a/netbox/dcim/tests/test_models.py +++ b/netbox/dcim/tests/test_models.py @@ -893,6 +893,77 @@ class ModuleBayTestCase(TestCase): nested_bay = module.modulebays.get(name='Sub-bay 1-1') self.assertEqual(nested_bay.position, '1-1') + @tag('regression') # #20474 + def test_single_module_token_at_nested_depth(self): + """ + A module type with a single {module} token should install at depth > 1 + without raising a token count mismatch error, resolving to the immediate + parent bay's position. + """ + manufacturer = Manufacturer.objects.first() + site = Site.objects.first() + device_role = DeviceRole.objects.first() + + device_type = DeviceType.objects.create( + manufacturer=manufacturer, + model='Chassis with Rear Card', + slug='chassis-with-rear-card' + ) + ModuleBayTemplate.objects.create( + device_type=device_type, + name='Rear card slot', + position='1' + ) + + rear_card_type = ModuleType.objects.create( + manufacturer=manufacturer, + model='Rear Card' + ) + ModuleBayTemplate.objects.create( + module_type=rear_card_type, + name='SFP slot 1', + position='1' + ) + ModuleBayTemplate.objects.create( + module_type=rear_card_type, + name='SFP slot 2', + position='2' + ) + + sfp_type = ModuleType.objects.create( + manufacturer=manufacturer, + model='SFP Module' + ) + InterfaceTemplate.objects.create( + module_type=sfp_type, + name='SFP {module}', + type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS + ) + + device = Device.objects.create( + name='Test Chassis', + device_type=device_type, + role=device_role, + site=site + ) + + rear_card_bay = device.modulebays.get(name='Rear card slot') + rear_card = Module.objects.create( + device=device, + module_bay=rear_card_bay, + module_type=rear_card_type + ) + + sfp_bay = rear_card.modulebays.get(name='SFP slot 2') + sfp_module = Module.objects.create( + device=device, + module_bay=sfp_bay, + module_type=sfp_type + ) + + interface = sfp_module.interfaces.first() + self.assertEqual(interface.name, 'SFP 2') + @tag('regression') # #20912 def test_module_bay_parent_cleared_when_module_removed(self): """Test that the parent field is properly cleared when a module bay's module assignment is removed""" diff --git a/netbox/dcim/utils.py b/netbox/dcim/utils.py index 5dcc20035..fc4bae02c 100644 --- a/netbox/dcim/utils.py +++ b/netbox/dcim/utils.py @@ -3,6 +3,9 @@ from collections import defaultdict from django.apps import apps from django.contrib.contenttypes.models import ContentType from django.db import router, transaction +from django.utils.translation import gettext as _ + +from dcim.constants import MODULE_TOKEN def compile_path_node(ct_id, object_id): @@ -33,6 +36,51 @@ def path_node_to_object(repr): return ct.model_class().objects.filter(pk=object_id).first() +def get_module_bay_positions(module_bay): + """ + Given a module bay, traverse up the module hierarchy and return + a list of bay position strings from root to leaf. + """ + positions = [] + while module_bay: + positions.append(module_bay.position) + if module_bay.module: + module_bay = module_bay.module.module_bay + else: + module_bay = None + positions.reverse() + return positions + + +def resolve_module_placeholder(value, positions): + """ + Resolve {module} placeholder tokens in a string using the given + list of module bay positions (ordered root to leaf). + + A single {module} token resolves to the leaf (immediate parent) bay's position. + Multiple tokens must match the tree depth and resolve level-by-level. + + Returns the resolved string. + Raises ValueError if token count is greater than 1 and doesn't match tree depth. + """ + if MODULE_TOKEN not in value: + return value + + token_count = value.count(MODULE_TOKEN) + if token_count == 1: + return value.replace(MODULE_TOKEN, positions[-1]) + if token_count == len(positions): + for pos in positions: + value = value.replace(MODULE_TOKEN, pos, 1) + return value + raise ValueError( + _("Cannot install module with placeholder values in a module bay tree " + "{level} levels deep but {tokens} placeholders given.").format( + level=len(positions), tokens=token_count + ) + ) + + def create_cablepaths(objects): """ Create CablePaths for all paths originating from the specified set of nodes.