[Django] #28863: Regression in 2.0: an annotated then filtered db function using a Q object crashes
Django
crashes
-------------------------------------+-------------------------------------
Reporter: Adam | Owner: nobody
(Chainz) Johnson |
Type: Bug | Status: new
Component: Database | Version: 2.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
I noticed this when trying to test Django-MySQL against 2.0 in
https://github.com/adamchainz/django-mysql/pull/403 . Basically there's a
function called If() that takes a Q object, and it's tested in a variety
of scenarios. Only one of those scenarios was crashing with Django 2.0b1
and rc1.
I found a minimal test to reproduce it in the django test suite:
{{{
diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py
index ca331aeb03..d31935ddb4 100644
--- a/tests/expressions/tests.py
+++ b/tests/expressions/tests.py
@@ -73,6 +73,12 @@ class BasicExpressionsTests(TestCase):
],
)
+ def test_filtering_on_annoate_that_uses_q(self):
+ qs = Company.objects.annotate(
+ name_check_len=Length(Q(name__gt='T'))
+ ).filter(name_check_len__gt=1)
+ list(qs)
+
def test_filter_inter_attribute(self):
# We can filter on attribute relationships on same model obj,
e.g.
# find companies where the number of employees is greater
}}}
It fails with this stacktrace:
{{{
ERROR: test_filtering_on_annoate_that_uses_q
(expressions.tests.BasicExpressionsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File
"/Users/chainz/Documents/Projects/django/tests/expressions/tests.py", line
80, in test_filtering_on_annoate_that_uses_q
list(qs)
File
"/Users/chainz/Documents/Projects/django/django/db/models/query.py", line
272, in __iter__
self._fetch_all()
File
"/Users/chainz/Documents/Projects/django/django/db/models/query.py", line
1179, in _fetch_all
self._result_cache = list(self._iterable_class(self))
File
"/Users/chainz/Documents/Projects/django/django/db/models/query.py", line
54, in __iter__
results = compiler.execute_sql(chunked_fetch=self.chunked_fetch,
chunk_size=self.chunk_size)
File
"/Users/chainz/Documents/Projects/django/django/db/models/sql/compiler.py",
line 1050, in execute_sql
sql, params = self.as_sql()
File
"/Users/chainz/Documents/Projects/django/django/db/models/sql/compiler.py",
line 458, in as_sql
where, w_params = self.compile(self.where) if self.where is not None
else ("", [])
File
"/Users/chainz/Documents/Projects/django/django/db/models/sql/compiler.py",
line 390, in compile
sql, params = node.as_sql(self, self.connection)
File
"/Users/chainz/Documents/Projects/django/django/db/models/sql/where.py",
line 79, in as_sql
sql, params = compiler.compile(child)
File
"/Users/chainz/Documents/Projects/django/django/db/models/sql/compiler.py",
line 390, in compile
sql, params = node.as_sql(self, self.connection)
File
"/Users/chainz/Documents/Projects/django/django/db/models/lookups.py",
line 160, in as_sql
lhs_sql, params = self.process_lhs(compiler, connection)
File
"/Users/chainz/Documents/Projects/django/django/db/models/lookups.py",
line 151, in process_lhs
lhs_sql, params = super().process_lhs(compiler, connection, lhs)
File
"/Users/chainz/Documents/Projects/django/django/db/models/lookups.py",
line 77, in process_lhs
lhs = lhs.resolve_expression(compiler.query)
File
"/Users/chainz/Documents/Projects/django/django/db/models/expressions.py",
line 597, in resolve_expression
c.source_expressions[pos] = arg.resolve_expression(query, allow_joins,
reuse, summarize, for_save)
AttributeError: 'WhereNode' object has no attribute 'resolve_expression'
}}}
I then used git bisect with this test and found it bisected to:
{{{
commit bde86ce9ae17ee52aa5be9b74b64422f5219530d
Author: Sergey Fedoseev <fedosee...@gmail.com>
Date: Sat Apr 1 18:47:49 2017 +0500
Fixed #25605 -- Made GIS DB functions accept geometric expressions,
not only values, in all positions.
}}}
This change was the culprit, if you revert only this conditional, the test
passes:
{{{
diff --git a/django/db/models/lookups.py b/django/db/models/lookups.py
index d96c4468f5..c37fcabba4 100644
--- a/django/db/models/lookups.py
+++ b/django/db/models/lookups.py
@@ -76,6 +76,8 @@ class Lookup:
def process_lhs(self, compiler, connection, lhs=None):
lhs = lhs or self.lhs
+ if hasattr(lhs, 'resolve_expression'):
+ lhs = lhs.resolve_expression(compiler.query)
return compiler.compile(lhs)
def process_rhs(self, compiler, connection):
}}}
I'm not sure of the fix, maybe `WhereNode` should be given a
`resolve_expression` method? Or should the change in `Lookup`'s
`process_lhs` be more considered?
--
Ticket URL: <https://code.djangoproject.com/ticket/28863>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
crashes
-------------------------------------+-------------------------------------
Johnson |
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Adam (Chainz) Johnson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:1>
Django
crashes
-------------------------------------+-------------------------------------
Johnson |
Type: Bug | Status: new
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Normal => Release blocker
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:2>
Django
crashes
-------------------------------------+-------------------------------------
Johnson |
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Sergey Fedoseev (added)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:3>
Django
crashes
-------------------------------------+-------------------------------------
Johnson |
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
bde86ce9ae17ee52aa5be9b74b64422f5219530d:
{{{
commit bde86ce9ae17ee52aa5be9b74b64422f5219530d
Author: Sergey Fedoseev <fedosee...@gmail.com>
Date: Sat Apr 1 18:47:49 2017 +0500
Fixed #25605 -- Made GIS DB functions accept geometric expressions,
not only values, in all positions.
}}}
This change was the culprit, if you revert only this conditional, the test
passes:
{{{
diff --git a/django/db/models/lookups.py b/django/db/models/lookups.py
index d96c4468f5..c37fcabba4 100644
--- a/django/db/models/lookups.py
+++ b/django/db/models/lookups.py
@@ -76,6 +76,8 @@ class Lookup:
def process_lhs(self, compiler, connection, lhs=None):
lhs = lhs or self.lhs
+ if hasattr(lhs, 'resolve_expression'):
+ lhs = lhs.resolve_expression(compiler.query)
return compiler.compile(lhs)
def process_rhs(self, compiler, connection):
}}}
I'm not sure of the fix, maybe `WhereNode` should be given a
`resolve_expression` method? Or should the change in `Lookup`'s
`process_lhs` be more considered?
--
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:4>
Django
crashes
-------------------------------------+-------------------------------------
Johnson |
Type: Bug | Status: new
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Keryn Knight (added)
Comment:
Possible duplicate of #28826?
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:5>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Sergey Fedoseev
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:6>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam (Chainz) Johnson):
Yes it's a dupe, thanks Keryn. And didn't know about that ticket about
enforcing resolve_expression on all things
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:7>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/9400 PR] which has some test
failures.
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:8>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sergey Fedoseev):
It looks like this test fails on Postgres even on 1.11.
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:9>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sergey Fedoseev):
As this feature is not officially supported I'd like to be sure that this
crash should be considered as regression.
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:10>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Adam (Chainz) Johnson):
I was under the impression it's officially supported... My If() function
works with 1.8+ and doesn't do anything that Case(When()) doesn't...
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:11>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Sergey Fedoseev):
`When` handles `Q` a bit more specifically since
3eba9638ee69138c73efb1d1c1d1b806ddafc6cf :-).
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:12>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: assigned
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Josh Smeaton (added)
Comment:
I'm not sure. I noticed that #27021 says that something like
`Model.objects.annotate(boolfield=ExpressionWrapper(Q(field__gte=4),
output_field=BooleanField()))` has emerged as a common pattern but is
discouraged. The current PR generates invalid SQL on Oracle.
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:13>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Component: Database layer | Version: 2.0
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cf12257db23fa248c89a3da3f718aa01a50ca659" cf12257d]:
{{{
#!CommitTicketReference repository=""
revision="cf12257db23fa248c89a3da3f718aa01a50ca659"
Fixed #28863 -- Fixed filter on annotation that contains Q.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:14>
Django
crashes
-------------------------------------+-------------------------------------
Johnson | Fedoseev
Type: Bug | Status: closed
Component: Database layer | Version: 2.0
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"70da0420c2294cf9c851dc7703c8b729f1ed8956" 70da0420]:
{{{
#!CommitTicketReference repository=""
revision="70da0420c2294cf9c851dc7703c8b729f1ed8956"
[2.0.x] Fixed #28863 -- Fixed filter on annotation that contains Q.
Backport of cf12257db23fa248c89a3da3f718aa01a50ca659 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28863#comment:15>