Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#1535 closed defect (fixed)

[patch] ManyToMany query problems on magic-removal

Reported by: Russell Cloran <russell@…> Owned by: adrian
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: UI/UX:

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@… 9 years ago.
Russel's patch from the ticket description
mysqllogs.txt (681 bytes) - added by Russell Cloran <russell@…> 9 years ago.
(broken) SQL queries generated
pythonconsole.txt (882 bytes) - added by Russell Cloran <russell@…> 9 years ago.
My test session output
models.py (307 bytes) - added by Russell Cloran <russell@…> 9 years ago.
Entire test models.py
models.2.py (1.4 KB) - added by lukeplant 9 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 Changed 9 years ago by mir@…

  • Cc mir@… added

Changed 9 years ago by mir@…

Russel's patch from the ticket description

comment:2 Changed 9 years ago by mir@…

  • Summary changed from ManyToMany query problems on magic-removal to [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 Changed 9 years ago by lukeplant

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 Changed 9 years ago by Russell Cloran <russell@…>

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.

Changed 9 years ago by Russell Cloran <russell@…>

(broken) SQL queries generated

Changed 9 years ago by Russell Cloran <russell@…>

My test session output

Changed 9 years ago by Russell Cloran <russell@…>

Entire test models.py

Changed 9 years ago by lukeplant

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

comment:5 Changed 9 years ago by lukeplant

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 Changed 9 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from new to closed

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

comment:7 Changed 9 years ago by lukeplant

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

comment:8 Changed 9 years ago by akaihola

Is this related to #2080?

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