From c627da0ccc12861163f28177aa7538b420a9d310 Mon Sep 17 00:00:00 2001 From: Akis Kesoglou Date: Wed, 19 Feb 2014 20:01:55 +0200 Subject: [PATCH] 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 | 76 ++++++++++++--- docs/ref/checks.txt | 9 +- docs/ref/models/fields.txt | 49 ++++++++++ docs/releases/1.7.txt | 6 ++ docs/topics/db/models.txt | 32 ++++--- .../test_relative_fields.py | 92 ++++++++++++++++++- tests/m2m_through/models.py | 36 ++++++++ tests/m2m_through/tests.py | 29 +++++- 8 files changed, 296 insertions(+), 33 deletions(-) diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 23f8cd154b..e7b66217c5 100644 --- a/django/db/models/fields/related.py +++ b/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 = '' diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index 908a32d4c8..4f023d2844 100644 --- a/docs/ref/checks.txt +++ b/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 ````, but it has more than two foreign keys to ````, which is ambiguous. -* **fields.E334**: The model is used as an intermediate model by ````, but it has more than one foreign key from ````, which is ambiguous. -* **fields.E335**: The model is used as an intermediate model by ````, but it has more than one foreign key to ````, which is ambiguous. -* **fields.E336**: The model is used as an intermediary model by ````, but it does not have foreign key to ```` or ````." +* **fields.E333**: The model is used as an intermediate model by ````, but it has more than two foreign keys to ````, 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 ````, but it has more than one foreign key from ````, 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 ````, but it has more than one foreign key to ````, 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 ````, but it does not have foreign key to ```` or ````. +* **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 . Signals ~~~~~~~ diff --git a/docs/ref/models/fields.txt b/docs/ref/models/fields.txt index b3523334c0..ab24dd7e66 100644 --- a/docs/ref/models/fields.txt +++ b/docs/ref/models/fields.txt @@ -1337,6 +1337,55 @@ that control how the relationship functions. :ref:`extra data with a many-to-many relationship `. +.. 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 ` + 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 ` + -- 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 diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index 096415c5b8..4c0457e7c5 100644 --- a/docs/releases/1.7.txt +++ b/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 ` + argument. + Signals ^^^^^^^ diff --git a/docs/topics/db/models.txt b/docs/topics/db/models.txt index f5ad7ed95f..4caf9ad736 100644 --- a/docs/topics/db/models.txt +++ b/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. + 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 `. + 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). -* 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. +* 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 ` (see :ref:`the model field reference `). +.. 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 diff --git a/tests/invalid_models_tests/test_relative_fields.py b/tests/invalid_models_tests/test_relative_fields.py index 2d54650414..b04f2d4f17 100644 --- a/tests/invalid_models_tests/test_relative_fields.py +++ b/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) diff --git a/tests/m2m_through/models.py b/tests/m2m_through/models.py index 70d6baeca8..aaee0aea3b 100644 --- a/tests/m2m_through/models.py +++ b/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") diff --git a/tests/m2m_through/tests.py b/tests/m2m_through/tests.py index 0fa3d4aa55..39621bf958 100644 --- a/tests/m2m_through/tests.py +++ b/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)