Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fixed #22217 - ManyToManyField.through_fields fixes.

- Docs description of arguments mix up.
- Keep it from erroneously masking E332 check.
- Add checks E338 and E339, tweak message of E337.
  • Loading branch information...
commit aaad3e27ac7cfcbbfeac6353d17d27e8da523cc8 1 parent f4d9163
@dfunckt dfunckt authored ramiro committed
View
98 django/db/models/fields/related.py
@@ -1904,21 +1904,7 @@ def _check_relationship_model(self, from_model=None, **kwargs):
)
)
- 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 @@ def _check_relationship_model(self, from_model=None, **kwargs):
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 @@ def _get_m2m_attr(self, related, attr):
(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 @@ def _get_m2m_reverse_attr(self, related, attr):
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 = ''
View
4 docs/ref/checks.txt
@@ -94,7 +94,9 @@ Related Fields
* **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>.
+* **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 ``<model>``.
+* **fields.E338**: The intermediary model ``<through model>`` has no field ``<field name>``.
+* **fields.E339**: ``<model>.<field name>`` is not a foreign key to ``<model>``.
Signals
~~~~~~~
View
11 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,
View
4 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 <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
View
86 tests/invalid_models_tests/test_relative_fields.py
@@ -240,6 +240,32 @@ class Relationship(models.Model):
]
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 Fan(models.Model):
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 Invitation(models.Model):
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)

0 comments on commit aaad3e2

Please sign in to comment.
Something went wrong with that request. Please try again.