Fixed #14549 - Removed restriction of single FKs on intermediary tables

Thanks to Loic Bistuer for review. Minor changes to error messages
done by committer.

django/db/models/fields/related.py

@@ -1250,9 +1250,12 @@ class OneToOneRel(ManyToOneRel):
 
 class ManyToManyRel(object):
     def __init__(self, to, related_name=None, limit_choices_to=None,
-                 symmetrical=True, through=None, db_constraint=True, related_query_name=None):
+                 symmetrical=True, through=None, through_fields=None,
+                 db_constraint=True, related_query_name=None):
         if through and not db_constraint:
             raise ValueError("Can't supply a through model and db_constraint=False")
+        if through_fields and not through:
+            raise ValueError("Cannot specify through_fields without a through model")
         self.to = to
         self.related_name = related_name
         self.related_query_name = related_query_name
@@ -1262,6 +1265,7 @@ class ManyToManyRel(object):
         self.symmetrical = symmetrical
         self.multiple = True
         self.through = through
+        self.through_fields = through_fields
         self.db_constraint = db_constraint
 
     def is_hidden(self):
@@ -1849,6 +1853,7 @@ class ManyToManyField(RelatedField):
             limit_choices_to=kwargs.pop('limit_choices_to', None),
             symmetrical=kwargs.pop('symmetrical', to == RECURSIVE_RELATIONSHIP_CONSTANT),
             through=kwargs.pop('through', None),
+            through_fields=kwargs.pop('through_fields', None),
             db_constraint=db_constraint,
         )
 
@@ -1878,6 +1883,12 @@ class ManyToManyField(RelatedField):
         return []
 
     def _check_relationship_model(self, from_model=None, **kwargs):
+        if hasattr(self.rel.through, '_meta'):
+            qualified_model_name = "%s.%s" % (
+                self.rel.through._meta.app_label, self.rel.through.__name__)
+        else:
+            qualified_model_name = self.rel.through
+
         errors = []
 
         if self.rel.through not in apps.get_models(include_auto_created=True):
@@ -1885,13 +1896,28 @@ class ManyToManyField(RelatedField):
             errors.append(
                 checks.Error(
                     ("Field specifies a many-to-many relation through model "
-                     "'%s', which has not been installed.") % self.rel.through,
+                     "'%s', which has not been installed.") %
+                    qualified_model_name,
                     hint=None,
                     obj=self,
                     id='fields.E331',
                 )
             )
 
+        elif self.rel.through_fields is not None:
+            if not len(self.rel.through_fields) >= 2 or not (self.rel.through_fields[0] and self.rel.through_fields[1]):
+                errors.append(
+                    checks.Error(
+                        ("The field is given an iterable for through_fields, "
+                         "which does not provide the names for both link fields "
+                         "that Django should use for the relation through model "
+                         "'%s'.") % qualified_model_name,
+                        hint=None,
+                        obj=self,
+                        id='fields.E337',
+                    )
+                )
+
         elif not isinstance(self.rel.through, six.string_types):
 
             assert from_model is not None, \
@@ -1926,13 +1952,16 @@ class ManyToManyField(RelatedField):
                 seen_self = sum(from_model == getattr(field.rel, 'to', None)
                     for field in self.rel.through._meta.fields)
 
-                if seen_self > 2:
+                if seen_self > 2 and not self.rel.through_fields:
                     errors.append(
                         checks.Error(
                             ("The model is used as an intermediate model by "
                              "'%s', but it has more than two foreign keys "
-                             "to '%s', which is ambiguous.") % (self, from_model_name),
-                            hint=None,
+                             "to '%s', which is ambiguous. You must specify "
+                             "which two foreign keys Django should use via the "
+                             "through_fields keyword argument.") % (self, from_model_name),
+                            hint=("Use through_fields to specify which two "
+                                  "foreign keys Django should use."),
                             obj=self.rel.through,
                             id='fields.E333',
                         )
@@ -1945,12 +1974,14 @@ class ManyToManyField(RelatedField):
                 seen_to = sum(to_model == getattr(field.rel, 'to', None)
                     for field in self.rel.through._meta.fields)
 
-                if seen_from > 1:
+                if seen_from > 1 and not self.rel.through_fields:
                     errors.append(
                         checks.Error(
                             ("The model is used as an intermediate model by "
                              "'%s', but it has more than one foreign key "
-                             "from '%s', which is ambiguous.") % (self, from_model_name),
+                             "from '%s', which is ambiguous. You must specify "
+                             "which foreign key Django should use via the "
+                             "through_fields keyword argument.") % (self, from_model_name),
                             hint=('If you want to create a recursive relationship, '
                                   'use ForeignKey("self", symmetrical=False, '
                                   'through="%s").') % relationship_model_name,
@@ -1959,12 +1990,14 @@ class ManyToManyField(RelatedField):
                         )
                     )
 
-                if seen_to > 1:
+                if seen_to > 1 and not self.rel.through_fields:
                     errors.append(
                         checks.Error(
                             ("The model is used as an intermediate model by "
                              "'%s', but it has more than one foreign key "
-                             "to '%s', which is ambiguous.") % (self, to_model_name),
+                             "to '%s', which is ambiguous. You must specify "
+                             "which foreign key Django should use via the "
+                             "through_fields keyword argument.") % (self, to_model_name),
                             hint=('If you want to create a recursive '
                                   'relationship, use ForeignKey("self", '
                                   'symmetrical=False, through="%s").') % relationship_model_name,
@@ -2057,10 +2090,18 @@ class ManyToManyField(RelatedField):
         cache_attr = '_m2m_%s_cache' % attr
         if hasattr(self, cache_attr):
             return getattr(self, cache_attr)
+        if self.rel.through_fields is not None:
+            link_field_name = self.rel.through_fields[0]
+        else:
+            link_field_name = None
         for f in self.rel.through._meta.fields:
-            if hasattr(f, 'rel') and f.rel and f.rel.to == related.model:
+            if hasattr(f, 'rel') and f.rel and f.rel.to == related.model and \
+                    (link_field_name is None or link_field_name == f.name):
                 setattr(self, cache_attr, getattr(f, attr))
                 return getattr(self, cache_attr)
+        # We only reach here if we're given an invalid field name via the
+        # `through_fields` argument
+        raise FieldDoesNotExist(link_field_name)
 
     def _get_m2m_reverse_attr(self, related, attr):
         "Function that can be curried to provide the related accessor or DB column name for the m2m table"
@@ -2068,9 +2109,13 @@ class ManyToManyField(RelatedField):
         if hasattr(self, cache_attr):
             return getattr(self, cache_attr)
         found = False
+        if self.rel.through_fields is not None:
+            link_field_name = self.rel.through_fields[1]
+        else:
+            link_field_name = None
         for f in self.rel.through._meta.fields:
             if hasattr(f, 'rel') and f.rel and f.rel.to == related.parent_model:
-                if related.model == related.parent_model:
+                if link_field_name is None and related.model == related.parent_model:
                     # If this is an m2m-intermediate to self,
                     # the first foreign key you find will be
                     # the source column. Keep searching for
@@ -2080,10 +2125,15 @@ class ManyToManyField(RelatedField):
                         break
                     else:
                         found = True
-                else:
+                elif link_field_name is None or link_field_name == f.name:
                     setattr(self, cache_attr, getattr(f, attr))
                     break
-        return getattr(self, cache_attr)
+        try:
+            return getattr(self, cache_attr)
+        except AttributeError:
+            # We only reach here if we're given an invalid reverse field
+            # name via the `through_fields` argument
+            raise FieldDoesNotExist(link_field_name)
 
     def value_to_string(self, obj):
         data = ''

docs/ref/checks.txt

@@ -90,10 +90,11 @@ Related Fields
 * **fields.E330**: ManyToManyFields cannot be unique.
 * **fields.E331**: Field specifies a many-to-many relation through model ``%s``, which has not been installed.
 * **fields.E332**: Many-to-many fields with intermediate tables must not be symmetrical.
-* **fields.E333**: The model is used as an intermediate model by ``<model>``, but it has more than two foreign keys to ``<model>``, which is ambiguous.
-* **fields.E334**: The model is used as an intermediate model by ``<model>``, but it has more than one foreign key from ``<model>``, which is ambiguous.
-* **fields.E335**: The model is used as an intermediate model by ``<model>``, but it has more than one foreign key to ``<model>``, which is ambiguous.
-* **fields.E336**: The model is used as an intermediary model by ``<model>``, but it does not have foreign key to ``<model>`` or ``<model>``."
+* **fields.E333**: The model is used as an intermediate model by ``<model>``, but it has more than two foreign keys to ``<model>``, which is ambiguous. You must specify which two foreign keys Django should use via the through_fields keyword argument.
+* **fields.E334**: The model is used as an intermediate model by ``<model>``, but it has more than one foreign key from ``<model>``, which is ambiguous. You must specify which foreign key Django should use via the through_fields keyword argument.
+* **fields.E335**: The model is used as an intermediate model by ``<model>``, but it has more than one foreign key to ``<model>``, which is ambiguous. You must specify which foreign key Django should use via the through_fields keyword argument.
+* **fields.E336**: The model is used as an intermediary model by ``<model>``, but it does not have foreign key to ``<model>`` or ``<model>``.
+* **fields.E337**: The field is given an iterable for through_fields, which does not provide the names for both link fields that Django should use for the relation through <model>.
 
 Signals
 ~~~~~~~

docs/ref/models/fields.txt

@@ -1337,6 +1337,55 @@ that control how the relationship functions.
     :ref:`extra data with a many-to-many relationship
     <intermediary-manytomany>`.
 
+.. attribute:: ManyToManyField.through_fields
+
+    .. versionadded:: 1.7
+
+    Only used when a custom intermediary model is specified. Django will
+    normally determine which fields of the intermediary model to use in order
+    to establish a many-to-many relationship automatically. However,
+    consider the following models::
+
+        from django.db import models
+
+        class Person(models.Model):
+            name = models.CharField(max_length=50)
+
+        class Group(models.Model):
+            name = models.CharField(max_length=128)
+            members = models.ManyToManyField(Person, through='Membership', through_fields=('person', 'group'))
+
+        class Membership(models.Model):
+            person = models.ForeignKey(Person)
+            group = models.ForeignKey(Group)
+            inviter = models.ForeignKey(Person, related_name="membership_invites")
+            invite_reason = models.CharField(max_length=64)
+
+    ``Membership`` has *two* foreign keys to ``Person`` (``person`` and
+    ``inviter``), which makes the relationship ambiguous and Django can't know
+    which one to use. In this case, you must explicitly specify which
+    foreign keys Django should use using ``through_fields``, as in the example
+    above.
+
+    ``through_fields`` accepts a 2-tuple ``('field1', 'field2')``, where
+    ``field1`` is the name of the foreign key to the target model (``person``
+    in this case), and ``field2`` the name of the foreign key to the model the
+    :class:`ManyToManyField` is defined on (``group`` in this case).
+
+    When you have more than one foreign key on an intermediary model to any
+    (or even both) of the models participating in a many-to-many relationship,
+    you *must* specify ``through_fields``. This also applies to
+    :ref:`recursive relationships <recursive-relationships>`
+    when an intermediary model is used and there are more than two
+    foreign keys to the model, or you want to explicitly specify which two
+    Django should use.
+
+    Recursive relationships using an intermediary model are always defined as
+    non-symmetrical -- that is, with :attr:`symmetrical=False <ManyToManyField.symmetrical>`
+    -- therefore, there is the concept of a "source" and a "target". In that
+    case ``'field1'`` will be treated as the "source" of the relationship and
+    ``'field2'`` as the "target".
+
 .. attribute:: ManyToManyField.db_table
 
     The name of the table to create for storing the many-to-many data. If this

docs/releases/1.7.txt

@@ -658,6 +658,12 @@ Models
 * You can use a single list for :attr:`~django.db.models.Options.index_together`
   (rather than a list of lists) when specifying a single set of fields.
 
+* Custom intermediate models having more than one foreign key to any of the
+  models participating in a many-to-many relationship are now permitted,
+  provided you explicitly specify which foreign keys should be used by setting
+  the new :attr:`ManyToManyField.through_fields <django.db.models.ManyToManyField.through_fields>`
+  argument.
+
 Signals
 ^^^^^^^
 

docs/topics/db/models.txt

@@ -434,30 +434,38 @@ something like this::
         invite_reason = models.CharField(max_length=64)
 
 When you set up the intermediary model, you explicitly specify foreign
-keys to the models that are involved in the ManyToMany relation. This
+keys to the models that are involved in the many-to-many relationship. This
 explicit declaration defines how the two models are related.
 
 There are a few restrictions on the intermediate model:
 
 * Your intermediate model must contain one - and *only* one - foreign key
-  to the target model (this would be ``Person`` in our example). If you
-  have more than one foreign key, a validation error will be raised.
-
-* Your intermediate model must contain one - and *only* one - foreign key
-  to the source model (this would be ``Group`` in our example). If you
-  have more than one foreign key, a validation error will be raised.
-
-* The only exception to this is a model which has a many-to-many
-  relationship to itself, through an intermediary model. In this
-  case, two foreign keys to the same model are permitted, but they
-  will be treated as the two (different) sides of the many-to-many
-  relation.
+  to the target model (this would be ``Person`` in our example), or you must
+  explicitly specify the foreign keys Django should use for the relationship
+  using :attr:`ManyToManyField.through_fields <ManyToManyField.through_fields>`.
+  If you have more than one foreign key and ``through_fields`` is not
+  specified, a validation error will be raised. A similar restriction applies
+  to the foreign key to the source model (this would be ``Group`` in our
+  example).
+
+* For a model which has a many-to-many relationship to itself through an
+  intermediary model, two foreign keys to the same model are permitted, but
+  they will be treated as the two (different) sides of the many-to-many
+  relationship. If there are *more* than two foreign keys though, you
+  must also specify ``through_fields`` as above, or a validation error
+  will be raised.
 
 * When defining a many-to-many relationship from a model to
   itself, using an intermediary model, you *must* use
   :attr:`symmetrical=False <ManyToManyField.symmetrical>` (see
   :ref:`the model field reference <manytomany-arguments>`).
 
+.. versionchanged:: 1.7
+
+    In Django 1.6 and earlier, intermediate models containing more than one
+    foreign key to any of the models involved in the many-to-many relationship
+    used to be prohibited.
+
 Now that you have set up your :class:`~django.db.models.ManyToManyField` to use
 your intermediary model (``Membership``, in this case), you're ready to start
 creating some many-to-many relationships. You do this by creating instances of

tests/invalid_models_tests/test_relative_fields.py

@@ -3,6 +3,7 @@ from __future__ import unicode_literals
 
 from django.core.checks import Error
 from django.db import models
+from django.db.models.fields import FieldDoesNotExist
 from django.test.utils import override_settings
 from django.test.testcases import skipIfDBFeature
 
@@ -81,7 +82,9 @@ class RelativeFieldTests(IsolatedModelsTestCase):
             Error(
                 ("The model is used as an intermediate model by "
                  "'invalid_models_tests.Group.field', but it has more than one "
-                 "foreign key to 'Person', which is ambiguous."),
+                 "foreign key to 'Person', which is ambiguous. You must specify "
+                 "which foreign key Django should use via the through_fields "
+                 "keyword argument."),
                 hint=('If you want to create a recursive relationship, use '
                       'ForeignKey("self", symmetrical=False, '
                       'through="AmbiguousRelationship").'),
@@ -205,8 +208,10 @@ class RelativeFieldTests(IsolatedModelsTestCase):
             Error(
                 ("The model is used as an intermediate model by "
                  "'invalid_models_tests.Person.friends', but it has more than two "
-                 "foreign keys to 'Person', which is ambiguous."),
-                hint=None,
+                 "foreign keys to 'Person', which is ambiguous. You must specify "
+                 "which two foreign keys Django should use via the through_fields "
+                 "keyword argument."),
+                hint='Use through_fields to specify which two foreign keys Django should use.',
                 obj=InvalidRelationship,
                 id='fields.E333',
             ),
@@ -1051,3 +1056,84 @@ class ComplexClashTests(IsolatedModelsTestCase):
             ),
         ]
         self.assertEqual(errors, expected)
+
+
+class M2mThroughFieldsTests(IsolatedModelsTestCase):
+    def test_m2m_field_argument_validation(self):
+        """
+        Tests that ManyToManyField accepts the ``through_fields`` kwarg
+        only if an intermediary table is specified.
+        """
+        class Fan(models.Model):
+            pass
+
+        self.assertRaisesMessage(
+            ValueError, 'Cannot specify through_fields without a through model',
+            models.ManyToManyField, Fan, through_fields=('f1', 'f2'))
+
+    def test_invalid_m2m_field(self):
+        """
+        Tests that providing invalid source field name to ManyToManyField.through_fields
+        raises FieldDoesNotExist.
+        """
+        class Fan(models.Model):
+            pass
+
+        class Event(models.Model):
+            invitees = models.ManyToManyField(Fan, through='Invitation', through_fields=('invalid_field', 'invitee'))
+
+        class Invitation(models.Model):
+            event = models.ForeignKey(Event)
+            invitee = models.ForeignKey(Fan)
+            inviter = models.ForeignKey(Fan, related_name='+')
+
+        with self.assertRaisesMessage(FieldDoesNotExist, 'invalid_field'):
+            Event().invitees.all()
+
+    def test_invalid_m2m_reverse_field(self):
+        """
+        Tests that providing invalid reverse field name to ManyToManyField.through_fields
+        raises FieldDoesNotExist.
+        """
+        class Fan(models.Model):
+            pass
+
+        class Event(models.Model):
+            invitees = models.ManyToManyField(Fan, through='Invitation', through_fields=('event', 'invalid_field'))
+
+        class Invitation(models.Model):
+            event = models.ForeignKey(Event)
+            invitee = models.ForeignKey(Fan)
+            inviter = models.ForeignKey(Fan, related_name='+')
+
+        with self.assertRaisesMessage(FieldDoesNotExist, 'invalid_field'):
+            Event().invitees.all()
+
+    def test_explicit_field_names(self):
+        """
+        Tests that if ``through_fields`` kwarg is given, it must specify both
+        link fields of the intermediary table.
+        """
+        class Fan(models.Model):
+            pass
+
+        class Event(models.Model):
+            invitees = models.ManyToManyField(Fan, through='Invitation', through_fields=(None, 'invitee'))
+
+        class Invitation(models.Model):
+            event = models.ForeignKey(Event)
+            invitee = models.ForeignKey(Fan)
+            inviter = models.ForeignKey(Fan, related_name='+')
+
+        field = Event._meta.get_field('invitees')
+        errors = field.check(from_model=Event)
+        expected = [
+            Error(
+                ("The field is given an iterable for through_fields, "
+                 "which does not provide the names for both link fields "
+                 "that Django should use for the relation through model "
+                 "'invalid_models_tests.Invitation'."),
+                hint=None,
+                obj=field,
+                id='fields.E337')]
+        self.assertEqual(expected, errors)

tests/m2m_through/models.py

@@ -77,3 +77,39 @@ class Friendship(models.Model):
     first = models.ForeignKey(PersonSelfRefM2M, related_name="rel_from_set")
     second = models.ForeignKey(PersonSelfRefM2M, related_name="rel_to_set")
     date_friended = models.DateTimeField()
+
+
+# Custom through link fields
+@python_2_unicode_compatible
+class Event(models.Model):
+    title = models.CharField(max_length=50)
+    invitees = models.ManyToManyField(Person, through='Invitation', through_fields=('event', 'invitee'), related_name='events_invited')
+
+    def __str__(self):
+        return self.title
+
+
+class Invitation(models.Model):
+    event = models.ForeignKey(Event, related_name='invitations')
+    # field order is deliberately inverted. the target field is "invitee".
+    inviter = models.ForeignKey(Person, related_name='invitations_sent')
+    invitee = models.ForeignKey(Person, related_name='invitations')
+
+
+@python_2_unicode_compatible
+class Employee(models.Model):
+    name = models.CharField(max_length=5)
+    subordinates = models.ManyToManyField('self', through="Relationship", through_fields=('source', 'target'), symmetrical=False)
+
+    class Meta:
+        ordering = ('pk',)
+
+    def __str__(self):
+        return self.name
+
+
+class Relationship(models.Model):
+    # field order is deliberately inverted.
+    another = models.ForeignKey(Employee, related_name="rel_another_set", null=True)
+    target = models.ForeignKey(Employee, related_name="rel_target_set")
+    source = models.ForeignKey(Employee, related_name="rel_source_set")

tests/m2m_through/tests.py

@@ -6,7 +6,7 @@ from operator import attrgetter
 from django.test import TestCase
 
 from .models import (Person, Group, Membership, CustomMembership,
-    PersonSelfRefM2M, Friendship)
+    PersonSelfRefM2M, Friendship, Event, Invitation, Employee, Relationship)
 
 
 class M2mThroughTests(TestCase):
@@ -276,6 +276,33 @@ class M2mThroughTests(TestCase):
             attrgetter("name")
         )
 
+    def test_through_fields(self):
+        """
+        Tests that relations with intermediary tables with multiple FKs
+        to the M2M's ``to`` model are possible.
+        """
+        event = Event.objects.create(title='Rockwhale 2014')
+        Invitation.objects.create(event=event, inviter=self.bob, invitee=self.jim)
+        Invitation.objects.create(event=event, inviter=self.bob, invitee=self.jane)
+        self.assertQuerysetEqual(event.invitees.all(), [
+            'Jane',
+            'Jim',
+        ], attrgetter('name'))
+
+    def test_through_fields_self_referential(self):
+        john = Employee.objects.create(name='john')
+        peter = Employee.objects.create(name='peter')
+        mary = Employee.objects.create(name='mary')
+        harry = Employee.objects.create(name='harry')
+        Relationship.objects.create(source=john, target=peter, another=None)
+        Relationship.objects.create(source=john, target=mary, another=None)
+        Relationship.objects.create(source=john, target=harry, another=peter)
+        self.assertQuerysetEqual(john.subordinates.all(), [
+            'peter',
+            'mary',
+            'harry',
+        ], attrgetter('name'))
+
     def test_query_tests(self):
         Membership.objects.create(person=self.jim, group=self.rock)
         m2 = Membership.objects.create(person=self.jane, group=self.rock)