summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSimon Charette <charette.s@gmail.com>2021-04-24 01:07:18 -0400
committerMariusz Felisiak <felisiak.mariusz@gmail.com>2021-04-28 20:27:42 +0200
commitd5add5d3a26f98e961dfbcad67bb04d936f2f332 (patch)
tree8932d754cda985f7b46e3ead22fdb867a5c974fa
parent55cb3c8ac186f7664b38d66fb4c7e678fe524741 (diff)
[3.2.x] Fixed #32632, Fixed #32657 -- Removed flawed support for Subquery deconstruction.
Subquery deconstruction support required implementing complex and expensive equality rules for sql.Query objects for little benefit as the latter cannot themselves be made deconstructible to their reference to model classes. Making Expression @deconstructible and not BaseExpression allows interested parties to conform to the "expression" API even if they are not deconstructible as it's only a requirement for expressions allowed in Model fields and meta options (e.g. constraints, indexes). Thanks Phillip Cutter for the report. This also fixes a performance regression in bbf141bcdc31f1324048af9233583a523ac54c94. Backport of c8b659430556dca0b2fe27cf2ea0f8290dbafecd from main
-rw-r--r--django/db/models/expressions.py26
-rw-r--r--django/db/models/query_utils.py10
-rw-r--r--django/db/models/sql/query.py9
-rw-r--r--docs/releases/3.2.1.txt4
-rw-r--r--tests/queries/test_q.py19
-rw-r--r--tests/queries/test_query.py28
6 files changed, 31 insertions, 65 deletions
diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py
index e5291ab8d3..d945e57f06 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -147,7 +147,6 @@ class Combinable:
)
-@deconstructible
class BaseExpression:
"""Base class for all query expressions."""
@@ -389,6 +388,11 @@ class BaseExpression:
return self.output_field.select_format(compiler, sql, params)
return sql, params
+
+@deconstructible
+class Expression(BaseExpression, Combinable):
+ """An expression that can be combined with other expressions."""
+
@cached_property
def identity(self):
constructor_signature = inspect.signature(self.__init__)
@@ -409,7 +413,7 @@ class BaseExpression:
return tuple(identity)
def __eq__(self, other):
- if not isinstance(other, BaseExpression):
+ if not isinstance(other, Expression):
return NotImplemented
return other.identity == self.identity
@@ -417,11 +421,6 @@ class BaseExpression:
return hash(self.identity)
-class Expression(BaseExpression, Combinable):
- """An expression that can be combined with other expressions."""
- pass
-
-
_connector_combinators = {
connector: [
(fields.IntegerField, fields.IntegerField, fields.IntegerField),
@@ -1085,7 +1084,7 @@ class Case(Expression):
return super().get_group_by_cols(alias)
-class Subquery(Expression):
+class Subquery(BaseExpression, Combinable):
"""
An explicit subquery. It may contain OuterRef() references to the outer
query which will be resolved when it is applied to that query.
@@ -1099,16 +1098,6 @@ class Subquery(Expression):
self.extra = extra
super().__init__(output_field)
- def __getstate__(self):
- state = super().__getstate__()
- args, kwargs = state['_constructor_args']
- if args:
- args = (self.query, *args[1:])
- else:
- kwargs['queryset'] = self.query
- state['_constructor_args'] = args, kwargs
- return state
-
def get_source_expressions(self):
return [self.query]
@@ -1185,6 +1174,7 @@ class Exists(Subquery):
return sql, params
+@deconstructible
class OrderBy(BaseExpression):
template = '%(expression)s %(ordering)s'
conditional = False
diff --git a/django/db/models/query_utils.py b/django/db/models/query_utils.py
index 04564e5a0d..e8ad7cd635 100644
--- a/django/db/models/query_utils.py
+++ b/django/db/models/query_utils.py
@@ -5,6 +5,7 @@ Factored out from django.db.models.query to avoid making the main module very
large and/or so that they can be used by other modules without getting into
circular import difficulties.
"""
+import copy
import functools
import inspect
import warnings
@@ -71,14 +72,11 @@ class Q(tree.Node):
if not(isinstance(other, Q) or getattr(other, 'conditional', False) is True):
raise TypeError(other)
- # If the other Q() is empty, ignore it and just use `self`.
- if not other:
+ if not self:
+ return other.copy() if hasattr(other, 'copy') else copy.copy(other)
+ elif isinstance(other, Q) and not other:
_, args, kwargs = self.deconstruct()
return type(self)(*args, **kwargs)
- # Or if this Q is empty, ignore it and just use `other`.
- elif not self:
- _, args, kwargs = other.deconstruct()
- return type(other)(*args, **kwargs)
obj = type(self)()
obj.connector = conn
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 73c9e5ca4f..ab920c1211 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -39,7 +39,6 @@ from django.db.models.sql.where import (
)
from django.utils.deprecation import RemovedInDjango40Warning
from django.utils.functional import cached_property
-from django.utils.hashable import make_hashable
from django.utils.tree import Node
__all__ = ['Query', 'RawQuery']
@@ -253,14 +252,6 @@ class Query(BaseExpression):
for alias in self.alias_map:
return alias
- @property
- def identity(self):
- identity = (
- (arg, make_hashable(value))
- for arg, value in self.__dict__.items()
- )
- return (self.__class__, *identity)
-
def __str__(self):
"""
Return the query as a string of SQL with the parameter values
diff --git a/docs/releases/3.2.1.txt b/docs/releases/3.2.1.txt
index 1c8776a0bf..545c9adce3 100644
--- a/docs/releases/3.2.1.txt
+++ b/docs/releases/3.2.1.txt
@@ -68,3 +68,7 @@ Bugfixes
* Fixed a regression in Django 3.2 where the calling process environment would
not be passed to the ``dbshell`` command on PostgreSQL (:ticket:`32687`).
+
+* Fixed a performance regression in Django 3.2 when building complex filters
+ with subqueries (:ticket:`32632`). As a side-effect the private API to check
+ ``django.db.sql.query.Query`` equality is removed.
diff --git a/tests/queries/test_q.py b/tests/queries/test_q.py
index 24a705f07f..d75751be63 100644
--- a/tests/queries/test_q.py
+++ b/tests/queries/test_q.py
@@ -1,4 +1,5 @@
-from django.db.models import Exists, F, OuterRef, Q
+from django.db.models import BooleanField, Exists, F, OuterRef, Q
+from django.db.models.expressions import RawSQL
from django.test import SimpleTestCase
from .models import Tag
@@ -37,6 +38,16 @@ class QTests(SimpleTestCase):
with self.assertRaisesMessage(TypeError, str(obj)):
q & obj
+ def test_combine_negated_boolean_expression(self):
+ tagged = Tag.objects.filter(category=OuterRef('pk'))
+ tests = [
+ Q() & ~Exists(tagged),
+ Q() | ~Exists(tagged),
+ ]
+ for q in tests:
+ with self.subTest(q=q):
+ self.assertIs(q.negated, True)
+
def test_deconstruct(self):
q = Q(price__gt=F('discounted_price'))
path, args, kwargs = q.deconstruct()
@@ -88,10 +99,10 @@ class QTests(SimpleTestCase):
self.assertEqual(kwargs, {})
def test_deconstruct_boolean_expression(self):
- tagged = Tag.objects.filter(category=OuterRef('pk'))
- q = Q(Exists(tagged))
+ expr = RawSQL('1 = 1', BooleanField())
+ q = Q(expr)
_, args, kwargs = q.deconstruct()
- self.assertEqual(args, (Exists(tagged),))
+ self.assertEqual(args, (expr,))
self.assertEqual(kwargs, {})
def test_reconstruct(self):
diff --git a/tests/queries/test_query.py b/tests/queries/test_query.py
index 5db9d96163..523fa607f0 100644
--- a/tests/queries/test_query.py
+++ b/tests/queries/test_query.py
@@ -150,31 +150,3 @@ class TestQuery(SimpleTestCase):
msg = 'Cannot filter against a non-conditional expression.'
with self.assertRaisesMessage(TypeError, msg):
query.build_where(Func(output_field=CharField()))
-
- def test_equality(self):
- self.assertNotEqual(
- Author.objects.all().query,
- Author.objects.filter(item__name='foo').query,
- )
- self.assertEqual(
- Author.objects.filter(item__name='foo').query,
- Author.objects.filter(item__name='foo').query,
- )
- self.assertEqual(
- Author.objects.filter(item__name='foo').query,
- Author.objects.filter(Q(item__name='foo')).query,
- )
-
- def test_hash(self):
- self.assertNotEqual(
- hash(Author.objects.all().query),
- hash(Author.objects.filter(item__name='foo').query)
- )
- self.assertEqual(
- hash(Author.objects.filter(item__name='foo').query),
- hash(Author.objects.filter(item__name='foo').query),
- )
- self.assertEqual(
- hash(Author.objects.filter(item__name='foo').query),
- hash(Author.objects.filter(Q(item__name='foo')).query),
- )