Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixed #22204: Bad circular-dep-breaking if more than one per run

  • Loading branch information...
commit e46e15e5a17c1c42d01a343e7044d2d21588493d 1 parent 8ce3ea6
Andrew Godwin andrewgodwin authored
14 django/db/migrations/autodetector.py
View
@@ -94,9 +94,14 @@ def _detect_changes(self):
# Phase 2 is progressively adding pending models, splitting up into two
# migrations if required.
pending_new_fks = []
+ added_phase_2 = set()
while pending_add:
# Is there one we can add that has all dependencies satisfied?
- satisfied = [(m, rf) for m, rf in pending_add.items() if all((al, mn) not in pending_add for f, al, mn in rf)]
+ satisfied = [
+ (m, rf)
+ for m, rf in pending_add.items()
+ if all((al, mn) not in pending_add for f, al, mn in rf)
+ ]
if satisfied:
(app_label, model_name), related_fields = sorted(satisfied)[0]
model_state = self.to_state.models[app_label, model_name]
@@ -107,7 +112,10 @@ def _detect_changes(self):
fields=model_state.fields,
options=model_state.options,
bases=model_state.bases,
- )
+ ),
+ # If it's already been added in phase 2 put it in a new
+ # migration for safety.
+ new=any((al, mn) in added_phase_2 for f, al, mn in related_fields),
)
for field_name, other_app_label, other_model_name in related_fields:
# If it depends on a swappable something, add a dynamic depend'cy
@@ -117,6 +125,7 @@ def _detect_changes(self):
elif app_label != other_app_label:
self.add_dependency(app_label, other_app_label)
del pending_add[app_label, model_name]
+ added_phase_2.add((app_label, model_name))
# Ah well, we'll need to split one. Pick deterministically.
else:
(app_label, model_name), related_fields = sorted(pending_add.items())[0]
@@ -342,6 +351,7 @@ def arrange_for_graph(self, changes, graph):
else:
new_name = "%04i_%s" % (next_number, self.suggest_name(migration.operations))
name_map[(app_label, migration.name)] = (app_label, new_name)
+ next_number += 1
migration.name = new_name
# Now fix dependencies
for app_label, migrations in changes.items():
5 tests/migrations/test_autodetector.py
View
@@ -24,6 +24,7 @@ class AutodetectorTests(TestCase):
author_proxy_notproxy = ModelState("testapp", "AuthorProxy", [], {}, ("testapp.author", ))
publisher = ModelState("testapp", "Publisher", [("id", models.AutoField(primary_key=True)), ("name", models.CharField(max_length=100))])
publisher_with_author = ModelState("testapp", "Publisher", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("testapp.Author")), ("name", models.CharField(max_length=100))])
+ publisher_with_book = ModelState("testapp", "Publisher", [("id", models.AutoField(primary_key=True)), ("author", models.ForeignKey("otherapp.Book")), ("name", models.CharField(max_length=100))])
other_pony = ModelState("otherapp", "Pony", [("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))])
@@ -243,7 +244,7 @@ def test_circular_fk_dependency(self):
"""
# Make state
before = self.make_project_state([])
- after = self.make_project_state([self.author_with_book, self.book])
+ after = self.make_project_state([self.author_with_book, self.book, self.publisher_with_book])
autodetector = MigrationAutodetector(before, after)
changes = autodetector._detect_changes()
# Right number of migrations?
@@ -251,7 +252,7 @@ def test_circular_fk_dependency(self):
self.assertEqual(len(changes['otherapp']), 2)
# Right number of actions?
migration1 = changes['testapp'][0]
- self.assertEqual(len(migration1.operations), 1)
+ self.assertEqual(len(migration1.operations), 2)
migration2 = changes['otherapp'][0]
self.assertEqual(len(migration2.operations), 1)
migration3 = changes['otherapp'][1]

2 comments on commit e46e15e

Ben Davis

This commit seems to have created some odd behavior with initial migrations. Consider these models: https://gist.github.com/anonymous/9541461

When you run the initial migration, it gets split up into multiple migrations when it's not really needed. The behavior appears to be fairly random. If you rename OneSubModel to ModelFive, it gets created in 0001_initial instead of 0004. Some migrations are grouped together, as in the last one, even when they're not really related.

Migrations for 'myapp':
  0001_initial.py:
    - Create model ModelOne
  0002_modelfour.py:
    - Create model ModelFour
  0003_modeltwo.py:
    - Create model ModelTwo
  0004_modelthree_onesubmodel.py:
    - Create model ModelThree
    - Create model OneSubModel

While it's not really a blocking issue, the creation of multiple files when not necessary is undesirable.

Andrew Godwin

Yes, it's designed to err on the side of caution and make multiple files (this is preferable to it not working at all). If I can I'll make it less cautious later on, but what's there for now is good enough to ship with.

Please sign in to comment.
Something went wrong with that request. Please try again.