Code

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#17332 closed Uncategorized (wontfix)

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

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

Description

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 akaariai 3 years ago.
POC patch

Download all attachments as: .zip

Change History (5)

comment:1 Changed 3 years ago by akaariai

  • Cc akaariai 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 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...

Changed 3 years ago by akaariai

POC patch

comment:2 Changed 3 years ago by lukeplant

  • Triage Stage changed from Unreviewed to Design 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 2 years ago by akaariai

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

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

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.