Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22432 closed Bug (fixed)

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

Reported by: Stephen Burrows Owned by: Raphaël Barrois
Component: Migrations Version: 1.7-beta-1
Severity: Release blocker Keywords:
Cc: stephen.r.burrows@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (3)

22432-failing-test.patch (3.0 KB ) - added by Raphaël Barrois 10 years ago.
Failing test to reproduce the issue.
22432-fix-m2m-repointing.patch (8.8 KB ) - added by Raphaël Barrois 10 years ago.
Patch+tests to fix M2M repointing on SQLite
22432-prevent-m2m-repointing.patch (7.2 KB ) - added by Raphaël Barrois 10 years ago.
Patch+tests to prevent M2M alters on all databases

Download all attachments as: .zip

Change History (19)

comment:1 by Stephen Burrows, 10 years ago

Cc: stephen.r.burrows@… added

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.

comment:2 by Baptiste Mispelon, 10 years ago

Triage Stage: UnreviewedAccepted

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

comment:3 by Tim Graham, 10 years ago

Owner: changed from nobody to Tim Graham
Status: newassigned

comment:4 by Tim Graham, 10 years ago

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.

comment:5 by Andrew Godwin, 10 years ago

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?

comment:6 by Stephen Burrows, 10 years ago

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

comment:7 by Andrew Godwin, 10 years ago

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 had changed to point to bar.

Version 1, edited 10 years ago by Andrew Godwin (previous) (next) (diff)

comment:8 by Tim Graham, 10 years ago

Owner: Tim Graham removed
Status: assignednew

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

comment:9 by Stephen Burrows, 10 years ago

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.

comment:10 by Raphaël Barrois, 10 years ago

Owner: set to Raphaël Barrois
Status: newassigned

comment:11 by Raphaël Barrois, 10 years ago

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

The bug indeed occurs with sqlite, not with postgresql.

by Raphaël Barrois, 10 years ago

Attachment: 22432-failing-test.patch added

Failing test to reproduce the issue.

comment:12 by Raphaël Barrois, 10 years ago

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

by Raphaël Barrois, 10 years ago

Patch+tests to fix M2M repointing on SQLite

by Raphaël Barrois, 10 years ago

Patch+tests to prevent M2M alters on all databases

comment:13 by Raphaël Barrois, 10 years ago

Has patch: set

comment:14 by Andrew Godwin <andrew@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 03900a02d5fd2efd6ae98bdb9974b78146f63040:

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

comment:15 by Andrew Godwin, 10 years ago

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

comment:16 by Tim Graham <timograham@…>, 10 years ago

In fd62bc165ce6eabca077c51bdd6292eef27b84c0:

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

Backport of 03900a02d5 from master

Note: See TracTickets for help on using tickets.
Back to Top