Opened 6 years ago

Closed 5 years ago

Last modified 2 years ago

#13914 closed New feature (fixed)

Add natural keys to contrib.auth.User and Group models

Reported by: Juarez Bochi Owned by: Aymeric Augustin
Component: contrib.auth Version: master
Severity: Normal Keywords: Contrib, Auth, User, Group, natural keys
Cc: erik@…, cesar.canassa@…, jbochi@…, ckarrie@…, chris+django@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Permissions and content types already have natural keys. Having natural keys for User and Group models could be useful too.

Attachments (7)

contrib_auth_user_group_natural_key.patch (1.8 KB) - added by Juarez Bochi 6 years ago.
tests.diff (2.1 KB) - added by Juarez Bochi 6 years ago.
doc.diff (576 bytes) - added by Juarez Bochi 6 years ago.
13914.diff (4.4 KB) - added by Juarez Bochi 6 years ago.
13914.2.diff (4.4 KB) - added by Flaviu Simihaian 6 years ago.
13914.3.diff (7.0 KB) - added by Juarez Bochi 5 years ago.
django_13914_group_natural_keys.py (4.7 KB) - added by Walter Doekes 2 years ago.
Backport of natural group keys for older Django

Download all attachments as: .zip

Change History (35)

Changed 6 years ago by Juarez Bochi

comment:1 Changed 6 years ago by Juarez Bochi

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Version: 1.2SVN

comment:2 Changed 6 years ago by erikrose

Cc: erik@… added
milestone: 1.3

Going out on a limb and assigning this to 1.3 so it gets some notice. I'd be happy to write tests if needed.

comment:3 Changed 6 years ago by Łukasz Rekucki

Triage Stage: UnreviewedAccepted

Sounds reasonable. I have no idea if this needs tests so leaving for someone else to review.

comment:4 Changed 6 years ago by Cesar Canassa

Cc: cesar.canassa@… added

I am using his patch right now, so I can confirm that it works.

comment:5 Changed 6 years ago by Paul McMillan

Needs documentation: set
Needs tests: set

At the very least this patch still needs tests and documentation before it's RFC. I'm agnostic about the actual functionality.

comment:6 Changed 6 years ago by Gabriel Hurley

Agreed, needs tests and docs, but otherwise the patch seems fine to me.

Changed 6 years ago by Juarez Bochi

Attachment: tests.diff added

Changed 6 years ago by Juarez Bochi

Attachment: doc.diff added

comment:7 Changed 6 years ago by Juarez Bochi

Cc: jbochi@… added
Needs documentation: unset
Needs tests: unset

I have added two small patches for tests and documentation. I am willing to improve/modify them if needed.

comment:8 Changed 6 years ago by Łukasz Rekucki

Patch needs improvement: set

Would be good, if you can join all three patches into one. It's easier to review that way.

Changed 6 years ago by Juarez Bochi

Attachment: 13914.diff added

comment:9 Changed 6 years ago by Juarez Bochi

Patch needs improvement: unset

As per lrekucki suggestion, I have combined the three files into 139414.diff

comment:10 Changed 6 years ago by diogobaeder

This feature would be greatly useful for me, too; I also confirm that the patch works. Please apply it as soon as convenient.

comment:11 Changed 6 years ago by Flaviu Simihaian

13914.diff was not done at the root. I attached 13914.2.diff which updated the patch and verified the tests.

Changed 6 years ago by Flaviu Simihaian

Attachment: 13914.2.diff added

comment:12 Changed 6 years ago by Gabriel Hurley

Triage Stage: AcceptedReady for checkin

I know we're extremely late in the 1.3 cycle here, but it would be really nice to see this slip its way in rather than having to wait for another entire release cycle.

comment:13 Changed 6 years ago by Russell Keith-Magee

milestone: 1.31.4
Triage Stage: Ready for checkinDesign decision needed

Yes, we are *very* late in the cycle; this is something that has fairly major consequences to serialization, so it needs to be considered carefully, and shouldn't be committed between RC and final.

I'm also not entirely convinced that this is a good idea, either -- it has some fairly major consequences to existing serializations. I think it needs more discussion before it gets committed.

comment:14 Changed 6 years ago by Gabriel Hurley

mmmm... I hadn't previously been aware that the natural key deserialization happening in core.serializers.python.Deserializer wasn't written to fall back completely cleanly should it encounter a "mixed" set of serialized objects where it might be expecting a natural key but got a PK instead. I can see where this gets complicated now that I've taken a close read of the code for how it works. Russ is right, as is par for the course. ;-)

comment:15 Changed 5 years ago by Graham King

Severity: Normal
Type: New feature

comment:16 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:17 Changed 5 years ago by marcin.tustin@…

Easy pickings: unset
UI/UX: unset

I would very much like to see this added (even if only in some optional way). Is there a way to help progress this?

comment:18 Changed 5 years ago by Aymeric Augustin

Since the patch doesn't apply to trunk, you could update it for the latest SVN revision.

You could also investigate the issues Russell and Gabriel hint at in commets 13 and 14, and how to mitigate them. Would this change would break all existing user and group fixtures?

comment:19 Changed 5 years ago by Christian Karrié

Cc: ckarrie@… added

comment:20 Changed 5 years ago by Aymeric Augustin

Needs documentation: set
Needs tests: set
Triage Stage: Design decision neededAccepted

I'm going to accept this ticket with one condition: check that existing fixtures (without natural keys) still load properly after the change. This should be tested.

Changed 5 years ago by Juarez Bochi

Attachment: 13914.3.diff added

comment:21 Changed 5 years ago by Juarez Bochi

I have checked that data dumped without natural keys can still be loaded after this change. The tests were included in the new diff.

comment:22 Changed 5 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin

comment:23 Changed 5 years ago by Claude Paroz

Needs documentation: unset
Needs tests: unset
Triage Stage: AcceptedReady for checkin

Looks nice for me, thanks.

comment:24 Changed 5 years ago by Chris Wilson

Cc: chris+django@… added

comment:25 Changed 5 years ago by Chris Wilson

Users can retrofit this, without modifying Django source, using the following monkey patch:

from django.contrib.auth import models as auth_models
from django.db import models as db_models
class GroupManagerWithNaturalKey(db_models.Manager):
    def get_by_natural_key(self, name):
        return self.get(name=name)
auth_models.Group.objects = GroupManagerWithNaturalKey()
def group_natural_key(self):
    return (self.name)
auth_models.Group.natural_key = group_natural_key

comment:26 in reply to:  25 Changed 5 years ago by Simon Charette

Replying to gcc:

Users can retrofit this, without modifying Django source, using the following monkey patch:

from django.contrib.auth import models as auth_models
from django.db import models as db_models
class GroupManagerWithNaturalKey(db_models.Manager):
    def get_by_natural_key(self, name):
        return self.get(name=name)
auth_models.Group.objects = GroupManagerWithNaturalKey()
def group_natural_key(self):
    return (self.name)
auth_models.Group.natural_key = group_natural_key

I think you might want to return a tuple in group_natural_key .

def group_natural_key(self):
    return (self.name,)

comment:27 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [17429]:

Fixed #13914 -- Added natural keys to User and Group models in auth contrib app. Thanks, jbochi and closedbracket.

Changed 2 years ago by Walter Doekes

Backport of natural group keys for older Django

comment:28 in reply to:  25 Changed 2 years ago by Walter Doekes

Replying to gcc:

Users can retrofit this, without modifying Django source, using the following monkey patch:

That fix was incomplete.

If you want to load Groups without PKs, like this:

    <object model="auth.group">
        <field type="CharField" name="name">Billing and invoicing</field>
        <field to="auth.permission" name="permissions" rel="ManyToManyRel">
            <object><natural>add_backupinfo</natural><natural>backupinfo</natural><natural>backupinfo</natural></object>
            <object><natural>change_backupinfo</natural><natural>backupinfo</natural><natural>backupinfo</natural></object>
    ...

Then you'll also need this feature:

New in Django 1.7.
Deserialization of objects with no primary key will always check
whether the model’s manager has a get_by_natural_key() method and
if so, use it to populate the deserialized object’s primary key.

The attached file django_13914_group_natural_keys.py does a monkey patch to introduce the required functionality.

Tested on Django 1.3.7.

Cheers and thanks for Django,
Walter Doekes
OSSO B.V.

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