[Django] #23758: Going beyond 5 levels of subqueries causes AssertionError in bump_prefix
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7
(models, ORM) | Keywords: bump_prefix subquery
Severity: Normal | alias
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Regression from Django 1.6
The following code in the interactive shell will reproduce the issue:
{{{#!python
from django.contrib.auth.models import User
i = 0
x = User.objects.filter(pk=1)
while True:
x = User.objects.filter(pk__in=x)
i+=1
}}}
In django 1.7 it will throw the following AssertionError
{{{#!python
...
... in bump_prefix(self, outer_query)
829 while self.alias_prefix in self.subq_aliases:
830 self.alias_prefix = chr(ord(self.alias_prefix) + 1)
--> 831 assert self.alias_prefix < 'Z'
832 self.subq_aliases =
self.subq_aliases.union([self.alias_prefix])
833 outer_query.subq_aliases =
outer_query.subq_aliases.union(self.subq_aliases)
AssertionError:
}}}
while in django 1.6 it will loop infinitely.
I have tested this using multiple models in the loop, with the same
outcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/23758>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7
Severity: Normal | Triage Stage:
Keywords: bump_prefix | Unreviewed
subquery alias | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by prathik):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
While debugging I saw that the list self.subq_aliases grows from U to Z in
bump_prefix and upon reaching Z the AssertionError is thrown, not sure if
it is done on purpose to prevent infinite loops from happening or a side-
effect of something else.
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7
Severity: Normal | Triage Stage:
Keywords: bump_prefix | Unreviewed
subquery alias | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by inglesp):
The behaviour changed in dcdc579d162b750ee3449e34efd772703592faca.
I suppose a fix would be to start the aliases at A and, after reaching Z,
use AA, AB, and so on. But I'm not sure that such a deeply nested query
is going to be hit in real life. richardhowardsparx, how did you hit
this?
It does feel like we're mis-using `assert` -- in a framework like Django,
I think they should be used more to guard against the possibility that the
framework has got itself into an invalid state, while in this case, it's
being used to guard against arguably-invalid user code. Would a better
error message be an acceptable resolution?
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.7
Severity: Normal | Triage Stage: Accepted
Keywords: bump_prefix | Needs documentation: 0
subquery alias | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* stage: Unreviewed => Accepted
Comment:
The breakage is non-intentional. Before Django used conflicting aliases
for nested subqueries that could result to problems in some cases.
We could just extend the alphabet, start from Z and then continue from AA
after that. Some limit for recursion depth might make sense, but 5 is
definitely too low.
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Component: Database layer | Status: assigned
(models, ORM) | Version: 1.7
Severity: Normal | Resolution:
Keywords: bump_prefix | Triage Stage: Accepted
subquery alias | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by piotrpawlaczek):
* status: new => assigned
* owner: nobody => piotrpawlaczek
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Component: Database layer | Status: assigned
(models, ORM) | Version: 1.7
Severity: Normal | Resolution:
Keywords: bump_prefix | Triage Stage: Accepted
subquery alias | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by piotrpawlaczek):
There is a pull request created for this:
https://github.com/django/django/pull/3557
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Component: Database layer | Status: assigned
(models, ORM) | Version: 1.7
Severity: Normal | Resolution:
Keywords: bump_prefix | Triage Stage: Accepted
subquery alias | Needs documentation: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by MarkusH):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Component: Database layer | Status: assigned
(models, ORM) | Version: 1.7
Severity: Normal | Resolution:
Keywords: bump_prefix | Triage Stage: Accepted
subquery alias | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Component: Database layer | Status: assigned
(models, ORM) | Version: 1.7
Severity: Normal | Resolution:
Keywords: bump_prefix | Triage Stage: Accepted
subquery alias | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
The patch looks good except that the prefix_gen() is a bit too hard to
read, and the loop using prefix_gen could be simplified.
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Type: Bug | Status: assigned
Component: Database layer | Version: 1.7
(models, ORM) |
Keywords: bump_prefix | Triage Stage: Ready for
subquery alias | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Keywords: bump_prefix | Triage Stage: Ready for
subquery alias | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"41fc1c0b5eac156e200a10233c7c9210a1c0fed8"]:
{{{
#!CommitTicketReference repository=""
revision="41fc1c0b5eac156e200a10233c7c9210a1c0fed8"
Fixed #23758 -- Allowed more than 5 levels of subqueries
Refactored bump_prefix() to avoid infinite loop and allow more than
than 5 subquires by extending the alphabet to use multi-letters.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Type: Bug | Status: closed
(models, ORM) |
Keywords: bump_prefix | Triage Stage: Ready for
subquery alias | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"63dee96cc7a3c929e16f88f2039812490b4b02b2"]:
{{{
#!CommitTicketReference repository=""
revision="63dee96cc7a3c929e16f88f2039812490b4b02b2"
Fixed test from refs #23758.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: richardhowardsparx | Owner:
Type: Bug | Status: closed
(models, ORM) |
Keywords: bump_prefix | Triage Stage: Ready for
subquery alias | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"e11ff3975f69b96521d94417bbe4125f4abb2774"]:
{{{
#!CommitTicketReference repository=""
revision="e11ff3975f69b96521d94417bbe4125f4abb2774"
[1.7.x] Fixed #23758 -- Allowed more than 5 levels of subqueries
Refactored bump_prefix() to avoid infinite loop and allow more than
than 5 subquires by extending the alphabet to use multi-letters.
Backport of 41fc1c0b5eac156e200a10233c7c9210a1c0fed8 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:12>
Django
-------------------------------------+-------------------------------------
| Pawlaczek
Type: Bug | Status: closed
Component: Database layer | Version: 1.7
(models, ORM) |
Keywords: bump_prefix | Triage Stage: Ready for
subquery alias | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"0cb40624828f3590ea2460e2e0cf1bd5a2b52496" 0cb4062]:
{{{
#!CommitTicketReference repository=""
revision="0cb40624828f3590ea2460e2e0cf1bd5a2b52496"
Refs #23758 -- Used RecursionError instead of RuntimeError to raise nested
subquery errors.
RecursionError was introduced in Python 3.5 and subclasses RuntimeError.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23758#comment:13>