Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#32651 closed Bug (duplicate)

Combining Q with a Q containing Exists crashes

Reported by: Jaap Roes Owned by: nobody
Component: Database layer (models, ORM) Version: 3.2
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Change History (9)

comment:1 by Jaap Roes, 4 years ago

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.

comment:2 by Jaap Roes, 4 years ago

Summary: Combining Q with a Q containing exists crashesCombining Q with a Q containing Exists crashes

comment:3 by Jaap Roes, 4 years ago

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.

Version 0, edited 4 years ago by Jaap Roes (next)

in reply to:  description comment:4 by Mariusz Felisiak, 4 years ago

Replying to 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.

comment:5 by Jaap Roes, 4 years ago

@Mariusz Nice! Does that backport also fix the weird behaviour in my previous comment?

comment:6 by Mariusz Felisiak, 4 years ago

Resolution: duplicate
Status: newclosed

in reply to:  5 comment:7 by Mariusz Felisiak, 4 years ago

Cc: Simon Charette added

Replying to 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.

comment:8 by Simon Charette, 4 years ago

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__.

comment:9 by Jaap Roes, 4 years ago

I created #32657 for the q = Q() & ~Exists(Book.objects.all()) issue

Note: See TracTickets for help on using tickets.
Back to Top