Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#24555 closed Bug (fixed)

Wrong content in inclusion templatetags with variable parent template

Reported by: Benjamin Rigaud Owned by: Tim Graham <timograham@…>
Component: Template system Version: 1.8rc1
Severity: Release blocker Keywords: templatetags extends block cache
Cc: Preston Timmons Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If two custom inclusion tags specify their parent template through their context and use the same block name, the rendered content of the second tag is replaced by the content of the first one

Sandbox project: https://github.com/benjaminrigaud/extends-templatetags-bug
Gist: https://gist.github.com/benjaminrigaud/e646f51e1a6dfe232c68

It works fine if we replace the custom tags with standard include tags.

It is a regression from 1.7 (tested with 1.7.7).

It may be related to the cache mechanism of the BlockContext: https://github.com/django/django/blob/master/django/template/loader_tags.py#L115

Attachments (1)

repro-24555.diff (3.1 KB ) - added by Baptiste Mispelon 10 years ago.
Reproduction testcase

Download all attachments as: .zip

Change History (13)

comment:1 by Baptiste Mispelon, 10 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Hi,

I can indeed confirm the regression and I bisected it down to commit e53495ba3352c2c0fdb6178f2b333c30cb6b5d46.

Bumping to release blocker status because of the regression.

Thanks.

by Baptiste Mispelon, 10 years ago

Attachment: repro-24555.diff added

Reproduction testcase

comment:2 by Aymeric Augustin, 10 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:3 by Preston Timmons, 10 years ago

The problem here happens on line 1325:

return self.nodelist.render(new_context)

Unlike Template.render, NodeList.render doesn't push the context stack. Hence, blocks stored in the render_context persist where they should not.

There are a few solutions:

1) Wrap this line in with new_context.render_context.push():.

2) Remove the internal InclusionNode caching and just call t.render(new_context).

3) Cache the template object rather than the nodelist and use that.

The easiest solution is to remove internal caching and leave it to the loaders. That also fixes #23441 that's caused by InclusionNode setting self.nodelist at render time. The downside is a performance regression if InclusionNode is used in a for loop.

If internal caching is kept, it should be done in the render_context rather than on the node.

Last edited 10 years ago by Preston Timmons (previous) (diff)

comment:4 by Aymeric Augustin, 10 years ago

Cc: Preston Timmons added

Preston, honestly, you understand this code much better than I do. What is your recommendation?

comment:5 by Preston Timmons, 10 years ago

I'll write a patch for option 3 so we don't introduce a performance regression. Preferably, we can fix this at the engine level later so the node doesn't have to take care of it's own caching.

comment:6 by Aymeric Augustin, 10 years ago

Owner: Aymeric Augustin removed
Status: assignednew

Great. Thank you so much for your help!

comment:7 by Preston Timmons, 10 years ago

Has patch: set

comment:8 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham <timograham@…>, 10 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 0808ccc:

Fixed #23441, #24555 -- Improved the behavior of InclusionNode.

This change:

  • Makes the InclusionNode cache-safe by removing render-time side effects to its nodelist.
  • Ensures the render_context stack is properly scoped and reset by updating the render call to use Template.render rather than Nodelist.render.

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

In 5c63c45:

[1.8.x] Fixed #23441, #24555 -- Improved the behavior of InclusionNode.

This change:

  • Makes the InclusionNode cache-safe by removing render-time side effects to its nodelist.
  • Ensures the render_context stack is properly scoped and reset by updating the render call to use Template.render rather than Nodelist.render.

Backport of 0808ccce3808235c5b5a56e3f689cec0d4bc0ebf from master

comment:11 by Neal Todd, 10 years ago

Although the ticket is closed I'm adding this comment in case anyone stumbles across the same esoteric gotcha I came across related to this change:

After updating a project from 1.8c1 to 1.8 a bunch of tests that had been running cleanly started failing on assertFormError in the format of AssertionError: The form 'form' in context X does not contain the field 'foo'.

Long story short, the template had a 'main' form that was supplied via a context variable named 'form', but the base template also used an inclusion tag to render a site search form, and the inclusion tag's template was also supplied via a context variable named 'form'. The caching introduced in this commit meant that the inclusion tag's template now also appeared in the response.context, and so, when calling self.assertFormError(response, "form", "foo", "expected error message") assertFormError looped over the context and found a form (the search form) without the intended field foo (as well as finding it as expected on the main form).

The simple fix in this case was to scope the context key name of the search form to, e.g. 'search_form' for the inclusion tag's template. However as, ordinarily, a clash of key names used in the contexts for different templates shouldn't matter, I thought I'd note it here as it left me scratching my head for a while until I tracked in down.

comment:12 by Preston Timmons, 10 years ago

I see. This behavior change isn't due to caching. Instead it's due to the change from self.nodelist.render() to template.render(). This causes the template_rendered signal to be sent for inclusion tags, whereas it wasn't before. The listeners on this signal update response.context, response.templates, etc.

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