Opened 3 years ago

Closed 3 years ago

Last modified 3 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: melinath 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 3 years ago.
Failing test to reproduce the issue.
22432-fix-m2m-repointing.patch (8.8 KB) - added by Raphaël Barrois 3 years ago.
Patch+tests to fix M2M repointing on SQLite
22432-prevent-m2m-repointing.patch (7.2 KB) - added by Raphaël Barrois 3 years ago.
Patch+tests to prevent M2M alters on all databases

Download all attachments as: .zip

Change History (19)

comment:1 Changed 3 years ago by melinath

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 Changed 3 years ago by Baptiste Mispelon

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 Changed 3 years ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned

comment:4 Changed 3 years ago by 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.

comment:5 Changed 3 years ago by 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?

comment:6 Changed 3 years ago by melinath

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 Changed 3 years ago by 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.

Last edited 3 years ago by Andrew Godwin (previous) (diff)

comment:8 Changed 3 years ago by Tim Graham

Owner: Tim Graham deleted
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 Changed 3 years ago by melinath

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 Changed 3 years ago by Raphaël Barrois

Owner: set to Raphaël Barrois
Status: newassigned

comment:11 Changed 3 years ago by Raphaël Barrois

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

The bug indeed occurs with sqlite, not with postgresql.

Changed 3 years ago by Raphaël Barrois

Attachment: 22432-failing-test.patch added

Failing test to reproduce the issue.

comment:12 Changed 3 years ago by 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).

Changed 3 years ago by Raphaël Barrois

Patch+tests to fix M2M repointing on SQLite

Changed 3 years ago by Raphaël Barrois

Patch+tests to prevent M2M alters on all databases

comment:13 Changed 3 years ago by Raphaël Barrois

Has patch: set

comment:14 Changed 3 years ago by Andrew Godwin <andrew@…>

Resolution: fixed
Status: assignedclosed

In 03900a02d5fd2efd6ae98bdb9974b78146f63040:

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

comment:15 Changed 3 years ago by Andrew Godwin

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

comment:16 Changed 3 years ago by Tim Graham <timograham@…>

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