nalourie / django-macros

macros accepting positional and keyword arguments, and repeated block tags in the django template system.
MIT License
27 stars 8 forks source link

AttributeError: object has no attribute 'resolve' #7

Open drdaeman opened 9 years ago

drdaeman commented 9 years ago

Hi,

I've experienced a weird condition at templatetags/macros/macros.py:L56: it seems that v could be something other than Variable (say SafeText or None) so the template rendering fails with an AttributeError exception. I haven't ever seen this in development environment and it only happened to me on second or third request to the production server, so I suspect it has something to do with CachedTemplateLoader.

Template example:

{% macro modal_header title=None %}
  <div class="modal-header">
    <h4 class="modal-title">{{ title|default:"..." }}</h4>
  </div>
{% endmacro %}

Exception: None object has no attribute 'resolve. Similar exceptions, but mentioningSafeTexthappened when I hadtitle="Some text"instead oftitle=None`.

Traceback:

54.     # recall all defaults are template variables
55.    self.kwargs = {k: v.resolve(context)
56.                   for k, v in self.kwargs.items()}

Captured frame contained k = u'title' and v = None (unfortunately, Sentry captures reprs and doesn't dig into objects so it lost exact type information and ability to introspect into self).

A quick-and-dirty workaround v.resolve(context) if isinstance(v, template.Variable) else v seemingly did the trick, but sadly I don't have any idea how such values occurred in self.kwargs in the first place and whenever my fix is correct. Don't have a time at the moment to try to replicate this in a clean empty project - sorry! - but wanted to report this right away.

Any ideas/suggestions?

drdaeman commented 9 years ago

I investigated and understood the bug.

I can now confirm, the issue is indeed with django.template.loaders.cached.Loader — on the second Template.render invocation self.kwargs are already rewritten with resolved values from the previous render call.

Here's the essence of the issue:

t = Template(
    "{% load macros %}"
    "{% macro test kwarg=foo %}{{ kwarg }}{% endmacro %}"
    "{% use_macro test %}"
)
c1 = Context({"foo": "one"})
c2 = Context({"foo": "two"})
assert t.render(c1) != t.render(c2), "The results must be different"

My originally proposed fix was completely wrong. The correct solution is to not do anything to self.kwargs in DefineMacroNode.render, but to store everything in independent variable. Then, use that variable in UseMacroNode.render instead of self.macro.kwargs. E.g. something along the lines of:

--- a/test_project_python27/test_project_python27/macros/templatetags/macros.py
+++ b/test_project_python27/test_project_python27/macros/templatetags/macros.py
@@ -52,8 +52,8 @@ class DefineMacroNode(template.Node):
         # variables.
         #
         # recall all defaults are template variables
-        self.kwargs = {k: v.resolve(context)
-                       for k, v in self.kwargs.items()}
+        self.kwresolved = {k: v.resolve(context)
+                           for k, v in self.kwargs.items()}

         # empty string - {% macro %} tag has no output
         return ''
@@ -202,7 +202,7 @@ class UseMacroNode(template.Node):
                 context[arg] = ""

         # add all of use_macros kwargs into context
-        for name, default in self.macro.kwargs.items():
+        for name, default in self.macro.kwresolved.items():
             if name in self.kwargs:
                 context[name] = self.kwargs[name].resolve(context)
             else:
nalourie commented 9 years ago

Thanks so much!

I'll try to get a fix in this week.

Best, Nick On Sep 8, 2015 12:04 PM, "Aleksey Zhukov" notifications@github.com wrote:

I investigated and understood the bug.

I can now confirm, the issue is indeed with django.template.loaders.cached.Loader — on the second Template.render invocation self.kwargs are already rewritten with resolved values from the previous render call.

Here's the essence of the issue:

t = Template( "{% load macros %}" "{% macro test kwarg=foo %}{{ kwarg }}{% endmacro %}" "{% use_macro test %}" ) c1 = Context({"foo": "one"}) c2 = Context({"foo": "two"})assert t.render(c1) != t.render(c2), "The results must be different"

My originally proposed fix was completely wrong. The correct solution is to not do anything to self.kwargs in DefineMacroNode.render, but to store everything in independent variable. Then, use that variable in UseMacroNode.render instead of self.macro.kwargs. E.g. something along the lines of:

--- a/test_project_python27/test_project_python27/macros/templatetags/macros.py+++ b/test_project_python27/test_project_python27/macros/templatetags/macros.py@@ -52,8 +52,8 @@ class DefineMacroNode(template.Node):

variables.

     #
     # recall all defaults are template variables-        self.kwargs = {k: v.resolve(context)-                       for k, v in self.kwargs.items()}+        self.kwresolved = {k: v.resolve(context)+                           for k, v in self.kwargs.items()}
     # empty string - {% macro %} tag has no output
     return ''@@ -202,7 +202,7 @@ class UseMacroNode(template.Node):
             context[arg] = ""

     # add all of use_macros kwargs into context-        for name, default in self.macro.kwargs.items():+        for name, default in self.macro.kwresolved.items():
         if name in self.kwargs:
             context[name] = self.kwargs[name].resolve(context)
         else:

— Reply to this email directly or view it on GitHub https://github.com/nalourie/django-macros/issues/7#issuecomment-138612791 .

drdaeman commented 9 years ago

Glad to help. No need to rush, I don't think it's anything urgent.

For a time being, for our own uses, I've just replaced PyPI dependency to my fork. You may consider cherry-picking my commit to save time, if you consider it a proper solution.

(Completely unrelated to the issue, I've also did some source tree cleanups and added tox testing against various Python&Django versions, so you may be interested in some other changes in my fork. Not sending proper pull requests yet, because my changes are somewhat dirty and I'm not sure I really did things the right way.)