Opened 12 years ago

Last modified 5 days ago

#2259 assigned Bug

Primary keys should be readonly by default in admin

Reported by: ed@… Owned by: Pathangi Jatinshravan
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: sime, Russell Keith-Magee, anssi.kaariainen@…, django@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


When creating a simple model with a non-integer primary key, the Admin interface shows the field as editable, but when making a change, it seems to execute an incorrect query.

For example:

class Group(models.Model):
    name = models.CharField(maxlength=32, primary_key=True)

    def __str__(self):

    class Admin:
        list_display = ('name',)
        search_fields = ('name',)

When changing the primary key field (e.g. from 'Test' to 'TestX') when editing a record, a ProgrammingError exception is raised. The SQL that is generated is:

UPDATE "config_group" SET WHERE "name"='Test'

This is missing the primary key field to be changed after SET.

This is with the latest SVN code. It seems to be an issue around line 179 of db/models/ where it only adds the non-pk columns after SET.

Attachments (4)

pk_change.diff (685 bytes) - added by pigletto 10 years ago.
2259_r13817.diff (11.4 KB) - added by Carl Meyer 7 years ago.
updatable_pk.diff (15.0 KB) - added by Anssi Kääriäinen 6 years ago.
2259-poc.diff (2.9 KB) - added by Bouke Haarsma 4 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 12 years ago by ed@…

This seems to be a problem with the DB layer too since it doesn't work in the shell either. When trying to modify the primary key, it inserts a new record instead of updating the existing record.

>>> from myproject.config.models import Group
>>> Group.objects.all()
>>> g = Group(name="Test")
>>> Group.objects.all()
[<Group: Test>]
>>> g = Group.objects.get(name="Test")
>>> = "TestX"
>>> Group.objects.all()
[<Group: Test>, <Group: TestX>]

comment:2 Changed 11 years ago by Chris Beaven

Component: Admin interfaceDatabase wrapper
Needs tests: set
Triage Stage: UnreviewedAccepted

Someone should write some tests up for this.

comment:3 Changed 10 years ago by pigletto

Owner: changed from nobody to pigletto
Status: newassigned

Changed 10 years ago by pigletto

Attachment: pk_change.diff added

comment:4 Changed 10 years ago by pigletto

Needs tests: unset
Owner: changed from pigletto to nobody
Status: assignednew
Triage Stage: AcceptedDesign decision needed

This bug causes that it is not possible to change primary key value of an object.
It has nothing specific to Admin interface and also the bug appears for integers primary keys as well as for non-integer primary keys.

I've added a patch file that contains tests for this issue.

The problem is with django.db.models.base.Model class.
If we have object that has primary key value set eg.:

>>> g = Group(name="Test")
>>> print

then if we change id like: = 32 

then method will use new(!) id value while checking for object existence in database.
Of course if this object doesn't exists it will be created (INSERT statement) what is not an expected behaviour here (there should be an UPDATE I think).
The question is what should happen if pk is set to another existing value.

This is because pk_val = self._get_pk_val() always returns current value of the object attibute and this is wrong for PK check.
Possible solution should store somewhere original PK value and after successful model save (after executing cursor.commit()) should update this original value.

I wondered about using post_save signal for this but in case of 'managed' transaction it is wrong I think as setting original pk value should be bound to
sucessful RDBMS commit only.

Changes at this area seems to have huge impact on whole framework so I think design decision is needed here.

comment:5 Changed 10 years ago by Jacob

I'm -1 on this -- for better or worse, an instance's PK is what uniquely identifies the object.

comment:6 Changed 10 years ago by Philippe Raoult

-1. Let's just fix the docs and set editable = false on PKs. If someone absolutely, positively wants to update his PKs he can write custom SQL.

comment:7 Changed 10 years ago by Jacob

Resolution: wontfix
Status: newclosed

comment:8 Changed 10 years ago by Simon Litchfield <simon@…>

Resolution: wontfix
Status: closedreopened

This bug still exists. Whether we allow updating PK's or not, the admin still shows them as editable, and it fails to update it. As PhiR says, we need to at least set them editable=False.

BTW, I'm +1 on allowing them to be edited.

comment:9 Changed 10 years ago by Marc Fargas

Summary: Unable to update non-integer primary key in AdminPK Change creates new object instead of update

The bug is there, but maybe the thing is *to not allow pk changes*.
Anyway what happens right now is that Django will insert a new item with the new pk, instead of updating the old one.

comment:10 Changed 10 years ago by Ville Säävuori

I was bitten by this a while go in a so called "real world application". The case was a legacy product database with tens of thousands of products which integrated with a 3rd party solution that used the PK:s in the API. The workflow of adding a product in the database was almost always in two parts because the needed PK came from the manufacturer; first they put the product in with basic info and a dummy PK. Then, when the actual product (and it's PK) arrived from manufacturer the PK was changed and all was fine.

We couldn't get Django Admin to cope with this so we wrote our own admin (no big deal). But yes, this is a bug and it should be fixed (and also documented, especially if the fix is "you cannot change PK:s with Django Admin").

comment:11 in reply to:  10 Changed 10 years ago by Marc Fargas

Replying to Uninen:

We couldn't get Django Admin to cope with this so we wrote our own admin (no big deal). But yes, this is a bug and it should be fixed (and also documented, especially if the fix is "you cannot change PK:s with Django Admin").

Not exactly, you can't do that out of admin also. Django will INSERT a new object instead of updating the old one, as before UPDATE it does SELECT ... WHERE pk = <new pk>. So your custom admin maybe inserting new rows (if you're running trunk).

If you can add an autoincrement field to the database (ignored by the legacy app) your life will be much easier.

comment:12 Changed 10 years ago by sime

Cc: sime Russell Keith-Magee added
milestone: 1.0 alpha
Needs documentation: set
Patch needs improvement: set

This issue has popped up on too many projects now, usually in the context of admin.

Either we allow custom PK value changes or not.

I propose we do, right from the ORM level up; and UPDATE instead of INSERT if an _original_pk was present but is different from the new PK at time of saving. Problem solved, and Django can then claim full support of custom single-field primary keys.

If not, then I guess that's acceptable too; but at the very least it needs to be made clear in the documentation, and enforced in newforms-admin.

comment:13 Changed 10 years ago by Malcolm Tredinnick

milestone: 1.0 alphapost-1.0

Please do not change the milestone (particularly since you've already decided to reopen a ticket that was wontfixed without a django-dev discussion). We've already decided on the 1.0 alpha and beta features and this is an enhancement request.

This can quite comfortably be put off until after 1.0. I'm probably +1 on finding a way for primary keys to be updated, but the current default behaviour should probably remain. Having a paramater to control this won't be the end of the world, since it's not the most common use-case on the planet (but I agree that when you need to do it, you really, really need it).

Marking as post-1.0 for now.

comment:14 Changed 9 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:15 Changed 8 years ago by kevin.howerton@…

I think this issue is actually illustrating a greater flaw in the overall transactional structure the Django ORM implements.

Right now a save transaction works like this:


if exists: UPDATE
else: INSERT

There are several issues with this. In a write heave database this will likely lead to serious performance issues. If an object gets deleted, there is the chance in a race-condition that it will be re-inserted... and obviously you can't change the primary key, because the whole transaction works based off of whether an object exists with the same PK.

How it should work:

obj = MyModel.objects.get(id = 1) # there is a private variable set with the original PK
obj.field = 1 # because there is the original PK set, it does an update

So how it works here is based off of private variables. Going beyond having a PK that is private, you could also have the rest of the fields have non-mutable private variables to generate cleaner, shorter SQL statements ... though I'm not sure that would have any real benefit.

There should only be inserts on new objects...

obj = MyModel()

... and there is absolutely no reason to do a SELECT at the start of the transactional block. In write heavy applications, this will be horrible for performance.

comment:16 Changed 7 years ago by Malcolm Tredinnick

Triage Stage: Design decision neededAccepted

Changing the (manual) primary key intentionally creates a new object. Code relies on that, so we cannot (and are not inclined to) change it. Developer code that wants to update an existing row can write MyModel.objects.filter(, for example. Forms can do similar. In the admin interface (the place this bites people internally), we will make a change that any changeform which has the manual pk changed will do the update trick to avoid creating a new entry. We can do this because the admin changelist page already knows the old pk value (it's part of the URL, for example and, if necessary, we can add a hidden field of the original value).

If there turns out to be other Django-shipped apps that allow editing a manual primary key field, it should behave the same way.

comment:17 Changed 7 years ago by Carl Meyer

Component: Database layer (models, ORM)django.contrib.admin

comment:18 Changed 7 years ago by Carl Meyer

Triage Stage: AcceptedDesign decision needed

So after looking at this a bit on the flight yesterday, the tricky bit turns out to be referential integrity. As in, updating a PK breaks it, if there are any foreign keys pointing to that row. Which results in broken foreign keys on databases that don't enforce referential integrity, and an immediate IntegrityError on databases that do.

For databases that support it, ON UPDATE CASCADE would fix this (which would mean this ticket blocks on #7539). Otherwise, we could manually emulate ON UPDATE CASCADE, which doen't actually sound terribly onerous (one query per referencing table), but is likely to be somewhat fiddly in practice and kinda gives me the willies.

I don't see it as a viable option to provide a "feature" in the admin that breaks referential integrity in your database. So setting this back to DDN.

I do have a patch that works fine, aside from the referential integrity problem, which I'll upload. I added an optional argument to rather than using the .update() trick so as to make it work without adding any additional queries. This choice would be easy to reverse if the additional save arg is unacceptable, but not going to bother with that until the bigger issues are resolved.

Changed 7 years ago by Carl Meyer

Attachment: 2259_r13817.diff added

comment:19 Changed 7 years ago by Łukasz Rekucki

Type: defectBug

comment:20 Changed 7 years ago by Łukasz Rekucki

Severity: normalNormal

comment:21 Changed 6 years ago by Anssi Kääriäinen

Cc: anssi.kaariainen@… added
Easy pickings: unset
Has patch: set
UI/UX: unset

I know this is unlikely to get accepted because of backwards compatibility reasons, but here goes anyways.

The attached patch implements a version of save_base that automatically does the following things when the PK has been changed since last loaded from the DB:

  • If there is already a row with the new pk and force_update is not set, it errors out.
  • If there is already a row with the new pk and force_update is set it updates the row with the new pk and leaves the old pk alone.
  • If there is no row with new or old pk, it inserts a new row.
  • If pk has not been changed, everything works as always.
  • The only way (without resorting to undocumented features) to get a pk update is to use a model that comes from DB. save() counts as come from DB.

The above tries to be intelligent and do the right thing. The current code also tries to be intelligent and do the right thing, but IMHO it fails when the PK has been changed. What it does is not what one would expect. On the other hand the specification of what save() does is very clear. I think the problem here is that we have the single method save() which should always do the right thing. With changing PKs it is not entirely clear what the right thing is.

Other possible ways to solve this ticket is to make a new method or to add additional optional parameters to save. In this case I think .save() without force_update or force_insert should raise an Error if the primary key has changed.

The biggest backwards incompatibility is this (present also in Django test suite):

Lets save some objects:
obj = T() = 123 = 'foob' = 456 = 'faab'
# Or better yet, do that in loop...

Old code would have saved two objects, the new code updates the first saved. So as is this patch has no change to get in. On the other hand, if/when composite primary keys are going to be included in Django, the current save() is going to cause a lot of wondering...

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

Attachment: updatable_pk.diff added

comment:22 Changed 6 years ago by Anssi Kääriäinen

I have been thinking about this a bit... And to me it seems entirely clear that the current situation where changing primary key inserts a new record is not acceptable if/when composite primary keys are included in Django. Composite primary keys mean more use of natural primary keys, and they do have the habit of changing.

Here is an example why the current situation must be changed. Consider a person model with a pk of first_name, last_name and the following code:

p = Person.objects.get(first_name='John', last_name='Smith')
p.first_name = 'Jack'

Who expects that to end up with John and Jack in the DB?

As I see it, there are two choices:

  1. Throw an error when saving an object with PK changed. The error should say that Django does not know what you want in this situation, use kwargs to specify the wanted action.
  2. Make Django intelligent enough to just do the right thing. My patch above tries to do that but I am afraid it is not good enough.

In both cases new kwargs to save are needed to make it possible to specify/override the action on save().

I favor choice 1. Choice 2 will mean complicating the logic of save_base. Also, can Django guess the right action in every case?

My suggestion: deprecate current behavior of save() in 1.4.

comment:23 Changed 6 years ago by Carl Meyer

Summary: PK Change creates new object instead of updatePrimary keys should be readonly by default in admin
Triage Stage: Design decision neededAccepted

In discussion with Alex at the sprint, the need for cascading updates of foreign keys pushes this beyond the bounds of what's reasonable for Django to try to support. Primary keys should not change.

We should, however, update the admin so that primary key fields are included in readonly_fields by default, to avoid the confusing and unexpected creation of new records.

comment:24 Changed 5 years ago by Chris Streeter

Cc: django@… added

I was considering tackling this ticket, and had a question about a possible implementation. Is this as simple as checking for a zero length readonly_fields property and then adding in the attribute name for the pk in the ModelAdmin constructor? The problem with this implementation, as I see it, is it does not let the user override this value by setting readonly_fields to () because we wouldn't be able to tell the difference between the default value and an override.

Another option would be to include 'pk' in the default tuple for readonly_fields, and then translate 'pk' into the attribute name for the primary key in the constructor. Is this what you guys had in mind? This second way seems more robust, but I wanted to check with you guys first.

I did a quick first pass on my branch here (commit I plan to add tests, but want to make sure I'm on the right path first.

comment:25 Changed 5 years ago by Aymeric Augustin

Status: reopenednew

comment:26 Changed 4 years ago by Bouke Haarsma

Cc: Bouke Haarsma added

After diving into the logic responsible for generating the form, I suspect that ModelForm generates the disputed form field. So it appears that the fix should be in ModelForm, which shouldn't generate an editable field for primary keys in the first place. However, then one wouldn't be able to create a new instance from the admin as the non-editable field is mandatory.

Another possibility would be to alter ModelAdmin.get_readonly_fields to return the primary key field for existing instances. Somewhat like below, although that makes the normally hidden primary key fields visible as well.

The first would be the most consistent solution; primary key fields never editable. It would be up to the developers to dynamically add the primary key field back in if needed.

    def get_readonly_fields(self, request, obj=None):
        Hook for specifying custom readonly fields.
        if obj is None:
            return self.readonly_fields
            return self.readonly_fields + (,)
Last edited 4 years ago by Bouke Haarsma (previous) (diff)

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

I've been thinking about this same issue when reviewing the composite fields patch.

I think the right thing to do is:

  1. Make primary keys non-editable when editing objects.
  2. Make primary keys editable when inserting objects.
  3. There should be opt-in for always editable and never editable.

The reason for voting for edit on insert ability is that otherwise the generated add form is unusable. The pk field needs a value, but you can't insert anything into them. That isn't user friendly default.

comment:28 Changed 4 years ago by Bouke Haarsma

Needs documentation: unset
Owner: changed from nobody to Bouke Haarsma
Patch needs improvement: unset
Status: newassigned
Version: master


I have added BaseModelAdmin.auto_pk_readonly_fiel, which controls whether primary keys should be read-only when editing options. This should cover all scenarios:

  1. -- default --
  2. -- default --
  3. Either set auto_pk_readonly_field=False or include the field name in readonly_fields
Last edited 4 years ago by Bouke Haarsma (previous) (diff)

Changed 4 years ago by Bouke Haarsma

Attachment: 2259-poc.diff added

comment:29 Changed 4 years ago by Bouke Haarsma

Cc: Bouke Haarsma removed
Has patch: unset
Owner: Bouke Haarsma deleted
Status: assignednew

The proposed solution works for ModelAdmin forms, however it doesn't work with InlineModelAdmin as get_readonly_fields is passed the parent object, not the object regarding the InlineModelAdmin. Discussing with akaariair the solution could be found by setting editable=models.EDITABLE_ON_INSERT, using a fastened deprecation, as the default for primary key fields.

Version 1, edited 4 years ago by Bouke Haarsma (previous) (next) (diff)

comment:30 Changed 5 days ago by Pathangi Jatinshravan

Owner: set to Pathangi Jatinshravan
Status: newassigned
Note: See TracTickets for help on using tickets.
Back to Top