Switching model on an M2M field results in a broken db (no change to the automatic through table)

[Stephen Burrows]

I had a field that looked like this:

class EventPerson(models.Model)
    person_prefer = models.ManyToManyField('self')

But that was wrong (copy-paste mistake) so I switched it to this:

class EventPerson(models.Model):
    person_prefer = models.ManyToManyField(Person)

Then I ran makemigrations and migrate. And now I'm getting database errors because the auto-generated through table still looks like this:

CREATE TABLE "brambling_eventperson_person_prefer"
("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
 "from_eventperson_id" integer NOT NULL REFERENCES "brambling_eventperson" ("id"),
 "to_eventperson_id" integer NOT NULL REFERENCES "brambling_eventperson" ("id"),
UNIQUE ("from_eventperson_id", "to_eventperson_id"));

... and I have no easy way to recover.

[Stephen Burrows]

This seems pretty severe, with potential for data loss/unexpected crashes, so I marked it as a release blocker. Apologies if I shouldn't have.

[Baptiste Mispelon]

Hi,

I can indeed reproduce this with the following models:

from django.db import models

class Foo(models.Model):
    pass


class Bar(models.Model):
    pass


class Baz(models.Model):
    m2m = models.ManyToManyField(Foo)

After changing to models.ManyToManyField(Bar) and running makemigrations and migrate, I can see (using ./manage.py dbshell) that the schema for the m2m table is wrong (the m2m table still references Foo instead of Bar).

I agree that this is a release blocker.

Thanks

[Tim Graham]

​The check here prevents any action from being taken if the many to many table name hasn't changed which is the case if you simply change the model. I'm not sure exactly how we should handle this as I would guess existing data should probably be dropped. I checked how south handled this, but it looks like schemamigration --auto doesn't detect a change if the ManyToManyField model is changed.

[Andrew Godwin]

This is a tricky one to fix. I'm tempted to just block any changes on M2M fields (for this and #22476) in 1.7 and ship it so that makemigrations dies saying "you can't alter M2Ms, if you want to change it delete and add a new one!", as there's no way we can write generic M2M alteration code in time, and it's not a regression, as you couldn't do any alteration at all before.

Thoughts?

[Stephen Burrows]

Blocking this one with an error makes sense, because it's a weird edge case (and probably shouldn't be done, honestly) but #22476 is related to changes to fields that *don't* affect the database (blank, help_text, etc) but still generate migrations) so it should probably still get fixed (IMO).

[Andrew Godwin]

I cannot replicate this locally - the foreign key is correctly repointed - and we even have a test for this, it turns out, in schema.tests.SchemaTests.test_m2m_repoint.

Could the reporter or other contributors please try and replicate and tell me their results? I followed the bmispelon's reproduction notes - made the three models, changed the field to models.ManyToManyField(Bar), ran makemigrations and migrate, and voila, the foreign key constraint on the M2M table had changed to point to bar, along with its column name.

[Tim Graham]

I think this only affects SQLite -- the check I linked in comment 4 that causes the issue is only in the SQLite schema editor. In addition, test_m2m_repoint is largely skipped on SQlite since supports_foreign_keys = False (although newer version of it do support FKs; see #14204).

[Stephen Burrows]

Yes, I can still replicate this with the latest 1.7, using the method described by bmispelon. After creating the models and then creating and running the migrations, the database looks like this:

sqlite> .schema brambling_baz_m2m
CREATE TABLE "brambling_baz_m2m" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "baz_id" integer NOT NULL REFERENCES "brambling_baz" ("id"), "foo_id" integer NOT NULL REFERENCES "brambling_foo" ("id"), UNIQUE ("baz_id", "foo_id"));
CREATE INDEX brambling_baz_m2m_60ef55ec ON "brambling_baz_m2m" ("baz_id");
CREATE INDEX brambling_baz_m2m_a8d1fe7d ON "brambling_baz_m2m" ("foo_id");

The second FK *should* be bar_id and reference brambling_bar ("id").

Behavior that would make sense to me:

1. Delete the through table (perhaps with a warning when creating the migration), then create the new table.
2. Raise an error to prevent this type of change for now until a better solution is found.

[Raphaël Barrois]

I've added a failing test, I'm attaching the patch.

The bug indeed occurs with sqlite, not with postgresql.

[Raphaël Barrois]

Failing test to reproduce the issue.

[Raphaël Barrois]

After careful examination, the code indicates that altering the to= of a ManyToManyField was meant to be forbidden in all cases:

# backends/schema.py:490
raise ValueError("Cannot alter field %s into %s - they are not compatible types (you cannot alter to or from M2M fields, or add or remove through= on M2M fields)")

There are two problems with this approach:

• Changing the to= field actually works on all backends but SQLite (provided the new ForeignKey constraint applies properly, i.e the primary key of all instances of the old to= field are found in the pk column of the new to= field)
• The standard alter_field code can't be called on SQLite (remaking the table is required), and the SQLite-specific version doesn't execute anything when changing DB-related attributes of a ManyToManyField.

We have two options here:

• Go for the intended direction of forbidding altering a ManyToManyField through migrations
• Implement the ManyToManyField-altering logic for SQLite

The second approach sounds more useful to me, but I've provided both patches.

Note: Changing the to= of a ManyToManyField is actually simply changing the target of a ForeignKey, which is supported by migrations as long as the FK constraint is valid when using the new target column (all source pks are present in the target column).

[Raphaël Barrois]

Patch+tests to fix M2M repointing on SQLite

[Raphaël Barrois]

Patch+tests to prevent M2M alters on all databases

[Andrew Godwin <andrew@…>]

In 03900a02d5fd2efd6ae98bdb9974b78146f63040:

Fixed #22432: SQLite M2M repointing now works. Thanks to xelnor.

[Andrew Godwin]

xelnor: You are amazing. It's like a choose-your-own-adventure of patches.

[Tim Graham <timograham@…>]

In fd62bc165ce6eabca077c51bdd6292eef27b84c0:

[1.7.x] Fixed #22432: SQLite M2M repointing now works. Thanks to xelnor.

Backport of 03900a02d5 from master