From 9f632dc702a1b8b4c8044fb34bb3d5f2445dd4ee Mon Sep 17 00:00:00 2001 From: Patryk Zawadzki Date: Fri, 3 Apr 2015 23:36:35 +0200 Subject: [PATCH] [1.8.x] Fixed #24513 -- Made sure a model is only rendered once during reloads This also prevents state modifications from corrupting previous states. Previously, when a model defining a relation was unregistered first, clearing the cache would cause its related models' _meta to be cleared and would result in the old models losing track of their relations. Backport of 0385dad073270c37f8c4a5f13edce43f2a69ba8a from master --- django/db/migrations/state.py | 26 ++++++++--------- docs/releases/1.8.1.txt | 3 ++ tests/migrations/test_state.py | 53 +++++++++++++++++++++++++++++++++- 3 files changed, 67 insertions(+), 15 deletions(-) diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py index 136b63ae8e..9cf63a2cee 100644 --- a/django/db/migrations/state.py +++ b/django/db/migrations/state.py @@ -84,6 +84,9 @@ class ProjectState(object): del self.models[app_label, model_name] if 'apps' in self.__dict__: # hasattr would cache the property self.apps.unregister_model(app_label, model_name) + # Need to do this explicitly since unregister_model() doesn't clear + # the cache automatically (#24513) + self.apps.clear_cache() def reload_model(self, app_label, model_name): if 'apps' in self.__dict__: # hasattr would cache the property @@ -105,29 +108,25 @@ class ProjectState(object): rel_app_label, rel_model_name = _get_app_label_and_model_name(field.rel.to, app_label) related_models.add((rel_app_label, rel_model_name.lower())) + # Include the model itself + related_models.add((app_label, model_name)) + # Unregister all related models for rel_app_label, rel_model_name in related_models: self.apps.unregister_model(rel_app_label, rel_model_name) + # Need to do it once all models are unregistered to avoid corrupting + # existing models' _meta + self.apps.clear_cache() - # Unregister the current model - self.apps.unregister_model(app_label, model_name) - + states_to_be_rendered = [] # Gather all models states of those models that will be rerendered. # This includes: - # 1. The current model - try: - model_state = self.models[app_label, model_name] - except KeyError: - states_to_be_rendered = [] - else: - states_to_be_rendered = [model_state] - - # 2. All related models of unmigrated apps + # 1. All related models of unmigrated apps for model_state in self.apps.real_models: if (model_state.app_label, model_state.name_lower) in related_models: states_to_be_rendered.append(model_state) - # 3. All related models of migrated apps + # 2. All related models of migrated apps for rel_app_label, rel_model_name in related_models: try: model_state = self.models[rel_app_label, rel_model_name] @@ -284,7 +283,6 @@ class StateApps(Apps): del self.app_configs[app_label].models[model_name] except KeyError: pass - self.clear_cache() class ModelState(object): diff --git a/docs/releases/1.8.1.txt b/docs/releases/1.8.1.txt index 65d32f190c..ac986b3fc0 100644 --- a/docs/releases/1.8.1.txt +++ b/docs/releases/1.8.1.txt @@ -25,3 +25,6 @@ Bugfixes * Stripped microseconds from ``datetime`` values when using an older version of the MySQLdb DB API driver as it does not support fractional seconds (:ticket:`24584`). + +* Fixed a migration crash when altering + :class:`~django.db.models.ManyToManyField`\s (:ticket:`24513`). diff --git a/tests/migrations/test_state.py b/tests/migrations/test_state.py index aad50df739..663815d6f9 100644 --- a/tests/migrations/test_state.py +++ b/tests/migrations/test_state.py @@ -1,6 +1,8 @@ from django.apps.registry import Apps from django.db import models -from django.db.migrations.operations import DeleteModel, RemoveField +from django.db.migrations.operations import ( + AlterField, DeleteModel, RemoveField, +) from django.db.migrations.state import ( InvalidBasesError, ModelState, ProjectState, get_related_models_recursive, ) @@ -413,6 +415,55 @@ class StateTests(TestCase): self.assertEqual(len(model_a_old._meta.related_objects), 1) self.assertEqual(len(model_a_new._meta.related_objects), 0) + def test_self_relation(self): + """ + #24513 - Modifying an object pointing to itself would cause it to be + rendered twice and thus breaking its related M2M through objects. + """ + class A(models.Model): + to_a = models.ManyToManyField('something.A', symmetrical=False) + + class Meta: + app_label = "something" + + def get_model_a(state): + return [mod for mod in state.apps.get_models() if mod._meta.model_name == 'a'][0] + + project_state = ProjectState() + project_state.add_model((ModelState.from_model(A))) + self.assertEqual(len(get_model_a(project_state)._meta.related_objects), 1) + old_state = project_state.clone() + + operation = AlterField( + model_name="a", + name="to_a", + field=models.ManyToManyField("something.A", symmetrical=False, blank=True) + ) + # At this point the model would be rendered twice causing its related + # M2M through objects to point to an old copy and thus breaking their + # attribute lookup. + operation.state_forwards("something", project_state) + + model_a_old = get_model_a(old_state) + model_a_new = get_model_a(project_state) + self.assertIsNot(model_a_old, model_a_new) + + # Tests that the old model's _meta is still consistent + field_to_a_old = model_a_old._meta.get_field("to_a") + self.assertEqual(field_to_a_old.m2m_field_name(), "from_a") + self.assertEqual(field_to_a_old.m2m_reverse_field_name(), "to_a") + self.assertIs(field_to_a_old.related_model, model_a_old) + self.assertIs(field_to_a_old.rel.through._meta.get_field('to_a').related_model, model_a_old) + self.assertIs(field_to_a_old.rel.through._meta.get_field('from_a').related_model, model_a_old) + + # Tests that the new model's _meta is still consistent + field_to_a_new = model_a_new._meta.get_field("to_a") + self.assertEqual(field_to_a_new.m2m_field_name(), "from_a") + self.assertEqual(field_to_a_new.m2m_reverse_field_name(), "to_a") + self.assertIs(field_to_a_new.related_model, model_a_new) + self.assertIs(field_to_a_new.rel.through._meta.get_field('to_a').related_model, model_a_new) + self.assertIs(field_to_a_new.rel.through._meta.get_field('from_a').related_model, model_a_new) + def test_equality(self): """ Tests that == and != are implemented correctly.