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