Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#27722 closed Bug (fixed)

if a template context is an instance of get_template(), it will raise "TypeError: context must be a dict rather than RequestContext"

Reported by: Sayid Munawar Owned by: nobody
Component: Template system Version: 1.11
Severity: Release blocker Keywords:
Cc: FunkyBob, Aymeric Augustin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Sayid Munawar)

This simple view:

from django.views.generic import TemplateView
from django.template.loader import get_template

class Ngopi(TemplateView):
    template_name = 'base.html'

    def get_context_data(self, **kwargs):
        context = super(Ngopi, self).get_context_data(**kwargs)
        context.update({
            'menu': get_template('menu.html')
            })
        return context

with base.html:

<html>
	<body>
		<h1>Ngeteh</h1>
		{% include menu %}
	</body>
</html>

and menu.html:

<ul>
	<li>Ngeteh</li>
	<li>Ngopi</li>
</ul>

This situation will raise this exception:

Traceback (most recent call last):
  File "/Users/ayik/Repo/Django/django/django/core/handlers/exception.py", line 38, in inner
    response = get_response(request)
  File "/Users/ayik/Repo/Django/django/django/core/handlers/base.py", line 217, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/Users/ayik/Repo/Django/django/django/core/handlers/base.py", line 215, in _get_response
    response = response.render()
  File "/Users/ayik/Repo/Django/django/django/template/response.py", line 107, in render
    self.content = self.rendered_content
  File "/Users/ayik/Repo/Django/django/django/template/response.py", line 84, in rendered_content
    content = template.render(context, self._request)
  File "/Users/ayik/Repo/Django/django/django/template/backends/django.py", line 66, in render
    return self.template.render(context)
  File "/Users/ayik/Repo/Django/django/django/template/base.py", line 207, in render
    return self._render(context)
  File "/Users/ayik/Repo/Django/django/django/template/base.py", line 199, in _render
    return self.nodelist.render(context)
  File "/Users/ayik/Repo/Django/django/django/template/base.py", line 990, in render
    bit = node.render_annotated(context)
  File "/Users/ayik/Repo/Django/django/django/template/base.py", line 957, in render_annotated
    return self.render(context)
  File "/Users/ayik/Repo/Django/django/django/template/loader_tags.py", line 212, in render
    return template.render(context)
  File "/Users/ayik/Repo/Django/django/django/template/backends/django.py", line 64, in render
    context = make_context(context, request, autoescape=self.backend.engine.autoescape)
  File "/Users/ayik/Repo/Django/django/django/template/context.py", line 285, in make_context
    raise TypeError('context must be a dict rather than %s.' % context.__class__.__name__)
TypeError: context must be a dict rather than RequestContext.

This is fine with django 1.10, but not master (mine is currently 1.11.dev20170109230310)

a quick workaround is to edit django/template/backends/django.py Line 63 to be like this:

    def render(self, context=None, request=None):
        if isinstance(context, dict):  # <-- my temporary workaround
            context = make_context(context, request, autoescape=self.backend.engine.autoescape)
        try:
            return self.template.render(context)
        except TemplateDoesNotExist as exc:
            reraise(exc, self.backend)

Change History (18)

comment:1 by Tim Graham, 7 years ago

Does get_template() create some behavior difference as opposed to context.update({'menu': 'menu.html'})? See #27258 for the reason this error was added. Your proposed change effectively removes that fix.

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:2 by Sayid Munawar, 7 years ago

Resolution: fixed
Status: newclosed

sorry Tim, my proposed change didn't mean to really fix the problem, just a workaround so that i can continue to code.

using context.update({'menu': 'menu.html'}) solved my problem. it just weird since it was working fine until 1.10

comment:3 by Tim Graham, 7 years ago

Resolution: fixedwontfix

It's allowed to call {% include %} with a django.template.base.Template but not django.template.backends.django.Template (added in 5cdacbda034af928f5033c9afc7b50ee0b13f75c).

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:4 by Tim Graham, 7 years ago

Cc: FunkyBob added

Curtis, as the author of 5cdacbda034af928f5033c9afc7b50ee0b13f75c, I wonder if you have any input here? I'm not happy that I broke backwards compatibility (due to #27258) with such a cryptic error message here. At least the {% include %} docs that say, "The variable may also be any object with a render() method that accepts a context." might need some clarification.

The use case is described in the django-material issue.

comment:5 by Sayid Munawar, 7 years ago

Description: modified (diff)

comment:6 by Sayid Munawar, 7 years ago

Hi Tim, sorry to continue to discuss about this, from your last comment:

It's allowed to call {% include %} with a django.template.base.Template but not django.template.backends.django.Template

but get_template() will return django.template.backends.django.Template.

further investigation using ipdb, i found that make_context will raise type_error since the context is already an instance of RequestContext in the 2nd iteration (see below):

> /Users/ayik/Repo/Django/django/django/template/backends/django.py(65)render()
     63     def render(self, context=None, request=None):
     64         import ipdb; ipdb.set_trace()
---> 65         context = make_context(context, request, autoescape=self.backend.engine.autoescape)
     66         try:
     67             return self.template.render(context)

ipdb> type(context)
<class 'dict'>
ipdb> c
> /Users/ayik/Repo/Django/django/django/template/backends/django.py(65)render()
     63     def render(self, context=None, request=None):
     64         import ipdb; ipdb.set_trace()
---> 65         context = make_context(context, request, autoescape=self.backend.engine.autoescape)
     66         try:
     67             return self.template.render(context)

ipdb> type(context)
<class 'django.template.context.RequestContext'>
ipdb>

comment:7 by Tim Graham, 7 years ago

The question in my mind is whether or not to keep the fix for #27258 (maybe it's too restrictive -- in your use case, I'm not sure if django.Template.render() with RequestContext will give any surprising behaviors) or maybe some change could be made to {% include %} to either raise a more descriptive error message or to allow the use case to keep working.

comment:8 by Tim Graham, 7 years ago

Resolution: wontfix
Severity: NormalRelease blocker
Status: closednew
Triage Stage: UnreviewedAccepted
Version: master1.11

At least documentation must be clarified.

comment:9 by Sayid Munawar, 7 years ago

Quoting Mikhail Podgurskiy from the duplicated ticket:

As stated in the django docs, the {% include %} tag could accept a template instance to render - ​https://docs.djangoproject.com/en/1.10/ref/templates/builtins/#include

include

Loads a template and renders it with the current context. This is a way of “including” other templates within a template.

...

The variable may also be any object with a render() method that accepts a context. This allows you to reference a compiled Template in your context.

This leads to RequestContext from the main template been passed to the included template.render method and this became prohibited within https://code.djangoproject.com/ticket/27258 changeset

comment:10 by Tim Graham, 7 years ago

Cc: Aymeric Augustin added

Aymeric, do you have any advice on this one? It seems the technique described in this ticket doesn't have the issue of no context processors running (#27258) because the RequestContext that the django.Template() is called with already has the context processors run for the parent template. If backwards-compatibility must be kept for this case, I'm inclined to revert 6a7495051304d75865add6ff96422018984e1663 rather than try to devise a more complicated solution.

comment:11 by Aymeric Augustin, 7 years ago

I didn't know this was possible. I'm not seeing obvious use cases -- why would you pass a template instance rather than a template identifier and let the template loader do its job? But indeed it's documented.

Anyway, {% include compiled_template %} path wraps a Context in another Context, while {% include 'template_to_compile.html' %} doesn't. I don't see why these two code paths handle context differently when compiled_template = get_template('template_to_compile.html'). This is likely a bug or a potential source of bugs, and the root cause of the problem described here.

Of course, if no one wants to look into that, reverting 6a7495051304d75865add6ff96422018984e1663 is a reasonable way to avoid a backwards incompatibility. It would still be worth creating another ticket for resolving the inconsistency in context handling.

comment:12 by Tim Graham, 7 years ago

A use case for calling {% include %} with a compiled template is described in #17356. The use case was for {% include %} with a django.template.base.Template, however, it had the side effect of also allowing django.template.backends.django.Template since the check is if the template argument has a render() method.

I don't think the issue here has to do with how contexts are handled but rather that {% include "string" %} uses engine.get_template() to load the template which returns base.Template. The use case of this ticket instead uses loader.get_template() which returns a django.Template.

In ticket:27258#comment:6 you said, "template.backends.django.Template.render mustn't be called with a RequestContext (I feel strongly about that one)." but that's what happening in the latter case (apparently without any ill effects?).

in reply to:  12 comment:13 by Aymeric Augustin, 7 years ago

Replying to Tim Graham:

A use case for calling {% include %} with a compiled template is described in #17356. The use case was for {% include %} with a django.template.base.Template, however, it had the side effect of also allowing django.template.backends.django.Template since the check is if the template argument has a render() method.

Perhaps {% include %} could unwrap the django.template.base.Template from the django.template.backends.django.Template when this happens, after checking that both templates were loaded with the same engine? If you load them with different engines, you may get hilarious results, especially if you find XSS vulnerabilities funny and one engine has autoescape turned off.

Alternatively it could reject django.template.base.Template instances, but that would only result in another ticket asking to support it.

In ticket:27258#comment:6 you said, "template.backends.django.Template.render mustn't be called with a RequestContext (I feel strongly about that one)." but that's what happening in the latter case (apparently without any ill effects?).

You could say that mostly me being obsessed with technical correctness and never having really come to terms with duck-typing and you wouldn't be totally wrong but not totally right either.

The underlying motivation was to discourage confusion between:

  • template.backends.django.Template which is the backend-agnostic, compatible API that expects a dict and optionally a request
  • django.template.base.Template which is the Django-templates-only API that expects a Context or RequestContext

The root cause of the bug we're discussing here is one type being used where the other is expected. So that motivation isn't without merit.

Last edited 7 years ago by Aymeric Augustin (previous) (diff)

comment:14 by Tim Graham, 7 years ago

Makes sense, here's a PR for the suggested approach. I didn't investigate the issue of restricting {% include %} to the same engine yet. Actually, there's ticket #24577 for that. I didn't think about if it's a prerequisite for making this change.

comment:15 by Tim Graham, 7 years ago

Has patch: set

I attached a start of #24577 to that ticket, but I don't think that needs to be completed now or backported to 1.11 as the fix for this ticket does. Do you see the situation differently, Aymeric?

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:16 by Aymeric Augustin, 7 years ago

I think you can go ahead with this and leave engine restrictions to #24577.

comment:17 by GitHub <noreply@…>, 7 years ago

Resolution: fixed
Status: newclosed

In fe2d288:

Fixed #27722 -- Reallowed using django.Template in {% include %}.

comment:18 by Tim Graham <timograham@…>, 7 years ago

In 9658f0d2:

[1.11.x] Fixed #27722 -- Reallowed using django.Template in {% include %}.

Backport of fe2d2884345e1e6a1daadd76e9404c62791ab589 from master

Note: See TracTickets for help on using tickets.
Back to Top