Inconsistency when rolling back a migration that adds a primary key

[Tim Martin]

Hi all,

I've been looking at bug #28542, in which a migration can be played

forwards but not reversed. I think I can see what's happening, but I'm

not 100% sure what the right resolution is, so I wanted to get some

input.

The migration in question looks something like this:

    # Start out with field "id" as the primary key

    migrations.AddField(

       model_name="student",

       name="new_id",

       field=models.IntegerField(null=True, primary_key=False))

    migrations.RemoveField('student', 'id')

    migrations.AlterField(

       model_name="student",

       name="new_id",

       field=models.IntegerField(primary_key=True))

It fails to be reversed because when the primary key constraint is

removed from the field (i.e. reversing the addition) this doesn't

trigger removal of the primary key constraint in the database. When

the id field is re-added, it conflicts.

This seems to be deliberate: in BaseDatabaseSchemaEditor._alter_field

a comment says:

    # Note that we don't detect unsetting of a PK, as we assume another field

    # will always come along and replace it.

I think the assumption is that when the primary key flag is removed

from one field it will be added to another existing field. If this

happens then the existing primary key will be removed. But another

alternative is that the PK flag is removed from one field and another

whole field is added, and in this case it won't remove the existing

primary key (it will give a duplicate primary key error).

This error is masked in a lot of cases because if a unique constraint

is removed from a field at the same time as removing PK then all

constraints that have unique=True are removed (schema.py:528). This

also removes the primary key constraint. This means in a typical case

(demoting a key from primary key to unique key) then the constraint

will be deleted anyway.

So, the question is: what is the right behaviour? My first thought is

that playing a migration operation forwards then backwards should

leave the state back where it started. However, in order for this to

happen AlterField has to operate on fields other than the main field

it is declared to operate on: if playing it forward removes a PK

constraint from an unrelated field, then playing it backward must

reinstate it.

On the other hand, the logic about implicitly removing PKs that are

superceded currently lies in BaseDatabaseSchemaEditor, so adding

related logic to AlterField seems wrong. We could add this logic to

BaseDatabaseSchemaEditor, but IIUC it doesn't know about the states

it's rolling between, so it has no way to tell when a PK should be

added, only when one should be removed.

Perhaps the right thing to do is to move the logic for implicitly

deleting PK constraints out of BaseDatabaseSchemaEditor. The

AlterField operation should know when it is replacing a primary key,

and it should consistently remove it when going forwards and re-add it

when going backwards.

Another possibility is to do as the comment suggests, and just wait

for the old primary key to be re-added. This would mean that the

RemoveField operation would have to remove any existing primary key

field when adding a primary key field. I don't like this option much.

Does anyone have any suggestions on the right choice here?

Thanks,

Tim

[Tim Martin]

I've implemented a fix for this that works, in which I deleted the

code that automatically drops the old primary key when a new one is

added. I think this is consistent with the way it's intended to be

used: if I autogenerate a migration for changing the primary key, then

the old one is explicitly deleted with a RemoveField / AlterField

operation before the new primary key is added.

However, this means that my patch will fail with any manual migrations

that rely on the implicit removal of primary keys. I think these

migrations are broken anyway, since they won't do the right thing when

reversed. Is it reasonable to just mention this in the release notes?

The other issue is that this change regresses one unit test:

SchemaTests.test_alter_int_pk_to_int_unique. This test explicitly

creates a new primary key before removing the old one. Again, I think

this test is wrong and I should just switch the order of the

operations. If I'm missing something, please let me know.

Tim