Opened 18 years ago

Closed 18 years ago

Last modified 17 years ago

#1535 closed defect (fixed)

[patch] ManyToMany query problems on magic-removal

Reported by: Russell Cloran <russell@…> Owned by: Adrian Holovaty
Component: Database layer (models, ORM) Version: magic-removal
Severity: normal Keywords:
Cc: mir@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi,

When querying a model where one of the query terms includes a ManyToMany relation, some data which should appear in the result doesn't. This occurs if the ManyToMany relation does not have any data. To be more clear, here is an example:

class Issue:
    ...
    cc = models.ManyToManyField(User, blank=True)
    client = models.ForeignKey(User)
    ....

>>> Issue.objects.filter(Q(client=u.id))
[Issue 1, Issue 7]
>>> Issue.objects.filter(Q(cc__id__exact=u.id))
[Issue 7, Issue 8]
>>> Issue.objects.filter(Q(cc__id__exact=u.id) | Q(client=u.id))
[Issue 7, Issue 8]

The result for the last query should include "Issue 1", as far as I can tell. The following patch solves the problem, but it was derived by watching which queries happened and hacking the code to make my case work, so I don't know what it breaks :)

===================================================================
--- django/db/models/query.py   (revision 2546)
+++ django/db/models/query.py   (working copy)
@@ -769,7 +769,7 @@
     if intermediate_table:
         joins[backend.quote_name(current_table)] = (
             backend.quote_name(intermediate_table),
-            "INNER JOIN",
+            "LEFT OUTER JOIN",
             "%s.%s = %s.%s" % \
                 (backend.quote_name(table),
                 backend.quote_name(current_opts.pk.column),

Attachments (5)

query.diff (448 bytes ) - added by mir@… 18 years ago.
Russel's patch from the ticket description
mysqllogs.txt (681 bytes ) - added by Russell Cloran <russell@…> 18 years ago.
(broken) SQL queries generated
pythonconsole.txt (882 bytes ) - added by Russell Cloran <russell@…> 18 years ago.
My test session output
models.py (307 bytes ) - added by Russell Cloran <russell@…> 18 years ago.
Entire test models.py
models.2.py (1.4 KB ) - added by Luke Plant 18 years ago.
models.py - can be inserted into a new folder in modeltests to run tests.

Download all attachments as: .zip

Change History (13)

comment:1 by mir@…, 18 years ago

Cc: mir@… added

by mir@…, 18 years ago

Attachment: query.diff added

Russel's patch from the ticket description

comment:2 by mir@…, 18 years ago

Summary: ManyToMany query problems on magic-removal[patch] ManyToMany query problems on magic-removal
  • there's a patch for the problem in the ticket. Note that I haven't checked or verified the problem, I merely cleaned the ticket up.

comment:3 by Luke Plant, 18 years ago

Strange - I have an equivalent model and it works fine (except I have to add .distinct() to remove the duplicated related item). Could you post a patch in the form of an addition to the test suite?

comment:4 by Russell Cloran <russell@…>, 18 years ago

Luke - which database are you using? I'm *far* from a SQL guru, so I'm not sure if this will make a difference (afaict, it *shouldn't*). I am using MySQL. I will attach complete test output shortly.

by Russell Cloran <russell@…>, 18 years ago

Attachment: mysqllogs.txt added

(broken) SQL queries generated

by Russell Cloran <russell@…>, 18 years ago

Attachment: pythonconsole.txt added

My test session output

by Russell Cloran <russell@…>, 18 years ago

Attachment: models.py added

Entire test models.py

by Luke Plant, 18 years ago

Attachment: models.2.py added

models.py - can be inserted into a new folder in modeltests to run tests.

comment:5 by Luke Plant, 18 years ago

OK, I've attached a version of models.py that can be slotted right into the tests. The tests do fail, on both postgres and mysql (I'm not sure what was different about my 'equivalent' model -- it obviously wasn't equivalent!). With your patch to query.py, all the tests pass.

I think the patch is good. Normally when you (implicitly) add a join in Django, you are also adding a WHERE clause that tests just the 'right hand side' of the join, so INNER JOIN and LEFT OUTER JOIN are identical. With the combination of a clause that tests the left hand side (containing the foreign key) and a clause that tests the m2m table on the right hand side, combined using OR, you have the edge case where they are different, and you need the LEFT JOIN. If this patch adds complications with more complicated joins and queries, I don't know what they are, and it's difficult to work it out without tests or more concrete examples.

I have added the tests to the test suite, and will commit the patch shortly. Thanks for catching this Russell!

comment:6 by Luke Plant, 18 years ago

Resolution: fixed
Status: newclosed

(In [2613]) magic-removal: Fixed #1535 (bug with combined m2m and m2o to the same model)

comment:7 by Luke Plant, 18 years ago

Oops, sorry Russell, I meant to acknowledge the patch in my log message. I fixed it up afterwards for the record.

comment:8 by Antti Kaihola, 18 years ago

Is this related to #2080?

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