Template extends tag can cause recursion

[galedragon]

I just wanted to bring this to the community's attention.

If someone has this in "somepage.html":
{% extends "somepage.html" %}

So basically you try to extend the page you're working on.

It causes a Recursion error. It's an easy fix and can be fairly obvious, but I was wondering if this was the intentional effect.

Filing under optimization because it's not exactly a bug.

First ticket, so be gentle :)

[Russell Keith-Magee]

I'd call it a bug; hitting a system level recursion error as a result of accidentally naming the wrong parent in an extends should't be the expected behaviour, if only because it's computationally expensive to hit maximum stack depth.

It's definitely an error though - I can't think of any way an internally recursive template could render in a meaningful way.

So - yes, this should be caught at the template load level -- a simple "if parent == self, raise error" check should do the trick.

[Nathan Smith]

Pull Request: ​https://github.com/django/django/pull/2431

This patch checks the template node list for self extension.

During the exploration of this issue we noticed that there may be another case when a recursion error could occur. This issue addresses the case when a template extends itself, but a circular extension could occur further up the ancestry. Should I handle this case as well? It is not immediately clear how I could do this efficiently.

[Russell Keith-Magee]

Hrm. This is an interesting problem. On the one hand, it would be nice to raise a humane error message, but if it's only going to catch the one-level recursion case, I'm starting to wonder if it's worth the effort. Adding a potentially expensive check to the top of every template load so that you can present a nicer error message in the edge case of recursive templates seems like too high a price to pay.

If we can't find an efficient way to catch the complex circular case, I'm inclined to turn this into a documentation fix (ie. noting in the discussion around extends that circular loops may cause stack overflow errors).

One option might be to handle this at render time, rather than at load time. If you put something in the context that tracks which templates have been used, and you try to re-use a template name, that indicates a circular loop, and you can raise an error. This should be relatively cheap (a set lookup & insertion on each new template file used); as long as we can find a clean place to track the data, it might suffice as a solution.

[Nathan Smith]

I updated the pull request (​https://github.com/django/django/pull/2431) with your recommended solution. It keeps a list of the extended templates in the render_context if there are any ExtendsNode nodes. I have removed the previous attempt as this new approach catches both the old and the new. Let me know what you think.

[Nathan Smith]

Any thoughts on this?

[Marc Egli]

Why not add the current template to the list during the instantiation? Then you would catch a recursion error one step earlier?

[Areski Belaid]

To be noted that this PR is being refactored ​https://github.com/django/django/pull/3014

[anonymous]

Apologies, looks like my comment from yesterday didn't go though. I refactored Nathan's pull request at the PyConAu sprints yesterday. Was my first time working with the Django source. Thanks for the suggestions areski, will look into them now.

[kateseamer]

Note that the message above was from me, looks like the login form that is presented after registration does not work. I have created a new ticket here: https://code.djangoproject.com/ticket/23204

[Tim Graham]

Please uncheck "Patch needs improvement" when the comments on the PR have been updated, thanks.

[Sambhav Satija]

If it's still pending, I'd like to work on it from scratch. Should I ?

[Sambhav Satija]

Please find PR here: ​https://github.com/django/django/pull/2431

[Preston Timmons]

FYI - This behavior is also addressed as part of #15053. If this patch is committed, it's implementation via a context variable may be obsoleted by the new origin api discussed here:

​https://groups.google.com/forum/#!topic/django-developers/VFBLAoPSplI

[Sambhav Satija]

Replying to prestontimmons:

FYI - This behavior is also addressed as part of #15053. If this patch is committed, it's implementation via a context variable may be obsoleted by the new origin api discussed here:

​https://groups.google.com/forum/#!topic/django-developers/VFBLAoPSplI

Since this was my first attempt at contributing, I have no clue what the logical next step should be. Should this ticket be put on hold or is there any other way to proceed? And if the Loader API is implemented, I guess that fixes this ticket as well.

[Preston Timmons]

Your patch was marked RFC, so you don't need to change anything unless you get feedback otherwise, especially if this patch is considered a bug fix that can land in Django 1.8. My note above is more directed toward potential committers if this only goes into the 1.9 release cycle.

[Tim Graham]

Preston, let's assume your work will be committed for 1.9. Should we close this ticket or is there anything remaining to address the goals of this ticket that isn't addressed in your current PR for #15053?

[Preston Timmons]

There's one edge case not strictly fixed by #15053: 3rd party loaders that haven't updated to the recursive api through the deprecation period. The PR does have an explicit check for this, but it's something that's easy to overlook. Therefore, I don't feel strongly about closing this ticket yet.

[Preston Timmons]

This is fixed now that #15053 is merged.