Regression in 2.0: an annotated then filtered db function using a Q object crashes

[Adam Johnson]

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?

[Keryn Knight]

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")

[Adam Johnson]

Yes it's a dupe, thanks Keryn. And didn't know about that ticket about enforcing resolve_expression on all things

[Tim Graham]

​PR which has some test failures.

[Sergey Fedoseev]

It looks like this test fails on Postgres even on 1.11.

[Sergey Fedoseev]

As this feature is not officially supported I'd like to be sure that this crash should be considered as regression.

[Adam 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...

[Sergey Fedoseev]

When handles Q a bit more specifically since 3eba9638ee69138c73efb1d1c1d1b806ddafc6cf :-).

[Tim Graham]

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.

[Tim Graham <timograham@…>]

In cf12257d:

Fixed #28863 -- Fixed filter on annotation that contains Q.

[Tim Graham <timograham@…>]

In 70da0420:

[2.0.x] Fixed #28863 -- Fixed filter on annotation that contains Q.

Backport of cf12257db23fa248c89a3da3f718aa01a50ca659 from master