When deleting a model with foreignkey, redundant RemoveField created.

[Aur Saraf]

$ pip freeze
Django==1.7.1
wsgiref==0.1.2

I add a model that inherits from another model, makemigrations, delete that model, makemigrations, and the SQLite backend generates this illegal SQL when trying to delete the last field "model_ptr" from the table:

INSERT INTO "bug_b__new" () SELECT  FROM "bug_b"

Easy repro:

$ mkvirtualenv bug
$ git clone git@github.com:SonOfLilit/django-sqlite-migrations-bug.git
$ cd django-sqlite-migrations-bug
$ python manage.py test

Here is what happens: ​https://dpaste.de/LYqP

---

RELATED ISSUE:

Consider following:

class ModelA(models.Model):
    field1 = models.BooleanField(default=False)

class ModelB(models.Model):
    another_field1 = models.TextField(blank=True)
    related_field = models.ForeignKey(ModelA)

After creating inital migration, I deleted model, so migration will look like:

class Migration(migrations.Migration):

    dependencies = [
        ('blankapp', '0001_initial'),
    ]

    operations = [
        migrations.RemoveField(
            model_name='modelb',
            name='related_field',
        ),
        migrations.DeleteModel(
            name='ModelB',
        ),
    ]

And, when you will try to migrate backwards, LookupError: App 'blankapp' doesn't have a 'modelb' model. will be raised, because migration will try to add related_field to model which does not exists yet.

[Aur Saraf]

My workaround is to comment out the problematic RemoveField in the migration. Does it have any adverse effects? I'm not aware of any...

    operations = [
        # bug in SQLite backend
        # https://code.djangoproject.com/ticket/24061
        # migrations.RemoveField(
            # model_name='student',
            # name='user_ptr',
        # ),
        migrations.RemoveField(
            model_name='assignment',
            name='student',
        ),
        migrations.RemoveField(
            model_name='exercise',
            name='students',
        ),
        migrations.DeleteModel(
            name='Student',
        ),
    ]

[Andriy Sokolovskiy]

Checked on sqlite and django 1.7.2, issue exists - redundant RemoveField has been created.
I'll check on other backends, I think issue exists not only for sqlite.

(Leaving unaccepted to be reviewed by core devs, maybe this ticket related to another one).
I'll try to fix it will be accepted.

[Andriy Sokolovskiy]

Checked on postgres and current django master, issue exists.
I'm checking this as accepted.
After fix this should be backported to 1.7 branch.

[Andriy Sokolovskiy]

Opened question:

truecoldmind> Can anyone explain the exact reason why while deleting model in migration need to create foreign key deletion operations?
<truecoldmind> I'm talking about this https://github.com/django/django/blob/572ad9a92e08797f12e547477efcce7893159cfb/django/db/migrations/autodetector.py#L619
<truecoldmind> if I understand right, when deleting model, where was a foreign key, deleting a whole model will delete foreign key index too. But field operation removal will be created anyway.

After this I found another related bug.
Consider following:

class ModelA(models.Model):
    field1 = models.BooleanField(default=False)

class ModelB(models.Model):
    another_field1 = models.TextField(blank=True)
    related_field = models.ForeignKey(ModelA)

After creating inital migration, I deleted model, so migration will look like:

class Migration(migrations.Migration):

    dependencies = [
        ('blankapp', '0001_initial'),
    ]

    operations = [
        migrations.RemoveField(
            model_name='modelb',
            name='related_field',
        ),
        migrations.DeleteModel(
            name='ModelB',
        ),
    ]

And, when you will try to migrate backwards, LookupError: App 'blankapp' doesn't have a 'modelb' model. will be raised, because migration will try to add related_field to model which does not exists yet.

Should this issue be resolved in this ticket or in another?

[Markus Holtermann]

Regarding your first question: the autodetector needs to remove all foreign key fields in the same app that point towards to model you are removing to keep a working state.

Regarding your second question: I don't see a correlation between those two issues. Please open a new ticket for that problem.

[Andriy Sokolovskiy]

<truecoldmind> MarkusH, and issue that I posted in last comment is related if RemoveField operation on foreignkey (when deleting a whole model) is redundant 
<MarkusH> truecoldmind: a state that is working.
<MarkusH> if you keep a fk field around w/o the reverenced model being available, it is as if you define a fk w/o the model being available.
<MarkusH> truecoldmind: Try a models.py with https://dpaste.de/WAud
<MarkusH> Django will complain and fail to start
<MarkusH> if the FK fields are not removed, you end up with an internal state that is exactly as shown there
<shaib> MarkusH: At a glance, it looks like those operations remove the FKs from the models to be removed, not those pointing at them.
<truecoldmind> MarkusH, lets look on example https://dpaste.de/ThsG. When deleting ModelB, RemoveField operation will be created. For what? We remove only ModelB, this foreignkey is the field of ModelB, and will be deleted if ModelB will be dropped
<shaib> truecoldmind: As I said above, I think this is to make sure that the reverse relationships are removed from the models that remain.
<MarkusH> ok, I'll have a more detailed look at the code
<truecoldmind> shaib, in that case foreignkey is only part of ModelB and does not affects other models
<shaib> no, ModelA has a modelb attribute. Look up reverse relationships.
<shaib> Sorry, modelb_set.
<truecoldmind> shaib, it have it because foreignkey is present in model. How it is related to schema migration ?
<MarkusH> This is indeed weired.
<shaib> migrations also manage the models in memory (so that data migrations can use them)
<MarkusH> I don't see a reason for the RemoveField operations (yet)
<MarkusH> truecoldmind: the reverse relation for a FK field (e.g. FK from Book to Author: book_set) is created by the FK field
<MarkusH> shaib: please correct me if I'm wrong ;)
<MarkusH> So, if I delete the Book model (and thus don't have a FK to Author anymore), there can't be a reverse relation
<shaib> It would be better IMO if removing a model would clean up all reverse relations created by FKs in that model, but I'm not sure this is the case.
<apollo13> that ^
<truecoldmind> MarkusH, yes. and with Book as OneToOne field will be as author.book. But I don't understand how it is related to table drop operation
<MarkusH> shaib: that's what I'd expected as well
<MarkusH> I guess I'm missing something
<apollo13> if you look at the comment of that function, might be that the optimizer is missing something…
<MarkusH> apollo13: I don't see the reason for adding the RemoveField operations in the first place
<apollo13> maybe simplicity for now
<truecoldmind> apollo13, what simplicity? If you look at #24061, this thing is producing at least 2 bugs (first is it the title of ticket, second is in comments)
<ticketbot> https://code.djangoproject.com/ticket/24061
<apollo13> so?
<truecoldmind> apollo13, so what is the exact reason for RemoveField in this concrete example?
<apollo13> truecoldmind: as I said: maybe simplicity
<shaib> I'll elaborate: Perhaps the people who wrote migrations realized they need to clean up reverse relationships and thought it was simpler to do in the autodetector than in the model-deletion operation.

---

Some thoughts how it can be resolved:

<truecoldmind> maybe another operation is needed here, where only  `state_forwards` (without database operations) will be available?
<truecoldmind> for example, we can create `StateOperation(Operation)`, then prohibit database operations here, so we will can do cleanup in that way. Or it is must be hidden from user that will be reading the migration source?

[Andriy Sokolovskiy]

Sorry, I was under the impression there was an optimiser rule for this, but I just checked in the rules list [1] and there's not one, so I was mistaken. The fix is to add a collapse rule there.

Reverse relationship cleanup should already be handled by DeleteModel; if not, that's a _separate_ bug from the RemoveFields not being collapsed in. Both would need the cleanup logic as both are possible.

[1] https://github.com/django/django/blob/master/django/db/migrations/optimizer.py#L29

On Wed, Jan 7, 2015 at 10:58 AM, coldmind <sokandpal@yandex.ru> wrote:

    Thanks for your response!

    I don't understand clearly what you mean in "if you're still seeing them".
    Autodetector should add RemoveField to migrations only in these specific cases, and not always, right?

    And what about situation when it is not a specific case, but we need to do reverse-relationship cleanup?
    It maybe should be placed as I proposed in gist.


    On 01/07/2015 08:42 PM, Andrew Godwin wrote:
>     It's not necessary, and should be optimised away by the optimiser - if you're still seeing them then the optimiser is the place to patch.
>
>     Your proposed patch would remove them entirely and break the operational dependencies system (sometimes you need to remove a foreignkey first, so you can remove another model, so you can remove the original model).
>
>     Andrew
>
>     On Mon, Jan 5, 2015 at 8:26 AM, Andriy Sokolosvkiy <sokandpal@yandex.ru> wrote:
>
>         Hi!
>         I need some advice from you with ticket https://code.djangoproject.com/ticket/24061, because you wrote those part of code.
>          
>         The main question is - what is the exact purpose of RemoveField, when deleting a model?
>         If it is only for state cleanup, I think this code can be moved like this, to prevent bugs, that RemoveField brings:
>         https://gist.github.com/coldmind/75325c5649e010367422
>
>

[Andriy Sokolovskiy]

Seems some commit (maybe ​https://github.com/django/django/commit/fdc2cc948725866212a9bcc97b9b7cf21bb49b90) fixed backwards migration. It is working now, at least it is not crashing (checked two issues from ticket description).
But maybe we can at least add an optimizer rule here to make migration more cleaner.

[Markus Holtermann]

Could you open a new ticket for your thought on "make migration more clearner". I'll mark this as fixed.