Opened 5 years ago

Closed 3 years ago

Last modified 11 months ago

#13914 closed New feature (fixed)

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

Reported by: jbochi Owned by: aaugustin
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 jbochi 5 years ago.
tests.diff (2.1 KB) - added by jbochi 5 years ago.
doc.diff (576 bytes) - added by jbochi 5 years ago.
13914.diff (4.4 KB) - added by jbochi 5 years ago.
13914.2.diff (4.4 KB) - added by closedbracket 4 years ago.
13914.3.diff (7.0 KB) - added by jbochi 3 years ago.
django_13914_group_natural_keys.py (4.7 KB) - added by wdoekes 11 months ago.
Backport of natural group keys for older Django

Download all attachments as: .zip

Change History (35)

Changed 5 years ago by jbochi

comment:1 Changed 5 years ago by jbochi

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 1.2 to SVN

comment:2 Changed 5 years ago by erikrose

  • Cc erik@… added
  • milestone set to 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 5 years ago by lrekucki

  • Triage Stage changed from Unreviewed to Accepted

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

comment:4 Changed 5 years ago by canassa

  • Cc cesar.canassa@… added

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

comment:5 Changed 5 years ago by PaulM

  • 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 5 years ago by gabrielhurley

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

Changed 5 years ago by jbochi

Changed 5 years ago by jbochi

comment:7 Changed 5 years ago by jbochi

  • 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 5 years ago by lrekucki

  • Patch needs improvement set

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

Changed 5 years ago by jbochi

comment:9 Changed 5 years ago by jbochi

  • Patch needs improvement unset

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

comment:10 Changed 4 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 4 years ago by closedbracket

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

Changed 4 years ago by closedbracket

comment:12 Changed 4 years ago by gabrielhurley

  • Triage Stage changed from Accepted to Ready 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 4 years ago by russellm

  • milestone changed from 1.3 to 1.4
  • Triage Stage changed from Ready for checkin to Design 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 4 years ago by gabrielhurley

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 4 years ago by graham_king

  • Severity set to Normal
  • Type set to New feature

comment:16 Changed 4 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:17 Changed 3 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 3 years ago by aaugustin

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 3 years ago by ckarrie

  • Cc ckarrie@… added

comment:20 Changed 3 years ago by aaugustin

  • Needs documentation set
  • Needs tests set
  • Triage Stage changed from Design decision needed to Accepted

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 3 years ago by jbochi

comment:21 Changed 3 years ago by jbochi

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 3 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:23 Changed 3 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Triage Stage changed from Accepted to Ready for checkin

Looks nice for me, thanks.

comment:24 Changed 3 years ago by gcc

  • Cc chris+django@… added

comment:25 follow-ups: Changed 3 years ago by 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

comment:26 in reply to: ↑ 25 Changed 3 years ago by charettes

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 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [17429]:

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

Changed 11 months ago by wdoekes

Backport of natural group keys for older Django

comment:28 in reply to: ↑ 25 Changed 11 months ago by wdoekes

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