Compare commits

...

7 Commits

Author SHA1 Message Date
Mark Coleman
3680b0ccd4 Refactor: move resolve_module_placeholders from constants.py to utils.py
Some checks are pending
CI / build (20.x, 3.12) (push) Waiting to run
CI / build (20.x, 3.13) (push) Waiting to run
CI / build (20.x, 3.14) (push) Waiting to run
Constants should only contain constant values, not functions with logic.
The helper function now lives in dcim/utils.py alongside other utilities
like update_interface_bridges and create_port_mappings.
2026-01-19 19:06:42 +01:00
Mark Coleman
702b1f8210 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
2026-01-19 18:20:59 +01:00
Mark Coleman
1c6adc40b3 Refactor: centralize module token substitution logic
Per sigprof's review feedback, extract the duplicated token substitution
logic into a single resolve_module_token() helper in constants.py.

This addresses two review comments:
1. Duplication between ModuleCommonForm.clean() and resolve_name()
2. Duplication between resolve_name() and resolve_label()

Benefits:
- Single source of truth for substitution logic
- MODULE_TOKEN_SEPARATOR constant for future configurability
- Cleaner, more maintainable code (-7 net lines)
- Easier to modify separator handling in one place
2026-01-19 16:18:54 +01:00
Mark Coleman
bcd3851f4e Address sigprof review: stricter token validation
Per sigprof's feedback, the previous validation (depth >= token_count)
allowed a questionable case where token_count > 1 but < depth, which
would lose position information for some levels.

New validation: token_count must be either 1 (full path expansion) or
exactly match the tree depth (level-by-level substitution).

Updated test T2 to verify this mismatched case is now rejected.
2026-01-19 16:18:54 +01:00
Mark Coleman
850bfba9e4 Fix PEP8: remove trailing whitespace from blank lines 2026-01-19 16:18:54 +01:00
Mark Coleman
1df6eee467 Add position field resolution for module bays (fixes #20467)
- Add resolve_position() method to ModularComponentTemplateModel
- Update ModuleBayTemplate.instantiate() to resolve {module} in position field
- Add test_module_bay_position_resolves_placeholder test

This completes the fix for nested module placeholder issues by ensuring
the position field also resolves {module} placeholders, which is required
for building correct full paths in 3+ level hierarchies.
2026-01-19 16:18:54 +01:00
Mark Coleman
e613b55ada Fix nested module bay placeholder: single {module} resolves to full path (e.g., 1/1) 2026-01-19 16:18:54 +01:00
5 changed files with 1273 additions and 30 deletions

View File

@@ -79,6 +79,8 @@ NONCONNECTABLE_IFACE_TYPES = VIRTUAL_IFACE_TYPES + WIRELESS_IFACE_TYPES
#
MODULE_TOKEN = '{module}'
MODULE_PATH_TOKEN = '{module_path}'
MODULE_TOKEN_SEPARATOR = '/'
MODULAR_COMPONENT_TEMPLATE_MODELS = Q(
app_label='dcim',

View File

@@ -3,6 +3,7 @@ from django.utils.translation import gettext_lazy as _
from dcim.choices import *
from dcim.constants import *
from dcim.utils import resolve_module_placeholders
from utilities.forms import get_field_value
__all__ = (
@@ -119,25 +120,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.")
)
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)
# 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.")
)
)
for module_bay in module_bays:
resolved_name = resolved_name.replace(MODULE_TOKEN, module_bay.position, 1)
# 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 placeholder substitution
positions = [mb.position for mb in module_bays]
resolved_name = resolve_module_placeholders(resolved_name, positions)
existing_item = installed_components.get(resolved_name)

View File

@@ -8,6 +8,7 @@ from mptt.models import MPTTModel, TreeForeignKey
from dcim.choices import *
from dcim.constants import *
from dcim.models.base import PortMappingBase
from dcim.utils import resolve_module_placeholders
from dcim.models.mixins import InterfaceValidationMixin
from netbox.models import ChangeLoggedModel
from utilities.fields import ColorField, NaturalOrderingField
@@ -170,29 +171,33 @@ class ModularComponentTemplateModel(ComponentTemplateModel):
return modules
def resolve_name(self, module):
if MODULE_TOKEN not in self.name:
return self.name
"""Resolve {module} and {module_path} placeholders in component name."""
if module:
modules = self._get_module_tree(module)
name = self.name
for module in modules:
name = name.replace(MODULE_TOKEN, module.module_bay.position, 1)
return name
positions = [m.module_bay.position for m in self._get_module_tree(module)]
return resolve_module_placeholders(self.name, positions)
return self.name
def resolve_label(self, module):
if MODULE_TOKEN not in self.label:
return self.label
"""Resolve {module} and {module_path} placeholders in component label."""
if module:
modules = self._get_module_tree(module)
label = self.label
for module in modules:
label = label.replace(MODULE_TOKEN, module.module_bay.position, 1)
return label
positions = [m.module_bay.position for m in self._get_module_tree(module)]
return resolve_module_placeholders(self.label, positions)
return self.label
def resolve_position(self, position, module):
"""
Resolve {module} placeholder in position field.
This is used by ModuleBayTemplate to resolve positions like "{module}/1"
to actual values like "A/1" when the parent module is installed in bay "A".
Fixes Issue #20467.
"""
if module:
positions = [m.module_bay.position for m in self._get_module_tree(module)]
return resolve_module_placeholders(position, positions)
return position
class ConsolePortTemplate(ModularComponentTemplateModel):
"""
@@ -722,10 +727,11 @@ class ModuleBayTemplate(ModularComponentTemplateModel):
verbose_name_plural = _('module bay templates')
def instantiate(self, **kwargs):
module = kwargs.get('module')
return self.component_model(
name=self.resolve_name(kwargs.get('module')),
label=self.resolve_label(kwargs.get('module')),
position=self.position,
name=self.resolve_name(module),
label=self.resolve_label(module),
position=self.resolve_position(self.position, module),
**kwargs
)
instantiate.do_not_call_in_templates = True

File diff suppressed because it is too large Load Diff

View File

@@ -4,6 +4,54 @@ from django.apps import apps
from django.contrib.contenttypes.models import ContentType
from django.db import router, transaction
from dcim.constants import MODULE_PATH_TOKEN, MODULE_TOKEN, MODULE_TOKEN_SEPARATOR
def resolve_module_placeholders(text, positions):
"""
Substitute {module} and {module_path} placeholders in text with position values.
Args:
text: String potentially containing {module} or {module_path} placeholders
positions: List of position strings from the module tree (root to leaf)
Returns:
Text with placeholders replaced according to these rules:
{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:
return text
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
def compile_path_node(ct_id, object_id):
return f'{ct_id}:{object_id}'