#32717: Incorrect SQL generation filtering OR-combined queries
-------------------------------------+-------------------------------------
Reporter: Shaheed Haque | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
I started looking into this one and it seems the issue is two fold.
One
[https://github.com/django/django/blob/205c36b58fed5a1a0ff462593fc61b58189027d8/django/utils/tree.py#L93-L94
the ''optimization''] in `Node.add` strikes again this time for `Lookup`
equality as it was introduced in bbf141bcdc31f1324048af9233583a523ac54c94.
As we already know [https://code.djangoproject.com/ticket/32632#comment:4
this code is poorly tested] and this issue just uncovered that it's
actually flawed as it should also make sure that connectors are matching.
What's happening here is that the query has a `WHERE` tree of the form
`(jurisdiction = 'US' OR ...)` and it wrongfully consider that an addition
of a `AND jurisdiction = 'US'` is noop because it doesn't compare the
connector `AND != OR`. The following patch should address that.
{{{#!diff
diff --git a/django/utils/tree.py b/django/utils/tree.py
index 302cd37d5f..f8d3e3586a 100644
--- a/django/utils/tree.py
+++ b/django/utils/tree.py
@@ -90,7 +90,7 @@ def add(self, data, conn_type, squash=True):
If `squash` is False the data is prepared and added as a child to
this tree without further logic.
"""
- if data in self.children:
+ if (self.connector == conn_type or len(self) == 1) and data in
self.children:
return data
if not squash:
self.children.append(data)
}}}
The reason why this is not an issue for most cases though is that the only
way to craft a query with a root `Where` node with an `OR` connector is
through a `|` combination of query like described here. That explains why
queries of the form `filter(Q(foo="bar") |
Q(baz="foo")).filter(foo="bar")` are not affected by this `Node.add` bug
and resulting in a large number of failures in the test suite.
I personally think that we should prevent `Query.combine` from ever
generating queries with root `Where(connector='OR')` as I wouldn't be
surprised if it caused other subtle issues down the line given the poor
coverage we have for queryset combinations. If that makes sense to you
I'll prepare a patch with two commits, one for the `Node.add` adjustments
and a following one with the `Query.combine` proposed adjustments meant to
be backported.
I also think that we should just nuke the `if data in self.children`
entirely in the main branch as I've expressed in #32632 as it's wasteful
99% of the time and I'm willing to shepherd an optimization ticket if
that's deemed appropriate.
--
Ticket URL: <https://code.djangoproject.com/ticket/32717#comment:4>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/070.a5bc8d0e914d432eadc090dce4e399ce%40djangoproject.com.
