[Django] #22232: Template extends tag can cause recursion
Django
--------------------------------------+--------------------
Reporter: galedragon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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 :)
--
Ticket URL: <https://code.djangoproject.com/ticket/22232>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: galedragon | Owner: nobody
Type: Bug | Status: new
Component: Template system | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+------------------------------------
Changes (by russellm):
* needs_better_patch: => 0
* needs_tests: => 0
* easy: 0 => 1
* needs_docs: => 0
* type: Cleanup/optimization => Bug
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:1>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Component: Template system | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Saidin7979):
* owner: nobody => Saidin7979
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:2>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by Saidin7979):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:3>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by russellm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:4>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
* cc: nathanrandal@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:5>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by Saidin7979):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:6>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:7>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by Saidin7979):
Any thoughts on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:8>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by frog32):
Why not add the current template to the list during the instantiation?
Then you would catch a recursion error one step earlier?
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:9>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by areski):
To be noted that this PR is being refactored
https://github.com/django/django/pull/3014
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:10>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
* cc: areski@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:11>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:12>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:13>
Django
Reporter: galedragon | Owner: Saidin7979
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
---------------------------------+--------------------------------------
* needs_better_patch: 0 => 1
Comment:
Please uncheck "Patch needs improvement" when the comments on the PR have
been updated, thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:14>
Django
Reporter: galedragon | Owner: ChillarAnand
Type: Bug | Status: assigned
Component: Template system | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by ChillarAnand):
* owner: Saidin7979 => ChillarAnand
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:15>
Django
Reporter: galedragon | Owner: ChillarAnand
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
---------------------------------+----------------------------------------
* cc: f.ssat95@… (added)
Comment:
If it's still pending, I'd like to work on it from scratch. Should I ?
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:16>
Django
Reporter: galedragon | Owner: darkryder
Type: Bug | Status: assigned
Component: Template system | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
---------------------------------+-------------------------------------
* owner: ChillarAnand => darkryder
Comment:
Please find PR here: https://github.com/django/django/pull/2431
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:17>
Django
Reporter: galedragon | Owner: darkryder
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by MarkusH):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:18>
Django
Reporter: galedragon | Owner: darkryder
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:19>
Django
Reporter: galedragon | Owner: darkryder
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by darkryder):
Replying to [comment:19 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:20>
Django
Reporter: galedragon | Owner: darkryder
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by prestontimmons):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:21>
Django
Reporter: galedragon | Owner: darkryder
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 1 => 0
* easy: 1 => 0
* stage: Ready for checkin => Accepted
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:22>
Django
Reporter: galedragon | Owner: darkryder
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:23>
Django
Reporter: galedragon | Owner: darkryder
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by prestontimmons):
This is fixed now that #15053 is merged.
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:24>
Django
Reporter: galedragon | Owner: darkryder
Component: Template system | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by prestontimmons):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/22232#comment:25>