Opened 5 months ago

Closed 5 months ago

#35551 closed Bug (wontfix)

Fix changing PK via admin

Reported by: Csirmaz Bendegúz Owned by: Csirmaz Bendegúz
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Csirmaz Bendegúz)

I have noticed two bugs when changing the primary key field via admin.

Given:

# models.py
class Dummy(models.Model):
    id = models.IntegerField(primary_key=True)

# admin.py
admin.site.register(Dummy)
  1. Create Dummy (1), Dummy (2).
  2. Change Dummy (2)'s Id field -> 1
  3. Dummy (1) is overwritten with Dummy (2), Dummy(2) stays unchanged.
  4. Change Dummy (1)'s Id field -> 3
  5. A new Dummy (3) is created, Dummy (1) stays unchanged.

I believe both step 3 and step 5 are bugs.

Change History (12)

comment:1 by Csirmaz Bendegúz, 5 months ago

Description: modified (diff)

comment:2 by Sarah Boyce, 5 months ago

Triage Stage: UnreviewedAccepted

Great catch thank you!

I believe step 5 should never happen, since step 3 should fail with a unique check error.

Agreed 👍 if you were to add another field to the model then you slightly override Dummy (1) with whatever is in Dummy (2) - which is not nice.

comment:3 by Csirmaz Bendegúz, 5 months ago

Owner: changed from nobody to Csirmaz Bendegúz
Status: newassigned

comment:4 by Csirmaz Bendegúz, 5 months ago

So actually there are 2 issues:

  1. The unique check is skipped when PK is changed.
  2. The object is duplicated when PK is changed.

I'm looking into this.

comment:5 by Csirmaz Bendegúz, 5 months ago

Description: modified (diff)

comment:6 by Csirmaz Bendegúz, 5 months ago

Description: modified (diff)

comment:7 by Csirmaz Bendegúz, 5 months ago

Has patch: set

comment:8 by Csirmaz Bendegúz, 5 months ago

I have submitted a basic fix for raising a ValidationError on PK conflict.

The other issue I would like to address is the fact that Django creates a duplicate if there's no PK conflict.

Should I create another ticket for this?

Last edited 5 months ago by Csirmaz Bendegúz (previous) (diff)

comment:9 by Csirmaz Bendegúz, 5 months ago

I submitted another PR for the above, please review both! Thanks!

Last edited 5 months ago by Csirmaz Bendegúz (previous) (diff)

comment:10 by Csirmaz Bendegúz, 5 months ago

Description: modified (diff)
Summary: Unique checks skipped when changing primary keyFix changing PK via admin

I edited the ticket to reflect my findings.

comment:11 by Tim Graham, 5 months ago

I'm not sure that trying to allow editing primary keys is the right course of action. There is a long discussion in #2259 (which I didn't read in detail).

in reply to:  11 comment:12 by Csirmaz Bendegúz, 5 months ago

Cc: Csirmaz Bendegúz removed
Resolution: wontfix
Status: assignedclosed

Replying to Tim Graham:

I'm not sure that trying to allow editing primary keys is the right course of action. There is a long discussion in #2259 (which I didn't read in detail).

I see, thanks. I read #2259 - the issue is not with editing the primary keys, but with all the foreign keys that refer to it.
It's because Django doesn't support ON UPDATE in foreign keys (neither on the DB level, nor on the ORM level).
So yes, the simplest solution would be to disable editing primary keys via the admin.

Note: See TracTickets for help on using tickets.
Back to Top