Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Django nodes cache #40

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 44 additions & 20 deletions render_block/django.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from copy import copy
import hashlib

from django.template import Context, RequestContext
from django.template.base import TextNode
Expand All @@ -12,6 +13,8 @@

from render_block.exceptions import BlockNotFound

_NODES_CACHE = {}


def django_render_block(template, block_name, context, request=None):
# Create a Django Context if needed
Expand All @@ -30,24 +33,47 @@ def django_render_block(template, block_name, context, request=None):

# Get the underlying django.template.base.Template object.
template = template.template
cache_key = _make_node_cache_key(template, block_name)

# Bind the template to the context.
with context_instance.bind_template(template):
# Before trying to render the template, we need to traverse the tree of
# parent templates and find all blocks in them.
parent_template = _build_block_context(template, context_instance)

try:
return _render_template_block(template, block_name, context_instance)
except BlockNotFound:
# The block wasn't found in the current template.
node, render_context = _NODES_CACHE[cache_key]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly worried that a global cache might not be safe to use here, but it likely is OK in almost every case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Django has a cache per instance of template cached loader. https://github.com/django/django/blob/21d8ea4eb3e22d458515b5278c8cd3a15b069799/django/template/loaders/cached.py#L16

Django template loaders have to handle multiple loader instances and do things like template overriding, that are impossible with a global cache.

This lib is a lot simpler than Django template loaders, so the global cache would work fine.

except KeyError:
# Before trying to render the template, we need to traverse the tree of
# parent templates and find all blocks in them.
parent_template = _build_block_context(template, context_instance)

# If there's no parent template (i.e. no ExtendsNode), re-raise.
if not parent_template:
raise
try:
node, render_context = _find_template_block(
template, block_name, context_instance
)
except BlockNotFound:
# The block wasn't found in the current template.

# If there's no parent template (i.e. no ExtendsNode), re-raise.
if not parent_template:
raise

# Check the parent template for this block.
return _render_template_block(parent_template, block_name, context_instance)
# Check the parent template for this block.
node, render_context = _find_template_block(
parent_template, block_name, context_instance
)

if cache_key and not template.engine.debug:
_NODES_CACHE[cache_key] = node, render_context

context_instance.render_context = render_context
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a comment added in #25 that implies reusing render contexts might be unsafe. That does have a test though so hopefully it is OK.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is ok.

The issue #25 fixed Context object reuse, but we're just reusing the render_context inside a new Context. https://github.com/django/django/blob/main/django/template/loaders/cached.py#L72

return node.render(context_instance)


def _make_node_cache_key(template, block_name):
if template.name:
key = template.name
else:
source = template.source.encode()
key = hashlib.md5(source).hexdigest()
return f"{key}@{block_name}"


def _build_block_context(template, context):
Expand Down Expand Up @@ -82,12 +108,12 @@ def _build_block_context(template, context):
break


def _render_template_block(template, block_name, context):
"""Renders a single block from a template."""
return _render_template_block_nodelist(template.nodelist, block_name, context)
def _find_template_block(template, block_name, context):
"""Finds a single block from a template."""
return _find_template_block_nodelist(template.nodelist, block_name, context)


def _render_template_block_nodelist(nodelist, block_name, context):
def _find_template_block_nodelist(nodelist, block_name, context):
"""Recursively iterate over a node to find the wanted block."""

# Attempt to find the wanted block in the current template.
Expand All @@ -99,7 +125,7 @@ def _render_template_block_nodelist(nodelist, block_name, context):

# If the name matches, you're all set and we found the block!
if node.name == block_name:
return node.render(context)
return node, context.render_context
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having some trouble wrapping my brain around some of this change, it seems like _find_template_block_nodelist really only cares about the render_context after this change and not the context itself.

(This is also true of _find_template_block.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Context/RequestContext has the data to be rendered. The context.render_context has only the template structure that can be reused, so I always need a new Context, but can reuse the render context.


# If a node has children, recurse into them. Based on
# django.template.base.Node.get_nodes_by_type.
Expand All @@ -111,9 +137,7 @@ def _render_template_block_nodelist(nodelist, block_name, context):

# Try to find the block recursively.
try:
return _render_template_block_nodelist(
new_nodelist, block_name, context
)
return _find_template_block_nodelist(new_nodelist, block_name, context)
except BlockNotFound:
continue

Expand Down
27 changes: 26 additions & 1 deletion tests/tests.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
from collections import namedtuple
from unittest import skip

from django.template import Context
from django.template import Context, Template
from django.test import RequestFactory, TestCase, modify_settings, override_settings

from render_block import BlockNotFound, UnsupportedEngine, render_block_to_string
from render_block.django import _NODES_CACHE, django_render_block


class TestDjango(TestCase):
Expand Down Expand Up @@ -142,6 +144,29 @@ def test_request_context(self):

self.assertEqual(result, "/dummy-url")

def test_node_cache(self):
"""Test rendering from cache."""
render_block_to_string("test1.html", "block1")
_NODES_CACHE["test1.html@fakeblock"] = _NODES_CACHE["test1.html@block1"]
result = render_block_to_string("test1.html", "fakeblock")
self.assertEqual(result, "block1 from test1")

def test_node_cache_anonymous_template(self):
"""
Test rendering from cache for anonymous templates.

Cache key must be created from template source instead of template name
to avoid clashes.
"""
T = namedtuple("T", ["template"])
template_1 = Template("{% block b %}foo {{ foo }}{% endblock %}")
template_2 = Template("{% block b %}bar {{ foo }}{% endblock %}")

result = django_render_block(T(template_1), "b", {"foo": "1"})
self.assertEqual(result, "foo 1")
result = django_render_block(T(template_2), "b", {"foo": "2"})
self.assertEqual(result, "bar 2")


@override_settings(
TEMPLATES=[
Expand Down