Opened 2 years ago

Closed 2 years ago

Last modified 18 months ago

#29229 closed Bug (fixed)

QuerySet.values_list() combined with .extra() or .annotate() may produce wrong .union()

Reported by: master Owned by: nobody
Component: Database layer (models, ORM) Version: 1.11
Severity: Release blocker Keywords: union values_list
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by master)

Regression introduced in 2.0, still present in 2.0.3 [Edit: and backported in 1.11.8].

Easy context to reproduce the problem: suppose a Message model, in a postman app. Only one ordinary field is enough for the demo.

qs1 = Message.objects.extra(select={'count': 0}).values_list('id', 'count').order_by()
print(qs1.query)  # as expected:
# SELECT (0) AS "count", "postman_message"."id"
# FROM "postman_message"

qs2 = Message.objects.values('somefield').annotate(count=models.Count('pk')).annotate(id=models.Max('pk')).values_list('id', 'count').order_by()
print(qs2.query)  # as expected:
# SELECT COUNT("postman_message"."id") AS "count",  MAX("postman_message"."id") AS "id"
# FROM "postman_message" GROUP BY "postman_message"."somefield"

print(qs1.union(qs2).query)  # !! WRONG !! the qs2 part is truncated:
# SELECT (0) AS "count", "postman_message"."id" FROM "postman_message"
# UNION
# SELECT MAX("postman_message"."id") AS "id" FROM "postman_message" GROUP BY "postman_message"."somefield"

Compared to version 1.11 [Edit: I run 1.11.7 precisely], it comes from this addition in db/models/sql/compiler.py/get_combinator_sql():

if not compiler.query.values_select and self.query.values_select:
    compiler.query.set_values(self.query.values_select)

Here, self.query.values_select is ('id',).
For qs2, values_select is indeed empty, as coded in db/models/sql/query.py/set_values(),
because in this case the two values_list() arguments are managed in annotation_names[].

Attachments (1)

regression_test_29229.txt (1.3 KB) - added by Dimas Ari 2 years ago.
regression test result

Download all attachments as: .zip

Change History (9)

Changed 2 years ago by Dimas Ari

Attachment: regression_test_29229.txt added

regression test result

comment:1 Changed 2 years ago by Tim Graham

Easy pickings: unset
Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Version: master2.0

comment:2 Changed 2 years ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin
Version: 2.01.11

This affects Django 1.11.8 and later, due to 67316720603821ebb64dfe8fa592ba6edcef5f3e.

comment:3 Changed 2 years ago by master

Description: modified (diff)

Precisions about versions.

comment:4 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In a0c03c62:

Fixed #29229 -- Fixed column mismatch crash when combining two annotated values_list() querysets with union(), difference(), or intersection().

Regression in 7316720603821ebb64dfe8fa592ba6edcef5f3e.

comment:5 Changed 2 years ago by Tim Graham <timograham@…>

In 9123fd7:

[2.0.x] Fixed #29229 -- Fixed column mismatch crash when combining two annotated values_list() querysets with union(), difference(), or intersection().

Regression in 7316720603821ebb64dfe8fa592ba6edcef5f3e.

Backport of a0c03c62a8ac586e5be5b21393c925afa581efaf from master

comment:6 Changed 2 years ago by Tim Graham <timograham@…>

In c5bb472:

[1.11.x] Fixed #29229 -- Fixed column mismatch crash when combining two annotated values_list() querysets with union(), difference(), or intersection().

Regression in 7316720603821ebb64dfe8fa592ba6edcef5f3e.

Backport of a0c03c62a8ac586e5be5b21393c925afa581efaf from master

comment:7 Changed 18 months ago by Tim Graham

It seems that the regression test added here doesn't match the original steps to reproduce closely enough as the original issue has reoccurred after #29286 and a new ticket (#29694) is created.

comment:8 Changed 18 months ago by master

The regression test added by the PR (test_union_with_two_annotated_values_list()) seems to have an unnecessarily complex combination, just to comply with a mandatory ordering of fields imposed by the union.
Instead of:

.annotate(count=F('num'),).annotate(num=Value(1, IntegerField()),

This writing is more understandable:

.annotate(num=F('num'),).annotate(count=Value(1, IntegerField()),
Note: See TracTickets for help on using tickets.
Back to Top