[Django] #36388: QuerySet.union no longer supports empty args

[Django]

#36388: QuerySet.union no longer supports empty args
--------------------------------+-----------------------------------------
Reporter: Antoine Humeau | Type: Bug
Status: new | Component: Uncategorized
Version: 5.2 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+-----------------------------------------
Hello there,

Up until Django 5.1 (included), it was possible to use the union queryset
method with an empty arg list, example:

{{{
User.objects.union()
}}}

This example is obviously useless but the following pattern would be
equivalent and would be more reallistic:


{{{
def unite_querysets(querysets):
queryset, *other_querysets = querysets
queryset = queryset.union(*other_querysets)
}}}


Since Django 5.2, that triggers the following exception:


{{{
AttributeError: 'NoneType' object has no attribute 'elide_empty'
}}}

The line that triggers the exception is the following:
https://github.com/django/django/blob/5.2.1/django/db/models/sql/compiler.py#L616
empty_compiler is never initialized.


I am not sure this really qualifies as a bug, it might be a unsupported
usage, still it is a regression.
--
Ticket URL: <https://code.djangoproject.com/ticket/36388>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36388: QuerySet.union no longer supports empty args
--------------------------------+--------------------------------------

Reporter: Antoine Humeau | Owner: (none)

Type: Bug | Status: new
Component: Uncategorized | Version: 5.2

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
--------------------------------+--------------------------------------

Changes (by Sarah Boyce):

* cc: Simon Charette (added)

Comment:

git bisect to 9cb8baa0c4fa2c10789c5c8b65f4465932d4d172

Possible regression test
{{{#!diff
diff --git a/tests/queries/test_qs_combinators.py
b/tests/queries/test_qs_combinators.py
index ba44b5ed87..d262e2f612 100644
--- a/tests/queries/test_qs_combinators.py
+++ b/tests/queries/test_qs_combinators.py
@@ -88,6 +88,10 @@ class QuerySetSetOperationTests(TestCase):
qs3 = qs1.union(qs2)
self.assertNumbersEqual(qs3[:1], [0])

+ def test_empty_union_slice(self):
+ qs = Number.objects.union()
+ self.assertNumbersEqual(qs[:1], [0])
+
def test_union_all_none_slice(self):
qs = Number.objects.filter(id__in=[])
with self.assertNumQueries(0):
}}}

However, I agree that this usage isn't supported. Perhaps a breaking
change note is enough here? We might want a nicer error also
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:1>

[Django]

#36388: QuerySet.union no longer supports empty args

-------------------------------------+-------------------------------------

Reporter: Antoine Humeau | Owner: (none)
Type: Bug | Status: new

Component: Database layer | Version: 5.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

-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* component: Uncategorized => Database layer (models, ORM)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted

Comment:

Marking as a release blocker but willing to downgrade
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:2>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.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 Antoine Humeau):

Unsupported usage with an explicit exception seems sensible to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:3>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.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):

Thanks for the report and triage folks. I think it's worth fixing by
having `QuerySet.union` return `self` if `not other_qs` given it's a non-
invasive patch and dealing with empty sequence in a different manner would
be annoyance.
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:4>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------

Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned

Component: Database layer | Version: 5.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
-------------------------------------+-------------------------------------

Changes (by Colleen Dunlap):

* owner: (none) => Colleen Dunlap
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:5>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Colleen Dunlap):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:6>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* needs_better_patch: 0 => 1

Comment:

Left some comments on the PR, the proposed approach generates an
unnecessary `UNION` against itself that causes the improper results to be
returned when `all` is used as they are not deduped.
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:7>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Colleen Dunlap):

* needs_better_patch: 1 => 0

Comment:

Hi Simon! I responded. It's already returning self and it's still
breaking. Also do I make my PR pointing to the 5.2 branch or main?
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:8>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Colleen Dunlap):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:9>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

It should point at `main` which your new PR does, thank you for that.

Please have a look at
[https://github.com/django/django/pull/19471#discussion_r2091852049 this
comment]. The fact the tests are passing is not a good indicative that the
right thing is happening here as the adjusted test I provided demonstrates
(it does a `UNION` against itself which is not safe when `all` is used).

I still believe that the right way of fixing this is to adjust
`QuerySet.union` to return `self` when `not other_qs`.
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:10>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Colleen Dunlap):

is that not what this is doing already?
https://github.com/django/django/blob/0b2ed4f7c8396c8d9aa8428a40e6b25c31312889/django/db/models/query.py#L1548
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:11>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Colleen Dunlap):

ok nevermind I'm rereading this and it looks like I may have
misinterpreted...
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:12>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

No, the branch you linked is gated by a `if isinstance(self,
EmptyQuerySet)` and `qs` is a list of all non-empty querysets. Django is
able to detect when a queryset cannot match any rows (the most common
example is `filter(pk=[])`) and the logic you linked deals with this case.

What I mean is that if the `*other_qs` args capture is empty then you can
immediately return as `some_set OR not other set` means `some_set`.
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:13>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Colleen Dunlap):

Got it! I updated accordingly
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:14>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: assigned
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_better_patch: 1 => 0

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:15>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap

Type: Bug | Status: closed

Component: Database layer | Version: 5.2
(models, ORM) |

Severity: Release blocker | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"802baf5da5b8d8b44990a8214a43b951e7ab8b39" 802baf5]:
{{{#!CommitTicketReference repository=""
revision="802baf5da5b8d8b44990a8214a43b951e7ab8b39"
Fixed #36388 -- Made QuerySet.union() return self when called with no
arguments.

Regression in 9cb8baa0c4fa2c10789c5c8b65f4465932d4d172.
Thank you to Antoine Humeau for the report and Simon Charette for the
review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:16>

[Django]

#36388: QuerySet.union no longer supports empty args
-------------------------------------+-------------------------------------
Reporter: Antoine Humeau | Owner: Colleen
| Dunlap
Type: Bug | Status: closed
Component: Database layer | Version: 5.2
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Sarah Boyce <42296566+sarahboyce@…>):

In [changeset:"787f3130f751283140fe2be8188eb5299552232d" 787f3130]:
{{{#!CommitTicketReference repository=""
revision="787f3130f751283140fe2be8188eb5299552232d"
[5.2.x] Fixed #36388 -- Made QuerySet.union() return self when called with

no arguments.

Regression in 9cb8baa0c4fa2c10789c5c8b65f4465932d4d172.
Thank you to Antoine Humeau for the report and Simon Charette for the
review.

Backport of 802baf5da5b8d8b44990a8214a43b951e7ab8b39 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36388#comment:17>