[Django] #26352: models.E003 check incorrectly prevents duplicate ManyToMany through-self that differ by through_fields
Django
that differ by through_fields
-------------------------------------+-------------------------------------
Reporter: simonw | Owner: nobody
Type: Bug | Status: new
Component: Database | Version: 1.9
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Let's say we're building a Twitter-style friendship model, where a user
can follow other users, and can have users that follow them.
A sensible way to model that might look like this:
{{{#!python
class User(models.Model):
friends = models.ManyToManyField(
'self', through='Followship', symmetrical=False,
through_fields=('user', 'target'), related_name='+'
)
followers = models.ManyToManyField(
'self', through='Followship', symmetrical=False,
through_fields=('target', 'user'), related_name='+'
)
class Followship(models.Model):
user = models.ForeignKey(User, models.CASCADE, related_name='+')
target = models.ForeignKey(User, models.CASCADE, related_name='+')
}}}
Here we have an intermediary table called "Followship", which relates a
user to the target user that they are following.
We also have two helpful convenience ManyToMany fields defined on the
user: `user.friends.all()` returns all other users that the user is
following, while `user.followers.all()` returns the users that are
following our current user.
The above models should work fine... but they don't, because Django's
model checking framework throws the following error:
users.User: (models.E003) The model has two many-to-many relations
through the intermediate model 'users.Followship'.
I don't think this warning is warranted in this case, because the
`through_fields` arguments provided to the friends/followers
ManyToManyFields mean that the relationships defined here make sense and
the models should work correctly.
I've tested this theory by disabling the warning entirely using an over-
ride class method on user, like this:
{{{#!python
class User(models.Model):
friends = models.ManyToManyField(
'self', through='Followship', symmetrical=False,
through_fields=('user', 'target'), related_name='+'
)
followers = models.ManyToManyField(
'self', through='Followship', symmetrical=False,
through_fields=('target', 'user'), related_name='+'
)
@classmethod
def _check_m2m_through_same_relationship(cls):
# Disable models.E003 check for this model
return []
}}}
This does the trick: the check is suppressed, and the models work as
expected.
I think the model check framework is being overly strict here. I think it
should be modified to enable this pattern, provided the through_fields=
parameters on the duplicate ManyToMany fields are present and
differentiate the fields correctly.
--
Ticket URL: <https://code.djangoproject.com/ticket/26352>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
that differ by through_fields
-------------------------------------+-------------------------------------
Reporter: simonw | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
I've posted a pull request with a potential fix to Github:
https://github.com/django/django/pull/6295
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:1>
Django
that differ by through_fields
-------------------------------------+-------------------------------------
Reporter: simonw | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by simonw):
As Simon Charette pointed out in a comment on the pull request, this may
not be necessary. In the above example the same effect can be achieved
using the related_name property, like this:
{{{#!python
class User(models.Model):
friends = models.ManyToManyField(
'self', through='Followship', symmetrical=False,
through_fields=('user', 'target'),
related_name='followers'
)
}}}
In which case, is there a real world use-case in which it's useful to be
able to specify multiple ManyToManyFields in the way proposed by this
ticket?
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:2>
Django
that differ by through_fields
-------------------------------------+-------------------------------------
Reporter: simonw | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by charettes):
The only use-case I can think of right now is working around limitations
of reverse relationships (#897 seems to mention this exact workaround).
If we ever unify the reverse relationships API by exposing them as normal
fields I think an explicit way of defining them would be necessary (to
define options such as `verbose_name`).
In the meantime I'm not sure about what should be done here as I vaguely
remember relying on a similar field definition in the past and it
definitely worked in Django 1.2. On the other hand I wouldn't be surprised
if it breaks Django in subtle ways as it seems untested (migrations come
to mind here).
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:3>
Django
that differ by through_fields
-------------------------------------+-------------------------------------
Reporter: simonw | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
In absence of a reported use case, I don't mind closing the issue for now.
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:4>
Django
that differ by through_fields
-------------------------------------+-------------------------------------
Reporter: simonw | Owner: nobody
Component: Database layer | Version: 1.9
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:5>
Django
that differ by through_fields
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.9
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: wontfix =>
Comment:
This ticket got closed because of a missing use case. For what it is
worth, our "real world use case" is the following:
{{{#!python
class ShippingMethod(models.Model):
....
to_countries = models.ManyToManyField(
Country, through='carriers.ShippingMethodPrice',
through_fields=('method', 'to_country'))
# This is the 2nd ManyToManyField causing E003:
#
# from_countries = models.ManyToManyField(
# Country, through='carriers.ShippingMethodPrice',
# through_fields=('method', 'from_country'), related_name='+')
class ShippingMethodPrice(models.Model):
...
method = models.ForeignKey(
ShippingMethod,
related_name='prices'
)
to_country = models.ForeignKey(Country)
from_country = models.ForeignKey(Country, related_name='+')
price = models.DecimalField()
}}}
So, basically, we have list of shipping methods, and each shipping method
has a price list, containing a specific price per from/to-country
combination.
Given a method, `method.from_countries` should represent the set of all
countries that are occur in the price list as a from-country.
I do not think the workaround suggested in
https://code.djangoproject.com/ticket/26352#comment:2 works for us, as our
case involves 3 models.
The hack to disable E003 seems to work, though a real fix (or suggestions
how to receive the same result otherwise) would be appreciated.
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:6>
Django
that differ by through_fields
Reporter: Simon Willison | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* component: Database layer (models, ORM) => Core (System checks)
* stage: Unreviewed => Accepted
Comment:
Does the original pull request solve it? If so, feel free to update and
resend it.
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:7>
Django
that differ by through_fields
Reporter: Simon Willison | Owner: nobody
Type: Bug | Status: new
Component: Core (System checks) | Version: 1.9
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Willison):
I've opened a new pull request that applies this change against the latest
master of Django 2.x
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:8>
Django
that differ by through_fields
Reporter: Simon Willison | Owner: nobody
Type: Bug | Status: closed
Component: Core (System checks) | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"586a9dc4295357de1f5ad0590ad34bf2bc008f79" 586a9dc4]:
{{{
#!CommitTicketReference repository=""
revision="586a9dc4295357de1f5ad0590ad34bf2bc008f79"
Fixed #26352 -- Made system check allow ManyToManyField to target the same
model if through_fields differs.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26352#comment:9>