mirror of
				https://github.com/django/django.git
				synced 2025-10-26 07:06:08 +00:00 
			
		
		
		
	Improve migration optimizer to be able to optimize through other ops
This commit is contained in:
		| @@ -64,6 +64,18 @@ class Operation(object): | |||||||
|         """ |         """ | ||||||
|         return "%s: %s" % (self.__class__.__name__, self._constructor_args) |         return "%s: %s" % (self.__class__.__name__, self._constructor_args) | ||||||
|  |  | ||||||
|  |     def references_model(self, name, app_label=None): | ||||||
|  |         """ | ||||||
|  |         Returns True if there is a chance this operation references the given | ||||||
|  |         model name (as a string), with an optional app label for accuracy. | ||||||
|  |  | ||||||
|  |         Used for optimization. If in doubt, return True; | ||||||
|  |         returning a false positive will merely make the optimizer a little | ||||||
|  |         less efficient, while returning a false negative may result in an | ||||||
|  |         unusable optimized migration. | ||||||
|  |         """ | ||||||
|  |         return True | ||||||
|  |  | ||||||
|     def __repr__(self): |     def __repr__(self): | ||||||
|         return "<%s %s%s>" % ( |         return "<%s %s%s>" % ( | ||||||
|             self.__class__.__name__, |             self.__class__.__name__, | ||||||
|   | |||||||
| @@ -1,4 +1,5 @@ | |||||||
| from .base import Operation | from .base import Operation | ||||||
|  | from django.utils import six | ||||||
| from django.db import models, router | from django.db import models, router | ||||||
| from django.db.models.options import normalize_unique_together | from django.db.models.options import normalize_unique_together | ||||||
| from django.db.migrations.state import ModelState | from django.db.migrations.state import ModelState | ||||||
| @@ -33,6 +34,23 @@ class CreateModel(Operation): | |||||||
|     def describe(self): |     def describe(self): | ||||||
|         return "Create model %s" % (self.name, ) |         return "Create model %s" % (self.name, ) | ||||||
|  |  | ||||||
|  |     def references_model(self, name, app_label=None): | ||||||
|  |         strings_to_check = [self.name] | ||||||
|  |         # Check we didn't inherit from the model | ||||||
|  |         for base in self.bases: | ||||||
|  |             if isinstance(base, six.string_types): | ||||||
|  |                 strings_to_check.append(base.split(".")[-1]) | ||||||
|  |         # Check we have no FKs/M2Ms with it | ||||||
|  |         for fname, field in self.fields: | ||||||
|  |             if field.rel: | ||||||
|  |                 if isinstance(field.rel.to, six.string_types): | ||||||
|  |                     strings_to_check.append(field.rel.to.split(".")[-1]) | ||||||
|  |         # Now go over all the strings and compare them | ||||||
|  |         for string in strings_to_check: | ||||||
|  |             if string.lower() == name.lower(): | ||||||
|  |                 return True | ||||||
|  |         return False | ||||||
|  |  | ||||||
|     def __eq__(self, other): |     def __eq__(self, other): | ||||||
|         return ( |         return ( | ||||||
|             (self.__class__ == other.__class__) and |             (self.__class__ == other.__class__) and | ||||||
| @@ -66,6 +84,9 @@ class DeleteModel(Operation): | |||||||
|         if router.allow_migrate(schema_editor.connection.alias, model): |         if router.allow_migrate(schema_editor.connection.alias, model): | ||||||
|             schema_editor.create_model(model) |             schema_editor.create_model(model) | ||||||
|  |  | ||||||
|  |     def references_model(self, name, app_label=None): | ||||||
|  |         return name.lower() == self.name.lower() | ||||||
|  |  | ||||||
|     def describe(self): |     def describe(self): | ||||||
|         return "Delete model %s" % (self.name, ) |         return "Delete model %s" % (self.name, ) | ||||||
|  |  | ||||||
| @@ -97,6 +118,9 @@ class AlterModelTable(Operation): | |||||||
|     def database_backwards(self, app_label, schema_editor, from_state, to_state): |     def database_backwards(self, app_label, schema_editor, from_state, to_state): | ||||||
|         return self.database_forwards(app_label, schema_editor, from_state, to_state) |         return self.database_forwards(app_label, schema_editor, from_state, to_state) | ||||||
|  |  | ||||||
|  |     def references_model(self, name, app_label=None): | ||||||
|  |         return name.lower() == self.name.lower() | ||||||
|  |  | ||||||
|     def describe(self): |     def describe(self): | ||||||
|         return "Rename table for %s to %s" % (self.name, self.table) |         return "Rename table for %s to %s" % (self.name, self.table) | ||||||
|  |  | ||||||
| @@ -131,6 +155,9 @@ class AlterUniqueTogether(Operation): | |||||||
|     def database_backwards(self, app_label, schema_editor, from_state, to_state): |     def database_backwards(self, app_label, schema_editor, from_state, to_state): | ||||||
|         return self.database_forwards(app_label, schema_editor, from_state, to_state) |         return self.database_forwards(app_label, schema_editor, from_state, to_state) | ||||||
|  |  | ||||||
|  |     def references_model(self, name, app_label=None): | ||||||
|  |         return name.lower() == self.name.lower() | ||||||
|  |  | ||||||
|     def describe(self): |     def describe(self): | ||||||
|         return "Alter unique_together for %s (%s constraints)" % (self.name, len(self.unique_together)) |         return "Alter unique_together for %s (%s constraints)" % (self.name, len(self.unique_together)) | ||||||
|  |  | ||||||
| @@ -164,5 +191,8 @@ class AlterIndexTogether(Operation): | |||||||
|     def database_backwards(self, app_label, schema_editor, from_state, to_state): |     def database_backwards(self, app_label, schema_editor, from_state, to_state): | ||||||
|         return self.database_forwards(app_label, schema_editor, from_state, to_state) |         return self.database_forwards(app_label, schema_editor, from_state, to_state) | ||||||
|  |  | ||||||
|  |     def references_model(self, name, app_label=None): | ||||||
|  |         return name.lower() == self.name.lower() | ||||||
|  |  | ||||||
|     def describe(self): |     def describe(self): | ||||||
|         return "Alter index_together for %s (%s constraints)" % (self.name, len(self.index_together)) |         return "Alter index_together for %s (%s constraints)" % (self.name, len(self.index_together)) | ||||||
|   | |||||||
| @@ -11,7 +11,7 @@ class MigrationOptimizer(object): | |||||||
|     nothing. |     nothing. | ||||||
|     """ |     """ | ||||||
|  |  | ||||||
|     def optimize(self, operations): |     def optimize(self, operations, app_label=None): | ||||||
|         """ |         """ | ||||||
|         Main optimization entry point. Pass in a list of Operation instances, |         Main optimization entry point. Pass in a list of Operation instances, | ||||||
|         get out a new list of Operation instances. |         get out a new list of Operation instances. | ||||||
| @@ -27,17 +27,20 @@ class MigrationOptimizer(object): | |||||||
|         The inner loop is run until the starting list is the same as the result |         The inner loop is run until the starting list is the same as the result | ||||||
|         list, and then the result is returned. This means that operation |         list, and then the result is returned. This means that operation | ||||||
|         optimization must be stable and always return an equal or shorter list. |         optimization must be stable and always return an equal or shorter list. | ||||||
|  |  | ||||||
|  |         The app_label argument is optional, but if you pass it you'll get more | ||||||
|  |         efficient optimization. | ||||||
|         """ |         """ | ||||||
|         # Internal tracking variable for test assertions about # of loops |         # Internal tracking variable for test assertions about # of loops | ||||||
|         self._iterations = 0 |         self._iterations = 0 | ||||||
|         while True: |         while True: | ||||||
|             result = self.optimize_inner(operations) |             result = self.optimize_inner(operations, app_label) | ||||||
|             self._iterations += 1 |             self._iterations += 1 | ||||||
|             if result == operations: |             if result == operations: | ||||||
|                 return result |                 return result | ||||||
|             operations = result |             operations = result | ||||||
|  |  | ||||||
|     def optimize_inner(self, operations): |     def optimize_inner(self, operations, app_label=None): | ||||||
|         """ |         """ | ||||||
|         Inner optimization loop. |         Inner optimization loop. | ||||||
|         """ |         """ | ||||||
| @@ -52,7 +55,7 @@ class MigrationOptimizer(object): | |||||||
|                     new_operations.extend(operations[i+1:i+1+j]) |                     new_operations.extend(operations[i+1:i+1+j]) | ||||||
|                     new_operations.extend(operations[i+j+2:]) |                     new_operations.extend(operations[i+j+2:]) | ||||||
|                     return new_operations |                     return new_operations | ||||||
|                 if not self.can_optimize_through(operation, other): |                 if not self.can_optimize_through(operation, other, app_label): | ||||||
|                     new_operations.append(operation) |                     new_operations.append(operation) | ||||||
|                     break |                     break | ||||||
|             else: |             else: | ||||||
| @@ -95,10 +98,22 @@ class MigrationOptimizer(object): | |||||||
|  |  | ||||||
|     #### THROUGH CHECKS #### |     #### THROUGH CHECKS #### | ||||||
|  |  | ||||||
|     def can_optimize_through(self, operation, other): |     def can_optimize_through(self, operation, other, app_label=None): | ||||||
|         """ |         """ | ||||||
|         Returns True if it's possible to optimize 'operation' with something |         Returns True if it's possible to optimize 'operation' with something | ||||||
|         the other side of 'other'. This is possible if, for example, they |         the other side of 'other'. This is possible if, for example, they | ||||||
|         affect different models. |         affect different models. | ||||||
|         """ |         """ | ||||||
|  |         MODEL_LEVEL_OPERATIONS = ( | ||||||
|  |             migrations.CreateModel, | ||||||
|  |             migrations.DeleteModel, | ||||||
|  |             migrations.AlterModelTable, | ||||||
|  |             migrations.AlterUniqueTogether, | ||||||
|  |             migrations.AlterIndexTogether, | ||||||
|  |         ) | ||||||
|  |         # If it's a model level operation, let it through if there's | ||||||
|  |         # nothing that looks like a reference to us in 'other'. | ||||||
|  |         if isinstance(operation, MODEL_LEVEL_OPERATIONS): | ||||||
|  |             if not other.references_model(operation.name, app_label): | ||||||
|  |                 return True | ||||||
|         return False |         return False | ||||||
|   | |||||||
| @@ -93,3 +93,65 @@ class OptimizerTests(TestCase): | |||||||
|             ], |             ], | ||||||
|             [], |             [], | ||||||
|         ) |         ) | ||||||
|  |  | ||||||
|  |     def test_optimize_through_create(self): | ||||||
|  |         """ | ||||||
|  |         We should be able to optimize away create/delete through a create or delete | ||||||
|  |         of a different model, but only if the create operation does not mention the model | ||||||
|  |         at all. | ||||||
|  |         """ | ||||||
|  |         # These should work | ||||||
|  |         self.assertOptimizesTo( | ||||||
|  |             [ | ||||||
|  |                 migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), | ||||||
|  |                 migrations.CreateModel("Bar", [("size", models.IntegerField())]), | ||||||
|  |                 migrations.DeleteModel("Foo"), | ||||||
|  |             ], | ||||||
|  |             [ | ||||||
|  |                 migrations.CreateModel("Bar", [("size", models.IntegerField())]), | ||||||
|  |             ], | ||||||
|  |         ) | ||||||
|  |         self.assertOptimizesTo( | ||||||
|  |             [ | ||||||
|  |                 migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), | ||||||
|  |                 migrations.CreateModel("Bar", [("size", models.IntegerField())]), | ||||||
|  |                 migrations.DeleteModel("Bar"), | ||||||
|  |                 migrations.DeleteModel("Foo"), | ||||||
|  |             ], | ||||||
|  |             [], | ||||||
|  |         ) | ||||||
|  |         self.assertOptimizesTo( | ||||||
|  |             [ | ||||||
|  |                 migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), | ||||||
|  |                 migrations.CreateModel("Bar", [("size", models.IntegerField())]), | ||||||
|  |                 migrations.DeleteModel("Foo"), | ||||||
|  |                 migrations.DeleteModel("Bar"), | ||||||
|  |             ], | ||||||
|  |             [], | ||||||
|  |         ) | ||||||
|  |         # This should not work - FK should block it | ||||||
|  |         self.assertOptimizesTo( | ||||||
|  |             [ | ||||||
|  |                 migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), | ||||||
|  |                 migrations.CreateModel("Bar", [("other", models.ForeignKey("testapp.Foo"))]), | ||||||
|  |                 migrations.DeleteModel("Foo"), | ||||||
|  |             ], | ||||||
|  |             [ | ||||||
|  |                 migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), | ||||||
|  |                 migrations.CreateModel("Bar", [("other", models.ForeignKey("testapp.Foo"))]), | ||||||
|  |                 migrations.DeleteModel("Foo"), | ||||||
|  |             ], | ||||||
|  |         ) | ||||||
|  |         # This should not work - bases should block it | ||||||
|  |         self.assertOptimizesTo( | ||||||
|  |             [ | ||||||
|  |                 migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), | ||||||
|  |                 migrations.CreateModel("Bar", [("size", models.IntegerField())], bases=("testapp.Foo", )), | ||||||
|  |                 migrations.DeleteModel("Foo"), | ||||||
|  |             ], | ||||||
|  |             [ | ||||||
|  |                 migrations.CreateModel("Foo", [("name", models.CharField(max_length=255))]), | ||||||
|  |                 migrations.CreateModel("Bar", [("size", models.IntegerField())], bases=("testapp.Foo", )), | ||||||
|  |                 migrations.DeleteModel("Foo"), | ||||||
|  |             ], | ||||||
|  |         ) | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user