#28863 closed Bug (fixed)
Regression in 2.0: an annotated then filtered db function using a Q object crashes
Reported by: | Adam Johnson | Owned by: | Sergey Fedoseev |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 2.0 |
Severity: | Release blocker | Keywords: | |
Cc: | Adam Johnson, Sergey Fedoseev, Keryn Knight, Josh Smeaton | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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 bde86ce9ae17ee52aa5be9b74b64422f5219530d:
commit bde86ce9ae17ee52aa5be9b74b64422f5219530d Author: Sergey Fedoseev <fedoseev.sergey@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?
Change History (15)
comment:1 by , 7 years ago
Cc: | added |
---|
comment:2 by , 7 years ago
Severity: | Normal → Release blocker |
---|
comment:3 by , 7 years ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 7 years ago
Description: | modified (diff) |
---|
comment:6 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:7 by , 7 years ago
Yes it's a dupe, thanks Keryn. And didn't know about that ticket about enforcing resolve_expression on all things
comment:8 by , 7 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
PR which has some test failures.
comment:10 by , 7 years ago
As this feature is not officially supported I'd like to be sure that this crash should be considered as regression.
comment:11 by , 7 years ago
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...
comment:12 by , 7 years ago
When
handles Q
a bit more specifically since 3eba9638ee69138c73efb1d1c1d1b806ddafc6cf :-).
comment:13 by , 7 years ago
Cc: | added |
---|
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.
Possible duplicate of #28826?
Addendum:
WhereNode.resolve_expression
is also discussed as an example as part of #25425 ("Enforce calling resolve_expression before as_sql on all expressions")