Merge pull request #491 from cbjadwani/develop

Fix offline compression with template inheritance

compressor/management/commands/compress.py

@@ -179,7 +179,13 @@ class Command(NoArgsCommand):
                 if verbosity > 0:
                     log.write("UnicodeDecodeError while trying to read "
                               "template %s\n" % template_name)
-            nodes = list(parser.walk_nodes(template))
+            try:
+                nodes = list(parser.walk_nodes(template))
+            except (TemplateDoesNotExist, TemplateSyntaxError) as e:
+                # Could be an error in some base template
+                if verbosity > 0:
+                    log.write("Error parsing template %s: %s\n" % (template_name, e))
+                continue
             if nodes:
                 template.template_name = template_name
                 compressor_nodes.setdefault(template, []).extend(nodes)

compressor/offline/django.py

@@ -1,87 +1,91 @@
 from __future__ import absolute_import
 import io
-from types import MethodType
+from copy import copy
 
 from django import template
+from django.conf import settings
 from django.template import Template
-from django.template.loader_tags import (ExtendsNode, BlockNode,
-                                         BLOCK_CONTEXT_KEY)
+from django.template import Context
+from django.template.base import Node, VariableNode, TextNode, NodeList
+from django.template.defaulttags import IfNode
+from django.template.loader_tags import ExtendsNode, BlockNode, BlockContext
 
 
 from compressor.exceptions import TemplateSyntaxError, TemplateDoesNotExist
 from compressor.templatetags.compress import CompressorNode
 
 
-def patched_render(self, context):
-    # 'Fake' _render method that just returns the context instead of
-    # rendering. It also checks whether the first node is an extend node or
-    # not, to be able to handle complex inheritance chain.
-    self._render_firstnode = MethodType(patched_render_firstnode, self)
-    self._render_firstnode(context)
+def handle_extendsnode(extendsnode, block_context=None):
+    """Create a copy of Node tree of a derived template replacing
+    all blocks tags with the nodes of appropriate blocks.
+    Also handles {{ block.super }} tags.
+    """
+    if block_context is None:
+        block_context = BlockContext()
+    blocks = dict((n.name, n) for n in
+                  extendsnode.nodelist.get_nodes_by_type(BlockNode))
+    block_context.add_blocks(blocks)
 
-    # Cleanup, uninstall our _render monkeypatch now that it has been called
-    self._render = self._old_render
-    return context
+    context = Context(settings.COMPRESS_OFFLINE_CONTEXT)
+    compiled_parent = extendsnode.get_parent(context)
+    parent_nodelist = compiled_parent.nodelist
+    # If the parent template has an ExtendsNode it is not the root.
+    for node in parent_nodelist:
+        # The ExtendsNode has to be the first non-text node.
+        if not isinstance(node, TextNode):
+            if isinstance(node, ExtendsNode):
+                return handle_extendsnode(node, block_context)
+            break
+    # Add blocks of the root template to block context.
+    blocks = dict((n.name, n) for n in
+                  parent_nodelist.get_nodes_by_type(BlockNode))
+    block_context.add_blocks(blocks)
+
+    block_stack = []
+    new_nodelist = remove_block_nodes(parent_nodelist, block_stack, block_context)
+    return new_nodelist
 
 
-def patched_render_firstnode(self, context):
-    # If this template has a ExtendsNode, we want to find out what
-    # should be put in render_context to make the {% block ... %}
-    # tags work.
-    #
-    # We can't fully render the base template(s) (we don't have the
-    # full context vars - only what's necessary to render the compress
-    # nodes!), therefore we hack the ExtendsNode we found, patching
-    # its get_parent method so that rendering the ExtendsNode only
-    # gives us the blocks content without doing any actual rendering.
-    extra_context = {}
-    try:
-        firstnode = self.nodelist[0]
-    except IndexError:
-        firstnode = None
-    if isinstance(firstnode, ExtendsNode):
-        firstnode._log = self._log
-        firstnode._log_verbosity = self._log_verbosity
-        firstnode._old_get_parent = firstnode.get_parent
-        firstnode.get_parent = MethodType(patched_get_parent, firstnode)
-        try:
-            extra_context = firstnode.render(context)
-            context.render_context = extra_context.render_context
-            # We aren't rendering {% block %} tags, but we want
-            # {{ block.super }} inside {% compress %} inside {% block %}s to
-            # work. Therefore, we need to pop() the last block context for
-            # each block name, to emulate what would have been done if the
-            # {% block %} had been fully rendered.
-            for blockname in firstnode.blocks.keys():
-                context.render_context[BLOCK_CONTEXT_KEY].pop(blockname)
-        except (IOError, template.TemplateSyntaxError,
-                template.TemplateDoesNotExist):
-            # That first node we are trying to render might cause more errors
-            # that we didn't catch when simply creating a Template instance
-            # above, so we need to catch that (and ignore it, just like above)
-            # as well.
-            if self._log_verbosity > 0:
-                self._log.write("Caught error when rendering extend node from "
-                                "template %s\n" % getattr(self, 'name', self))
-            return None
-        finally:
-            # Cleanup, uninstall our get_parent monkeypatch now that it has been called
-            firstnode.get_parent = firstnode._old_get_parent
-    return extra_context
+def remove_block_nodes(nodelist, block_stack, block_context):
+    new_nodelist = NodeList()
+    for node in nodelist:
+        if isinstance(node, VariableNode):
+            var_name = node.filter_expression.token.strip()
+            if var_name == 'block.super':
+                if not block_stack:
+                    continue
+                node = block_context.get_block(block_stack[-1].name)
+        if isinstance(node, BlockNode):
+            expanded_block = expand_blocknode(node, block_stack, block_context)
+            new_nodelist.extend(expanded_block)
+        else:
+            # IfNode has nodelist as a @property so we can not modify it
+            if isinstance(node, IfNode):
+                node = copy(node)
+                for i, (condition, sub_nodelist) in enumerate(node.conditions_nodelists):
+                    sub_nodelist = remove_block_nodes(sub_nodelist, block_stack, block_context)
+                    node.conditions_nodelists[i] = (condition, sub_nodelist)
+            else:
+                for attr in node.child_nodelists:
+                    sub_nodelist = getattr(node, attr, None)
+                    if sub_nodelist:
+                        sub_nodelist = remove_block_nodes(sub_nodelist, block_stack, block_context)
+                        node = copy(node)
+                        setattr(node, attr, sub_nodelist)
+            new_nodelist.append(node)
+    return new_nodelist
 
 
-def patched_get_parent(self, context):
-    # Patch template returned by extendsnode's get_parent to make sure their
-    # _render method is just returning the context instead of actually
-    # rendering stuff.
-    # In addition, this follows the inheritance chain by looking if the first
-    # node of the template is an extend node itself.
-    compiled_template = self._old_get_parent(context)
-    compiled_template._log = self._log
-    compiled_template._log_verbosity = self._log_verbosity
-    compiled_template._old_render = compiled_template._render
-    compiled_template._render = MethodType(patched_render, compiled_template)
-    return compiled_template
+def expand_blocknode(node, block_stack, block_context):
+    popped_block = block = block_context.pop(node.name)
+    if block is None:
+        block = node
+    block_stack.append(block)
+    expanded_nodelist = remove_block_nodes(block.nodelist, block_stack, block_context)
+    block_stack.pop()
+    if popped_block is not None:
+        block_context.push(node.name, popped_block)
+    return expanded_nodelist
 
 
 class DjangoParser(object):
@@ -98,26 +102,13 @@ class DjangoParser(object):
                 raise TemplateDoesNotExist(str(e))
 
     def process_template(self, template, context):
-        template._render_firstnode = MethodType(patched_render_firstnode, template)
-        template._extra_context = template._render_firstnode(context)
-
-        if template._extra_context is None:
-            # Something is wrong - ignore this template
-            return False
-
         return True
 
     def get_init_context(self, offline_context):
         return offline_context
 
     def process_node(self, template, context, node):
-        if template._extra_context and node._block_name:
-            # Give a block context to the node if it was found inside
-            # a {% block %}.
-            context['block'] = context.render_context[BLOCK_CONTEXT_KEY].get_block(node._block_name)
-
-            if context['block']:
-                context['block'].context = context
+        pass
 
     def render_nodelist(self, template, context, node):
         return node.nodelist.render(context)
@@ -126,18 +117,27 @@ class DjangoParser(object):
         return node.render(context, forced=True)
 
     def get_nodelist(self, node):
-        # Check if node is an ```if``` switch with true and false branches
-        if hasattr(node, 'nodelist_true') and hasattr(node, 'nodelist_false'):
-            return node.nodelist_true + node.nodelist_false
-        return getattr(node, "nodelist", [])
+        if isinstance(node, ExtendsNode):
+            try:
+                return handle_extendsnode(node)
+            except template.TemplateSyntaxError as e:
+                raise TemplateSyntaxError(str(e))
+            except template.TemplateDoesNotExist as e:
+                raise TemplateDoesNotExist(str(e))
 
-    def walk_nodes(self, node, block_name=None):
+        # Check if node is an ```if``` switch with true and false branches
+        nodelist = []
+        if isinstance(node, Node):
+            for attr in node.child_nodelists:
+                nodelist += getattr(node, attr, [])
+        else:
+            nodelist = getattr(node, 'nodelist', [])
+        return nodelist
+
+    def walk_nodes(self, node):
         for node in self.get_nodelist(node):
-            if isinstance(node, BlockNode):
-                block_name = node.name
             if isinstance(node, CompressorNode) and node.is_offline_compression_enabled(forced=True):
-                node._block_name = block_name
                 yield node
             else:
-                for node in self.walk_nodes(node, block_name=block_name):
+                for node in self.walk_nodes(node):
                     yield node

compressor/tests/test_offline.py

@@ -146,7 +146,7 @@ class OfflineGenerationBlockSuperTestCase(OfflineTestCaseMixin, TestCase):
 
 class OfflineGenerationBlockSuperMultipleTestCase(OfflineTestCaseMixin, TestCase):
     templates_dir = "test_block_super_multiple"
-    expected_hash = "2f6ef61c488e"
+    expected_hash = "f8891c416981"
     # Block.super not supported for Jinja2 yet.
     engines = ("django",)
 
@@ -341,6 +341,43 @@ class OfflineGenerationTestCase(OfflineTestCaseMixin, TestCase):
             settings.TEMPLATE_LOADERS = old_loaders
 
 
+class OfflineGenerationBlockSuperBaseCompressed(OfflineTestCaseMixin, TestCase):
+    template_names = ["base.html", "base2.html", "test_compressor_offline.html"]
+    templates_dir = 'test_block_super_base_compressed'
+    expected_hash = ['028c3fc42232', '2e9d3f5545a6', 'f8891c416981']
+    # Block.super not supported for Jinja2 yet.
+    engines = ("django",)
+
+    def setUp(self):
+        super(OfflineGenerationBlockSuperBaseCompressed, self).setUp()
+
+        self.template_paths = []
+        self.templates = []
+        for template_name in self.template_names:
+            template_path = os.path.join(settings.TEMPLATE_DIRS[0], template_name)
+            self.template_paths.append(template_path)
+            with io.open(template_path, encoding=settings.FILE_CHARSET) as file:
+                template = Template(file.read())
+            self.templates.append(template)
+
+    def _render_template(self, template, engine):
+        if engine == "django":
+            return template.render(Context(settings.COMPRESS_OFFLINE_CONTEXT))
+        elif engine == "jinja2":
+            return template.render(settings.COMPRESS_OFFLINE_CONTEXT) + "\n"
+        else:
+            return None
+
+    def _test_offline(self, engine):
+        count, result = CompressCommand().compress(log=self.log, verbosity=self.verbosity, engine=engine)
+        self.assertEqual(len(self.expected_hash), count)
+        for expected_hash, template in zip(self.expected_hash, self.templates):
+            expected_output = '<script type="text/javascript" src="/static/CACHE/js/%s.js"></script>' % (expected_hash, )
+            self.assertIn(expected_output, result)
+            rendered_template = self._render_template(template, engine)
+            self.assertEqual(rendered_template, expected_output + '\n')
+
+
 class OfflineGenerationInlineNonAsciiTestCase(OfflineTestCaseMixin, TestCase):
     templates_dir = "test_inline_non_ascii"
 

compressor/tests/test_templates/test_block_super_base_compressed/base.html

@@ -0,0 +1,10 @@
+{% load compress %}{% spaceless %}
+
+{% compress js %}
+{% block js %}
+    <script type="text/javascript">
+        alert("test using multiple inheritance and block.super");
+    </script>
+{% endblock %}
+{% endcompress %}
+{% endspaceless %}

compressor/tests/test_templates/test_block_super_base_compressed/base2.html

@@ -0,0 +1,8 @@
+{% extends "base.html" %}
+
+{% block js %}{% spaceless %}
+    {{ block.super }}
+    <script type="text/javascript">
+        alert("this alert should be included");
+    </script>
+{% endspaceless %}{% endblock %}

compressor/tests/test_templates/test_block_super_base_compressed/test_compressor_offline.html

@@ -0,0 +1,8 @@
+{% extends "base2.html" %}
+
+{% block js %}{% spaceless %}
+    {{ block.super }}
+    <script type="text/javascript">
+        alert("this alert shouldn't be alone!");
+    </script>
+{% endspaceless %}{% endblock %}

compressor/tests/test_templates/test_block_super_multiple/base2.html

@@ -1,3 +1,10 @@
 {% extends "base.html" %}
 
+{% block js %}{% spaceless %}
+    {{ block.super }}
+    <script type="text/javascript">
+        alert("this alert should be included");
+    </script>
+{% endspaceless %}{% endblock %}
+
 {% block css %}{% endblock %}