[Django] #32651: Combining Q with a Q containing exists crashes
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 3.2
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'm in the process of upgrading a project from Django 3.1 to 3.2 and one
of the tests keeps crashing. I reduced the code to the following
statement:
{{{
Q() & Q(Exists(Book.objects.all()))
}}}
This worked correctly in Django 3.1.
I found this ticket: #32548 and can confirm (that after some struggles
related to incompatibilities in 3rd party packges) I was able to get the
tests to pass on Django main. Is it possible to backport the fix to Django
3.2?
--
Ticket URL: <https://code.djangoproject.com/ticket/32651>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
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
-------------------------------------+-------------------------------------
Comment (by Jaap Roes):
From #32548 I gather that `Q() & Exists(Book.objects.all())` is supposed
to be equivalent to `Q() & Q(Exists(Book.objects.all()))`.
I tried this as a workaround, but our tests fail because the returned data
after applying the filters is not the same. On Django 3.1 this crashes
with a `TypeError` raised from `query_utils.py._combine`.
This makes it impossible to upgrade to Django 3.2 at this point in time.
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jaap Roes):
I figured out why the tests results where different, for some reason
combining a `Q` with a negated `Exists` (using the `~` operator) results
in an unnegated `Exists`:
{{{
>>> q = Q() & ~Exists(Book.objects.all())
>>> q.negated
False
}}}
Setting `negated` on the constructor does work:
{{{
>>> q = Q() & Exists(Vacancy.objects.all(), negated=True)
>>> q.negated
True
}}}
This might be a separate bug.
At least I'm now able to finish the Django 3.2 upgrade.
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [ticket:32651 Jaap Roes]:
> I'm in the process of upgrading a project from Django 3.1 to 3.2 and one
of the tests keeps crashing. I reduced the code to the following
statement:
>
> {{{
> Q() & Q(Exists(Book.objects.all()))
> }}}
>
> This worked correctly in Django 3.1.
>
> I found this ticket: #32548 and can confirm (that after some struggles
related to incompatibilities in 3rd party packges) I was able to get the
tests to pass on Django main. Is it possible to backport the fix to Django
3.2?
Thanks I didn't notice that it's a regression in
466920f6d726eee90d5566e0a9948e92b33a122e (backported in
0e2979e95d0f68f28c92c84c14655e7510d4e789). I will prepare a backport to
Django 3.2.
Let's close it as a duplicate of #32548.
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jaap Roes):
@Mariusz Nice! Does that backport also fix the weird behaviour in my
previous comment?
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Component: Database layer | Version: 3.2
(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
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => duplicate
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Simon Charette (added)
Comment:
Replying to [comment:5 Jaap Roes]:
> @Mariusz Nice! Does that backport also fix the weird behaviour in my
previous comment?
Unfortunately no, it's a separate issue related with the way how
`Exists.__invert__()` and `Subquery.__getstate__()` are defined. It's
quite serious, see
{{{
q = Exists(Employee.objects.all())
assert hash(q) != hash(~q)
}}}
Can you create a separate ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Right, yet another issue with how `BaseExpression.identity` is based off
`_constructor_args`. Defining a proper `Subquery.identity` should address
this issue but it might not even be necessary if we go forward with
https://code.djangoproject.com/ticket/32632#comment:4 and drop partial
support `Subquery.deconstruct`, `__eq__`, and `__hash__`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Jaap Roes):
I created #32657 for the `q = Q() & ~Exists(Book.objects.all())` issue
--
Ticket URL: <https://code.djangoproject.com/ticket/32651#comment:9>