Opened 12 months ago

Closed 11 months ago

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

Download all attachments as: .zip

Change History (19)

comment:1 Changed 12 months ago by melinath

  • Cc stephen.r.burrows@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 12 months ago by bmispelon

  • Triage Stage changed from Unreviewed to Accepted

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 12 months ago by timo

  • Owner changed from nobody to timo
  • Status changed from new to assigned

comment:4 Changed 12 months ago by timo

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 12 months ago by andrewgodwin

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 12 months 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 12 months ago by andrewgodwin

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 12 months ago by andrewgodwin (previous) (diff)

comment:8 Changed 11 months ago by timo

  • Owner timo deleted
  • Status changed from assigned to new

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 11 months 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 11 months ago by xelnor

  • Owner set to xelnor
  • Status changed from new to assigned

comment:11 Changed 11 months ago by xelnor

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

The bug indeed occurs with sqlite, not with postgresql.

Changed 11 months ago by xelnor

Failing test to reproduce the issue.

comment:12 Changed 11 months ago by xelnor

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 11 months ago by xelnor

Patch+tests to fix M2M repointing on SQLite

Changed 11 months ago by xelnor

Patch+tests to prevent M2M alters on all databases

comment:13 Changed 11 months ago by xelnor

  • Has patch set

comment:14 Changed 11 months ago by Andrew Godwin <andrew@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 03900a02d5fd2efd6ae98bdb9974b78146f63040:

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

comment:15 Changed 11 months ago by andrewgodwin

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

comment:16 Changed 11 months 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