Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#12252 closed (fixed)

Queryset unions are sometimes incorrect.

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

Description

Queryset unions are in this case incorrect. It appears that an 'inner join' is
not being converted into an 'outer join' when it should be. See the example below.

import unittest
from django.db import models

class ObjectA(models.Model):
    name = models.CharField(max_length=50)

class ObjectB(models.Model):
    name = models.CharField(max_length=50)
    objecta = models.ForeignKey(ObjectA)
    number = models.PositiveSmallIntegerField()

class TestStuff(unittest.TestCase):

    def test_stuff(self):
        names = ['one', 'two', 'three', 'four', 'five', 'six', 'seven']
        obs = [ObjectA(name=name) for name in names]
        for o in obs:
            o.save()
        b_infos = [('fish', 5, obs[0]), ('turtle', 9, obs[0]),
                   ('fish', 6, obs[3])]
        ob_bs = [ObjectB(name=name, objecta=objecta, number=number)
                 for name, number, objecta in b_infos]
        for o in ob_bs:
            o.save()
        all_query = ObjectA.objects.all()
        filter1_query = ObjectA.objects.filter(objectb__name='fish')
        filter2_query = ObjectA.objects.filter(objectb__name='fish',
                                               objectb__number=6)
        # The tests with filter1_query pass for me.
	self.assertEqual(set(all_query | filter1_query),
                         set(all_query) | set(filter1_query))
	self.assertEqual(set(filter1_query | all_query),
                         set(all_query) | set(filter1_query))
        # The second test with filter2_query fails for me.
        self.assertEqual(set(all_query | filter2_query),
                         set(all_query) | set(filter2_query))
        self.assertEqual(set(filter2_query | all_query),
                         set(all_query) | set(filter2_query))

Attachments (5)

12252a.diff (5.8 KB) - added by Ben 7 years ago.
12252b.diff (5.8 KB) - added by Ben 7 years ago.
second attempt to get the patch to display correctly
12252.diff (5.7 KB) - added by Karen Tracey 6 years ago.
Updated patch to apply cleanly
12252.1.diff (5.3 KB) - added by Ramiro Morales 6 years ago.
Karen, no enough ORM fu here, but updated the patch removing unneeded code in tests (init(), setUp())
12252_1.2.diff (5.7 KB) - added by Philippe Raoult 5 years ago.
patch for 1.2.5 in case anyone needs it (fix is commited in 1.2.X branch)

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by Johannes Dollinger

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 7 years ago by Ben

Owner: changed from nobody to Ben

Changed 7 years ago by Ben

Attachment: 12252a.diff added

comment:3 Changed 7 years ago by Ben

Has patch: set

I've uploaded a patch (although it's not displaying properly) which appears to correct this problem, and a regression test to go with it.

Changed 7 years ago by Ben

Attachment: 12252b.diff added

second attempt to get the patch to display correctly

Changed 6 years ago by Karen Tracey

Attachment: 12252.diff added

Updated patch to apply cleanly

comment:4 Changed 6 years ago by Mark Lavin

milestone: 1.3

I'm seeing a similar issue in my project where queryset OR is not commutative. One way produces a LEFT OUTER JOIN and the other produces an INNER JOIN. This patch resolves this issue for me.

comment:5 Changed 6 years ago by Karen Tracey

Triage Stage: AcceptedReady for checkin

Near as I can tell, this is good to go, though I'd be happier if someone with more ORM clues than me would give it look...

Changed 6 years ago by Ramiro Morales

Attachment: 12252.1.diff added

Karen, no enough ORM fu here, but updated the patch removing unneeded code in tests (init(), setUp())

comment:6 Changed 6 years ago by Russell Keith-Magee

Resolution: fixed
Status: newclosed

In [15726]:

Fixed #12252 -- Ensure that queryset unions are commutative. Thanks to benreynwar for the report, and draft patch, and to Karen and Ramiro for the review eyeballs and patch updates.

comment:7 Changed 6 years ago by Russell Keith-Magee

In [15727]:

[1.2.X] Fixed #12252 -- Ensure that queryset unions are commutative. Thanks to benreynwar for the report, and draft patch, and to Karen and Ramiro for the review eyeballs and patch updates.

Backport of r15726 from trunk.

Changed 5 years ago by Philippe Raoult

Attachment: 12252_1.2.diff added

patch for 1.2.5 in case anyone needs it (fix is commited in 1.2.X branch)

comment:8 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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