Going beyond 5 levels of subqueries causes AssertionError in bump_prefix

[Richard Howard]

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.

[Prathik Rajendran M]

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.

[Peter Inglesby]

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?

[Anssi Kääriäinen]

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.

[Piotr Pawlaczek]

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

[Anssi Kääriäinen]

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.

[Tim Graham <timograham@…>]

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.

[Tim Graham <timograham@…>]

In 63dee96cc7a3c929e16f88f2039812490b4b02b2:

Fixed test from refs #23758.

[Tim Graham <timograham@…>]

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0cb4062:

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

RecursionError was introduced in Python 3.5 and subclasses RuntimeError.