Skip to content

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

Closed
wants to merge 1 commit into from

3 participants

@dfunckt
dfunckt commented Feb 19, 2014

This is ticket: https://code.djangoproject.com/ticket/14549

Edit: Changed the commit message and pull request title.

@akaariai akaariai commented on an outdated diff Mar 3, 2014
django/db/models/fields/related.py
errors.append(
checks.Error(
('The model is used as an intermediary model by '
'%s, but it has more than two foreign keys '
- 'to %s, which is ambiguous and is not permitted.') % (self, from_model_name),
+ '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),
@akaariai
Django member
akaariai added a note Mar 3, 2014

Maybe use hint="Use through_fields to specify which two foreign keys Django should use".? Seems consistent with the usage of hints in the next errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@akaariai
Django member
akaariai commented Mar 3, 2014

IMO there should be checks that through_fields actually references fields that point to the source and target model, and those fields aren't nullable, and that they are unique. I am not 100% sure of the last point - that is, I am not sure if it is allowed to use non-unique through models for m2m fields currently.

@loic loic commented on an outdated diff Mar 4, 2014
docs/topics/db/models.txt
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.
+ 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 (see
+ :ref:`the model field reference <manytomany-arguments>`). A similar
+ restriction applies to the foreign key to the source model (this would be
+ ``Group`` in our example).
* The only exception to this is a model which has a many-to-many
@loic
Django member
loic added a note Mar 4, 2014

"The only exception" doesn't really work anymore since we already have an "exception" to the rule.

I would rephrase the paragraph starting with: "For models which have a many-to-many relationship to itself, ..."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@loic loic and 1 other commented on an outdated diff Mar 4, 2014
docs/topics/db/models.txt
* 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>`).
@loic
Django member
loic added a note Mar 4, 2014

"(see :ref:`the model field reference <manytomany-arguments>`).", was this removed accidentally?

@dfunckt
dfunckt added a note Mar 4, 2014

I moved it upwards, i thought i shouldn't link again. Will rephrase and add it back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@loic loic and 1 other commented on an outdated diff Mar 4, 2014
docs/ref/models/fields.txt
+ 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).
+
+ You must specify ``through_fields`` when you have more than one
+ foreign key to any (or even both) of the models participating in a
+ many-to-many relationship. This also applies to
+ :ref:`recursive <recursive-relationships>` 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.
@loic
Django member
loic added a note Mar 4, 2014

This still doesn't address the specific case of relationships to self with a customthrough / through_fields where what is "source" and what is "target" matters.

Quick and dirty attempt to illustrate the idea:

Recursive relationships using an intermediary model imply/require
`symmetrical=False`, there is therefore a concept of "source" and "target".
In that case 'field1' will be treated as "source" of the relationship and 'field2'
as the "target".
@loic
Django member
loic added a note Mar 4, 2014

Of course we should confirm from the tests that field1 is actually treated as the source ;)

@dfunckt
dfunckt added a note Mar 4, 2014

I will expand based on your idea, and also provide a simple example:

class Person:
    followers = ManyToManyField('self', through='FollowerInfo', symmetrical=False, related_name='followed_by')
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dfunckt
dfunckt commented Mar 4, 2014

Edited docs as suggested and added tests for the recursive m2m relationship. Pushed as one big squashed commit :)

I didn't expand a lot on the recursive m2m relationships (let alone be able to think of a nice example -- it's a very contrived case: non-symmetric recursive rel, with more than two FKs on the intermediary model). I'd argue that there should be a new ticket about creating a section for recursive relationships and expanding there.

Any comments appreciated.

@dfunckt
dfunckt commented Mar 4, 2014

Rebased to master, and added a note about the new feature in the release notes.

@akaariai
Django member
akaariai commented Mar 5, 2014

Committed manually in c627da0

@akaariai akaariai closed this Mar 5, 2014
@dfunckt dfunckt deleted the dfunckt:ticket_14549 branch Aug 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.