fix: Validate template component names #333
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideAdds validation for cms_component template tag names to ensure they are string Python identifiers and extends tests to cover valid and invalid component names, improving error clarity and robustness of template component usage. Flow diagram for cms_component template tag validationflowchart TD
A[start cms_component_tag] --> B[check _cms_components in context]
B -->|not present| C[return empty string]
B -->|present| D[check len args == 1]
D -->|no| E[raise ValueError: requires exactly one positional argument]
D -->|yes| F[check isinstance args_0 str]
F -->|no| G[raise ValueError: component name must be a string]
F -->|yes| H[check args_0 isidentifier]
H -->|no| I[raise ValueError: component name must be a valid Python identifier]
H -->|yes| J["append (args, kwargs) to context _cms_components cms_component"]
J --> K[return empty string]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
==========================================
+ Coverage 89.00% 89.01% +0.01%
==========================================
Files 124 124
Lines 3383 3387 +4
Branches 287 289 +2
==========================================
+ Hits 3011 3015 +4
Misses 256 256
Partials 116 116 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
cms_component, consider extracting the argument validation (arity check, type check, identifier check) into a small helper function so that the tag implementation remains focused on registering the component and the validation logic is easier to reuse or adjust. - In
test_cms_component_invalid_identifier, using the sameContextinstance for both the successful and failing calls couples the invalid-name checks to the side effects of the first call; consider using a freshContextfor each assertion or resetting_cms_componentsbetween checks to keep each case independent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `cms_component`, consider extracting the argument validation (arity check, type check, identifier check) into a small helper function so that the tag implementation remains focused on registering the component and the validation logic is easier to reuse or adjust.
- In `test_cms_component_invalid_identifier`, using the same `Context` instance for both the successful and failing calls couples the invalid-name checks to the side effects of the first call; consider using a fresh `Context` for each assertion or resetting `_cms_components` between checks to keep each case independent.
## Individual Comments
### Comment 1
<location> `djangocms_frontend/templatetags/cms_component.py:33-36` </location>
<code_context>
if "_cms_components" in context:
if len(args) != 1: # pragma: no cover
raise ValueError("The cms_component tag requires exactly one positional argument: the component name.")
+ if not isinstance(args[0], str):
+ raise ValueError("The component name must be a string.")
+ if not args[0].isidentifier():
+ raise ValueError("The component name must be a valid Python identifier.")
context["_cms_components"]["cms_component"].append((args, kwargs))
return ""
</code_context>
<issue_to_address>
**suggestion:** Reconsider coupling component-name validation to Python identifiers via `isidentifier()`.
`str.isidentifier()` applies Python identifier rules (no hyphens, dots, spaces, etc.), which may be stricter than intended for template component names. If you just need to reject clearly invalid values (empty/whitespace/unsafe characters), a dedicated check (e.g., regex or explicit allowed character set) would give more control and avoid rejecting names like `hero-banner` or `card.1`. It’d be good to confirm the expected naming convention and align validation with that instead of Python identifier semantics.
Suggested implementation:
```python
import re
from django import template
```
```python
register = template.Library()
# Allow simple, URL‑style component names like "hero-banner" or "card.1".
# Restrict to a safe, predictable character set: letters, digits, underscore, hyphen, and dot.
COMPONENT_NAME_RE = re.compile(r"[A-Za-z0-9_.-]+")
```
```python
if not isinstance(args[0], str):
raise ValueError("The component name must be a string.")
if not COMPONENT_NAME_RE.fullmatch(args[0]):
raise ValueError(
"The component name may only contain letters, numbers, hyphens (-), underscores (_), and dots (.)."
)
```
</issue_to_address>
### Comment 2
<location> `tests/test_autocomponent.py:110-115` </location>
<code_context>
with self.assertRaises(TemplateSyntaxError):
Template(invalid_template)
+ def test_cms_component_invalid_identifier(self):
+ # Test that cms_component tag raises ValueError for invalid identifiers
+ from django.template import Context
+ from djangocms_frontend.templatetags.cms_component import cms_component
+
+ context = Context({"_cms_components": {"cms_component": []}})
+
+ # Valid identifier should work
</code_context>
<issue_to_address>
**issue (testing):** Add explicit tests for non-string component names as mentioned in the PR description.
This test only exercises invalid string identifiers, but the fix also covers non-string component names. Please add cases like `cms_component(context, 123)`, `cms_component(context, None)`, and a custom object, each asserting that a `ValueError` is raised with the expected "component name must be a string" message.
</issue_to_address>
### Comment 3
<location> `docs/source/tutorial/template_components.rst:14-15` </location>
<code_context>
.. versionadded:: 2.1
-**Template components** are the easiest approach to creating or porting your own custom
+**Template components** are the easiest approach to creating or porting your own custom
frontend components, allowing you to define custom components **using django templates,
-without needing to write any Python code**.
+without needing to write any Python code**.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider capitalizing "Django" when referring to the framework.
Here, please capitalize “Django” to match the framework’s proper name and align with standard documentation style.
```suggestion
frontend components, allowing you to define custom components **using Django templates,
without needing to write any Python code**.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fsbraun
commented
Dec 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Validate cms_component template names and document their constraints.
Bug Fixes:
Documentation:
Tests: