Opened 6 years ago

Closed 6 years ago

Last modified 6 years 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 6 years ago.
regression test result

Download all attachments as: .zip

Change History (9)

by Dimas Ari, 6 years ago

Attachment: regression_test_29229.txt added

regression test result

comment:1 by Tim Graham, 6 years ago

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

comment:2 by Tim Graham, 6 years ago

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 by master, 6 years ago

Description: modified (diff)

Precisions about versions.

comment:4 by Tim Graham <timograham@…>, 6 years ago

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 by Tim Graham <timograham@…>, 6 years ago

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 by Tim Graham <timograham@…>, 6 years ago

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 by Tim Graham, 6 years ago

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 by master, 6 years ago

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