Opened 14 years ago

Closed 12 years ago

Last modified 10 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: dev
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 14 years ago.
tests.diff (2.1 KB ) - added by Juarez Bochi 13 years ago.
doc.diff (576 bytes ) - added by Juarez Bochi 13 years ago.
13914.diff (4.4 KB ) - added by Juarez Bochi 13 years ago.
13914.2.diff (4.4 KB ) - added by Flaviu Simihaian 13 years ago.
13914.3.diff (7.0 KB ) - added by Juarez Bochi 12 years ago.
django_13914_group_natural_keys.py (4.7 KB ) - added by Walter Doekes 10 years ago.
Backport of natural group keys for older Django

Download all attachments as: .zip

Change History (35)

by Juarez Bochi, 14 years ago

comment:1 by Juarez Bochi, 14 years ago

Version: 1.2SVN

comment:2 by erikrose, 14 years ago

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 by Łukasz Rekucki, 14 years ago

Triage Stage: UnreviewedAccepted

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

comment:4 by Cesar Canassa, 14 years ago

Cc: cesar.canassa@… added

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

comment:5 by Paul McMillan, 13 years ago

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 by Gabriel Hurley, 13 years ago

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

by Juarez Bochi, 13 years ago

Attachment: tests.diff added

by Juarez Bochi, 13 years ago

Attachment: doc.diff added

comment:7 by Juarez Bochi, 13 years ago

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 by Łukasz Rekucki, 13 years ago

Patch needs improvement: set

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

by Juarez Bochi, 13 years ago

Attachment: 13914.diff added

comment:9 by Juarez Bochi, 13 years ago

Patch needs improvement: unset

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

comment:10 by diogobaeder, 13 years ago

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 by Flaviu Simihaian, 13 years ago

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

by Flaviu Simihaian, 13 years ago

Attachment: 13914.2.diff added

comment:12 by Gabriel Hurley, 13 years ago

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 by Russell Keith-Magee, 13 years ago

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 by Gabriel Hurley, 13 years ago

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 by Graham King, 13 years ago

Severity: Normal
Type: New feature

comment:16 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:17 by marcin.tustin@…, 12 years ago

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 by Aymeric Augustin, 12 years ago

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 by Christian Karrié, 12 years ago

Cc: ckarrie@… added

comment:20 by Aymeric Augustin, 12 years ago

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.

by Juarez Bochi, 12 years ago

Attachment: 13914.3.diff added

comment:21 by Juarez Bochi, 12 years ago

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 by Aymeric Augustin, 12 years ago

Owner: changed from nobody to Aymeric Augustin

comment:23 by Claude Paroz, 12 years ago

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

Looks nice for me, thanks.

comment:24 by Chris Wilson, 12 years ago

Cc: chris+django@… added

comment:25 by Chris Wilson, 12 years ago

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

in reply to:  25 comment:26 by Simon Charette, 12 years ago

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 by Jannis Leidel, 12 years ago

Resolution: fixed
Status: newclosed

In [17429]:

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

by Walter Doekes, 10 years ago

Backport of natural group keys for older Django

in reply to:  25 comment:28 by Walter Doekes, 10 years ago

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