فهرست منبع

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
Mark Robert Coleman 17 ساعت پیش
والد
کامیت
c7bbfb24c5

+ 56 - 1
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(

+ 6 - 26
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)
 

+ 10 - 23
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(

+ 71 - 0
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"""

+ 48 - 0
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.