Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[1.7.x] Fixed #22397 -- Issues removing M2M field with explicit throu…
…gh 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.

Backport of 956bd64 from master
  • Loading branch information
andrewsg authored and charettes committed Apr 18, 2014
1 parent b06e45b commit bc5d568
Show file tree
Hide file tree
Showing 7 changed files with 169 additions and 27 deletions.
12 changes: 8 additions & 4 deletions django/db/backends/sqlite3/schema.py
Expand Up @@ -121,11 +121,15 @@ def remove_field(self, model, field):
Removes a field from a model. Usually involves deleting a column, Removes a field from a model. Usually involves deleting a column,
but for M2Ms may involve deleting a table. but for M2Ms may involve deleting a table.
""" """
# Special-case implicit M2M tables # M2M fields are a special case
if isinstance(field, ManyToManyField) and field.rel.through._meta.auto_created: if isinstance(field, ManyToManyField):
return self.delete_model(field.rel.through) # 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. # For everything else, remake.
self._remake_table(model, delete_fields=[field]) else:
self._remake_table(model, delete_fields=[field])


def alter_field(self, model, old_field, new_field, strict=False): def alter_field(self, model, old_field, new_field, strict=False):
""" """
Expand Down
20 changes: 10 additions & 10 deletions django/db/migrations/autodetector.py
Expand Up @@ -223,16 +223,6 @@ def _rel_agnostic_fields_def(fields):
unique_together=unique_together 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 # Changes within models
kept_models = set(old_model_keys).intersection(new_model_keys) kept_models = set(old_model_keys).intersection(new_model_keys)
old_fields = set() old_fields = set()
Expand Down Expand Up @@ -348,6 +338,16 @@ def _rel_agnostic_fields_def(fields):
) )
for app_label, operation in unique_together_operations: for app_label, operation in unique_together_operations:
self.add_to_migration(app_label, operation) 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 # Alright, now add internal dependencies
for app_label, migrations in self.migrations.items(): for app_label, migrations in self.migrations.items():
for m1, m2 in zip(migrations, migrations[1:]): for m1, m2 in zip(migrations, migrations[1:]):
Expand Down
2 changes: 1 addition & 1 deletion django/db/migrations/migration.py
Expand Up @@ -127,7 +127,7 @@ def unapply(self, project_state, schema_editor, collect_sql=False):
to_run.reverse() to_run.reverse()
for operation, to_state, from_state in to_run: for operation, to_state, from_state in to_run:
operation.database_backwards(self.app_label, schema_editor, from_state, to_state) operation.database_backwards(self.app_label, schema_editor, from_state, to_state)

return project_state


def swappable_dependency(value): def swappable_dependency(value):
""" """
Expand Down
10 changes: 10 additions & 0 deletions django/db/migrations/state.py
Expand Up @@ -51,6 +51,16 @@ def render(self):
if len(new_unrendered_models) == len(unrendered_models): if len(new_unrendered_models) == len(unrendered_models):
raise InvalidBasesError("Cannot resolve bases for %r" % new_unrendered_models) raise InvalidBasesError("Cannot resolve bases for %r" % new_unrendered_models)
unrendered_models = 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 return self.apps


@classmethod @classmethod
Expand Down
57 changes: 54 additions & 3 deletions tests/migrations/test_autodetector.py
Expand Up @@ -33,11 +33,15 @@ class AutodetectorTests(TestCase):
other_stable = ModelState("otherapp", "Stable", [("id", models.AutoField(primary_key=True))]) other_stable = ModelState("otherapp", "Stable", [("id", models.AutoField(primary_key=True))])
third_thing = ModelState("thirdapp", "Thing", [("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 = 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_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_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 = 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_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")]}) 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"))]) 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))]) 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))]) knight = ModelState("eggs", "Knight", [("id", models.AutoField(primary_key=True))])
Expand Down Expand Up @@ -552,18 +556,65 @@ def test_replace_string_with_foreignkey(self):
""" """
# Make state # Make state
before = self.make_project_state([self.author_with_publisher_string]) 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) autodetector = MigrationAutodetector(before, after)
changes = autodetector._detect_changes() changes = autodetector._detect_changes()
# Right number of migrations? # Right number of migrations?
self.assertEqual(len(changes['testapp']), 1) self.assertEqual(len(changes['testapp']), 1)
# Right number of actions? # Right number of actions?
migration = changes['testapp'][0] migration = changes['testapp'][0]
self.assertEqual(len(migration.operations), 2) self.assertEqual(len(migration.operations), 3)
# Right actions? # Right actions?
action = migration.operations[0] 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.__class__.__name__, "AddField")
self.assertEqual(action.name, "publisher") self.assertEqual(action.name, "publisher")
action = migration.operations[1] action = migration.operations[2]
self.assertEqual(action.__class__.__name__, "RemoveField") self.assertEqual(action.__class__.__name__, "RemoveField")
self.assertEqual(action.name, "publisher_name") 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?
self.assertEqual(len(changes['testapp']), 1)
# Right number of actions?
migration = changes['testapp'][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")
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, "authors")
action = migration.operations[1]
self.assertEqual(action.__class__.__name__, "DeleteModel")
self.assertEqual(action.name, "Attribution")
54 changes: 45 additions & 9 deletions tests/migrations/test_operations.py
@@ -1,9 +1,12 @@
import unittest 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.models.fields import NOT_PROVIDED
from django.db.transaction import atomic from django.db.transaction import atomic
from django.db.utils import IntegrityError from django.db.utils import IntegrityError
from django.db.migrations.state import ProjectState
from .test_base import MigrationTestBase from .test_base import MigrationTestBase




Expand All @@ -15,15 +18,16 @@ class OperationTests(MigrationTestBase):
""" """


def apply_operations(self, app_label, project_state, operations): def apply_operations(self, app_label, project_state, operations):
new_state = project_state.clone() migration = Migration('name', app_label)
for operation in operations: migration.operations = operations
operation.state_forwards(app_label, new_state) with connection.schema_editor() as editor:
return migration.apply(project_state, editor)


# Set up the database def unapply_operations(self, app_label, project_state, operations):
migration = Migration('name', app_label)
migration.operations = operations
with connection.schema_editor() as editor: with connection.schema_editor() as editor:
for operation in operations: return migration.unapply(project_state, editor)
operation.database_forwards(app_label, editor, project_state, new_state)
return new_state


def set_up_test_model(self, app_label, second_model=False, related_model=False, mti_model=False): def set_up_test_model(self, app_label, second_model=False, related_model=False, mti_model=False):
""" """
Expand Down Expand Up @@ -396,6 +400,38 @@ def test_alter_field_m2m(self):
Pony = new_apps.get_model("test_alflmm", "Pony") Pony = new_apps.get_model("test_alflmm", "Pony")
self.assertTrue(Pony._meta.get_field('stables').blank) 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): def test_remove_field(self):
""" """
Tests the RemoveField operation. Tests the RemoveField operation.
Expand Down
41 changes: 41 additions & 0 deletions tests/migrations/test_state.py
Expand Up @@ -291,3 +291,44 @@ def test_equality(self):
)) ))
self.assertNotEqual(project_state, other_state) self.assertNotEqual(project_state, other_state)
self.assertEqual(project_state == other_state, False) 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()

0 comments on commit bc5d568

Please sign in to comment.