Feature: add custom object id field

[philippeducasse]

from issue #963

Adds an object_id_field parameter to register() that allows versioning objects using a custom field (e.g. a UUID or slug) as the stored object_id instead of the primary key. This is useful when migrating legacy data where the natural identifier is of the database pk.

limitation: get_deleted() still assumes object_id == pk and does not support object_id_field. This can be addressed in a follow-up.

[etianen]

Not a bad start, and the tests are appreciated!

I've added some suggestions to simplify it.

I'd also like to have get_deleted and any other functionality updated to respect this new registration option before merging anything,.

[philippeducasse]

Hi @etianen ! thanks for the feedback and suggestions. Ive refactored the code, hopefully its more logical now. I also refactored the get_deleted functionality. I hope I haven't forgotten anything!

[etianen]

Thanks for your changes. This feels a lot cleaner.

I've suggested one simplification, and a couple of admin optimizations. We're pretty close.

I'd also ask:

• You add this new register field to the docs.
• Grep the codebase for explicit uses of pk to make sure you've not missed anything.

[etianen]

There are some parts of the codebase using VersionOptions.object_id_field directly, and others using _get_object_id_field().

What if... we resolved object_id_field to a concrete field name in register, stored that as VersionOptions.object_id_field, and then used VersionOptions.object_id_field consistly everywhere, dropping this helper?

So make object_id_field default to None. If it's None, we resolve it to a concrete primary key field using ._meta.pk.name. If it's a string, we validate it using .get_field during register.

This means explicitly setting object_id_field="pk" becomes an error.

[philippeducasse]

resolving object_id_field here using ._meta.pk.name resulted in errors, for models with multi-table inheritence, so instead i used meta.pk.attname.

if object_id_field is None:
            id_field = model._meta.pk.attname
        else:
            model._meta.get_field(object_id_field)
            id_field = object_id_field

[etianen]

This feels worth optimizing for the case of object_id_field being the primary key. It'll save one query for 99.9% of django-reversion users.

So a check like ._meta.pk.name == object_id_field

[philippeducasse]

great yes thanks for catching this

[etianen]

As above, this is worth optimizing for the common case of object_id_field being the primary key.

[philippeducasse]

Added the parameter to docs, and also greped the codebase :

grep -n "obj.pk|get_object_or_404.*self.model|getattr.*object_id_field"

the only matches are in admin.py and revisions.py: Lines 211 and 323–324 in admin.py are both in else branches; the slow path that only runs when object_id_field != pk.attname. The two hits in revisions.py are unrelated: line 168 is an unsaved-object guard, line 394 is the registration resolution itself.