refactor: remove use of eval for node validation (#944)

src/django_components/node.py

@@ -10,8 +10,6 @@ from django_components.util.logger import trace_node_msg
 from django_components.util.misc import gen_id
 from django_components.util.template_tag import (
     TagAttr,
-    TagParam,
-    apply_params_in_original_order,
     parse_template_tag,
     resolve_params,
     validate_params,
@@ -159,8 +157,7 @@ class NodeMeta(type):
 
             # Validate the params against the signature
             #
-            # Unlike the call to `apply_params_in_original_order()` further below, this uses a signature
-            # that has been stripped of the `self` and `context` parameters. E.g.
+            # This uses a signature that has been stripped of the `self` and `context` parameters. E.g.
             #
             # `def render(name: str, **kwargs: Any) -> None`
             #
@@ -180,21 +177,14 @@ class NodeMeta(type):
             #
             # But cause we stripped the two parameters, then the error will be:
             # `render() takes from 1 positional arguments but 2 were given`
-            validate_params(self.tag, validation_signature, resolved_params_without_invalid_kwargs, invalid_kwargs)
+            args, kwargs = validate_params(
+                self.tag,
+                validation_signature,
+                resolved_params_without_invalid_kwargs,
+                invalid_kwargs,
+            )
 
-            # The code below calls the `orig_render()` function like so:
-            # `orig_render(self, context, arg1, arg2, kwarg1=val1, kwarg2=val2)`
-            #
-            # So it's called in the same order as what was passed to the template tag, e.g.
-            # `{% component arg1 arg2 kwarg1=val1 kwarg2=val2 %}`
-            #
-            # That's why we don't simply spread all args and kwargs as `*args, **kwargs`,
-            # because then Python's validation wouldn't catch such errors.
-            resolved_params_with_context = [
-                TagParam(key=None, value=self),
-                TagParam(key=None, value=context),
-            ] + resolved_params_without_invalid_kwargs
-            output = apply_params_in_original_order(orig_render, resolved_params_with_context, invalid_kwargs)
+            output = orig_render(self, context, *args, **kwargs)
 
             trace_node_msg("RENDER", self.tag, self.node_id, msg="...Done!")
             return output

src/django_components/util/template_tag.py

@@ -4,9 +4,8 @@ This file is for logic that focuses on transforming the AST of template tags
 """
 
 import inspect
-import sys
 from dataclasses import dataclass
-from typing import Any, Callable, Dict, Iterable, List, Mapping, NamedTuple, Optional, Set, Tuple, Union
+from typing import Any, Callable, Dict, Iterable, List, Mapping, NamedTuple, Optional, Set, Tuple
 
 from django.template import Context, NodeList
 from django.template.base import Parser, Token
@@ -23,7 +22,7 @@ def validate_params(
     signature: inspect.Signature,
     params: List["TagParam"],
     extra_kwargs: Optional[Dict[str, Any]] = None,
-) -> None:
+) -> Tuple[Tuple[Any, ...], Dict[str, Any]]:
     """
     Validates a list of TagParam objects against this tag's function signature.
 
@@ -33,17 +32,16 @@ def validate_params(
     """
 
     # Create a function that uses the given signature
-    def validator(*args: Any, **kwargs: Any) -> None:
-        # Let Python do the signature validation
-        bound = signature.bind(*args, **kwargs)
-        bound.apply_defaults()
+    def validator(*args: Any, **kwargs: Any) -> Tuple[Tuple[Any, ...], Dict[str, Any]]:
+        return args, kwargs
 
     validator.__signature__ = signature  # type: ignore[attr-defined]
 
     # Call the validator with our args and kwargs in the same order as they appeared
     # in the template, to let the Python interpreter validate on repeated kwargs.
     try:
-        apply_params_in_original_order(validator, params, extra_kwargs)
+        # Returns args, kwargs
+        return apply_params_in_original_order(validator, params, extra_kwargs)
     except TypeError as e:
         raise TypeError(f"Invalid parameters for tag '{tag}': {str(e)}") from None
 
@@ -244,82 +242,101 @@ def apply_params_in_original_order(
     {% component key1=value1 arg1 arg2 key2=value2 key3=value3 %}
     ```
 
-    Then `apply_params_in_original_order()` will call the `fn` like this:
-    ```
-    component(
-        key1=call_params[0], # kwarg 1
-        call_params[1], # arg 1
-        call_params[2], # arg 2
-        key2=call_params[3], # kwarg 2
-        key3=call_params[4], # kwarg 3
-        ...
-        **extra_kwargs,
-    )
-    ```
-
-    This way, this will be effectively the same as:
+    Then it will be as if the given function was called like this:
 
     ```python
-    component(key1=value1, arg1, arg2, key2=value2, key3=value3, ..., **extra_kwargs)
+    fn(key1=value1, arg1, arg2, key2=value2, key3=value3)
     ```
 
-    The problem this works around is that, dynamically, args and kwargs in Python
-    can be passed only with `*args` and `**kwargs`. But in such case, we're already
-    grouping all args and kwargs, which may not represent the original order of the params
-    as they appeared in the template tag.
+    This function validates that the template tag's parameters match the function's signature
+    and follow Python's function calling conventions. It will raise appropriate TypeError exceptions
+    for invalid parameter combinations, such as:
+    - Too few/many arguments (for non-variadic functions)
+    - Duplicate keyword arguments
+    - Mixed positional/keyword argument errors
+    - Positional args after kwargs
 
-    If you need to pass kwargs that are not valid Python identifiers, e.g. `data-id`, `class`, `:href`,
-    you can pass them in via `extra_kwargs`. These kwargs will be exempt from the validation, and will be
-    passed to the function as a dictionary spread.
+    Returns the result of calling fn with the validated parameters
     """
-    # Generate a script like so:
-    # ```py
-    # component(
-    #     key1=call_params[0],
-    #     call_params[1],
-    #     call_params[2],
-    #     key2=call_params[3],
-    #     key3=call_params[4],
-    #     ...
-    #     **extra_kwargs,
-    # )
-    # ```
-    #
-    # NOTE: Instead of grouping params into args and kwargs, we preserve the original order
-    #       of the params as they appeared in the template.
-    #
-    # NOTE: Because we use `eval()` here, we can't trust neither the param keys nor values.
-    #       So we MUST NOT reference them directly in the exec script, otherwise we'd be at risk
-    #       of injection attack.
-    #
-    #       Currently, the use of `eval()` is safe, because we control the input:
-    #       - List with indices is used so that we don't have to reference directly or try to print the values.
-    #         and instead refer to them as `call_params[0]`, `call_params[1]`, etc.
-    #       - List indices are safe, because we generate them.
-    #       - Kwarg names come from the user. But Python expects the kwargs to be valid identifiers.
-    #         So if a key is not a valid identifier, we'll raise an error. Before passing it to `eval()`
-    validator_call_script = "fn("
-    call_params: List[Union[List, Dict]] = []
-    for index, param in enumerate(params):
-        call_params.append(param.value)
+    signature = inspect.signature(fn)
+
+    # Track state as we process parameters
+    seen_kwargs = False  # To detect positional args after kwargs
+    used_param_names = set()  # To detect duplicate kwargs
+    validated_args = []
+    validated_kwargs = {}
+
+    # Get list of valid parameter names and analyze signature
+    params_by_name = signature.parameters
+    valid_params = list(params_by_name.keys())
+
+    # Check if function accepts variable arguments (*args, **kwargs)
+    has_var_positional = any(param.kind == inspect.Parameter.VAR_POSITIONAL for param in params_by_name.values())
+    has_var_keyword = any(param.kind == inspect.Parameter.VAR_KEYWORD for param in params_by_name.values())
+
+    # Find the last positional parameter index (excluding *args)
+    max_positional_index = 0
+    for i, signature_param in enumerate(params_by_name.values()):
+        if signature_param.kind in (
+            inspect.Parameter.POSITIONAL_ONLY,
+            inspect.Parameter.POSITIONAL_OR_KEYWORD,
+        ):
+            max_positional_index = i + 1
+        elif signature_param.kind == inspect.Parameter.VAR_POSITIONAL:
+            # Don't count *args in max_positional_index
+            break
+        # Parameter.KEYWORD_ONLY
+        # Parameter.VAR_KEYWORD
+        else:
+            break
+
+    next_positional_index = 0
+
+    # Process parameters in their original order
+    for param in params:
         if param.key is None:
-            validator_call_script += f"call_params[{index}], "
+            # This is a positional argument
+            if seen_kwargs:
+                raise TypeError("positional argument follows keyword argument")
+
+            # Only check position limit for non-variadic functions
+            if not has_var_positional and next_positional_index >= max_positional_index:
+                if max_positional_index == 0:
+                    raise TypeError(f"takes 0 positional arguments but {next_positional_index + 1} was given")
+                raise TypeError(f"takes {max_positional_index} positional argument(s) but more were given")
+
+            # For non-variadic arguments, get the parameter name this maps to
+            if next_positional_index < max_positional_index:
+                param_name = valid_params[next_positional_index]
+                # Check if this parameter was already provided as a kwarg
+                if param_name in used_param_names:
+                    raise TypeError(f"got multiple values for argument '{param_name}'")
+                used_param_names.add(param_name)
+
+            validated_args.append(param.value)
+            next_positional_index += 1
         else:
-            validator_call_script += f"{param.key}=call_params[{index}], "
+            # This is a keyword argument
+            seen_kwargs = True
 
-    validator_call_script += "**extra_kwargs, "
-    validator_call_script += ")"
+            # Check for duplicate kwargs
+            if param.key in used_param_names:
+                raise TypeError(f"got multiple values for argument '{param.key}'")
 
-    def applier(fn: Callable[..., Any]) -> Any:
-        locals = {
-            "fn": fn,
-            "call_params": call_params,
-            "extra_kwargs": extra_kwargs or {},
-        }
-        # NOTE: `eval()` changed API in Python 3.13
-        if sys.version_info >= (3, 13):
-            return eval(validator_call_script, globals={}, locals=locals)
-        else:
-            return eval(validator_call_script, {}, locals)
+            # Only validate kwarg names if the function doesn't accept **kwargs
+            if not has_var_keyword and param.key not in valid_params:
+                raise TypeError(f"got an unexpected keyword argument '{param.key}'")
 
-    return applier(fn)
+            validated_kwargs[param.key] = param.value
+            used_param_names.add(param.key)
+
+    # Add any extra kwargs
+    if extra_kwargs:
+        validated_kwargs.update(extra_kwargs)
+
+    # Final validation using signature.bind()
+    bound_args = signature.bind(*validated_args, **validated_kwargs)
+    bound_args.apply_defaults()
+
+    # Actually call the function with validated parameters
+    return fn(*bound_args.args, **bound_args.kwargs)

tests/test_attributes.py

@@ -128,7 +128,7 @@ class HtmlAttrsTests(BaseTestCase):
         template = Template(self.template_str)
 
         with self.assertRaisesMessage(
-            TypeError, "Invalid parameters for tag 'html_attrs': too many positional arguments"
+            TypeError, "Invalid parameters for tag 'html_attrs': takes 2 positional argument(s) but more were given"
         ):
             template.render(Context({"class_var": "padding-top-8"}))
 
@@ -269,7 +269,7 @@ class HtmlAttrsTests(BaseTestCase):
         template = Template(self.template_str)
 
         with self.assertRaisesMessage(
-            TypeError, "Invalid parameters for tag 'html_attrs': multiple values for argument 'attrs'"
+            TypeError, "Invalid parameters for tag 'html_attrs': got multiple values for argument 'attrs'"
         ):
             template.render(Context({"class_var": "padding-top-8"}))
 

tests/test_expression.py

@@ -737,7 +737,7 @@ class SpreadOperatorTests(BaseTestCase):
 
         template1 = Template(template_str1)
 
-        with self.assertRaisesMessage(SyntaxError, "keyword argument repeated"):
+        with self.assertRaisesMessage(TypeError, "got multiple values for argument 'x'"):
             template1.render(context)
 
         # But, similarly to python, we can merge multiple **kwargs by instead

tests/test_node.py

@@ -225,7 +225,7 @@ class NodeTests(BaseTestCase):
         """
         )
         with self.assertRaisesMessage(
-            TypeError, "Invalid parameters for tag 'mytag': multiple values for argument 'name'"
+            TypeError, "Invalid parameters for tag 'mytag': got multiple values for argument 'name'"
         ):
             template6.render(Context({}))
 
@@ -236,7 +236,7 @@ class NodeTests(BaseTestCase):
             {% mytag count=1 name='John' 123 %}
         """
         )
-        with self.assertRaisesMessage(SyntaxError, "positional argument follows keyword argument"):
+        with self.assertRaisesMessage(TypeError, "positional argument follows keyword argument"):
             template6.render(Context({}))
 
         # Extra kwargs
@@ -311,6 +311,62 @@ class NodeTests(BaseTestCase):
 
         TestNode1.unregister(component_tags.register)
 
+    def test_node_render_kwargs_only(self):
+        captured = None
+
+        class TestNode(BaseNode):
+            tag = "mytag"
+
+            def render(self, context: Context, **kwargs) -> str:
+                nonlocal captured
+                captured = kwargs
+                return ""
+
+        TestNode.register(component_tags.register)
+
+        # Test with various kwargs including non-identifier keys
+        template = Template(
+            """
+            {% load component_tags %}
+            {% mytag
+                name='John'
+                age=25
+                data-id=123
+                class="header"
+                @click="handleClick"
+                v-if="isVisible"
+            %}
+            """
+        )
+        template.render(Context({}))
+
+        # All kwargs should be accepted since the function accepts **kwargs
+        self.assertEqual(
+            captured,
+            {
+                "name": "John",
+                "age": 25,
+                "data-id": 123,
+                "class": "header",
+                "@click": "handleClick",
+                "v-if": "isVisible",
+            },
+        )
+
+        # Test with positional args (should fail since function only accepts kwargs)
+        template2 = Template(
+            """
+            {% load component_tags %}
+            {% mytag "John" name="Mary" %}
+            """
+        )
+        with self.assertRaisesMessage(
+            TypeError, "Invalid parameters for tag 'mytag': takes 0 positional arguments but 1 was given"
+        ):
+            template2.render(Context({}))
+
+        TestNode.unregister(component_tags.register)
+
 
 class DecoratorTests(BaseTestCase):
     def test_decorator_requires_tag(self):
@@ -488,7 +544,7 @@ class DecoratorTests(BaseTestCase):
         """
         )
         with self.assertRaisesMessage(
-            TypeError, "Invalid parameters for tag 'mytag': multiple values for argument 'name'"
+            TypeError, "Invalid parameters for tag 'mytag': got multiple values for argument 'name'"
         ):
             template6.render(Context({}))
 
@@ -499,7 +555,7 @@ class DecoratorTests(BaseTestCase):
             {% mytag count=1 name='John' 123 %}
         """
         )
-        with self.assertRaisesMessage(SyntaxError, "positional argument follows keyword argument"):
+        with self.assertRaisesMessage(TypeError, "positional argument follows keyword argument"):
             template6.render(Context({}))
 
         # Extra kwargs
@@ -568,3 +624,55 @@ class DecoratorTests(BaseTestCase):
         )
 
         render._node.unregister(component_tags.register)  # type: ignore[attr-defined]
+
+    def test_decorator_render_kwargs_only(self):
+        captured = None
+
+        @template_tag(component_tags.register, tag="mytag")  # type: ignore
+        def render(node: BaseNode, context: Context, **kwargs) -> str:
+            nonlocal captured
+            captured = kwargs
+            return ""
+
+        # Test with various kwargs including non-identifier keys
+        template = Template(
+            """
+            {% load component_tags %}
+            {% mytag
+                name='John'
+                age=25
+                data-id=123
+                class="header"
+                @click="handleClick"
+                v-if="isVisible"
+            %}
+            """
+        )
+        template.render(Context({}))
+
+        # All kwargs should be accepted since the function accepts **kwargs
+        self.assertEqual(
+            captured,
+            {
+                "name": "John",
+                "age": 25,
+                "data-id": 123,
+                "class": "header",
+                "@click": "handleClick",
+                "v-if": "isVisible",
+            },
+        )
+
+        # Test with positional args (should fail since function only accepts kwargs)
+        template2 = Template(
+            """
+            {% load component_tags %}
+            {% mytag "John" name="Mary" %}
+            """
+        )
+        with self.assertRaisesMessage(
+            TypeError, "Invalid parameters for tag 'mytag': takes 0 positional arguments but 1 was given"
+        ):
+            template2.render(Context({}))
+
+        render._node.unregister(component_tags.register)  # type: ignore[attr-defined]