Code

Opened 7 years ago

Closed 11 months ago

#3544 closed New feature (fixed)

Fix {% include %} to allow recursive includes

Reported by: David Danier <goliath.mailinglist@…> Owned by: nobody
Component: Template system Version: master
Severity: Normal Keywords: tplrf-patched
Cc: flavio.curella@…, RaceCondition, david@…, carbonXT, newmaniese@…, cvrebert, FunkyBob 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 you try to do recursive includes (for example to display hierarchical data) the include-tag will fail, because it trys to parse the template when loading (at least ConstantIncludeNode does). This can be fixed by saving the loaded templates and avoid multiple loading. Besides this should improve performance, if templates get included multiple times inside a request. Perhaps it would be best to include some form of caching in django.template.loader.get_template?

I have created a simple RecursiveIncludeNode, which caches the loaded templates. The drawback is, that I think the cache stays in RAM over multiple requests (should be no real problem, only if very much templates are used). Also this combines ConstantIncludeNode and IncludeNode (constant templates get cached when parsing, variable templates get cached inside render()).

But I think it would be best to put caching inside the get_template()-function of django (perhaps as a special loader?), as this could improve performance (and is no real drawback).

The Code:

class RecursiveIncludeNode(Node):
	cache = {}
	@staticmethod
	def _get_template(template_name):
		if not RecursiveIncludeNode._has_template(template_name):
			RecursiveIncludeNode._load_template(template_name)
		return RecursiveIncludeNode.cache[template_name]
	@staticmethod
	def _has_template(template_name):
		return RecursiveIncludeNode.cache.has_key(template_name)
	@staticmethod
	def _load_template(template_name):
		RecursiveIncludeNode.cache[template_name] = None
		try:
			RecursiveIncludeNode.cache[template_name] = get_template(template_name)
		except:
			del RecursiveIncludeNode.cache[template_name]
			raise
	def __init__(self, template_name):
		self.template_name = template_name
	def render(self, context):
		try:
			template_name = resolve_variable(self.template_name, context)
			t = RecursiveIncludeNode._get_template(template_name)
			return t.render(context)
		except TemplateSyntaxError, e:
			if settings.TEMPLATE_DEBUG:
				raise
			return ''
		except:
			return '' # Fail silently for invalid included templates.

def rec_include(parser, token):
	bits = token.contents.split()
	if len(bits) != 2:
		raise TemplateSyntaxError, "%r tag takes one argument: the name of the template to be included" % bits[0]
	path = bits[1]
	from django.template.loader_tags import IncludeNode
	if path[0] in ('"', "'") and path[-1] == path[0]:
		template_name = path[1:-1]
		if not RecursiveIncludeNode._has_template(template_name):
			RecursiveIncludeNode._load_template(template_name)
	return RecursiveIncludeNode(bits[1])

register.tag('rec_include', rec_include)

Attachments (7)

rec_include.diff (3.6 KB) - added by Flavio Curella <flavio.curella@…> 7 years ago.
rec_include-r6399.diff (3.2 KB) - added by Flavio Curella <flavio.curella@…> 7 years ago.
patch updated with new variable resolution
3544.contexttemplatecache.diff (2.6 KB) - added by emulbreh 6 years ago.
3544.recursiveincludetest.diff (1.6 KB) - added by emulbreh 6 years ago.
test for 3544.contexttemplatecache.diff
3544.contexttemplatecache.2.diff (10.5 KB) - added by emulbreh 6 years ago.
3544.contexttemplatecache.3.diff (19.4 KB) - added by emulbreh 6 years ago.
3544.contexttemplatecache.3.2.diff (19.4 KB) - added by emulbreh 6 years ago.
typo ..

Download all attachments as: .zip

Change History (43)

comment:1 Changed 7 years ago by Michael Radziej <mir@…>

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed

I checked 'patch needs improvement' because we prefer a patch created by diff as attachment (see contribution docs).

comment:2 Changed 7 years ago by David Danier <goliath.mailinglist@…>

Thats appropriate. The submitted code is not really intended to be a patch, I would call it "solution code example", but this cannot be selected when submitting tickets. ;-)
(As a side-effect it shows how such a cache could be used to merge the two Include-classes)

Besides the example has one drawback I didn't think of when writing it: The cache will survive multiple requests. If you change the template it will not get loaded again. "Fixed" this by clearing the cache on the "request_finished" and "got_request_exception"-signal.
If someone wants to use it, even though it's a hack (perhaps as a workaround, like I do):

from django.core import signals
from django.dispatch import dispatcher

def clear_template_include_cache():
	RecursiveIncludeNode.cache = {}

dispatcher.connect(clear_template_include_cache, signal=signals.request_finished)
dispatcher.connect(clear_template_include_cache, signal=signals.got_request_exception)

As I'm not into the template-code I'm sure there is some better way to implement this.

comment:3 Changed 7 years ago by (removed)

  • Cc ferringb@… added

comment:4 Changed 7 years ago by Flavio Curella <flavio.curella@…>

  • Cc flavio.curella@… added

Changed 7 years ago by Flavio Curella <flavio.curella@…>

comment:5 Changed 7 years ago by Flavio Curella <flavio.curella@…>

attached snippets from Daniel Davier (including cleaning cache by signals) as .diff

Changed 7 years ago by Flavio Curella <flavio.curella@…>

patch updated with new variable resolution

comment:6 Changed 7 years ago by jacob

Can you explain why you want/need recursive includes? A use case would go a long way towards helping me figure out how to handle this ticket.

comment:7 Changed 7 years ago by David Danier <goliath.mailinglist@…>

I use this for a tree-view of comments, as used here:
http://www.webmasterpro.de/coding/article/mehrere-checkboxen-mit-einer-de-bzw-aktivieren.html#comments

Each template includes {% include "display-comments.html" %}, which is the file itself.

Current code runs into endless loop.

Changed 6 years ago by emulbreh

comment:8 Changed 6 years ago by emulbreh

3544.contexttemplatecache.diff does the following:

  • Removes ConstantIncludeNode. {% include %} is always lazy.
  • Adds Template caching to Context (templates will not be cached between requests)

Cons: makes {% include %} lazy (?)

Pros: makes {% include %} lazy and therefore allows recursive includes. The following will not load debug.html if DEBUG is False:
{% if DEBUG %}{% include "debug.html" %}{% endif %}

Todo: cleanup, docs+tests, inclusion_tag should use this.

Changed 6 years ago by emulbreh

test for 3544.contexttemplatecache.diff

Changed 6 years ago by emulbreh

comment:9 Changed 6 years ago by emulbreh

3544.contexttemplatecache.2.diff:

  • Adds a template_cache to Context, accessible via get_template(name) and select_template(name_list).
  • Removes ConstantIncludeNode, {% include %} always uses Context.get_template(). This fixes #3544.
  • Makes ExtendsNode and inclusion_tag use Context.get_template().
  • Adds an optional kwarg loader to Context, which should be a templateloader (something with a get_template(name) method). This fixes #2949.
  • Alters {% with %} to accept multiple assignments: {% with foo as bar and x as y %} or {% with foo as bar, x as y %} (untestet, but normal tests pass).

Planned: I'd like to enable {% include "foo.html" with bar as foo, x as y %}, which would be equivalent to {% with ... %}{% include ... %}{% endwith %}.

Optional: Since {% include %} only needs something with a render(context) method it would work with nodelists. So it would be trivial to add something like

{% sub "my_sub" %}
    {{ foo }}
{% endsub %}

{% include "my_sub" with somevar as foo %}

comment:10 Changed 6 years ago by goliath.mailinglist@…>

Patch sounds great, would like to see this in Django before 1.0 ;-)

Changed 6 years ago by emulbreh

Changed 6 years ago by emulbreh

typo ..

comment:11 Changed 6 years ago by emulbreh

should have checked this "replace" checkbox ..

  • {% include "foo" with value as name %} syntax
  • separator_chars kwarg for smart_split
  • tests for smart_split and extended {% with %} and {% include %} syntax
  • attempt to fix docstrings ..

comment:12 Changed 6 years ago by emulbreh

Since the patch already exceeds the scope of this ticket and I'm not going to make split patches that will be rotten by the time 1.0 is out (and there's a chance to get a design decision again) I'll just keep all my django.template changes at http://einfallsreich.net/projekte/thenewstuff/django/ until then.

The current version contains the (merged) latest patches for #7076 and #5756.

comment:13 Changed 6 years ago by emulbreh

  • Keywords tplrf-patched added

Latest patch is at #7815.

comment:14 Changed 4 years ago by (removed)

  • Cc ferringb@… removed

comment:15 Changed 4 years ago by RaceCondition

  • Cc RaceCondition added

What exactly is keeping the diff from being added to trunk? And meanwhile, is there a workaround for displaying recursive data structures? inclusion tags?

comment:16 Changed 4 years ago by mtredinnick

  • Has patch unset
  • Triage Stage changed from Design decision needed to Accepted

Accepted in principle, since recursive includes would be useful. We'll have to check things work well with 1.2 template changes. Patch will no doubt need refreshing/rewriting. The common path (non-self-recursive includes) should not be impacted (slowed down) at all by allowing recursive includes.

So, providing no adverse side-effects to existing users, this is something to be implemented.

comment:17 Changed 4 years ago by robhudson

I may be missing a lot about the goal, but recursive template inclusion is already possible, though perhaps not as efficient as would be nice? e.g. by setting {% with "comments.html" as template %}, you can trigger the use of IncludeNode instead of ConstantIncludeNode when calling {% include template %} inside the with block.

For a more spelled out example, see: http://blog.elsdoerfer.name/2008/01/22/recursion-in-django-templates/

comment:18 Changed 4 years ago by twoolie

this should definitely be revisited. recursive includes are still sorely needed without with hacks.

p.s. i would LOVE include tags to get the new with notation patch. This is a seriously good thing, especially because almost every time you do an include, you probably need a with anyway.

comment:19 Changed 4 years ago by alex_dev

to David Danier:

just let you know: you can implement recursion by means of django template tags.
Here is example of my implementation.

P.S.:
I think the idea of 'include' tag to allow recursive load is bad: it can outcome in unobvious errors.

comment:20 follow-ups: Changed 4 years ago by David Danier <david.danier@…>

The current implementation does it right I think, it loads the template on compile time to check for syntax errors. This should definately stay, as this is a cool feature.

So what needs to be changes is, that {% include %} needs to hase some kind of registry which templates alreads were loaded and not to reload them. Meaning {% include "some/file.html" %} loads the template and fires up the parser. Every folowing {% include "some/file.html" %} should not do that, regardless of this happens as a recursive call or not. This could even improve performance in non-recursive use cases.

comment:21 in reply to: ↑ 20 Changed 4 years ago by lrekucki

Replying to David Danier <david.danier@team23.de>:

The current implementation does it right I think, it loads the template on compile time to check for syntax errors. This should definately stay, as this is a cool feature.

So what needs to be changes is, that {% include %} needs to hase some kind of registry which templates alreads were loaded and not to reload them. Meaning {% include "some/file.html" %} loads the template and fires up the parser. Every folowing {% include "some/file.html" %} should not do that, regardless of this happens as a recursive call or not. This could even improve performance in non-recursive use cases.

Isn't that what "django.template.loaders.cached.Loader" does ?

comment:22 in reply to: ↑ 20 Changed 4 years ago by twoolie

So what needs to be changes is, that {% include %} needs to hase some kind of registry which templates alreads were loaded and not to reload them. Meaning {% include "some/file.html" %} loads the template and fires up the parser. Every folowing {% include "some/file.html" %} should not do that, regardless of this happens as a recursive call or not. This could even improve performance in non-recursive use cases.

This is obviously beneficial

Isn't that what "django.template.loaders.cached.Loader" does ?

Not really, the caching system returns Template objects, whereas if the include tag doesn't need to touch the entire template discovery stack every time, that would be awesome. an include tag local cache only costs one reference per included template so why not? if it speeds up include performance AND allows recursive imports then why not?
also, embedded with syntax is insanely cool

comment:23 Changed 4 years ago by lbt

  • Cc david@… added

comment:24 Changed 4 years ago by carbonXT

  • Cc carbonXT added

comment:25 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:26 Changed 3 years ago by Mnewman

  • Cc newmaniese@… added
  • Easy pickings unset
  • UI/UX unset

comment:27 Changed 3 years ago by David Danier <david.danier@…>

Needed this again and decided, that you currently can use

{% with template_name="file/to_include.html" %}{% include template_name %}{% endwith %}

as an easy workaround.

comment:28 Changed 3 years ago by camilonova

We should include this patch or document the workaround, it is needed when you want to render information in a tree format, like the comments

comment:29 Changed 3 years ago by prestontimmons

I added a patch to #16147.

It also addresses the problem of this patch, and lets includes happen recursively.

comment:30 Changed 23 months ago by cvrebert

  • Cc cvrebert added

comment:31 Changed 18 months ago by anonymous

Any progress?

It seems quite demanded feature as soon as you deal with hierarchical trees rendering.

comment:32 Changed 16 months ago by prestontimmons

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Updated the patch and added a pull request for review.

https://github.com/django/django/pull/920

comment:33 Changed 11 months ago by FunkyBob

Whilst I agree we no longer need the separate classes, as we have the caching template loader to cover the performance gain, I do question the need for the "quiet" flag.

Under what circumstances do you seen someone wanting to do an include on a template that may not exist?

comment:34 Changed 11 months ago by FunkyBob

  • Cc FunkyBob added

I've made a PR for a simpler patch for this:

https://github.com/django/django/pull/1528

comment:35 Changed 11 months ago by loic84

  • Triage Stage changed from Accepted to Ready for checkin

comment:36 Changed 11 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

In e2f06226ea4a38377cdb69f2f5768e4e00c2d88e:

Improved {% include %} implementation

Merged BaseIncludeNode, ConstantIncludeNode and Include node.

This avoids raising TemplateDoesNotExist at parsing time, allows recursion
when passing a literal template name, and should make TEMPLATE_DEBUG behavior
consistant.

Thanks loic84 for help with the tests.

Fixed #3544, fixed #12064, fixed #16147

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.