Permalink
Browse files

Fixed #22275: unique_together broken if ForeignKey split into new file.

Thanks to bak1an for the patch.
  • Loading branch information...
andrewgodwin committed Mar 20, 2014
1 parent 356f064 commit 81f5408c7a16b8c79053950f05fe7a873506ca55
@@ -136,6 +136,7 @@ def _rel_agnostic_fields_def(fields):
# Phase 2 is progressively adding pending models, splitting up into two
# migrations if required.
pending_new_fks = []
+ pending_unique_together = []
added_phase_2 = set()
while pending_add:
# Is there one we can add that has all dependencies satisfied?
@@ -172,6 +173,11 @@ def _rel_agnostic_fields_def(fields):
else:
(app_label, model_name), related_fields = sorted(pending_add.items())[0]
model_state = self.to_state.models[app_label, model_name]
+ # Defer unique together constraints creation, see ticket #22275
+ unique_together_constraints = model_state.options.pop('unique_together', None)
+ if unique_together_constraints:
+ pending_unique_together.append((app_label, model_name,
+ unique_together_constraints))
# Work out the fields that need splitting out
bad_fields = dict((f, (al, mn)) for f, al, mn in related_fields if (al, mn) in pending_add)
# Create the model, without those
@@ -206,6 +212,15 @@ def _rel_agnostic_fields_def(fields):
self.add_swappable_dependency(app_label, swappable_setting)
elif app_label != other_app_label:
self.add_dependency(app_label, other_app_label)
+ # Phase 3.1 - unique together constraints
+ for app_label, model_name, unique_together in pending_unique_together:
+ self.add_to_migration(
+ app_label,
+ operations.AlterUniqueTogether(
+ name=model_name,
+ unique_together=unique_together
+ )
+ )
# Removing models
removed_models = set(old_model_keys) - set(new_model_keys)
for app_label, model_name in removed_models:
@@ -0,0 +1,96 @@
+diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py
+index cbc4604..152fde9 100644
+--- a/django/db/migrations/autodetector.py
++++ b/django/db/migrations/autodetector.py
+@@ -136,6 +136,7 @@ def _rel_agnostic_fields_def(fields):
+ # Phase 2 is progressively adding pending models, splitting up into two
+ # migrations if required.
+ pending_new_fks = []
++ pending_unique_together = []
+ added_phase_2 = set()
+ while pending_add:
+ # Is there one we can add that has all dependencies satisfied?
+@@ -172,6 +173,11 @@ def _rel_agnostic_fields_def(fields):
+ else:
+ (app_label, model_name), related_fields = sorted(pending_add.items())[0]
+ model_state = self.to_state.models[app_label, model_name]
++ # Defer unique together constraints creation, see ticket #22275
++ unique_together_constraints = model_state.options.pop('unique_together', None)
++ if unique_together_constraints:
++ pending_unique_together.append((app_label, model_name,
++ unique_together_constraints))
+ # Work out the fields that need splitting out
+ bad_fields = dict((f, (al, mn)) for f, al, mn in related_fields if (al, mn) in pending_add)
+ # Create the model, without those
+@@ -206,6 +212,15 @@ def _rel_agnostic_fields_def(fields):
+ self.add_swappable_dependency(app_label, swappable_setting)
+ elif app_label != other_app_label:
+ self.add_dependency(app_label, other_app_label)
++ # Phase 3.1 - unique together constraints
++ for app_label, model_name, unique_together in pending_unique_together:
++ self.add_to_migration(
++ app_label,
++ operations.AlterUniqueTogether(
++ name=model_name,
++ unique_together=unique_together
++ )
++ )
+ # Removing models
+ removed_models = set(old_model_keys) - set(new_model_keys)
+ for app_label, model_name in removed_models:
+diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py
+index c3ada83..40e540d 100644
+--- a/tests/migrations/test_autodetector.py
++++ b/tests/migrations/test_autodetector.py
+@@ -37,6 +37,8 @@ class AutodetectorTests(TestCase):
+ 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")]})
+ 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))])
++ rabbit = ModelState("eggs", "Rabbit", [("id", models.AutoField(primary_key=True)), ("knight", models.ForeignKey("eggs.Knight")), ("parent", models.ForeignKey("eggs.Rabbit"))], {"unique_together": [("parent", "knight")]})
+
+ def make_project_state(self, model_states):
+ "Shortcut to make ProjectStates from lists of predefined models"
+@@ -370,6 +372,42 @@ def test_same_app_circular_fk_dependency(self):
+ self.assertEqual(migration1.dependencies, [])
+ self.assertEqual(migration2.dependencies, [("testapp", "auto_1")])
+
++ def test_same_app_circular_fk_dependency_and_unique_together(self):
++ """
++ Tests that a migration with circular FK dependency does not try to
++ create unique together constraint before creating all required fields first.
++ See ticket #22275.
++ """
++ # Make state
++ before = self.make_project_state([])
++ after = self.make_project_state([self.knight, self.rabbit])
++ autodetector = MigrationAutodetector(before, after)
++ changes = autodetector._detect_changes()
++ # Right number of migrations?
++ self.assertEqual(len(changes['eggs']), 2)
++ # Right number of actions?
++ migration1 = changes['eggs'][0]
++ self.assertEqual(len(migration1.operations), 2)
++ migration2 = changes['eggs'][1]
++ self.assertEqual(len(migration2.operations), 2)
++ # Right actions?
++ action = migration1.operations[0]
++ self.assertEqual(action.__class__.__name__, "CreateModel")
++ action = migration1.operations[1]
++ self.assertEqual(action.__class__.__name__, "CreateModel")
++ # CreateModel action for Rabbit should not have unique_together now
++ self.assertEqual(action.name, "Rabbit")
++ self.assertFalse("unique_together" in action.options)
++ action = migration2.operations[0]
++ self.assertEqual(action.__class__.__name__, "AddField")
++ self.assertEqual(action.name, "parent")
++ action = migration2.operations[1]
++ self.assertEqual(action.__class__.__name__, "AlterUniqueTogether")
++ self.assertEqual(action.name, "rabbit")
++ # Right dependencies?
++ self.assertEqual(migration1.dependencies, [])
++ self.assertEqual(migration2.dependencies, [("eggs", "auto_1")])
++
+ def test_unique_together(self):
+ "Tests unique_together detection"
+ # Make state

This comment has been minimized.

Show comment Hide comment
@charettes

charettes Mar 20, 2014

Member

Leftover file?

@charettes

charettes Mar 20, 2014

Member

Leftover file?

This comment has been minimized.

Show comment Hide comment
@bak1an

bak1an Mar 20, 2014

Contributor

Looks like.

BTW, it is possible to apply patch without saving it to local file with:
curl $DIFF_URL | git apply - just apply diff, don't commit
or
curl $PATCH_URL | git am - make commits from patches
or
git fetch $REPO_URL $BRANCH - after this, branch with fix will be available as FETCH_HEAD, so you can cherry-pick commits from it, squash, etc. Cool thing about this that you don't need to add remote to fetch just one branch with interesting commits.

@bak1an

bak1an Mar 20, 2014

Contributor

Looks like.

BTW, it is possible to apply patch without saving it to local file with:
curl $DIFF_URL | git apply - just apply diff, don't commit
or
curl $PATCH_URL | git am - make commits from patches
or
git fetch $REPO_URL $BRANCH - after this, branch with fix will be available as FETCH_HEAD, so you can cherry-pick commits from it, squash, etc. Cool thing about this that you don't need to add remote to fetch just one branch with interesting commits.

This comment has been minimized.

Show comment Hide comment
@bmispelon

bmispelon Mar 20, 2014

Member

I removed it in 3ecc25b.

@bmispelon

bmispelon Mar 20, 2014

Member

I removed it in 3ecc25b.

@@ -37,6 +37,8 @@ class AutodetectorTests(TestCase):
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")]})
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))])
+ rabbit = ModelState("eggs", "Rabbit", [("id", models.AutoField(primary_key=True)), ("knight", models.ForeignKey("eggs.Knight")), ("parent", models.ForeignKey("eggs.Rabbit"))], {"unique_together": [("parent", "knight")]})
def make_project_state(self, model_states):
"Shortcut to make ProjectStates from lists of predefined models"
@@ -370,6 +372,42 @@ def test_same_app_circular_fk_dependency(self):
self.assertEqual(migration1.dependencies, [])
self.assertEqual(migration2.dependencies, [("testapp", "auto_1")])
+ def test_same_app_circular_fk_dependency_and_unique_together(self):
+ """
+ Tests that a migration with circular FK dependency does not try to
+ create unique together constraint before creating all required fields first.
+ See ticket #22275.
+ """
+ # Make state
+ before = self.make_project_state([])
+ after = self.make_project_state([self.knight, self.rabbit])
+ autodetector = MigrationAutodetector(before, after)
+ changes = autodetector._detect_changes()
+ # Right number of migrations?
+ self.assertEqual(len(changes['eggs']), 2)
+ # Right number of actions?
+ migration1 = changes['eggs'][0]
+ self.assertEqual(len(migration1.operations), 2)
+ migration2 = changes['eggs'][1]
+ self.assertEqual(len(migration2.operations), 2)
+ # Right actions?
+ action = migration1.operations[0]
+ self.assertEqual(action.__class__.__name__, "CreateModel")
+ action = migration1.operations[1]
+ self.assertEqual(action.__class__.__name__, "CreateModel")
+ # CreateModel action for Rabbit should not have unique_together now
+ self.assertEqual(action.name, "Rabbit")
+ self.assertFalse("unique_together" in action.options)
+ action = migration2.operations[0]
+ self.assertEqual(action.__class__.__name__, "AddField")
+ self.assertEqual(action.name, "parent")
+ action = migration2.operations[1]
+ self.assertEqual(action.__class__.__name__, "AlterUniqueTogether")
+ self.assertEqual(action.name, "rabbit")
+ # Right dependencies?
+ self.assertEqual(migration1.dependencies, [])
+ self.assertEqual(migration2.dependencies, [("eggs", "auto_1")])
+
def test_unique_together(self):
"Tests unique_together detection"
# Make state

0 comments on commit 81f5408

Please sign in to comment.