PermissionsMixin should define related name of groups and user_permissions related name

[Benjamin Dauvergne]

Third party expecting a group.user_set or permission.user_set reverse relationships are broken by custom user model whose class name is not 'User'.

An easy work around would be to add a related_name='user_set' to the definitions of those two fields.

diff --git a/django/contrib/auth/models.py b/django/contrib/auth/models.py
index 5709d25..e0a2dba 100644
--- a/django/contrib/auth/models.py
+++ b/django/contrib/auth/models.py
@@ -294,10 +294,12 @@ class PermissionsMixin(models.Model):
     groups = models.ManyToManyField(Group, verbose_name=_('groups'),
         blank=True, help_text=_('The groups this user belongs to. A user will '
                                 'get all permissions granted to each of '
-                                'his/her group.'))
+                                'his/her group.'),
+        related_name='user_set')
     user_permissions = models.ManyToManyField(Permission,
         verbose_name=_('user permissions'), blank=True,
-        help_text='Specific permissions for this user.')
+        help_text='Specific permissions for this user.',
+        related_name='user_set')
 
     class Meta:
         abstract = True

[Benjamin Dauvergne]

My proposition is not good: when you do not use related_name you get user_set for the name of the related field and user for the name of the field to use in query filters, but if you defined user_set as the related name, you will have user_set for the related field name but you will also have user_set for the related field name in query filters (example: Q(group__user_set=user)); that's no good ;(

So I maintain that custom user model break third party code expecting a fixed name for the reverse relation between group and the user model (exemple: django-cms); but that the current related_name option behaviour does not allow to simulate the normal behaviour, so I have no solution to propose.

[Russell Keith-Magee]

For the record - this is a backwards incompatible change. However, discussing this with the core devs at the DjangoCon sprints, we're comfortable with making this change. The affected demographic is:

• people who have started a project on 1.5
• who have a custom user model
• that isn't named User
• who are doing their own internal modifications of groups and permissions.

If this ticket had been raised pre 1.5, it would have been a no-brainer change, so for the sake of practicality, we should do it now before this is a deeply embedded API problem.

So - the patch for this needs to include a backwards compatibility entry in the release notes.

[Marc Tamlyn]

Upgrading this to a release blocker so it definitely gets done in 1.6.

[Russell Keith-Magee]

The core team discussion didn't cover the point raised in the first comment - that reverse lookups in the ORM are an issue here, and that didn't come up in the core team discussion.

The default handling names the attribute user_set, but lookups use user__XXX. However, when you move to an explicit related_name, that name is used for both the attribute and the look kwarg. This means we'd actually be breaking running code that was using reverse relations -- any query on group__user_set for example. We may need to take this to django-dev for further discussion.

[benjaoming]

Hi Russell! Thanks for your attention to this. The process of supporting the new custom user models as a reusable apps dev has been a bit of a strain. Until further notice about where the dev discussion takes place, here are my 2 cents...

The pitch: I think this issue and others affect the idea of models being swappable in the sense of that term. If we cannot achieve a proper transparent swap, then all these swapped cases become hacks with nasty implications. One example is if django.VERSION>=(1,5): User=get_user_model() else ... in every place that's using a User model for Django 1.5 -- can we even count that? :) In addition to that, we will need conditions on everything that the swapped model does not transparently let its swapped replacement do. This makes both forwards and backwards compatibility become very painful.

We need the swapped model to behave like its swappable predecessor in certain cases, and those specifically pertain some abstract class that we have stated a contract about, e.g. AbstractBaseUser or AbstractUser. Everytime we notice a case where a swapped user model that has all features of AbstractUser and yet does not work, I would say should result in a new issue.

The solution draft: I would propose that when related_name is not set, the swapped out model should always dictate the name for both the related attribute and query lookups. So, in case User has been swapped by CustomUser, the automatic naming mechanism that triggers when related_name is not set, will consider User and not CustomUser.

The cost: If this is introduced, then only existing custom user models with their unset related_names will break -- and honestly, that's far less severe than breaking everything else :)

Oh, and a related idea: In the future, consider a Warning to be fired when defining a relation with a swappable/swapped model and there is no explicit related_name.

[Marc Tamlyn]

I'm not convinced that any other swappable model with a foreign key pointing away should behave any differently. If your pluggable application depends on the behaviour of an relationship pointing away from User (so already doesn't work with contrib.auth.User) then it becomes part of your extended user contract that you have this functionality.

I'm not sure a warning that you're doing this is a good idea either, as someone in a totally custom environment without User like models linked to external applications may have perfectly reasonable reasons for wanting a different name.

I am however happy to put a recommendation into the documentation that where sensible you should call your pluggable user myapp.User to reduce these kind of problems when you change it.

[Marc Tamlyn]

Ok, having now understood was Russell was saying in his last comment, I don't think we can make this change - it's too backwardsly incompatible due to the fact that setting the related name changes the filter lookups.

It should be theoretically possible to implement the restriction benjaoming was suggesting, that swappable models automatically set their related attributes and query lookups based on the model they're swapping out. Personally I'm not in favour of this as it will likely cause more confusion for most users. Just because I've swapped out user in my small project for something called Profile doesn't mean that all of the normal rules about automatic naming should break.

As a result, I think the best solution is to document that wherever possible you should call your swapped out model User, and give the reasons why. In an external application which requires doing lookups based on Group, Permission or anything else the swappable user relates to should upgrade this recommendation to a requirement. By depending on the presence of PermissionsMixin's relationships, you are in fact already extending the contract for what a swappable user means. To my mind, including "this must be called User" is a reasonable restriction.

I have considered putting a proxy on Group and Permission to find the swappable model under a default name. This falls over somewhat when the user model is extended to add e.g. User.admin_groups, a m2m to groups the user can administer. Which relationship is now the "default" the proxy would point to?

I agree it would be nicer if we could make a concrete resolution here which would make swappable models easier in external applications. However it is more important to maintain both backwards compatibility and adaptability of the current system. I don't want a restriction that you *have* to name your model User, but equally I don't see much reason why if you want to integrate into a larger ecosystem of pluggable parts you shouldn't have to. Once migrations come around it should be much easier to rename models anyway if you do come across this problem half way through a project.

Documentation PR is at ​https://github.com/django/django/pull/1203

[benjaoming]

I agree with mjtamlyn's immediate changes to the docs. Maybe it should also note that the reason has to do with an unresolved issue in automatic naming of reverse lookups and related queryset attributes on PermissionMixin. Also, I'm pretty sure that people are using AUTH_USER_MODEL like crazy, because they think it's the way to go... so I will suggest a docs change after this one, that makes clear that the user model is not supposed to be swapped out in order to add profile information. Just to do some collateral.

In the longer run...

I don't think that restricting the model class name is the way to go in the long run. It looks like a 1999 javascript library doc :) Should we raise exception SwappedInModelMustBeSameName if a model is swapped out by another model that doesn't have the same name? ;)

Regarding reverse lookups, Q(user__XXX) and object.user_set, please note that this is _ONLY_ a suggestion to maintain the the swapped out model's reverse names in case no related_name parameter is given to the relationship. As soon as related_name is set for a relation to a swappable model, this will be respected! The real issue is that this has not been done on relations in PermissionMixin. There's no turning back from this because supposedly people have already deployed tons of custom user models that aren't named User, and how should they then refactor their code? Renaming tables and models is a real mess.

Btw. a quick count to illustrate the issue: django-cms relies on the "user_set" naming 16 places at least + "user" and "user" 13 places at least.

mjtamlyn:

In an external application which requires doing lookups based on Group, Permission or anything else the swappable user relates to should upgrade this recommendation to a requirement.

I don't agree to this, though. There should be two cases which constitute a contract: 1) If related_name is set, then that's the contract. Same as any other relation or 2) If related name is not set, then always default to the swappable model's name (Django 1.5). In Django 1.6, we could raise an exception instead of having case 2)... 1.6 release note could prompt people to change all "user" lookups to "user_set" if we do related_name="user_set" in PermissionMixin.

mjtamlyn:

By depending on the presence of PermissionsMixin's relationships, you are in fact already extending the contract for what a swappable user means.

No, I don't think so. If we needed relations with no contracts at all, we would be using generic relationships. A swappable model has to be something that we can assume something about, and if it is swapped out by e.g. a model that does not implement PermissionMixin, then the developer should be acknowledging that assumptions made by huge parts of the reusable app ecology would be violated. If, on the other hand, the Django docs did not seriously promote that swapped out user models should implement PermissionsMixin, then reusable apps would no longer be able to depend on that contract, and that would be a major blow. Especially considering that there are security and privacy issues at stake.

[Marc Tamlyn]

Ok, on further discussion with Russell and reading your comments, here's another idea.

Revisiting the idea of solidifying the functionality in PermissionsMixin, I think this is the right way to go. The problem is that at present setting related_name will also change the filter lookups, which is a backwards incompatibility we can't accept. However what we could do is extend the API for relations so that they can also define this extra customisation point - a new kwarg called something like related_filter_name. This would allow us to pin PermissionsMixin in a backwards-compat-to-1.4 way, and only break the small subset of use cases discussed before, which the core discussion in Warsaw deemed acceptable.

To my mind, this is also a good thing to do anyway as at present we have automatic naming functionality which cannot be exactly reproduced by explicit code. IMO this is a bug with the design.

Would this satisfy the requirements you need for Django CMS? It seems to me at least the DjCMS is requiring PermissionsMixin (and contrib.auth for that matter) for this functionality anyway - so this would at least ensure that any mode defined as a subclass of AbstractUser (not AbstractBaseUser) would work seamlessly.

To respond to one of your other points, I think pluggable user models are exactly the right solution to "I have this one extra char field I need to add to User" kind of profile information problems - not an additional table and all of the database complexity that introduces. But that's another discussion ;)

[Preston Holmes]

It would seem a reasonable API restriction for Groups in the specific case, and swappable models in the general case to say that related_names take the name of the swapped model

so group.user_set would remain, in this case 'user' is the role of the model, not the name of the spec

The relatively easy place to set this would be get_accessor_name

​https://github.com/django/django/blob/dffdca1109a2111f104f2419d081c0f971537fec/django/db/models/related.py#L50

However there is a big catch/flaw. The record keeping of swapped models only goes in one direction. When a custom user is installed, we can ask the User model if it is swapped, and find out from the setting what it is swapped with. But we can not take an instance of the CustomUser and ask what it was swapped_for. Without knowing what it was swapped for, get_accessor_name has no way to know the name of the model it was swapped in for.

If we could figure out a sane way to get a swapped_for attribute, then something like this might work:

    def get_accessor_name(self):
        # This method encapsulates the logic that decides what name to give an
        # accessor descriptor that retrieves related many-to-one or
        # many-to-many objects. It uses the lower-cased object_name + "_set",
        # but this can be overridden with the "related_name" option.
        model_name = self.opts.swapped_for or self.opts.model_name
        if self.field.rel.multiple:
            # If this is a symmetrical m2m relation on self, there is no reverse accessor.
            if getattr(self.field.rel, 'symmetrical', False) and self.model == self.parent_model:
                return None
            return self.field.rel.related_name or (model_name + '_set')
        else:
            return self.field.rel.related_name or model_name

[Marc Tamlyn]

This will only solve the reverse relationship descriptor, we would still need to do additional work to get the correct related_query_name for filter lookups. It seems to me your proposed solution isn't much different to fixing the names, except that it future proofs the mechanism for any other possible swappable models in the future.

[Russell Keith-Magee]

@mjtamlyn For me, that's the point -- it's ultimately *exactly* the same solution as manually setting the related_name and related_query_name, except that it's done automatically for any swapped model, since end users will need to do this consistently wherever a model is swapped out. As an aside, adding related_query_name handling may also be worthwhile, but that's orthogonal to this work if we can get the automated swapped_for version working.

[Preston Holmes]

Looking at how this might possibly work, and haven't found any great options.

I think this will have to happen at the level of the app_cache, after all models are loaded. It can traverse the loaded models, looking for those that are 'swappable' then checking for the appropriate setting, then setting a 'swapped_for' attribute on swapped in model. swapped_for as a term is already used in _swapped where I probably would have used swapped_with, but given this is all private API, we can change at will if it makes sense.

There isn't really any way I can see to discover this at Model creation time.

@russelm if you get some time in a bit to look at this and get a start, or at least a plan of attack, I can probably help finish up and/or review in the AM my time.

[Andrew Godwin]

While I understand that a nice generic solution would be great, I'm much more in favour of just adding a related_query_name parameter to get this bug fixed with the least amount of code and getting it out of the way of blocking a release.

If you want the automatic solution I'm going to need someone to step up and do it in the next week before beta, otherwise I'll land a patch for the related_query_name parameter and go with that solution as it's a nice minimum fix to the overall blocker here (which is that PermissionsMixin doesn't work right).

[Andrew Godwin <andrew@…>]

In 99b467f272da91b8894dc90d793d8d2c40b78d8c:

Add related_query_name to ForeignKey/M2M. Refs #20244

[Andrew Godwin <andrew@…>]

In f325f86971bf7fc30e3daafc9835f66cd7951e3a:

Fixed #20244: PermissionsMixin now defines a related_query_name for M2Ms

[antialiasis@…]

This change makes it incredibly difficult to migrate from the default user model to a custom user model with permissions. Which, yes, I realize Django recommends against, but given projects and requirements change it should definitely be kept in mind that this might need to be possible.

Various blogs and StackOverflow questions already describe how to migrate to a custom user model using South. These methods almost universally call for creating your custom user model while AUTH_USER_MODEL is still set to 'auth.User', then performing a migration to one way or another get the users from the auth_user table into a table your new user model can use, and then switching AUTH_USER_MODEL over to the new model. However, with this change, you can't have two models that inherit from PermissionsMixin exist in your project at the same time at any point, because it doesn't pass model validation:

CommandError: One or more models did not validate:
auth.user: Accessor for m2m field 'groups' clashes with related m2m field 'Group.user_set'. Add a related_name argument
to the definition for 'groups'.
auth.user: Accessor for m2m field 'user_permissions' clashes with related m2m field 'Permission.user_set'. Add a related
_name argument to the definition for 'user_permissions'.
accounts.user: Accessor for m2m field 'groups' clashes with related m2m field 'Group.user_set'. Add a related_name
argument to the definition for 'groups'.
accounts.user: Accessor for m2m field 'user_permissions' clashes with related m2m field 'Permission.user_set'. Add
a related_name argument to the definition for 'user_permissions'.

And without having both models active at the same time, South can't migrate data between them or otherwise do anything useful.

Migrating to custom user models is already enough of a quagmire. Is there no way to avoid this headache without breaking third-party apps that use user_set/user in related queries?

[Marc Tamlyn]

During that migration point, it should be straightforwards enough to copy the two fields form PermissionsMixin into your custom model, add a related_name, and then remove them and re-add PermissionsMixin again afterwards. Its worth noting that the same problem you're experiencing would occur with any custom User model named User, irrespective of these changes. What the change guarantees is that when your transitional period is complete, django and other third party applications have a defined api to use if they require Group or Permission.

[anonymous]

Should this be added to the custom user model docs or 1.6 release notes? It would be good to know about the changes to related_name and addition of query_filter_name are coded into PermissionsMixin (the same applies to the user_permissions ManyToManyField).

Example: My app's user model subclasses contrib.auth.AbstractUser and has a model that relates to contrib.auth.Group. I came across this while checking for compatibility with django 1.6 when MyModel.objects.filter(groups__myuser=user_obj) came back as an invalid query.