Opened 6 years ago

Closed 6 years ago

Last modified 16 months 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 Changed 6 years ago by 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.

comment:2 Changed 6 years ago by 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?

comment:3 Changed 6 years ago by Anssi Kääriäinen

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 Changed 6 years ago by Piotr Pawlaczek

Owner: changed from nobody to Piotr Pawlaczek
Status: newassigned

comment:5 Changed 6 years ago by Piotr Pawlaczek

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

comment:6 Changed 6 years ago by Markus Holtermann

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

comment:7 Changed 6 years ago by Tim Graham

Needs tests: unset
Patch needs improvement: unset

comment:8 Changed 6 years ago by Anssi Kääriäinen

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 Changed 6 years ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:10 Changed 6 years ago by Tim Graham <timograham@…>

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 Changed 6 years ago by Tim Graham <timograham@…>

comment:12 Changed 6 years ago by 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

comment:13 Changed 16 months ago by 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.

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