Fixed #8046 -- The first filter() call on a related manager for many-to-many

fields no longer creates duplicate copies of the join table(s). Basically, this
means filters on the join table (for ManyToManyField(through=...)) and complex
filters in the normal (non-through) case don't produce incorrect or duplicate
results.


git-svn-id: http://code.djangoproject.com/svn/django/trunk@8472 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/db/models/fields/related.py

@@ -376,7 +376,7 @@ def create_many_related_manager(superclass, through=False):
                 raise ValueError("%r instance needs to have a primary key value before a many-to-many relationship can be used." % instance.__class__.__name__)
 
         def get_query_set(self):
-            return superclass.get_query_set(self).filter(**(self.core_filters))
+            return superclass.get_query_set(self)._next_is_sticky().filter(**(self.core_filters))
 
         # If the ManyToMany relation has an intermediary model, 
         # the add and remove methods do not exist.

django/db/models/query.py

@@ -121,6 +121,7 @@ class QuerySet(object):
         self.query = query or sql.Query(self.model, connection)
         self._result_cache = None
         self._iter = None
+        self._sticky_filter = False
 
     ########################
     # PYTHON MAGIC METHODS #
@@ -589,7 +590,10 @@ class QuerySet(object):
     def _clone(self, klass=None, setup=False, **kwargs):
         if klass is None:
             klass = self.__class__
-        c = klass(model=self.model, query=self.query.clone())
+        query = self.query.clone()
+        if self._sticky_filter:
+            query.filter_is_sticky = True
+        c = klass(model=self.model, query=query)
         c.__dict__.update(kwargs)
         if setup and hasattr(c, '_setup_query'):
             c._setup_query()
@@ -607,6 +611,17 @@ class QuerySet(object):
             except StopIteration:
                 self._iter = None
 
+    def _next_is_sticky(self):
+        """
+        Indicates that the next filter call and the one following that should
+        be treated as a single filter. This is only important when it comes to
+        determining when to reuse tables for many-to-many filters. Required so
+        that we can filter naturally on the results of related managers.
+        """
+        obj = self._clone()
+        obj._sticky_filter = True
+        return obj
+
     def _merge_sanity_check(self, other):
         """
         Checks that we are merging two comparable QuerySet classes. By default

django/db/models/sql/query.py

@@ -58,6 +58,8 @@ class Query(object):
         self.select_fields = []
         self.related_select_fields = []
         self.dupe_avoidance = {}
+        self.used_aliases = set()
+        self.filter_is_sticky = False
 
         # SQL-related attributes
         self.select = []
@@ -78,7 +80,7 @@ class Query(object):
 
         # These are for extensions. The contents are more or less appended
         # verbatim to the appropriate clause.
-        self.extra_select = SortedDict()  # Maps col_alias -> col_sql.
+        self.extra_select = SortedDict()  # Maps col_alias -> (col_sql, params).
         self.extra_tables = ()
         self.extra_where = ()
         self.extra_params = ()
@@ -185,6 +187,11 @@ class Query(object):
         obj.extra_where = self.extra_where
         obj.extra_params = self.extra_params
         obj.extra_order_by = self.extra_order_by
+        if self.filter_is_sticky and self.used_aliases:
+            obj.used_aliases = self.used_aliases.copy()
+        else:
+            obj.used_aliases = set()
+        obj.filter_is_sticky = False
         obj.__dict__.update(kwargs)
         if hasattr(obj, '_setup_query'):
             obj._setup_query()
@@ -1148,31 +1155,32 @@ class Query(object):
         Can also be used to add anything that has an 'add_to_query()' method.
         """
         if used_aliases is None:
-            used_aliases = set()
+            used_aliases = self.used_aliases
         if hasattr(q_object, 'add_to_query'):
             # Complex custom objects are responsible for adding themselves.
             q_object.add_to_query(self, used_aliases)
-            return
-
-        if self.where and q_object.connector != AND and len(q_object) > 1:
-            self.where.start_subtree(AND)
-            subtree = True
         else:
-            subtree = False
-        connector = AND
-        for child in q_object.children:
-            if isinstance(child, Node):
-                self.where.start_subtree(connector)
-                self.add_q(child, used_aliases)
-                self.where.end_subtree()
+            if self.where and q_object.connector != AND and len(q_object) > 1:
+                self.where.start_subtree(AND)
+                subtree = True
             else:
-                self.add_filter(child, connector, q_object.negated,
-                        can_reuse=used_aliases)
-            connector = q_object.connector
-        if q_object.negated:
-            self.where.negate()
-        if subtree:
-            self.where.end_subtree()
+                subtree = False
+            connector = AND
+            for child in q_object.children:
+                if isinstance(child, Node):
+                    self.where.start_subtree(connector)
+                    self.add_q(child, used_aliases)
+                    self.where.end_subtree()
+                else:
+                    self.add_filter(child, connector, q_object.negated,
+                            can_reuse=used_aliases)
+                connector = q_object.connector
+            if q_object.negated:
+                self.where.negate()
+            if subtree:
+                self.where.end_subtree()
+        if self.filter_is_sticky:
+            self.used_aliases = used_aliases
 
     def setup_joins(self, names, opts, alias, dupe_multis, allow_many=True,
             allow_explicit_fk=False, can_reuse=None):

tests/regressiontests/m2m_through_regress/models.py

@@ -8,7 +8,7 @@ class Membership(models.Model):
     person = models.ForeignKey('Person')
     group = models.ForeignKey('Group')
     price = models.IntegerField(default=100)
-    
+
     def __unicode__(self):
         return "%s is a member of %s" % (self.person.name, self.group.name)
 
@@ -16,7 +16,7 @@ class UserMembership(models.Model):
     user = models.ForeignKey(User)
     group = models.ForeignKey('Group')
     price = models.IntegerField(default=100)
-    
+
     def __unicode__(self):
         return "%s is a user and member of %s" % (self.user.username, self.group.name)
 
@@ -31,10 +31,10 @@ class Group(models.Model):
     # Membership object defined as a class
     members = models.ManyToManyField(Person, through=Membership)
     user_members = models.ManyToManyField(User, through='UserMembership')
-    
+
     def __unicode__(self):
         return self.name
-        
+
 __test__ = {'API_TESTS':"""
 # Create some dummy data
 >>> bob = Person.objects.create(name='Bob')
@@ -46,7 +46,7 @@ __test__ = {'API_TESTS':"""
 >>> frank = User.objects.create_user('frank','frank@example.com','password')
 >>> jane = User.objects.create_user('jane','jane@example.com','password')
 
-# Now test that the forward declared Membership works 
+# Now test that the forward declared Membership works
 >>> Membership.objects.create(person=bob, group=rock)
 <Membership: Bob is a member of Rock>
 
@@ -83,7 +83,7 @@ Traceback (most recent call last):
 ...
 AttributeError: Cannot use create() on a ManyToManyField which specifies an intermediary model.  Use Membership's Manager instead.
 
-# Now test that the intermediate with a relationship outside 
+# Now test that the intermediate with a relationship outside
 # the current app (i.e., UserMembership) workds
 >>> UserMembership.objects.create(user=frank, group=rock)
 <UserMembership: frank is a user and member of Rock>
@@ -100,11 +100,11 @@ AttributeError: Cannot use create() on a ManyToManyField which specifies an inte
 >>> roll.user_members.all()
 [<User: frank>]
 
-# Regression test for #8134 -- 
+# Regression test for #8134 --
 # m2m-through models shouldn't be serialized as m2m fields on the model.
 
-# First, clean up a lot of objects we don't need. 
-# The serialization test only requires three objects to work - 
+# First, clean up a lot of objects we don't need.
+# The serialization test only requires three objects to work -
 # one for each end of the m2m, plus the through model.
 
 >>> User.objects.all().delete()
@@ -117,24 +117,24 @@ AttributeError: Cannot use create() on a ManyToManyField which specifies an inte
 >>> management.call_command('dumpdata', 'm2m_through_regress', format='json', indent=2)
 [
   {
-    "pk": 2, 
-    "model": "m2m_through_regress.membership", 
+    "pk": 2,
+    "model": "m2m_through_regress.membership",
     "fields": {
-      "person": 1, 
-      "price": 100, 
+      "person": 1,
+      "price": 100,
       "group": 2
     }
-  }, 
+  },
   {
-    "pk": 1, 
-    "model": "m2m_through_regress.person", 
+    "pk": 1,
+    "model": "m2m_through_regress.person",
     "fields": {
       "name": "Bob"
     }
-  }, 
+  },
   {
-    "pk": 2, 
-    "model": "m2m_through_regress.group", 
+    "pk": 2,
+    "model": "m2m_through_regress.group",
     "fields": {
       "name": "Roll"
     }
@@ -158,4 +158,18 @@ AttributeError: Cannot use create() on a ManyToManyField which specifies an inte
   </object>
 </django-objects>
 
-"""}
+## Regression test for #8046:
+Check that we don't involve too many copies of the intermediate table when
+doing a join.
+
+>>> bob = Person.objects.create(name='Bob')
+>>> jim = Person.objects.create(name='Jim')
+>>> rock = Group.objects.create(name='Rock')
+>>> roll = Group.objects.create(name='Roll')
+>>> _ = Membership.objects.create(person=bob, group=rock)
+>>> _ = Membership.objects.create(person=jim, group=rock, price=50)
+>>> _ = Membership.objects.create(person=bob, group=roll, price=50)
+>>> rock.members.filter(membership__price=50)
+[<Person: Jim>]
+
+"""}