Doing a save after primary key has changed should raise an error

[Anssi Kääriäinen]

Currently, if you change primary key and do a save(), you will get a duplicate in the database. In effect, the current behavior of save is: "save the object to DB, except when PK has changed, then do a clone". This API is not good for natural primary keys. Natural primary keys aren't that common currently, but if multicolumn primary keys are introduced into Django, they will be more common.

The idea is to deprecate the current clone behavior of save raise an error instead. This would need to go through the normal deprecation cycle. You can get the current behavior back by using .save(clone=True). For AutoFields, setting the pk to None will work.

For implementation the problems are: How to trac primary key changes? Some fields are mutable inplace, which makes the problem harder. Model __init__ should not be made much slower by this.

Some related tickets: #2259, #4102. There is also a ​Django-developers thread about this.

[Anssi Kääriäinen]

A proof of concept patch attached.

Some notes about the implementation:

• The implementation is based on field.get_immutable_value(obj). By default this returns the fields value as is. This is set to obj._state.old_pk_values, and then in save we compare the new pk value to that. If changes -> deprecation warning.
• I did some cleanup to how model._state is updated, this was required as there was a lot of non-DRYness after my changes.
• I also did some cleanups to model.save(). I think I also found a bug related to transaction handling, I will create another ticket about that.
• There is no compatibility flag, you can get the old behavior back by explicitly rotating the PK value to None and then to new value. That is:

  obj = qs.get(pk=1); obj.pk = 2; obj.save() -> deprecation warning
  obj = qs.get(pk=1); obj.pk = None; obj.pk = 2; obj.save() -> no warning, and will work later on.
  

There is no safe get_immutable_value for 3rd party fields. The idea is that custom fields have two releases to implement this if needed. If they fail to do this and are mutable based, then the tracking of changes will not work. I think this approach is now pretty good, although the current patch does not have tests. How to test PendingDeprecationWarnings?

The bad news about this approach is that model __init__ is now 10-20% slower for somewhat simple models (20% for just integer pk, 10% when there are additional 10 integer fields). The speed difference would likely be much smaller if the fields were large strings, but this still feels a bit bad. I don't know how to make this any faster, except if we track just the "main" primary key, that is, we skip tracking of parent class primary keys.

The next problem is what to do to ModelForms. They would need to have an editable field when inserting, and non-editable field when editing an already existing model. Bad news is that ModelForms do not support non-editable fields. A html attribute editable="false" + validation might work, but isn't too pretty approach...

[Anssi Kääriäinen]

POC patch

[Luke Plant]

On the basis of the discussion in the ticket, and given that this depends on various other pieces of work which may or may not materialize or be merged to trunk, both in terms of providing sufficient motivation for the change and sufficient support in other parts of the framework, I'm going to mark as DDN.

[Anssi Kääriäinen]

I am closing this ticket. Better way forward is having a model subclass/mixin which does automatic primary key updates for you (or at least raises an error on changed primary key). This is backwards compatible, explicit, and does not add any overhead to those who do not need this feature.

[Aymeric Augustin]

I agree — the B/C issues probably outweigh the advantages of this change.