Fix changing PK via admin

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

[Sarah Boyce]

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.

[Csirmaz Bendegúz]

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.

[Csirmaz Bendegúz]

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.
This should also resolve the race condition issue, i.e. what if an object is created by a separate process with the same PK after this check passes. It would still get overwritten as Sarah mentioned.
An IntegrityError should be raised in that case.
Should I create another ticket for this?

[Csirmaz Bendegúz]

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

[Csirmaz Bendegúz]

I edited the ticket to reflect my findings.

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

[Csirmaz Bendegúz]

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.