Primary keys should be readonly by default in admin

[ed@…]

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):
        return self.name

    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/base.py where it only adds the non-pk columns after SET.

[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")
>>> g.save()
>>> Group.objects.all()
[<Group: Test>]
>>> g = Group.objects.get(name="Test")
>>> g.name = "TestX"
>>> g.save()
>>> Group.objects.all()
[<Group: Test>, <Group: TestX>]

[Chris Beaven]

Someone should write some tests up for this.

[Maciej Wiśniowski]

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")
>>> g.save()
>>> print g.id
1

then if we change id like:

g.id = 32 

then django.db.models.base.Model.save() 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.

[Jacob]

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

[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.

[Simon Litchfield <simon@…>]

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.

[Marc Fargas]

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.

[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").

[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.

[sime]

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.

[Malcolm Tredinnick]

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.

[(none)]

Milestone post-1.0 deleted

[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:

obj.save

transaction>
SELECT obj

if exists: UPDATE
else: INSERT
<transaction

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
obj.save() # 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.

[Malcolm Tredinnick]

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(pk=self.pk).update(pk=new_pk), 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.

[Carl Meyer]

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 Model.save() 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.

[Anssi Kääriäinen]

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()
t.pk = 123
t.name = 'foob'
t.save()
t.pk = 456
t.name = 'faab'
t.save()
# 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...

[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'
p.save()

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.

[Carl Meyer]

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.

[Chris Streeter]

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 ​https://github.com/streeter/django/tree/ticket-2259 (commit ​https://github.com/streeter/django/commit/a9c039). I plan to add tests, but want to make sure I'm on the right path first.

[Bouke Haarsma]

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
        else:
            return self.readonly_fields + (self.opts.pk.attname,)

[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.

[Bouke Haarsma]

PR: ​https://github.com/django/django/pull/1879

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

[Bouke Haarsma]

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. However I'm not familiar with the inner workings of modelform factories and cannot own this ticket.

[Chason Chaffin]

Does not seem to be an issue in modern versions of Django. Attempts to add the primary_key field to an Admin page will result in an error.

[Mariusz Felisiak]

This is still a valid issue.

[Michael J. Nicholson]

Doesn't appear to be the same issue as it was to start with, if it still exists at all. I replicated the original test in comment #1 using the Group table, and it works fine now:

>>> from django.contrib.auth.models import Group
>>> Group.objects.all()
<QuerySet []>
>>> g = Group(name="test")
>>> g.save()
>>> Group.objects.all()
<QuerySet [<Group: test>]>
>>> g = Group.objects.get(name="test")
>>> g.name="testx"
>>> g.save()
>>> Group.objects.all()
<QuerySet [<Group: testx>]>

I then changed this back to "test" via the admin interface, which also resulted in a single record called "test" without any issues. As far as I can tell, the original issue no longer exists and this should be closed.

[Mariusz Felisiak]

This issue is still valid please check Anssi's proposition.

[jedie]

Note: https://code.djangoproject.com/ticket/14071 Row duplicated when modifying PK is related: I add comment https://code.djangoproject.com/ticket/14071#comment:2 to this about the problem to change the primary key in admin.

[Mariusz Felisiak]

#32728 was a duplicate. Primary keys should not be allowed in the ModelAdmin.list_editable.

[siddhartha-star-dev]

Replying to Mariusz Felisiak:

This issue is still valid please check Anssi's proposition.

Those comments primarily relate to admin right ? As far as I can see right now, the following code snipped results in the same problem:
obj = TestModel.objects.get(email="jondoe@gmail.com")
obj.email = "jon@gmail.com"
obj.save()
exit()

This creates a new object rather than updating the old one, should this not be fixed right here ? That would mean that if and when we chose to make the fields editable, we will just have to change how the form behaves in the admin...

So should we just try and fix the case with model.save() first and then move towards admin specifics ?

[Mariusz Felisiak]

obj.save()
exit()
This creates a new object rather than updating the old one, should this not be fixed right here?

This is a ​documented and expected behavior, which shouldn't be changed.

[siddhartha-star-dev]

I came up with two ways to change primary key

1. we store old pk in hidden field in the change form and we check for the hidden field's value and the current pk value, if they have changed, we create a new object with the new pk and delete the old one.
2. there will be a popup which will open a new form where two fields will be old pk and new pk, specifically to change the primary key if needed, the idea is to have an "edit" option besides the pk field in the current form which will then open a popup to change the pk of the given object

[siddhartha-star-dev]

Replying to Mariusz Felisiak:
Please look at my comment and reply if possible

[Mariusz Felisiak]

Replying to siddhartha-star-dev:

Replying to Mariusz Felisiak:
Please look at my comment and reply if possible

Have you seen Anssi's proposition? We don't want to allow for editing primary keys in admin. They should not be allowed in the ModelAdmin.list_editable and become read-only when editing.

[siddhartha-star-dev]

Replying to Mariusz Felisiak:

Have you seen Anssi's proposition? We don't want to allow for editing primary keys in admin. They should not be allowed in the ModelAdmin.list_editable and become read-only when editing.

I have seen Anssi's proposition , and I only put out those ways in reference to the 3 point in the proposition (That editable should be an opt-in), if we are ok in always having them non-editable, we should be good, I will start working on that for now, we can visit the 3 point later....

[Jacob Walls]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In dcebc5da:

Refs #2259 -- Disallowed primary keys in ModelAdmin.list_editable.

Refs #32728.

[Prashant Pandey]

Replying to Bouke Haarsmay:
Hi, I was following your solution and it gives same errors that you are getting(regression error for inline).
I just need help in that area.
Is there a way to access the context of the form inside the get_readonly_fields, means when we do:

1. Create new object where primary key field will be in editing state.
2. If we hit the change behaviour it should be readonly.

we can add a condition in the get_readonly_fields function where it will send pk as a readonly if user wants to modify the object.