refactor: fix slot context behavior (#445)

src/django_components/component.py

@@ -20,12 +20,7 @@ from django.views import View
 # way the two modules depend on one another.
 from django_components.component_registry import registry  # NOQA
 from django_components.component_registry import AlreadyRegistered, ComponentRegistry, NotRegistered, register  # NOQA
-from django_components.context import (
-    capture_root_context,
-    get_root_context,
-    set_root_context,
-    set_slot_component_association,
-)
+from django_components.context import make_isolated_context_copy, prepare_context, set_slot_component_association
 from django_components.logger import logger, trace_msg
 from django_components.middleware import is_dependency_middleware_active
 from django_components.node import walk_nodelist
@@ -268,6 +263,7 @@ class Component(View, metaclass=SimplifiedInterfaceMediaDefiningClass):
         # NOTE: This if/else is important to avoid nested Contexts,
         # See https://github.com/EmilStenstrom/django-components/issues/414
         context = context_data if isinstance(context_data, Context) else Context(context_data)
+        prepare_context(context, outer_context=self.outer_context or Context())
         template = self.get_template(context)
 
         # Associate the slots with this component for this context
@@ -347,10 +343,6 @@ class ComponentNode(Node):
         resolved_component_name = self.name_fexp.resolve(context)
         component_cls: Type[Component] = registry.get(resolved_component_name)
 
-        # If this is the outer-/top-most component node, then save the outer context,
-        # so it can be used by nested Slots.
-        capture_root_context(context)
-
         # Resolve FilterExpressions and Variables that were passed as args to the
         # component, then call component's context method
         # to get values to insert into the context
@@ -380,12 +372,7 @@ class ComponentNode(Node):
 
         # Prevent outer context from leaking into the template of the component
         if self.isolated_context:
-            # Even if contexts are isolated, we still need to pass down the
-            # original context so variables in slots can be rendered using
-            # the original context.
-            root_ctx = get_root_context(context)
-            context = context.new()
-            set_root_context(context, root_ctx)
+            context = make_isolated_context_copy(context)
 
         with context.update(component_context):
             rendered_component = component.render(context)

src/django_components/context.py

@@ -5,22 +5,65 @@ pass data across components, nodes, slots, and contexts.
 You can think of the Context as our storage system.
 """
 
-from copy import copy
 from typing import TYPE_CHECKING, Optional
 
 from django.template import Context
 
 from django_components.logger import trace_msg
+from django_components.utils import find_last_index
 
 if TYPE_CHECKING:
     from django_components.slots import FillContent
 
 
 _FILLED_SLOTS_CONTENT_CONTEXT_KEY = "_DJANGO_COMPONENTS_FILLED_SLOTS"
-_OUTER_CONTEXT_CONTEXT_KEY = "_DJANGO_COMPONENTS_OUTER_CONTEXT"
+_OUTER_ROOT_CTX_CONTEXT_KEY = "_DJANGO_COMPONENTS_OUTER_ROOT_CTX"
 _SLOT_COMPONENT_ASSOC_KEY = "_DJANGO_COMPONENTS_SLOT_COMP_ASSOC"
 
 
+def prepare_context(context: Context, outer_context: Optional[Context]) -> None:
+    """Initialize the internal context state."""
+    # This is supposed to run ALWAYS at Component.render
+    if outer_context is not None:
+        set_outer_root_context(context, outer_context)
+
+    # Initialize mapping dicts within this rendering run.
+    # This is shared across the whole render chain, thus we set it only once.
+    if _SLOT_COMPONENT_ASSOC_KEY not in context:
+        context[_SLOT_COMPONENT_ASSOC_KEY] = {}
+    if _FILLED_SLOTS_CONTENT_CONTEXT_KEY not in context:
+        context[_FILLED_SLOTS_CONTENT_CONTEXT_KEY] = {}
+
+    # If we're inside a forloop, we need to make a disposable copy of slot -> comp
+    # mapping, which can be modified in the loop. We do so by copying it onto the latest
+    # context layer.
+    #
+    # This is necessary, because otherwise if we have a nested loop with a same
+    # component used recursively, the inner slot -> comp mapping would leak into the outer.
+    #
+    # NOTE: If you ever need to debug this, insert a print/debug statement into
+    # `django.template.defaulttags.ForNode.render` to inspect the context object
+    # inside the for loop.
+    if "forloop" in context:
+        context.dicts[-1][_SLOT_COMPONENT_ASSOC_KEY] = context[_SLOT_COMPONENT_ASSOC_KEY].copy()
+
+
+def make_isolated_context_copy(context: Context) -> Context:
+    # Even if contexts are isolated, we still need to pass down the
+    # metadata so variables in slots can be rendered using the correct context.
+    root_ctx = get_outer_root_context(context)
+    slot_assoc = context.get(_SLOT_COMPONENT_ASSOC_KEY, {})
+    slot_fills = context.get(_FILLED_SLOTS_CONTENT_CONTEXT_KEY, {})
+
+    context_copy = context.new()
+    context_copy[_SLOT_COMPONENT_ASSOC_KEY] = slot_assoc
+    context_copy[_FILLED_SLOTS_CONTENT_CONTEXT_KEY] = slot_fills
+    set_outer_root_context(context_copy, root_ctx)
+    copy_forloop_context(context, context_copy)
+
+    return context_copy
+
+
 def get_slot_fill(context: Context, component_id: str, slot_name: str) -> Optional["FillContent"]:
     """
     Use this function to obtain a slot fill from the current context.
@@ -28,8 +71,8 @@ def get_slot_fill(context: Context, component_id: str, slot_name: str) -> Option
     See `set_slot_fill` for more details.
     """
     trace_msg("GET", "FILL", slot_name, component_id)
-    slot_key = (_FILLED_SLOTS_CONTENT_CONTEXT_KEY, component_id, slot_name)
-    return context.get(slot_key, None)
+    slot_key = f"{component_id}__{slot_name}"
+    return context[_FILLED_SLOTS_CONTENT_CONTEXT_KEY].get(slot_key, None)
 
 
 def set_slot_fill(context: Context, component_id: str, slot_name: str, value: "FillContent") -> None:
@@ -38,77 +81,82 @@ def set_slot_fill(context: Context, component_id: str, slot_name: str, value: "F
 
     Note that we make use of the fact that Django's Context is a stack - we can push and pop
     extra contexts on top others.
-
-    For the slot fills to be pushed/popped wth stack layer, they need to have keys defined
-    directly on the Context object.
     """
     trace_msg("SET", "FILL", slot_name, component_id)
-    slot_key = (_FILLED_SLOTS_CONTENT_CONTEXT_KEY, component_id, slot_name)
-    context[slot_key] = value
+    slot_key = f"{component_id}__{slot_name}"
+    context[_FILLED_SLOTS_CONTENT_CONTEXT_KEY][slot_key] = value
 
 
-def get_root_context(context: Context) -> Optional[Context]:
+def get_outer_root_context(context: Context) -> Optional[Context]:
     """
-    Use this function to get the root context.
+    Use this function to get the outer root context.
 
-    Root context is the top-most context, AKA the context that was passed to
-    the initial `Template.render()`.
-    We pass through the root context to allow configure how slot fills should be rendered.
-
-    See the `SLOT_CONTEXT_BEHAVIOR` setting.
+    See `set_outer_root_context` for more details.
     """
-    return context.get(_OUTER_CONTEXT_CONTEXT_KEY)
+    return context.get(_OUTER_ROOT_CTX_CONTEXT_KEY)
 
 
-def set_root_context(context: Context, root_ctx: Context) -> None:
+def set_outer_root_context(context: Context, outer_ctx: Optional[Context]) -> None:
     """
-    Use this function to set the root context.
+    Use this function to set the outer root context.
 
-    Root context is the top-most context, AKA the context that was passed to
-    the initial `Template.render()`.
-    We pass through the root context to allow configure how slot fills should be rendered.
+    When we consider a component's template, then outer context is the context
+    that was available just outside of the component's template (AKA it was in
+    the PARENT template).
 
-    See the `SLOT_CONTEXT_BEHAVIOR` setting.
+    Once we have the outer context, next we get the outer ROOT context. This is
+    the context that was available at the top level of the PARENT template.
+
+    We pass through this context to allow to configure how slot fills should be
+    rendered using the `SLOT_CONTEXT_BEHAVIOR` setting.
     """
-    context.push({_OUTER_CONTEXT_CONTEXT_KEY: root_ctx})
+    if outer_ctx and len(outer_ctx.dicts) > 1:
+        outer_root_context: Context = outer_ctx.new()
+        # NOTE_1:
+        # - Index 0 are the defaults set in BaseContext
+        # - Index 1 is the context generated by `Component.get_context_data`
+        #   of the parent's component
+        # - All later indices (2, 3, ...) are extra layers added by the rendering
+        #   logic (each Node usually adds it's own context layer)
+        outer_root_context.push(outer_ctx.dicts[1])
+    else:
+        outer_root_context = Context()
+
+    # Include the mappings.
+    if _SLOT_COMPONENT_ASSOC_KEY in context:
+        outer_root_context[_SLOT_COMPONENT_ASSOC_KEY] = context[_SLOT_COMPONENT_ASSOC_KEY]
+    if _FILLED_SLOTS_CONTENT_CONTEXT_KEY in context:
+        outer_root_context[_FILLED_SLOTS_CONTENT_CONTEXT_KEY] = context[_FILLED_SLOTS_CONTENT_CONTEXT_KEY]
+
+    context[_OUTER_ROOT_CTX_CONTEXT_KEY] = outer_root_context
 
 
-def capture_root_context(context: Context) -> None:
-    """
-    Set the root context if it was not set before.
-
-    Root context is the top-most context, AKA the context that was passed to
-    the initial `Template.render()`.
-    We pass through the root context to allow configure how slot fills should be rendered.
-
-    See the `SLOT_CONTEXT_BEHAVIOR` setting.
-    """
-    root_ctx_already_defined = _OUTER_CONTEXT_CONTEXT_KEY in context
-    if not root_ctx_already_defined:
-        set_root_context(context, copy(context))
-
-
-def set_slot_component_association(context: Context, slot_id: str, component_id: str) -> None:
+def set_slot_component_association(
+    context: Context,
+    slot_id: str,
+    component_id: str,
+) -> None:
     """
     Set association between a Slot and a Component in the current context.
 
-    We use SlotNodes to render slot fills. SlotNodes are created only at Template parse time.
-    However, when we are using components with slots in (another) template, we can render
-    the same component multiple time. So we can have multiple FillNodes intended to be used
-    with the same SlotNode.
+    We use SlotNodes to render slot fills. SlotNodes are created only at Template
+    parse time.
+    However, when we refer to components with slots in (another) template (using
+    `{% component %}`), we can render the same component multiple time. So we can
+    have multiple FillNodes intended to be used with the same SlotNode.
 
-    So how do we tell the SlotNode which FillNode to render? We do so by tagging the ComponentNode
-    and FillNodes with a unique component_id, which ties them together. And then we tell SlotNode
-    which component_id to use to be able to find the correct Component/Fill.
+    So how do we tell the SlotNode which FillNode to render? We do so by tagging
+    the ComponentNode and FillNodes with a unique component_id, which ties them
+    together. And then we tell SlotNode which component_id to use to be able to
+    find the correct Component/Fill.
 
-    We don't want to store this info on the Nodes themselves, as we need to treat them as
-    immutable due to caching of Templates by Django.
+    We don't want to store this info on the Nodes themselves, as we need to treat
+    them as immutable due to caching of Templates by Django.
 
-    Hence, we use the Context to store the associations of SlotNode <-> Component for
-    the current context stack.
+    Hence, we use the Context to store the associations of SlotNode <-> Component
+    for the current context stack.
     """
-    key = (_SLOT_COMPONENT_ASSOC_KEY, slot_id)
-    context[key] = component_id
+    context[_SLOT_COMPONENT_ASSOC_KEY][slot_id] = component_id
 
 
 def get_slot_component_association(context: Context, slot_id: str) -> str:
@@ -118,5 +166,17 @@ def get_slot_component_association(context: Context, slot_id: str) -> str:
 
     See `set_slot_component_association` for more details.
     """
-    key = (_SLOT_COMPONENT_ASSOC_KEY, slot_id)
-    return context[key]
+    return context[_SLOT_COMPONENT_ASSOC_KEY][slot_id]
+
+
+def copy_forloop_context(from_context: Context, to_context: Context) -> None:
+    """Forward the info about the current loop"""
+    # Note that the ForNode (which implements for loop behavior) does not
+    # only add the `forloop` key, but also keys corresponding to the loop elements
+    # So if the loop syntax is `{% for my_val in my_lists %}`, then ForNode also
+    # sets a `my_val` key.
+    # For this reason, instead of copying individual keys, we copy the whole stack layer
+    # set by ForNode.
+    if "forloop" in from_context:
+        forloop_dict_index = find_last_index(from_context.dicts, lambda d: "forloop" in d)
+        to_context.update(from_context.dicts[forloop_dict_index])

src/django_components/slots.py

@@ -10,7 +10,13 @@ from django.template.exceptions import TemplateSyntaxError
 from django.utils.safestring import SafeString, mark_safe
 
 from django_components.app_settings import SlotContextBehavior, app_settings
-from django_components.context import get_root_context, get_slot_component_association, get_slot_fill, set_slot_fill
+from django_components.context import (
+    copy_forloop_context,
+    get_outer_root_context,
+    get_slot_component_association,
+    get_slot_fill,
+    set_slot_fill,
+)
 from django_components.logger import trace_msg
 from django_components.node import nodelist_has_content
 from django_components.utils import gen_id
@@ -139,14 +145,16 @@ class SlotNode(Node):
 
         See SlotContextBehavior for the description of each option.
         """
-        root_ctx = get_root_context(context) or Context()
+        root_ctx = get_outer_root_context(context) or Context()
 
         if app_settings.SLOT_CONTEXT_BEHAVIOR == SlotContextBehavior.ALLOW_OVERRIDE:
             return context
         elif app_settings.SLOT_CONTEXT_BEHAVIOR == SlotContextBehavior.ISOLATED:
-            return root_ctx
+            new_context: Context = copy(root_ctx)
+            copy_forloop_context(context, new_context)
+            return new_context
         elif app_settings.SLOT_CONTEXT_BEHAVIOR == SlotContextBehavior.PREFER_ROOT:
-            new_context: Context = copy(context)
+            new_context = copy(context)
             new_context.update(root_ctx.flatten())
             return new_context
         else:
@@ -483,8 +491,8 @@ def _report_slot_errors(
     for fill_name in unmatched_fills:
         fuzzy_slot_name_matches = difflib.get_close_matches(fill_name, unfilled_slots, n=1, cutoff=0.7)
         msg = (
-            f"Component '{registered_name}' passed fill "
-            f"that refers to undefined slot: '{fill_name}'."
+            f"Component '{registered_name}' passed fill that refers to undefined slot:"
+            f" '{fill_name}'."
             f"\nUnfilled slot names are: {sorted(unfilled_slots)}."
         )
         if fuzzy_slot_name_matches:

src/django_components/utils.py

@@ -1,6 +1,6 @@
 import glob
 from pathlib import Path
-from typing import List, NamedTuple, Optional
+from typing import Any, Callable, List, NamedTuple, Optional
 
 from django.template.engine import Engine
 
@@ -49,3 +49,10 @@ def gen_id(length: int = 5) -> str:
 
     # Pad the ID with `0`s up to 4 digits, e.g. `0007`
     return f"{_id:04}"
+
+
+def find_last_index(lst: List, predicate: Callable[[Any], bool]) -> Any:
+    for r_idx, elem in enumerate(reversed(lst)):
+        if predicate(elem):
+            return len(lst) - 1 - r_idx
+    return -1

tests/test_templatetags.py

@@ -1,8 +1,9 @@
 import re
 import textwrap
-from typing import Callable, Optional
+from typing import Any, Callable, Dict, Optional
 
 from django.template import Context, Template, TemplateSyntaxError
+from django.test import override_settings
 
 # isort: off
 from .django_test_setup import *  # NOQA
@@ -85,6 +86,34 @@ class ComponentWithDefaultAndRequiredSlot(component.Component):
     template_name = "template_with_default_and_required_slot.html"
 
 
+class _ComplexChildComponent(component.Component):
+    template = """
+        {% load component_tags %}
+        <div>
+        {% slot "content" default %}
+            No slot!
+        {% endslot %}
+        </div>
+    """
+
+
+class _ComplexParentComponent(component.Component):
+    template = """
+        {% load component_tags %}
+        ITEMS: {{ items }}
+        {% for item in items %}
+        <li>
+            {% component "complex_child" %}
+                {{ item.value }}
+            {% endcomponent %}
+        </li>
+        {% endfor %}
+    """
+
+    def get_context_data(self, items, *args, **kwargs) -> Dict[str, Any]:
+        return {"items": items}
+
+
 class ComponentTemplateTagTest(BaseTestCase):
     def setUp(self):
         # NOTE: component.registry is global, so need to clear before each test
@@ -1019,6 +1048,8 @@ class ComponentNestingTests(BaseTestCase):
         super().setUpClass()
         component.registry.register("dashboard", _DashboardComponent)
         component.registry.register("calendar", _CalendarComponent)
+        component.registry.register("complex_child", _ComplexChildComponent)
+        component.registry.register("complex_parent", _ComplexParentComponent)
 
     @classmethod
     def tearDownClass(cls) -> None:
@@ -1052,6 +1083,100 @@ class ComponentNestingTests(BaseTestCase):
         """
         self.assertHTMLEqual(rendered, expected)
 
+    @override_settings(
+        COMPONENTS={
+            "context_behavior": "isolated",
+            "slot_context_behavior": "isolated",
+        }
+    )
+    def test_component_nesting_slot_inside_component_fill_isolated(self):
+        template = Template(
+            """
+            {% load component_tags %}
+            {% component "dashboard" %}{% endcomponent %}
+            """
+        )
+        rendered = template.render(Context({"items": [1, 2, 3]}))
+        expected = """
+        <div class="dashboard-component">
+          <div class="calendar-component">
+            <h1>
+              Welcome to your dashboard!
+            </h1>
+            <main>
+              Here are your to-do items for today:
+            </main>
+          </div>
+          <ol>
+          </ol>
+        </div>
+        """
+        self.assertHTMLEqual(rendered, expected)
+
+    @override_settings(
+        COMPONENTS={
+            "context_behavior": "isolated",
+            "slot_context_behavior": "isolated",
+        }
+    )
+    def test_component_nesting_slot_inside_component_fill_isolated_2(self):
+        template = Template(
+            """
+            {% load component_tags %}
+            {% component "dashboard" %}
+                {% fill "header" %}
+                    Whoa!
+                {% endfill %}
+            {% endcomponent %}
+            """
+        )
+        rendered = template.render(Context({"items": [1, 2, 3]}))
+        expected = """
+        <div class="dashboard-component">
+          <div class="calendar-component">
+            <h1>
+              Whoa!
+            </h1>
+            <main>
+              Here are your to-do items for today:
+            </main>
+          </div>
+          <ol>
+          </ol>
+        </div>
+        """
+        self.assertHTMLEqual(rendered, expected)
+
+    @override_settings(
+        COMPONENTS={
+            "context_behavior": "isolated",
+            "slot_context_behavior": "isolated",
+        }
+    )
+    def test_component_nesting_deep_slot_inside_component_fill_isolated(self):
+
+        template = Template(
+            """
+            {% load component_tags %}
+            {% component "complex_parent" items=items %}{% endcomponent %}
+            """
+        )
+        items = [{"value": 1}, {"value": 2}, {"value": 3}]
+        rendered = template.render(Context({"items": items}))
+        expected = """
+        ITEMS: [{&#x27;value&#x27;: 1}, {&#x27;value&#x27;: 2}, {&#x27;value&#x27;: 3}]
+        <li>
+            <div> 1 </div>
+        </li>
+        <li>
+            <div> 2 </div>
+        </li>
+        <li>
+            <div> 3 </div>
+        </li>
+        """
+        self.assertHTMLEqual(rendered, expected)
+
     def test_component_nesting_component_with_fill_and_super(self):
         template = Template(
             """

tests/testutils.py

@@ -1,6 +1,7 @@
+from typing import List
 from unittest.mock import Mock
 
-from django.template import Context
+from django.template import Context, Node
 from django.template.response import TemplateResponse
 from django.test import SimpleTestCase
 
@@ -32,3 +33,22 @@ def create_and_process_template_response(template, context=None, use_middleware=
     else:
         response.render()
     return response.content.decode("utf-8")
+
+
+def print_nodes(nodes: List[Node], indent=0) -> None:
+    """
+    Render a Nodelist, inlining child nodes with extra on separate lines and with
+    extra indentation.
+    """
+    for node in nodes:
+        child_nodes: List[Node] = []
+        for attr in node.child_nodelists:
+            attr_child_nodes = getattr(node, attr, None) or []
+            if attr_child_nodes:
+                child_nodes.extend(attr_child_nodes)
+
+        repr = str(node)
+        repr = "\n".join([(" " * 4 * indent) + line for line in repr.split("\n")])
+        print(repr)
+        if child_nodes:
+            print_nodes(child_nodes, indent=indent + 1)