Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#17332 closed Uncategorized (wontfix)

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

Reported by: Anssi Kääriäinen Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: Anssi Kääriäinen Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no


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.

Attachments (1)

17332.diff (14.4 KB) - added by Anssi Kääriäinen 5 years ago.
POC patch

Download all attachments as: .zip

Change History (5)

comment:1 Changed 5 years ago by Anssi Kääriäinen

Cc: Anssi Kääriäinen added
Has patch: set
Needs documentation: set
Needs tests: set
Patch needs improvement: unset

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 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); = 2; -> deprecation warning
    obj = qs.get(pk=1); = None; = 2; -> 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...

Changed 5 years ago by Anssi Kääriäinen

Attachment: 17332.diff added

POC patch

comment:2 Changed 5 years ago by Luke Plant

Triage Stage: UnreviewedDesign decision needed

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.

comment:3 Changed 4 years ago by Anssi Kääriäinen

Resolution: wontfix
Status: newclosed

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.

comment:4 Changed 4 years ago by Aymeric Augustin

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

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