innov
/
django1
1
0
Fork 0
Code Issues Pull Requests 1 Projects Releases Wiki Activity

Fixed #22397 -- Issues removing M2M field with explicit through model 

Changed the migration autodetector to remove models last so that FK
and M2M fields will not be left as dangling references. Added a check
in the migration state renderer to error out in the presence of
dangling references instead of leaving them as strings. Fixed a bug
in the sqlite backend to handle the deletion of M2M fields with
"through" models properly (i.e., do nothing successfully).

Thanks to melinath for report, loic for tests and andrewgodwin and
charettes for assistance with architecture.
This commit is contained in:
Andrew Gorcester 2014-04-08 12:30:25 +07:00 committed by Simon Charette
parent 26d118c3fe
commit 956bd64424
7 changed files with 171 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
django/db/backends/sqlite3/schema.py
View File

@ -121,10 +121,14 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
Removes a field from a model. Usually involves deleting a column,
but for M2Ms may involve deleting a table.
"""
# Special-case implicit M2M tables
if isinstance(field, ManyToManyField) and field.rel.through._meta.auto_created:
return self.delete_model(field.rel.through)
# M2M fields are a special case
if isinstance(field, ManyToManyField):
# For implicit M2M tables, delete the auto-created table
if field.rel.through._meta.auto_created:
self.delete_model(field.rel.through)
# For explicit "through" M2M fields, do nothing
# For everything else, remake.
else:
self._remake_table(model, delete_fields=[field])
def alter_field(self, model, old_field, new_field, strict=False):

20
django/db/migrations/autodetector.py
View File

@ -223,16 +223,6 @@ class MigrationAutodetector(object):
unique_together=unique_together
)
)
# Removing models
removed_models = set(old_model_keys) - set(new_model_keys)
for app_label, model_name in removed_models:
model_state = self.from_state.models[app_label, model_name]
self.add_to_migration(
app_label,
operations.DeleteModel(
model_state.name,
)
)
# Changes within models
kept_models = set(old_model_keys).intersection(new_model_keys)
old_fields = set()
@ -348,6 +338,16 @@ class MigrationAutodetector(object):
)
for app_label, operation in unique_together_operations:
self.add_to_migration(app_label, operation)
# Removing models
removed_models = set(old_model_keys) - set(new_model_keys)
for app_label, model_name in removed_models:
model_state = self.from_state.models[app_label, model_name]
self.add_to_migration(
app_label,
operations.DeleteModel(
model_state.name,
)
)
# Alright, now add internal dependencies
for app_label, migrations in self.migrations.items():
for m1, m2 in zip(migrations, migrations[1:]):

2
django/db/migrations/migration.py
View File

@ -127,7 +127,7 @@ class Migration(object):
to_run.reverse()
for operation, to_state, from_state in to_run:
operation.database_backwards(self.app_label, schema_editor, from_state, to_state)
return project_state
def swappable_dependency(value):
"""

10
django/db/migrations/state.py
View File

@ -51,6 +51,16 @@ class ProjectState(object):
if len(new_unrendered_models) == len(unrendered_models):
raise InvalidBasesError("Cannot resolve bases for %r" % new_unrendered_models)
unrendered_models = new_unrendered_models
# make sure apps has no dangling references
if self.apps._pending_lookups:
# Raise an error with a best-effort helpful message
# (only for the first issue). Error message should look like:
# "ValueError: Lookup failed for model referenced by
# field migrations.Book.author: migrations.Author"
dangling_lookup = list(self.apps._pending_lookups.items())[0]
raise ValueError("Lookup failed for model referenced by field {field}: {model[0]}.{model[1]}".format(
field=dangling_lookup[1][0][1],
model=dangling_lookup[0]))
return self.apps
@classmethod

59
tests/migrations/test_autodetector.py
View File

@ -33,11 +33,15 @@ class AutodetectorTests(TestCase):
other_stable = ModelState("otherapp", "Stable", [("id", models.AutoField(primary_key=True))])
third_thing = ModelState("thirdapp", "Thing", [("id", models.AutoField(primary_key=True))])
book = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))])
book_with_no_author = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("title", models.CharField(max_length=200))])
book_with_author_renamed = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Writer")), ("title", models.CharField(max_length=200))])
book_with_field_and_author_renamed = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("writer", models.ForeignKey("testapp.Writer")), ("title", models.CharField(max_length=200))])
book_with_multiple_authors = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("authors", models.ManyToManyField("testapp.Author")), ("title", models.CharField(max_length=200))])
book_with_multiple_authors_through_attribution = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("authors", models.ManyToManyField("testapp.Author", through="otherapp.Attribution")), ("title", models.CharField(max_length=200))])
book_unique = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("author", "title")]})
book_unique_2 = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("title", "author")]})
book_unique_3 = ModelState("otherapp", "Book", [("id", models.AutoField(primary_key=True)), ("newfield", models.IntegerField()), ("author", models.ForeignKey("testapp.Author")), ("title", models.CharField(max_length=200))], {"unique_together": [("title", "newfield")]})
attribution = ModelState("otherapp", "Attribution", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("book", models.ForeignKey("otherapp.Book"))])
edition = ModelState("thirdapp", "Edition", [("id", models.AutoField(primary_key=True)), ("book", models.ForeignKey("otherapp.Book"))])
custom_user = ModelState("thirdapp", "CustomUser", [("id", models.AutoField(primary_key=True)), ("username", models.CharField(max_length=255))])
knight = ModelState("eggs", "Knight", [("id", models.AutoField(primary_key=True))])
@ -552,7 +556,32 @@ class AutodetectorTests(TestCase):
"""
# Make state
before = self.make_project_state([self.author_with_publisher_string])
after = self.make_project_state([self.author_with_publisher])
after = self.make_project_state([self.author_with_publisher, self.publisher])
autodetector = MigrationAutodetector(before, after)
changes = autodetector._detect_changes()
# Right number of migrations?
self.assertEqual(len(changes['testapp']), 1)
# Right number of actions?
migration = changes['testapp'][0]
self.assertEqual(len(migration.operations), 3)
# Right actions?
action = migration.operations[0]
self.assertEqual(action.__class__.__name__, "CreateModel")
self.assertEqual(action.name, "Publisher")
action = migration.operations[1]
self.assertEqual(action.__class__.__name__, "AddField")
self.assertEqual(action.name, "publisher")
action = migration.operations[2]
self.assertEqual(action.__class__.__name__, "RemoveField")
self.assertEqual(action.name, "publisher_name")
def test_foreign_key_removed_before_target_model(self):
"""
Removing an FK and the model it targets in the same change must remove
the FK field before the model to maintain consistency.
"""
before = self.make_project_state([self.author_with_publisher, self.publisher])
after = self.make_project_state([self.author_name]) # removes both the model and FK
autodetector = MigrationAutodetector(before, after)
changes = autodetector._detect_changes()
# Right number of migrations?
@ -560,10 +589,32 @@ class AutodetectorTests(TestCase):
# Right number of actions?
migration = changes['testapp'][0]
self.assertEqual(len(migration.operations), 2)
# Right actions?
# Right actions in right order?
action = migration.operations[0]
self.assertEqual(action.__class__.__name__, "AddField")
self.assertEqual(action.__class__.__name__, "RemoveField")
self.assertEqual(action.name, "publisher")
action = migration.operations[1]
self.assertEqual(action.__class__.__name__, "DeleteModel")
self.assertEqual(action.name, "Publisher")
def test_many_to_many_removed_before_through_model(self):
"""
Removing a ManyToManyField and the "through" model in the same change must remove
the field before the model to maintain consistency.
"""
before = self.make_project_state([self.book_with_multiple_authors_through_attribution, self.author_name, self.attribution])
after = self.make_project_state([self.book_with_no_author, self.author_name]) # removes both the through model and ManyToMany
autodetector = MigrationAutodetector(before, after)
changes = autodetector._detect_changes()
# Right number of migrations?
self.assertEqual(len(changes['otherapp']), 1)
# Right number of actions?
migration = changes['otherapp'][0]
self.assertEqual(len(migration.operations), 2)
# Right actions in right order?
action = migration.operations[0]
self.assertEqual(action.__class__.__name__, "RemoveField")
self.assertEqual(action.name, "publisher_name")
self.assertEqual(action.name, "authors")
action = migration.operations[1]
self.assertEqual(action.__class__.__name__, "DeleteModel")
self.assertEqual(action.name, "Attribution")

56
tests/migrations/test_operations.py
View File

@ -1,9 +1,12 @@
import unittest
from django.db import connection, models, migrations, router
from django.db import connection, migrations, models, router
from django.db.migrations.migration import Migration
from django.db.migrations.state import ProjectState
from django.db.models.fields import NOT_PROVIDED
from django.db.transaction import atomic
from django.db.utils import IntegrityError
from django.db.migrations.state import ProjectState
from .test_base import MigrationTestBase
@ -15,15 +18,16 @@ class OperationTests(MigrationTestBase):
"""
def apply_operations(self, app_label, project_state, operations):
new_state = project_state.clone()
for operation in operations:
operation.state_forwards(app_label, new_state)
# Set up the database
migration = Migration('name', app_label)
migration.operations = operations
with connection.schema_editor() as editor:
for operation in operations:
operation.database_forwards(app_label, editor, project_state, new_state)
return new_state
return migration.apply(project_state, editor)
def unapply_operations(self, app_label, project_state, operations):
migration = Migration('name', app_label)
migration.operations = operations
with connection.schema_editor() as editor:
return migration.unapply(project_state, editor)
def set_up_test_model(self, app_label, second_model=False, related_model=False, mti_model=False):
"""
@ -396,6 +400,38 @@ class OperationTests(MigrationTestBase):
Pony = new_apps.get_model("test_alflmm", "Pony")
self.assertTrue(Pony._meta.get_field('stables').blank)
def test_remove_field_m2m(self):
project_state = self.set_up_test_model("test_rmflmm", second_model=True)
project_state = self.apply_operations("test_rmflmm", project_state, operations=[
migrations.AddField("Pony", "stables", models.ManyToManyField("Stable", related_name="ponies"))
])
self.assertTableExists("test_rmflmm_pony_stables")
operations = [migrations.RemoveField("Pony", "stables")]
self.apply_operations("test_rmflmm", project_state, operations=operations)
self.assertTableNotExists("test_rmflmm_pony_stables")
# And test reversal
self.unapply_operations("test_rmflmm", project_state, operations=operations)
self.assertTableExists("test_rmflmm_pony_stables")
def test_remove_field_m2m_with_through(self):
project_state = self.set_up_test_model("test_rmflmmwt", second_model=True)
self.assertTableNotExists("test_rmflmmwt_ponystables")
project_state = self.apply_operations("test_rmflmmwt", project_state, operations=[
migrations.CreateModel("PonyStables", fields=[
("pony", models.ForeignKey('test_rmflmmwt.Pony')),
("stable", models.ForeignKey('test_rmflmmwt.Stable')),
]),
migrations.AddField("Pony", "stables", models.ManyToManyField("Stable", related_name="ponies", through='test_rmflmmwt.PonyStables'))
])
self.assertTableExists("test_rmflmmwt_ponystables")
operations = [migrations.RemoveField("Pony", "stables")]
self.apply_operations("test_rmflmmwt", project_state, operations=operations)
def test_remove_field(self):
"""
Tests the RemoveField operation.

41
tests/migrations/test_state.py
View File

@ -291,3 +291,44 @@ class StateTests(TestCase):
))
self.assertNotEqual(project_state, other_state)
self.assertEqual(project_state == other_state, False)
def test_dangling_references_throw_error(self):
new_apps = Apps()
class Author(models.Model):
name = models.TextField()
class Meta:
app_label = "migrations"
apps = new_apps
class Book(models.Model):
author = models.ForeignKey(Author)
class Meta:
app_label = "migrations"
apps = new_apps
class Magazine(models.Model):
authors = models.ManyToManyField(Author)
class Meta:
app_label = "migrations"
apps = new_apps
# Make a valid ProjectState and render it
project_state = ProjectState()
project_state.add_model_state(ModelState.from_model(Author))
project_state.add_model_state(ModelState.from_model(Book))
project_state.add_model_state(ModelState.from_model(Magazine))
rendered_state = project_state.render()
self.assertEqual(len(rendered_state.get_models()), 3)
# now make an invalid one with a ForeignKey
project_state = ProjectState()
project_state.add_model_state(ModelState.from_model(Book))
with self.assertRaises(ValueError):
rendered_state = project_state.render()
# and another with ManyToManyField
project_state = ProjectState()
project_state.add_model_state(ModelState.from_model(Magazine))
with self.assertRaises(ValueError):
rendered_state = project_state.render()