Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 20, 2018

Conversation

AmrAnwar
Copy link
Contributor

@AmrAnwar AmrAnwar commented Mar 17, 2018

@charettes
Copy link
Member

Hello @AmrAnwar, could you add a test for it in tests/queries/test_qs_combinators.py as well?

@AmrAnwar
Copy link
Contributor Author

@charettes ok course, I didn't see that it need a test from the site here: https://code.djangoproject.com/ticket/29229
But I'll try now

@sauravjaiswalsj
Copy link

It is fixed i guess so.

@AmrAnwar
Copy link
Contributor Author

@charettes hope you check it now

@AmrAnwar AmrAnwar changed the title Fixed #29229 -- Added compiler.query.annotation & extra in the condition Fixed #29229 -- Added compiler.query.annotation in the condition Mar 17, 2018
@timgraham
Copy link
Member

Can the test check the results of the query rather than internal implementation details like .query.combined_queries[1].annotations?

Incidentally, is there a way to write your query without QuerySet.extra()? That's likely to be deprecated in the future once we can obsolete it.

@timgraham timgraham changed the title Fixed #29229 -- Added compiler.query.annotation in the condition Fixed #29229 -- Fixed QuerySet.values_list() combined with .extra() or .annotate() and union(). Mar 17, 2018
@timgraham timgraham changed the title Fixed #29229 -- Fixed QuerySet.values_list() combined with .extra() or .annotate() and union(). Fixed #29229 -- Fixed column mismatch crash when combining two annotated values_list() querysets with union(), difference(), or intersection(). Mar 19, 2018
count=Value(0, IntegerField()),
).values_list('id', 'count')
qs2 = Number.objects.filter(num=0).values('num').annotate(
id=F('pk'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on pk might be problematic since we shouldn't assume the values that the database might assign.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Maybe:

def test_union_with_two_annotated_values_list(self):
    qs1 = Number.objects.filter(num=1).annotate(
        count=Value(0, IntegerField()),
    ).values_list('num', 'count')
    qs2 = Number.objects.filter(num=2).values('pk').annotate(
        count=F('num'),
        num=Value(1, IntegerField()),
    ).values_list('num', 'count')
    self.assertCountEqual(qs1.union(qs2), [(1, 0), (2, 1)])

@timgraham timgraham requested a review from felixxm March 19, 2018 14:35
@@ -408,7 +408,8 @@ def get_combinator_sql(self, combinator, all):
# If the columns list is limited, then all combined queries
# must have the same columns list. Set the selects defined on
# the query on all combined queries, if not already set.
if not compiler.query.values_select and self.query.values_select:
if (not(compiler.query.values_select or compiler.query.annotations) and
self.query.values_select):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can fix this condition without nested parentheses, i.e.

if not compiler.query.values_select and not compiler.query.annotations and self.query.values_select:

Copy link
Contributor Author

@AmrAnwar AmrAnwar Mar 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to push the code in the test function but your code is better

        qs1 = Number.objects.filter(num=1).annotate(
            count=Value(0, IntegerField())
        ).values('count', num2=F('num')).order_by()
        qs2 = Number.objects.filter(num=0).values('num').annotate(
            num2=F("num")).annotate(
            count=Value(0, IntegerField()),
        ).values_list('count', "num2")
        qs1_union_qs2 = qs2.union(qs1).order_by('num2')
        self.assertEqual(list(qs1_union_qs2), [(0, 0), (1, 0)])

count=Value(0, IntegerField()),
).values_list('id', 'count')
qs2 = Number.objects.filter(num=0).values('num').annotate(
id=F('pk'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Maybe:

def test_union_with_two_annotated_values_list(self):
    qs1 = Number.objects.filter(num=1).annotate(
        count=Value(0, IntegerField()),
    ).values_list('num', 'count')
    qs2 = Number.objects.filter(num=2).values('pk').annotate(
        count=F('num'),
        num=Value(1, IntegerField()),
    ).values_list('num', 'count')
    self.assertCountEqual(qs1.union(qs2), [(1, 0), (2, 1)])

@timgraham
Copy link
Member

A couple builds failed, is the test non-deterministic?

@felixxm
Copy link
Member

felixxm commented Mar 19, 2018

It should be deterministic, because the query is really simple:

SELECT "queries_number"."num", 0 AS "count"
FROM "queries_number"
WHERE "queries_number"."num" = 1
UNION 
SELECT "queries_number"."num" AS "count", 1 AS "num"
FROM "queries_number"
WHERE "queries_number"."num" = 2

The test failed only python 3.5, so maybe it revealed an unrelated bug 🤔. I will take a closer look at this.

…ted values_list() querysets with union(), difference(), or intersection().

Regression in 7316720603821ebb64dfe8fa592ba6edcef5f3e.
@felixxm
Copy link
Member

felixxm commented Mar 19, 2018

@timgraham Splitting annotations in the qs2 fixed tests failure. QuerySet.annotate() doesn't preserve ordering of kwargs in python 3.5.

@timgraham timgraham merged commit a0c03c6 into django:master Mar 20, 2018
@AmrAnwar AmrAnwar deleted the ticket_29229 branch March 20, 2018 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants