diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 6260f92de9..2443bbf5c8 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -1904,21 +1904,7 @@ class ManyToManyField(RelatedField): ) ) - 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): + else: assert from_model is not None, \ "ManyToManyField with intermediate " \ @@ -2018,6 +2004,78 @@ class ManyToManyField(RelatedField): id='fields.E336', ) ) + + # Validate `through_fields` + if self.rel.through_fields is not None: + # Validate that we're given an iterable of at least two items + # and that none of them is "falsy" + if not (len(self.rel.through_fields) >= 2 and + self.rel.through_fields[0] and self.rel.through_fields[1]): + errors.append( + checks.Error( + ("Field specifies 'through_fields' but does not " + "provide the names of the two link fields that should be " + "used for the relation through model " + "'%s'.") % qualified_model_name, + hint=("Make sure you specify 'through_fields' as " + "through_fields=('field1', 'field2')"), + obj=self, + id='fields.E337', + ) + ) + + # Validate the given through fields -- they should be actual + # fields on the through model, and also be foreign keys to the + # expected models + else: + assert from_model is not None, \ + "ManyToManyField with intermediate " \ + "tables cannot be checked if you don't pass the model " \ + "where the field is attached to." + + source, through, target = from_model, self.rel.through, self.rel.to + source_field_name, target_field_name = self.rel.through_fields[:2] + + for field_name, related_model in ((source_field_name, source), + (target_field_name, target)): + + possible_field_names = [] + for f in through._meta.fields: + if hasattr(f, 'rel') and getattr(f.rel, 'to', None) == related_model: + possible_field_names.append(f.name) + if possible_field_names: + hint = ("Did you mean one of the following foreign " + "keys to '%s': %s?") % (related_model._meta.object_name, + ', '.join(possible_field_names)) + else: + hint = None + + try: + field = through._meta.get_field(field_name) + except FieldDoesNotExist: + errors.append( + checks.Error( + ("The intermediary model '%s' has no field '%s'.") % ( + qualified_model_name, field_name), + hint=hint, + obj=self, + id='fields.E338', + ) + ) + else: + if not (hasattr(field, 'rel') and + getattr(field.rel, 'to', None) == related_model): + errors.append( + checks.Error( + "'%s.%s' is not a foreign key to '%s'." % ( + through._meta.object_name, field_name, + related_model._meta.object_name), + hint=hint, + obj=self, + id='fields.E339', + ) + ) + return errors def deconstruct(self): @@ -2104,9 +2162,6 @@ class ManyToManyField(RelatedField): (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" @@ -2133,12 +2188,7 @@ class ManyToManyField(RelatedField): elif link_field_name is None or link_field_name == f.name: setattr(self, cache_attr, getattr(f, attr)) break - 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) + return getattr(self, cache_attr) def value_to_string(self, obj): data = '' diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index f7c2e721cb..98a6e00e73 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -94,7 +94,9 @@ Related Fields * **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 . +* **fields.E337**: Field specifies ``through_fields`` but does not provide the names of the two link fields that should be used for the relation through ````. +* **fields.E338**: The intermediary model ```` has no field ````. +* **fields.E339**: ``.`` is not a foreign key to ````. Signals ~~~~~~~ diff --git a/docs/ref/models/fields.txt b/docs/ref/models/fields.txt index ab24dd7e66..39af0c64b0 100644 --- a/docs/ref/models/fields.txt +++ b/docs/ref/models/fields.txt @@ -1353,11 +1353,11 @@ that control how the relationship functions. class Group(models.Model): name = models.CharField(max_length=128) - members = models.ManyToManyField(Person, through='Membership', through_fields=('person', 'group')) + members = models.ManyToManyField(Person, through='Membership', through_fields=('group', 'person')) class Membership(models.Model): - person = models.ForeignKey(Person) group = models.ForeignKey(Group) + person = models.ForeignKey(Person) inviter = models.ForeignKey(Person, related_name="membership_invites") invite_reason = models.CharField(max_length=64) @@ -1368,9 +1368,10 @@ that control how the relationship functions. 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). + ``field1`` is the name of the foreign key to the model the + :class:`ManyToManyField` is defined on (``group`` in this case), and + ``field2`` the name of the foreign key to the target model (``person`` + 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, diff --git a/docs/topics/db/models.txt b/docs/topics/db/models.txt index 4caf9ad736..d5a700f62e 100644 --- a/docs/topics/db/models.txt +++ b/docs/topics/db/models.txt @@ -440,12 +440,12 @@ 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), or you must + to the source model (this would be ``Group`` 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 + to the foreign key to the target model (this would be ``Person`` in our example). * For a model which has a many-to-many relationship to itself through an diff --git a/tests/invalid_models_tests/test_relative_fields.py b/tests/invalid_models_tests/test_relative_fields.py index b04f2d4f17..dabbe61723 100644 --- a/tests/invalid_models_tests/test_relative_fields.py +++ b/tests/invalid_models_tests/test_relative_fields.py @@ -240,6 +240,32 @@ class RelativeFieldTests(IsolatedModelsTestCase): ] self.assertEqual(errors, expected) + def test_symmetric_self_reference_with_intermediate_table_and_through_fields(self): + """Using through_fields in a m2m with an intermediate model shouldn't mask its incompatibility with symmetry.""" + class Person(models.Model): + # Explicit symmetrical=True. + friends = models.ManyToManyField('self', + symmetrical=True, + through="Relationship", + through_fields=('first', 'second')) + + class Relationship(models.Model): + first = models.ForeignKey(Person, related_name="rel_from_set") + second = models.ForeignKey(Person, related_name="rel_to_set") + referee = models.ForeignKey(Person, related_name="referred") + + field = Person._meta.get_field('friends') + errors = field.check(from_model=Person) + expected = [ + Error( + 'Many-to-many fields with intermediate tables must not be symmetrical.', + hint=None, + obj=field, + id='fields.E332', + ), + ] + self.assertEqual(errors, expected) + def test_foreign_key_to_abstract_model(self): class Model(models.Model): foreign_key = models.ForeignKey('AbstractModel') @@ -1071,43 +1097,69 @@ class M2mThroughFieldsTests(IsolatedModelsTestCase): ValueError, 'Cannot specify through_fields without a through model', models.ManyToManyField, Fan, through_fields=('f1', 'f2')) - def test_invalid_m2m_field(self): + def test_invalid_order(self): """ - Tests that providing invalid source field name to ManyToManyField.through_fields - raises FieldDoesNotExist. + Tests that mixing up the order of link fields to ManyToManyField.through_fields + triggers validation errors. """ class Fan(models.Model): pass class Event(models.Model): - invitees = models.ManyToManyField(Fan, through='Invitation', through_fields=('invalid_field', 'invitee')) + invitees = models.ManyToManyField(Fan, through='Invitation', through_fields=('invitee', 'event')) 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() + field = Event._meta.get_field('invitees') + errors = field.check(from_model=Event) + expected = [ + Error( + ("'Invitation.invitee' is not a foreign key to 'Event'."), + hint="Did you mean one of the following foreign keys to 'Event': event?", + obj=field, + id='fields.E339'), + Error( + ("'Invitation.event' is not a foreign key to 'Fan'."), + hint="Did you mean one of the following foreign keys to 'Fan': invitee, inviter?", + obj=field, + id='fields.E339'), + ] + self.assertEqual(expected, errors) - def test_invalid_m2m_reverse_field(self): + def test_invalid_field(self): """ - Tests that providing invalid reverse field name to ManyToManyField.through_fields - raises FieldDoesNotExist. + Tests that providing invalid field names to ManyToManyField.through_fields + triggers validation errors. """ class Fan(models.Model): pass class Event(models.Model): - invitees = models.ManyToManyField(Fan, through='Invitation', through_fields=('event', 'invalid_field')) + invitees = models.ManyToManyField(Fan, through='Invitation', through_fields=('invalid_field_1', 'invalid_field_2')) 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() + field = Event._meta.get_field('invitees') + errors = field.check(from_model=Event) + expected = [ + Error( + ("The intermediary model 'invalid_models_tests.Invitation' has no field 'invalid_field_1'."), + hint="Did you mean one of the following foreign keys to 'Event': event?", + obj=field, + id='fields.E338'), + Error( + ("The intermediary model 'invalid_models_tests.Invitation' has no field 'invalid_field_2'."), + hint="Did you mean one of the following foreign keys to 'Fan': invitee, inviter?", + obj=field, + id='fields.E338'), + ] + self.assertEqual(expected, errors) def test_explicit_field_names(self): """ @@ -1129,11 +1181,11 @@ class M2mThroughFieldsTests(IsolatedModelsTestCase): 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, + ("Field specifies 'through_fields' but does not provide the names " + "of the two link fields that should be used for the relation " + "through model 'invalid_models_tests.Invitation'."), + hint=("Make sure you specify 'through_fields' as " + "through_fields=('field1', 'field2')"), obj=field, id='fields.E337')] self.assertEqual(expected, errors)