Django

Code

Ticket #2259 (reopened)

Opened 3 years ago

Last modified 4 months ago

PK Change creates new object instead of update

Reported by: ed@edplese.com Assigned to: nobody
Milestone: Component: Database layer (models, ORM)
Version: Keywords:
Cc: sime, russellm Triage Stage: Design decision needed
Has patch: 0 Needs documentation: 1
Needs tests: 0 Patch needs improvement: 1

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

pk_change.diff (0.7 kB) - added by pigletto on 09/14/07 17:53:03.

Change History

06/28/06 15:19:38 changed by ed@edplese.com

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>]

02/21/07 15:03:59 changed by SmileyChris

  • component changed from Admin interface to Database wrapper.
  • needs_tests set to 1.
  • stage changed from Unreviewed to Accepted.

Someone should write some tests up for this.

09/14/07 15:36:36 changed by pigletto

  • owner changed from nobody to pigletto.
  • status changed from new to assigned.

09/14/07 17:53:03 changed by pigletto

  • attachment pk_change.diff added.

09/14/07 17:53:42 changed by pigletto

  • owner changed from pigletto to nobody.
  • status changed from assigned to new.
  • needs_tests deleted.
  • 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.

09/14/07 18:01:00 changed by jacob

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

09/14/07 18:23:19 changed 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.

12/02/07 12:28:29 changed by jacob

  • status changed from new to closed.
  • resolution set to wontfix.

12/10/07 23:40:44 changed by Simon Litchfield <simon@quo.com.au>

  • status changed from closed to reopened.
  • resolution deleted.

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.

06/15/08 13:26:13 changed 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.

(follow-up: ↓ 11 ) 06/19/08 06:59:26 changed 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").

(in reply to: ↑ 10 ) 06/19/08 11:27:26 changed 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.

07/14/08 20:40:15 changed by sime

  • cc set to sime, russellm.
  • needs_better_patch set to 1.
  • needs_docs set to 1.
  • milestone set to 1.0 alpha.

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.

07/15/08 11:42:57 changed 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.

02/25/09 13:51:44 changed by

  • milestone deleted.

Milestone post-1.0 deleted

Add/Change #2259 (PK Change creates new object instead of update)




Change Properties
Action