Просмотр исходного кода

Implement Option 2: {module_path} for full path, {module} for parent-only

Per sigprof's feedback, this implements two distinct placeholders:

- {module_path}: Always expands to full /-separated path (e.g., 1/2/3)
  Use case: Generic modules like SFPs that work at any depth

- {module} (single): Expands to parent bay position only
  Use case: Building custom paths via position field with user-controlled separators

- {module}/{module}: Level-by-level substitution (unchanged for backwards compat)

This design allows two ways to build module hierarchies:
1. Use {module_path} for automatic path joining (hardcodes / separator)
2. Use position field with {module} for custom separators

Fixes #20474, #20467, #19796
Mark Coleman 3 недель назад
Родитель
Сommit
702b1f8210

+ 35 - 21
netbox/dcim/constants.py

@@ -79,40 +79,54 @@ NONCONNECTABLE_IFACE_TYPES = VIRTUAL_IFACE_TYPES + WIRELESS_IFACE_TYPES
 #
 
 MODULE_TOKEN = '{module}'
+MODULE_PATH_TOKEN = '{module_path}'
 MODULE_TOKEN_SEPARATOR = '/'
 
 
-def resolve_module_token(text, positions):
+def resolve_module_placeholders(text, positions):
     """
-    Substitute {module} tokens in text with position values.
+    Substitute {module} and {module_path} placeholders in text with position values.
 
     Args:
-        text: String potentially containing {module} tokens
+        text: String potentially containing {module} or {module_path} placeholders
         positions: List of position strings from the module tree (root to leaf)
 
     Returns:
-        Text with {module} tokens replaced according to these rules:
-        - Single token: replaced with full path (positions joined by MODULE_TOKEN_SEPARATOR)
-        - Multiple tokens: replaced level-by-level (first token gets first position, etc.)
+        Text with placeholders replaced according to these rules:
 
-    This centralizes the substitution logic used by both ModuleCommonForm.clean()
-    and ModularComponentTemplateModel.resolve_*() methods.
+        {module_path}: Always expands to full path (positions joined by MODULE_TOKEN_SEPARATOR).
+                       Can only appear once in the text.
+
+        {module}: If used once, expands to the PARENT module bay position only (last in positions).
+                  If used multiple times, each token is replaced level-by-level.
+
+    This design (Option 2 per sigprof's feedback) allows two approaches:
+    1. Use {module_path} for automatic full-path expansion (hardcodes '/' separator)
+    2. Use {module} in position fields to build custom paths with user-controlled separators
     """
-    if not text or MODULE_TOKEN not in text:
+    if not text:
         return text
 
-    token_count = text.count(MODULE_TOKEN)
-
-    if token_count == 1:
-        # Single token: substitute with full path (e.g., "1/1" for depth 2)
-        full_path = MODULE_TOKEN_SEPARATOR.join(positions)
-        return text.replace(MODULE_TOKEN, full_path, 1)
-    else:
-        # Multiple tokens: substitute level-by-level (existing behavior)
-        result = text
-        for pos in positions:
-            result = result.replace(MODULE_TOKEN, pos, 1)
-        return result
+    result = text
+
+    # Handle {module_path} - always expands to full path
+    if MODULE_PATH_TOKEN in result:
+        full_path = MODULE_TOKEN_SEPARATOR.join(positions) if positions else ''
+        result = result.replace(MODULE_PATH_TOKEN, full_path)
+
+    # Handle {module} - parent-only for single token, level-by-level for multiple
+    if MODULE_TOKEN in result:
+        token_count = result.count(MODULE_TOKEN)
+        if token_count == 1 and positions:
+            # Single {module}: substitute with parent (immediate) bay position only
+            parent_position = positions[-1] if positions else ''
+            result = result.replace(MODULE_TOKEN, parent_position, 1)
+        else:
+            # Multiple {module}: substitute level-by-level (existing behavior)
+            for pos in positions:
+                result = result.replace(MODULE_TOKEN, pos, 1)
+
+    return result
 
 
 MODULAR_COMPONENT_TEMPLATE_MODELS = Q(

+ 26 - 14
netbox/dcim/forms/common.py

@@ -119,29 +119,41 @@ class ModuleCommonForm(forms.Form):
             # Get the templates for the module type.
             for template in getattr(module_type, templates).all():
                 resolved_name = template.name
+                has_module_token = MODULE_TOKEN in template.name
+                has_module_path_token = MODULE_PATH_TOKEN in template.name
+
                 # Installing modules with placeholders require that the bay has a position value
-                if MODULE_TOKEN in template.name:
+                if has_module_token or has_module_path_token:
                     if not module_bay.position:
                         raise forms.ValidationError(
                             _("Cannot install module with placeholder values in a module bay with no position defined.")
                         )
 
-                    token_count = template.name.count(MODULE_TOKEN)
-                    # A single token which gets expanded to the full path is always
-                    # allowed; otherwise the number of tokens needs to match the path length.
-                    if token_count != 1 and token_count != len(module_bays):
-                        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=token_count
+                    # Validate {module_path} - can only appear once
+                    if has_module_path_token:
+                        path_token_count = template.name.count(MODULE_PATH_TOKEN)
+                        if path_token_count > 1:
+                            raise forms.ValidationError(
+                                _("The {module_path} placeholder can only be used once per template.")
+                            )
+
+                    # Validate {module} - multi-token must match depth exactly
+                    if has_module_token:
+                        token_count = template.name.count(MODULE_TOKEN)
+                        # Multiple {module} tokens must match the tree depth exactly
+                        if token_count > 1 and token_count != len(module_bays):
+                            raise forms.ValidationError(
+                                _(
+                                    "Cannot install module with placeholder values in a module bay tree {level} deep "
+                                    "but {tokens} placeholders given."
+                                ).format(
+                                    level=len(module_bays), tokens=token_count
+                                )
                             )
-                        )
 
-                    # Use centralized helper for token substitution
+                    # Use centralized helper for placeholder substitution
                     positions = [mb.position for mb in module_bays]
-                    resolved_name = resolve_module_token(resolved_name, positions)
+                    resolved_name = resolve_module_placeholders(resolved_name, positions)
 
                 existing_item = installed_components.get(resolved_name)
 

+ 5 - 5
netbox/dcim/models/device_component_templates.py

@@ -170,17 +170,17 @@ class ModularComponentTemplateModel(ComponentTemplateModel):
         return modules
 
     def resolve_name(self, module):
-        """Resolve {module} placeholder(s) in component name."""
+        """Resolve {module} and {module_path} placeholders in component name."""
         if module:
             positions = [m.module_bay.position for m in self._get_module_tree(module)]
-            return resolve_module_token(self.name, positions)
+            return resolve_module_placeholders(self.name, positions)
         return self.name
 
     def resolve_label(self, module):
-        """Resolve {module} placeholder(s) in component label."""
+        """Resolve {module} and {module_path} placeholders in component label."""
         if module:
             positions = [m.module_bay.position for m in self._get_module_tree(module)]
-            return resolve_module_token(self.label, positions)
+            return resolve_module_placeholders(self.label, positions)
         return self.label
 
     def resolve_position(self, position, module):
@@ -194,7 +194,7 @@ class ModularComponentTemplateModel(ComponentTemplateModel):
         """
         if module:
             positions = [m.module_bay.position for m in self._get_module_tree(module)]
-            return resolve_module_token(position, positions)
+            return resolve_module_placeholders(position, positions)
         return position
 
 

+ 382 - 11
netbox/dcim/tests/test_models.py

@@ -877,9 +877,11 @@ class ModuleBayTestCase(TestCase):
 
     def test_nested_module_single_placeholder_full_path(self):
         """
-        Test that installing a module at depth=2 with a single {module} placeholder
+        Test that installing a module at depth=2 with a {module_path} placeholder
         in the interface template name resolves to the full path (e.g., "1/1").
         Regression test for transceiver modeling use case.
+
+        Updated for Option 2: Use {module_path} for full path, {module} for parent-only.
         """
         manufacturer = Manufacturer.objects.first()
         site = Site.objects.first()
@@ -915,15 +917,15 @@ class ModuleBayTestCase(TestCase):
             position='2'
         )
 
-        # Create SFP module type with interface using single {module} placeholder
+        # Create SFP module type with interface using {module_path} for full path
         sfp_type = ModuleType.objects.create(
             manufacturer=manufacturer,
             model='SFP Transceiver'
         )
         InterfaceTemplate.objects.create(
             module_type=sfp_type,
-            name='SFP {module}',
-            label='{module}',
+            name='SFP {module_path}',
+            label='{module_path}',
             type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS
         )
 
@@ -1377,15 +1379,15 @@ class ModuleBayTestCase(TestCase):
             position='2'
         )
 
-        # Create leaf module type with single-token name AND label
+        # Create leaf module type with {module_path} for full path in name AND label
         leaf_type = ModuleType.objects.create(
             manufacturer=manufacturer,
             model='T4 Leaf'
         )
         InterfaceTemplate.objects.create(
             module_type=leaf_type,
-            name='SFP {module}',
-            label='LBL {module}',
+            name='SFP {module_path}',
+            label='LBL {module_path}',
             type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS
         )
 
@@ -1455,8 +1457,8 @@ class ModuleBayTestCase(TestCase):
         )
         ConsolePortTemplate.objects.create(
             module_type=leaf_type,
-            name='Console {module}',
-            label='{module}'
+            name='Console {module_path}',
+            label='{module_path}'
         )
 
         # Create device and install modules
@@ -1518,14 +1520,14 @@ class ModuleBayTestCase(TestCase):
             position='2'
         )
 
-        # Create leaf module type with single-token interface template
+        # Create leaf module type with {module_path} for full path
         leaf_type = ModuleType.objects.create(
             manufacturer=manufacturer,
             model='T6 Leaf'
         )
         InterfaceTemplate.objects.create(
             module_type=leaf_type,
-            name='Gi{module}',
+            name='Gi{module_path}',
             type=InterfaceTypeChoices.TYPE_1GE_FIXED
         )
 
@@ -1674,6 +1676,375 @@ class ModuleBayTestCase(TestCase):
         interface = leaf_module.interfaces.first()
         self.assertEqual(interface.name, 'X1-Y2')
 
+    #
+    # Option 2 Tests: {module_path} for full path, {module} for parent-only
+    #
+
+    def test_module_path_placeholder_depth_1(self):
+        """
+        Test {module_path} at depth 1 resolves to just the bay position.
+        """
+        manufacturer = Manufacturer.objects.first()
+        site = Site.objects.first()
+        device_role = DeviceRole.objects.first()
+
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Depth1 Chassis',
+            slug='mp-depth1-chassis'
+        )
+        ModuleBayTemplate.objects.create(
+            device_type=device_type,
+            name='Bay 1',
+            position='A'
+        )
+
+        module_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Depth1 Module'
+        )
+        InterfaceTemplate.objects.create(
+            module_type=module_type,
+            name='Port {module_path}',
+            type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS
+        )
+
+        device = Device.objects.create(
+            name='MP Depth1 Device',
+            device_type=device_type,
+            role=device_role,
+            site=site
+        )
+
+        bay = device.modulebays.get(name='Bay 1')
+        module = Module.objects.create(
+            device=device,
+            module_bay=bay,
+            module_type=module_type
+        )
+
+        interface = module.interfaces.first()
+        self.assertEqual(interface.name, 'Port A')
+
+    def test_module_path_placeholder_depth_2(self):
+        """
+        Test {module_path} at depth 2 resolves to full path (e.g., "1/1").
+        This is the key use case for SFP/transceiver modeling.
+        """
+        manufacturer = Manufacturer.objects.first()
+        site = Site.objects.first()
+        device_role = DeviceRole.objects.first()
+
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Depth2 Chassis',
+            slug='mp-depth2-chassis'
+        )
+        ModuleBayTemplate.objects.create(
+            device_type=device_type,
+            name='Line Card Bay',
+            position='1'
+        )
+
+        line_card_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Line Card'
+        )
+        ModuleBayTemplate.objects.create(
+            module_type=line_card_type,
+            name='SFP Bay',
+            position='2'
+        )
+
+        sfp_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='MP SFP'
+        )
+        InterfaceTemplate.objects.create(
+            module_type=sfp_type,
+            name='SFP {module_path}',
+            label='{module_path}',
+            type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS
+        )
+
+        device = Device.objects.create(
+            name='MP Depth2 Device',
+            device_type=device_type,
+            role=device_role,
+            site=site
+        )
+
+        # Install line card
+        lc_bay = device.modulebays.get(name='Line Card Bay')
+        line_card = Module.objects.create(
+            device=device,
+            module_bay=lc_bay,
+            module_type=line_card_type
+        )
+
+        # Install SFP in nested bay
+        sfp_bay = line_card.modulebays.get(name='SFP Bay')
+        sfp = Module.objects.create(
+            device=device,
+            module_bay=sfp_bay,
+            module_type=sfp_type
+        )
+
+        interface = sfp.interfaces.first()
+        # {module_path} should give full path: 1/2
+        self.assertEqual(interface.name, 'SFP 1/2')
+        self.assertEqual(interface.label, '1/2')
+
+    def test_module_path_placeholder_depth_3(self):
+        """
+        Test {module_path} at depth 3 resolves to full path (e.g., "1/2/3").
+        """
+        manufacturer = Manufacturer.objects.first()
+        site = Site.objects.first()
+        device_role = DeviceRole.objects.first()
+
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Depth3 Chassis',
+            slug='mp-depth3-chassis'
+        )
+        ModuleBayTemplate.objects.create(
+            device_type=device_type,
+            name='Slot',
+            position='1'
+        )
+
+        level2_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Level2'
+        )
+        ModuleBayTemplate.objects.create(
+            module_type=level2_type,
+            name='SubSlot',
+            position='2'
+        )
+
+        level3_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Level3'
+        )
+        ModuleBayTemplate.objects.create(
+            module_type=level3_type,
+            name='Port',
+            position='3'
+        )
+
+        leaf_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Leaf'
+        )
+        InterfaceTemplate.objects.create(
+            module_type=leaf_type,
+            name='Interface {module_path}',
+            type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS
+        )
+
+        device = Device.objects.create(
+            name='MP Depth3 Device',
+            device_type=device_type,
+            role=device_role,
+            site=site
+        )
+
+        # Install 3 levels
+        bay1 = device.modulebays.get(name='Slot')
+        mod1 = Module.objects.create(device=device, module_bay=bay1, module_type=level2_type)
+
+        bay2 = mod1.modulebays.get(name='SubSlot')
+        mod2 = Module.objects.create(device=device, module_bay=bay2, module_type=level3_type)
+
+        bay3 = mod2.modulebays.get(name='Port')
+        mod3 = Module.objects.create(device=device, module_bay=bay3, module_type=leaf_type)
+
+        interface = mod3.interfaces.first()
+        # {module_path} should give full path: 1/2/3
+        self.assertEqual(interface.name, 'Interface 1/2/3')
+
+    def test_single_module_placeholder_parent_only_depth_2(self):
+        """
+        Test that single {module} at depth 2 resolves to PARENT position only,
+        not the full path. This is the key difference from {module_path}.
+        """
+        manufacturer = Manufacturer.objects.first()
+        site = Site.objects.first()
+        device_role = DeviceRole.objects.first()
+
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer,
+            model='Parent Only Chassis',
+            slug='parent-only-chassis'
+        )
+        ModuleBayTemplate.objects.create(
+            device_type=device_type,
+            name='Bay 1',
+            position='X'
+        )
+
+        line_card_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='Parent Only Line Card'
+        )
+        ModuleBayTemplate.objects.create(
+            module_type=line_card_type,
+            name='Nested Bay',
+            position='Y'
+        )
+
+        leaf_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='Parent Only Leaf'
+        )
+        InterfaceTemplate.objects.create(
+            module_type=leaf_type,
+            name='Port {module}',
+            type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS
+        )
+
+        device = Device.objects.create(
+            name='Parent Only Device',
+            device_type=device_type,
+            role=device_role,
+            site=site
+        )
+
+        bay1 = device.modulebays.get(name='Bay 1')
+        line_card = Module.objects.create(device=device, module_bay=bay1, module_type=line_card_type)
+
+        bay2 = line_card.modulebays.get(name='Nested Bay')
+        leaf = Module.objects.create(device=device, module_bay=bay2, module_type=leaf_type)
+
+        interface = leaf.interfaces.first()
+        # Single {module} should give PARENT position only: Y (not X/Y)
+        self.assertEqual(interface.name, 'Port Y')
+
+    def test_sigprof_nested_position_no_duplication(self):
+        """
+        Test sigprof's scenario: position field uses {module} to build path,
+        then child module uses {module} which should NOT cause duplication.
+
+        Scenario:
+        - device
+          - module bay (position=`2`)
+            - extension module
+              - module bay (position=`{module}/1` → `2/1`)
+                - tape drive (name=`Drive {module}`)
+                  → Should be `Drive 2/1`, NOT `Drive 2/2/1`
+        """
+        manufacturer = Manufacturer.objects.first()
+        site = Site.objects.first()
+        device_role = DeviceRole.objects.first()
+
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer,
+            model='Sigprof Chassis',
+            slug='sigprof-chassis'
+        )
+        ModuleBayTemplate.objects.create(
+            device_type=device_type,
+            name='Extension Slot',
+            position='2'
+        )
+
+        extension_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='Sigprof Extension'
+        )
+        ModuleBayTemplate.objects.create(
+            module_type=extension_type,
+            name='Drive Bay',
+            position='{module}/1'  # Should resolve to 2/1
+        )
+
+        drive_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='Sigprof Drive'
+        )
+        InterfaceTemplate.objects.create(
+            module_type=drive_type,
+            name='Drive {module}',
+            type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS
+        )
+
+        device = Device.objects.create(
+            name='Sigprof Device',
+            device_type=device_type,
+            role=device_role,
+            site=site
+        )
+
+        # Install extension module
+        ext_bay = device.modulebays.get(name='Extension Slot')
+        extension = Module.objects.create(device=device, module_bay=ext_bay, module_type=extension_type)
+
+        # Verify nested bay position resolved correctly
+        drive_bay = extension.modulebays.get(name='Drive Bay')
+        self.assertEqual(drive_bay.position, '2/1')
+
+        # Install drive module
+        drive = Module.objects.create(device=device, module_bay=drive_bay, module_type=drive_type)
+
+        # Single {module} should give parent bay position: 2/1 (NOT 2/2/1)
+        interface = drive.interfaces.first()
+        self.assertEqual(interface.name, 'Drive 2/1')
+
+    def test_module_path_validation_only_once(self):
+        """
+        Test that {module_path} can only appear once in a template.
+        Using it multiple times should fail validation.
+        """
+        from dcim.forms import ModuleForm
+
+        manufacturer = Manufacturer.objects.first()
+        site = Site.objects.first()
+        device_role = DeviceRole.objects.first()
+
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer,
+            model='MP Validation Chassis',
+            slug='mp-validation-chassis'
+        )
+        ModuleBayTemplate.objects.create(
+            device_type=device_type,
+            name='Bay 1',
+            position='1'
+        )
+
+        # Module type with {module_path} used twice - should fail
+        bad_module_type = ModuleType.objects.create(
+            manufacturer=manufacturer,
+            model='Bad Module'
+        )
+        InterfaceTemplate.objects.create(
+            module_type=bad_module_type,
+            name='{module_path}/{module_path}',
+            type=InterfaceTypeChoices.TYPE_10GE_SFP_PLUS
+        )
+
+        device = Device.objects.create(
+            name='MP Validation Device',
+            device_type=device_type,
+            role=device_role,
+            site=site
+        )
+
+        bay = device.modulebays.get(name='Bay 1')
+
+        form = ModuleForm(data={
+            'device': device.pk,
+            'module_bay': bay.pk,
+            'module_type': bad_module_type.pk,
+            'status': 'active',
+            'replicate_components': True,
+            'adopt_components': False,
+        })
+
+        self.assertFalse(form.is_valid())
+
 
 class CableTestCase(TestCase):