Code

Opened 8 years ago

Last modified 5 months ago

#2259 new Bug

Primary keys should be readonly by default in admin

Reported by: ed@… Owned by:
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: sime, russellm, 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

Description

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.

Attachments (4)

pk_change.diff (685 bytes) - added by pigletto 7 years ago.
2259_r13817.diff (11.4 KB) - added by carljm 4 years ago.
updatable_pk.diff (15.0 KB) - added by akaariai 3 years ago.
2259-poc.diff (2.9 KB) - added by bouke 5 months ago.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 8 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")
>>> 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>]

comment:2 Changed 7 years ago by SmileyChris

  • Component changed from Admin interface to Database wrapper
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

Someone should write some tests up for this.

comment:3 Changed 7 years ago by pigletto

  • Owner changed from nobody to pigletto
  • Status changed from new to assigned

Changed 7 years ago by pigletto

comment:4 Changed 7 years ago by pigletto

  • Needs tests unset
  • Owner changed from pigletto to nobody
  • Status changed from assigned to new
  • Triage Stage changed from Accepted to Design 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")
>>> 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.

comment:5 Changed 7 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 7 years ago by PhiR

-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 6 years ago by jacob

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

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

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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 6 years ago by telenieko

  • Summary changed from Unable to update non-integer primary key in Admin to PK 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 follow-up: Changed 6 years ago by Uninen

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 6 years ago by telenieko

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 6 years ago by sime

  • Cc sime, russellm added
  • milestone set to 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 6 years ago by mtredinnick

  • milestone changed from 1.0 alpha to post-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 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:15 Changed 4 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:

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.

comment:16 Changed 4 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

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.

comment:17 Changed 4 years ago by carljm

  • Component changed from Database layer (models, ORM) to django.contrib.admin

comment:18 Changed 4 years ago by carljm

  • Triage Stage changed from Accepted to Design 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 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.

Changed 4 years ago by carljm

comment:19 Changed 3 years ago by lrekucki

  • Type changed from defect to Bug

comment:20 Changed 3 years ago by lrekucki

  • Severity changed from normal to Normal

comment:21 Changed 3 years ago by akaariai

  • 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()
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...

Changed 3 years ago by akaariai

comment:22 Changed 3 years ago by akaariai

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.

comment:23 Changed 3 years ago by carljm

  • Summary changed from PK Change creates new object instead of update to Primary keys should be readonly by default in admin
  • Triage Stage changed from Design decision needed to Accepted

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 14 months ago by 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 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.

comment:25 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

comment:26 Changed 6 months ago by bouke

  • Cc bouke 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
        else:
            return self.readonly_fields + (self.opts.pk.attname,)
Last edited 6 months ago by bouke (previous) (diff)

comment:27 Changed 6 months ago by akaariai

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 6 months ago by bouke

  • Needs documentation unset
  • Owner changed from nobody to bouke
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Version set to master

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
Last edited 6 months ago by bouke (previous) (diff)

Changed 5 months ago by bouke

comment:29 Changed 5 months ago by bouke

  • Cc bouke removed
  • Has patch unset
  • Owner bouke deleted
  • Status changed from assigned to new

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 5 months ago by bouke (previous) (next) (diff)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from (none) to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


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

 
Note: See TracTickets for help on using tickets.