Opened 9 years ago

Closed 9 years ago

Last modified 5 years ago

#23758 closed Bug (fixed)

Going beyond 5 levels of subqueries causes AssertionError in bump_prefix

Reported by: Richard Howard Owned by: Piotr Pawlaczek
Component: Database layer (models, ORM) Version: 1.7
Severity: Normal Keywords: bump_prefix subquery alias
Cc: 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

Regression from Django 1.6

The following code in the interactive shell will reproduce the issue:

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

...
... 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.

Change History (13)

comment:1 by Prathik Rajendran M, 9 years ago

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.

comment:2 by Peter Inglesby, 9 years ago

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?

comment:3 by Anssi Kääriäinen, 9 years ago

Triage Stage: UnreviewedAccepted

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.

comment:4 by Piotr Pawlaczek, 9 years ago

Owner: changed from nobody to Piotr Pawlaczek
Status: newassigned

comment:5 by Piotr Pawlaczek, 9 years ago

There is a pull request created for this: https://github.com/django/django/pull/3557

comment:6 by Markus Holtermann, 9 years ago

Has patch: set
Needs tests: set
Patch needs improvement: set

comment:7 by Tim Graham, 9 years ago

Needs tests: unset
Patch needs improvement: unset

comment:8 by Anssi Kääriäinen, 9 years ago

Patch needs improvement: set

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.

comment:9 by Tim Graham, 9 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In 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.

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

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

In 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

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In 0cb4062:

Refs #23758 -- Used RecursionError instead of RuntimeError to raise nested subquery errors.

RecursionError was introduced in Python 3.5 and subclasses RuntimeError.

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