I'm working on porting [https://lavasoftware.org lava] to Django 3.2. When
I run the test suite, I find 2 issues. One is #32690, for which I have the
corresponding patch backported locally. The other one is this is a bunch
of crashes that look like this:
E django.db.utils.ProgrammingError: more than one row
returned by a subquery used as an expression
I wasn't able to produce a minimal reproducer app yet, but I will try to
point to the relevant code.
in lava we have a few special model managers for handling access control.
They are here:
https://git.lavasoftware.org/lava/lava/-/blob/master/lava_scheduler_app/managers.py
This issue can easily be reproduced in a shell session:
{{{
$ ./manage.py shell
Python 3.9.8 (main, Nov 7 2021, 15:47:09)
Type 'copyright', 'credits' or 'license' for more information
IPython 7.27.0 -- An enhanced Interactive Python. Type '?' for help.
In [1]: from lava_scheduler_app.models import TestJob, User
In [2]: TestJob.objects.visible_by_user(User.objects.last())
Out[2]:
---------------------------------------------------------------------------
CardinalityViolation Traceback (most recent call
last)
/usr/lib/python3/dist-packages/django/db/backends/utils.py in
_execute(self, sql, params, *ignored_wrapper_args)
83 else:
---> 84 return self.cursor.execute(sql, params)
85
CardinalityViolation: more than one row returned by a subquery used as an
expression
The above exception was the direct cause of the following exception:
ProgrammingError Traceback (most recent call
last)
/usr/lib/python3/dist-packages/IPython/core/formatters.py in
__call__(self, obj)
700 type_pprinters=self.type_printers,
701 deferred_pprinters=self.deferred_printers)
--> 702 printer.pretty(obj)
703 printer.flush()
704 return stream.getvalue()
/usr/lib/python3/dist-packages/IPython/lib/pretty.py in pretty(self, obj)
392 if cls is not object \
393 and
callable(cls.__dict__.get('__repr__')):
--> 394 return _repr_pprint(obj, self, cycle)
395
396 return _default_pprint(obj, self, cycle)
/usr/lib/python3/dist-packages/IPython/lib/pretty.py in _repr_pprint(obj,
p, cycle)
698 """A pprint that just redirects to the normal repr
function."""
699 # Find newlines and replace them with p.break_()
--> 700 output = repr(obj)
701 lines = output.splitlines()
702 with p.group():
/usr/lib/python3/dist-packages/django/db/models/query.py in __repr__(self)
254
255 def __repr__(self):
--> 256 data = list(self[:REPR_OUTPUT_SIZE + 1])
257 if len(data) > REPR_OUTPUT_SIZE:
258 data[-1] = "...(remaining elements truncated)..."
/usr/lib/python3/dist-packages/django/db/models/query.py in __len__(self)
260
261 def __len__(self):
--> 262 self._fetch_all()
263 return len(self._result_cache)
264
/usr/lib/python3/dist-packages/django/db/models/query.py in
_fetch_all(self)
1322 def _fetch_all(self):
1323 if self._result_cache is None:
-> 1324 self._result_cache = list(self._iterable_class(self))
1325 if self._prefetch_related_lookups and not
self._prefetch_done:
1326 self._prefetch_related_objects()
/usr/lib/python3/dist-packages/django/db/models/query.py in __iter__(self)
49 # Execute the query. This will also fill compiler.select,
klass_info,
50 # and annotations.
---> 51 results =
compiler.execute_sql(chunked_fetch=self.chunked_fetch,
chunk_size=self.chunk_size)
52 select, klass_info, annotation_col_map = (compiler.select,
compiler.klass_info,
53
compiler.annotation_col_map)
/usr/lib/python3/dist-packages/django/db/models/sql/compiler.py in
execute_sql(self, result_type, chunked_fetch, chunk_size)
1173 cursor = self.connection.cursor()
1174 try:
-> 1175 cursor.execute(sql, params)
1176 except Exception:
1177 # Might fail for server-side cursors (e.g. connection
closed)
/usr/lib/python3/dist-packages/django/db/backends/utils.py in
execute(self, sql, params)
96 def execute(self, sql, params=None):
97 with self.debug_sql(sql, params,
use_last_executed_query=True):
---> 98 return super().execute(sql, params)
99
100 def executemany(self, sql, param_list):
/usr/lib/python3/dist-packages/django/db/backends/utils.py in
execute(self, sql, params)
64
65 def execute(self, sql, params=None):
---> 66 return self._execute_with_wrappers(sql, params,
many=False, executor=self._execute)
67
68 def executemany(self, sql, param_list):
/usr/lib/python3/dist-packages/django/db/backends/utils.py in
_execute_with_wrappers(self, sql, params, many, executor)
73 for wrapper in reversed(self.db.execute_wrappers):
74 executor = functools.partial(wrapper, executor)
---> 75 return executor(sql, params, many, context)
76
77 def _execute(self, sql, params, *ignored_wrapper_args):
/usr/lib/python3/dist-packages/django/db/backends/utils.py in
_execute(self, sql, params, *ignored_wrapper_args)
82 return self.cursor.execute(sql)
83 else:
---> 84 return self.cursor.execute(sql, params)
85
86 def _executemany(self, sql, param_list,
*ignored_wrapper_args):
/usr/lib/python3/dist-packages/django/db/utils.py in __exit__(self,
exc_type, exc_value, traceback)
88 if dj_exc_type not in (DataError, IntegrityError):
89 self.wrapper.errors_occurred = True
---> 90 raise dj_exc_value.with_traceback(traceback) from
exc_value
91
92 def __call__(self, func):
/usr/lib/python3/dist-packages/django/db/backends/utils.py in
_execute(self, sql, params, *ignored_wrapper_args)
82 return self.cursor.execute(sql)
83 else:
---> 84 return self.cursor.execute(sql, params)
85
86 def _executemany(self, sql, param_list,
*ignored_wrapper_args):
ProgrammingError: more than one row returned by a subquery used as an
expression
}}}
If I downgrade Django to 2.2, that just works. This is reproducible with
Django 3.2 and with the current main branch from git.
--
Ticket URL: <https://code.djangoproject.com/ticket/33282>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Comment (by Antonio Terceiro):
Bisecting points to the same culprit as #32690:
{{{
35431298226165986ad07e91f9d3aca721ff38ec is the first bad commit
commit 35431298226165986ad07e91f9d3aca721ff38ec
Author: Simon Charette <chare...@gmail.com>
Date: Wed Mar 6 01:05:55 2019 -0500
Refs #27149 -- Moved subquery expression resolving to Query.
This makes Subquery a thin wrapper over Query and makes sure it
respects
the Expression source expression API by accepting the same number of
expressions as it returns. Refs #30188.
It also makes OuterRef usable in Query without Subquery wrapping. This
should allow Query's internals to more easily perform subquery push
downs
during split_exclude(). Refs #21703.
django/db/models/expressions.py | 64
+++++++----------------------------------
django/db/models/sql/query.py | 28 ++++++++++++++++--
django/db/models/sql/where.py | 15 ++++++++++
tests/queries/tests.py | 2 +-
4 files changed, 52 insertions(+), 57 deletions(-)
bisect run success
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:1>
Comment (by Antonio Terceiro):
FWIW the generated query that causes this is:
{{{
SELECT
COUNT (*)
AS "__count" FROM "lava_scheduler_app_testjob"
WHERE (((NOT "lava_scheduler_app_testjob".
"is_public" AND "lava_scheduler_app_testjob"."submitter_id" =
%s) OR ("lava_scheduler_app_testjob".
"is_public" AND "lava_scheduler_app_testjob".
"id" IN (SELECT U0.
"id" FROM "lava_scheduler_app_testjob" U0 LEFT
OUTER
JOIN "lava_scheduler_app_testjob_viewing_groups"
U1
ON (U0."id" =
U1."testjob_id") WHERE U1.
"group_id" IS NULL)
AND (("lava_scheduler_app_testjob".
"actual_device_id" IS NOT NULL AND
"lava_scheduler_app_testjob".
"actual_device_id" IN (SELECT V0.
"hostname" FROM
"lava_scheduler_app_device"
V0
LEFT OUTER JOIN
"lava_scheduler_app_groupdevicepermission"
V1 ON (V0."hostname" =
V1.
"device_id") LEFT
OUTER
JOIN "auth_group" V2 ON (V1.
"group_id"
=
V2.
"id")
LEFT OUTER JOIN
"auth_user_groups" V3 ON
(V2.
"id" =
V3.
"group_id")
LEFT OUTER JOIN
"auth_permission" V5 ON (V1.
"permission_id"
=
V5.
"id")
GROUP BY V0."hostname",
(SELECT U0.
"name" FROM
"lava_scheduler_app_devicetype"
U0 LEFT OUTER JOIN
"lava_scheduler_app_groupdevicetypepermission"
U1 ON (U0."name" =
U1.
"devicetype_id")
LEFT
OUTER JOIN "auth_group" U2
ON (U1."group_id" =
U2.
"id") LEFT OUTER JOIN
"auth_user_groups" U3 ON
(U2.
"id"
=
U3.
"group_id")
LEFT OUTER JOIN
"auth_permission" U5 ON
(U1.
"permission_id"
=
U5.
"id")
GROUP BY U0.
"name"
HAVING (SUM
(CASE
WHEN
(U5."codename" =
%s) THEN %
s ELSE % s END) =
%s OR
NOT (SUM
(CASE
WHEN (U3.
"user_id" =
%s AND
U5.
"codename"
IN (%s,
%s,
%s))
THEN % s ELSE
%
s END) =
%s)))
HAVING ((SUM
(CASE
WHEN
(V5."codename" =
%s) THEN %
s ELSE % s END) =
%s AND V0.
"device_type_id"
IN (SELECT U0.
"name" FROM
"lava_scheduler_app_devicetype"
U0 LEFT OUTER
JOIN
"lava_scheduler_app_groupdevicetypepermission"
U1 ON
(U0."name" =
U1.
"devicetype_id")
LEFT OUTER JOIN
"auth_group" U2
ON
(U1."group_id" =
U2.
"id") LEFT
OUTER JOIN
"auth_user_groups"
U3 ON (U2."id"
=
U3.
"group_id")
LEFT OUTER JOIN
"auth_permission"
U5 ON (U1.
"permission_id"
=
U5.
"id")
GROUP
BY U0.
"name"
HAVING (SUM
(CASE
WHEN
(U5.
"codename"
=
%s)
THEN %
s ELSE
%
s END)
=
%s OR
NOT
(SUM
(CASE
WHEN
(U3.
"user_id"
=
%s
AND
U5.
"codename"
IN
(%s,
%s,
%s))
THEN
%
s
ELSE
%
s
END)
=
%s))))
OR
NOT (SUM
(CASE
WHEN (V3.
"user_id" =
%s AND
V5.
"codename"
IN (%s,
%s,
%s))
THEN % s ELSE
%
s END) =
%s))))
OR ("lava_scheduler_app_testjob".
"actual_device_id" IS NULL AND
"lava_scheduler_app_testjob".
"requested_device_type_id" IN (SELECT U0.
"name" FROM
"lava_scheduler_app_devicetype"
U0 LEFT OUTER
JOIN
"lava_scheduler_app_groupdevicetypepermission"
U1 ON (U0."name"
=
U1.
"devicetype_id")
LEFT OUTER JOIN
"auth_group" U2
ON (U1."group_id"
=
U2.
"id") LEFT
OUTER
JOIN
"auth_user_groups" U3
ON (U2."id" =
U3.
"group_id")
LEFT
OUTER JOIN
"auth_permission"
U5
ON (U1.
"permission_id" =
U5.
"id") GROUP
BY
U0.
"name"
HAVING (SUM
(CASE
WHEN
(U5.
"codename"
=
%s)
THEN %
s ELSE %
s END) =
%s OR
NOT (SUM
(CASE
WHEN
(U3.
"user_id"
=
%s
AND
U5.
"codename"
IN
(%s,
%s,
%s))
THEN %
s
ELSE
%
s
END)
=
%s))))))
OR (NOT
("lava_scheduler_app_testjob".
"id" IN (SELECT U0.
"id" FROM "lava_scheduler_app_testjob" U0 LEFT OUTER
JOIN "lava_scheduler_app_testjob_viewing_groups" U1
ON (U0."id" =
U1."testjob_id") WHERE U1.
"group_id" IS NULL)) AND NOT (EXISTS (SELECT (1) AS
"a"
FROM
"lava_scheduler_app_testjob_viewing_groups"
V1 WHERE (V1.
"group_id"
IN
(SELECT
U0.
"id"
FROM
"auth_group"
U0
WHERE
NOT
(U0.
"id"
IN
(%s)))
AND
V1.
"testjob_id"
=
("lava_scheduler_app_testjob".
"id"))
LIMIT 1))))
AND
"lava_scheduler_app_testjob"."submitter_id" =
%s AND "lava_scheduler_app_testjob"."state" IN (%s, %s))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:2>
* cc: Simon Charette (added)
* status: new => closed
* resolution: => needsinfo
Comment:
The issue seems related an attempt at doing a `GROUP BY` by a subquery
that returns more than one row. See the `GROUP BY V0."hostname", (SELECT
U0."name" FROM "lava_scheduler_app_devicetype" ...)` part.
The `Q(actual_device__isnull=False, actual_device__in=accessible_devices)`
lookup in `RestrictedTestJobQuerySet.accessible_by_user` is causing that
somehow, and I assume `Device.objects.accessible_by_user` is the culprit
as it must do a subquery annotation somehow that is not limited to one
row.
By giving a cursory look at your code base I couldn't identify the origin
of the subquery annotation so I'll close with ''needsinfo'' for now but
please re-open if you can create a minimal test case as I'll be monitoring
this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:3>
Comment (by Antonio Terceiro):
Hi, I spent some time debugging this today. This seems to be caused by
`get_group_by_cols()` returning an incorrect value for a `Query` object.
In a pdb session against the main branch of the git repository, I get
this:
{{{
$ pdb3 ./manage.py runscript test
> /home/terceiro/src/linaro/lava/manage.py(22)<module>()
-> import argparse
(Pdb) break /home/terceiro/src/django/django/db/models/lookups.py:434
Breakpoint 1 at /home/terceiro/src/django/django/db/models/lookups.py:434
(Pdb) c
>
/home/terceiro/src/django/django/db/models/lookups.py(434)get_group_by_cols()
-> cols.extend(self.rhs.get_group_by_cols())
(Pdb) p self.rhs
<django.db.models.sql.query.Query object at 0x7f6ddb7bd580>
(Pdb) p self.rhs.get_group_by_cols()
[<django.db.models.sql.query.Query object at 0x7f6ddb7bd580>]
}}}
In commit 35431298226165986ad07e91f9d3aca721ff38ec, which caused this,
{{{django.db.models.sql.query.Query}}} is made a subclass of
{{{django.db.models.expressions.BaseExpression}}}.
BaseExpression.get_group_by_calls is implemented like this:
{{{
def get_group_by_cols(self, alias=None):
if not self.contains_aggregate:
return [self]
cols = []
for source in self.get_source_expressions():
cols.extend(source.get_group_by_cols())
return cols
}}}
Since BaseExpression also hardcodes {{{contains_aggregate}}} to False,
that first if statement always applies. Providing an appropriate
implementation of contains_aggregate to Query fixes things for me here:
{{{
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 2c5f11cbbf..f7c43ff622 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -414,6 +414,11 @@ class Query(BaseExpression):
annotation.set_source_expressions(new_exprs)
return annotation, col_cnt
+ @cached_property
+ def contains_aggregate(self):
+ annotations = self.annotations.values()
+ return any(getattr(ann, 'contains_aggregate', True) for ann in
annotations)
+
def get_aggregation(self, using, added_aggregate_names):
"""
Return the dictionary with the values of the existing
aggregations.
}}}
I wasn't able to write an unit tests for this so far, though.
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:4>
Comment (by Antonio Terceiro):
Just noticed that this change causes a few errors for other tests so it's
not the final fix.
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:5>
* status: closed => new
* resolution: needsinfo =>
Comment:
Unfortunately I wasn't able yet to write an unit test that triggers this
issue. I was, however, able to write a patch that both fixes my issue and
does not cause any existing unit test to fail:
https://github.com/django/django/pull/15129
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:6>
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:7>
* stage: Unreviewed => Accepted
Comment:
Managed to reproduce with this tests
{{{#!diff
diff --git a/tests/aggregation/tests.py b/tests/aggregation/tests.py
index 9ba70cb35e..f3277d2b5f 100644
--- a/tests/aggregation/tests.py
+++ b/tests/aggregation/tests.py
@@ -1335,6 +1335,14 @@ def
test_aggregation_nested_subquery_outerref(self):
).values_list('publisher_count', flat=True)
self.assertSequenceEqual(books_breakdown, [1] * 6)
+ def test_filter_in_subquery_or_aggregation(self):
+ authors = Author.objects.annotate(
+ books_count=Count('book'),
+ ).filter(
+ Q(pk__in=Book.objects.values('authors')) |
Q(books_count__gt=0)
+ )
+ self.assertQuerysetEqual(authors, Author.objects.all(),
ordered=False)
+
def test_aggregation_random_ordering(self):
"""Random() is not included in the GROUP BY when used for
ordering."""
authors =
Author.objects.annotate(contact_count=Count('book')).order_by('?')
}}}
The gist of it is that the logic that was added to
`Subquery.get_group_by_cols` should have been added to
`sql.Query.get_group_by_cols` and the latter should have proxied it.
The issue is triggered when a lookup containing a subquery is combined
using `OR` to a lookup against an aggregation. When this happens the ORM
has to choice but to push the filtering to the `HAVING` clause and thus
group by all inputs passed down to the lookups. Since `Subquery` resolves
to `sql.Query` and the latter defaults to returning `self` as inherited
from `BaseExpression` the ORM tries to group by a subquery that
potentially returns multiple rows.
Antonio if don't mind if I assign the issue to me I have a patch set
almost ready to address the problem and include extra regression tests for
new uses cases such as lookup annotations which is a newly supported in
4.0.
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:8>
Comment (by Antonio Terceiro):
Replying to [comment:8 Simon Charette]:
> Antonio if don't mind if I assign the issue to me I have a patch set
almost ready to address the problem and include extra regression tests for
new uses cases such as lookup annotations which is a newly supported in
4.0.
Sure, please go ahead. I just want this to get fixed.
It would be nice if it's possible to backport the fix to 3.2 as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:9>
* owner: nobody => Simon Charette
* status: new => assigned
Comment:
I should be able to submit a patch this week.
> It would be nice if it's possible to backport the fix to 3.2 as well.
Unfortunately that won't be possible per our backporting policies, too
much risk for regressions and 3.2 is only receiving security backports at
this point.
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:10>
Comment (by Antonio Terceiro):
Replying to [comment:10 Simon Charette]:
> I should be able to submit a patch this week.
>
> > It would be nice if it's possible to backport the fix to 3.2 as well.
>
> Unfortunately that won't be possible per our backporting policies, too
much risk for regressions and 3.2 is only receiving security backports at
this point.
this means that 3.x will be left with two crash-causing regressions (this
one and the one in #32690)
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:11>
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:12>
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:13>
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"e5a92d400acb4ca6a8e1375d1ab8121f2c7220be" e5a92d4]:
{{{
#!CommitTicketReference repository=""
revision="e5a92d400acb4ca6a8e1375d1ab8121f2c7220be"
Fixed #33282 -- Fixed a crash when OR'ing subquery and aggregation
lookups.
As a QuerySet resolves to Query the outer column references grouping logic
should be defined on the latter and proxied from Subquery for the cases
where
get_group_by_cols is called on unresolved expressions.
Thanks Antonio Terceiro for the report and initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33282#comment:14>