Opened 8 years ago

Closed 6 years ago

#26352 closed Bug (fixed)

models.E003 check incorrectly prevents duplicate ManyToMany through-self that differ by through_fields

Reported by: Simon Willison Owned by: nobody
Component: Core (System checks) Version: 1.9
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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:

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:

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.

Change History (9)

comment:1 by Simon Willison, 8 years ago

Has patch: set

I've posted a pull request with a potential fix to Github: https://github.com/django/django/pull/6295

comment:2 by Simon Willison, 8 years ago

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:

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?

comment:3 by Simon Charette, 8 years ago

The only use-case I can think of right now is working around limitations of reverse relationships (#897 seems to mention a variant of this 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).

Last edited 8 years ago by Simon Charette (previous) (diff)

comment:4 by Tim Graham, 8 years ago

In absence of a reported use case, I don't mind closing the issue for now.

comment:5 by Tim Graham, 8 years ago

Resolution: wontfix
Status: newclosed

comment:6 by Raymond Penners, 7 years ago

Resolution: wontfix
Status: closednew

This ticket got closed because of a missing use case. For what it is worth, our "real world use case" is the following:

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 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.

Last edited 7 years ago by Raymond Penners (previous) (diff)

comment:7 by Tim Graham, 7 years ago

Component: Database layer (models, ORM)Core (System checks)
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Does the original pull request solve it? If so, feel free to update and resend it.

comment:8 by Simon Willison, 6 years ago

I've opened a new pull request that applies this change against the latest master of Django 2.x

https://github.com/django/django/pull/10199

Last edited 6 years ago by Simon Willison (previous) (diff)

comment:9 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: newclosed

In 586a9dc4:

Fixed #26352 -- Made system check allow ManyToManyField to target the same model if through_fields differs.

Note: See TracTickets for help on using tickets.
Back to Top