From 37a0cbf5e4b028bdfc6c8d81918ea78152e45c17 Mon Sep 17 00:00:00 2001 From: Hanny Date: Fri, 4 Oct 2024 12:43:56 -0500 Subject: [PATCH] Fixed #35813 - Made migrations track all changes to unmanaged models. Previously, only create create and delete operations were generated. --- django/db/migrations/autodetector.py | 52 ++------ docs/releases/6.1.txt | 5 + tests/migrations/test_autodetector.py | 171 +++++++++++++++++++++++++- 3 files changed, 187 insertions(+), 41 deletions(-) diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index 0c2e215fcd5a..f100105e4d2a 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -3,7 +3,6 @@ from collections import defaultdict, namedtuple from enum import Enum from graphlib import TopologicalSorter -from itertools import chain from django.conf import settings from django.db import models @@ -152,23 +151,17 @@ def _detect_changes(self, convert_apps=None, graph=None): # proxy models and ignoring unmigrated apps. self.old_model_keys = set() self.old_proxy_keys = set() - self.old_unmanaged_keys = set() self.new_model_keys = set() self.new_proxy_keys = set() - self.new_unmanaged_keys = set() for (app_label, model_name), model_state in self.from_state.models.items(): - if not model_state.options.get("managed", True): - self.old_unmanaged_keys.add((app_label, model_name)) - elif app_label not in self.from_state.real_apps: + if app_label not in self.from_state.real_apps: if model_state.options.get("proxy"): self.old_proxy_keys.add((app_label, model_name)) else: self.old_model_keys.add((app_label, model_name)) for (app_label, model_name), model_state in self.to_state.models.items(): - if not model_state.options.get("managed", True): - self.new_unmanaged_keys.add((app_label, model_name)) - elif app_label not in self.from_state.real_apps or ( + if app_label not in self.from_state.real_apps or ( convert_apps and app_label in convert_apps ): if model_state.options.get("proxy"): @@ -238,7 +231,6 @@ def _prepare_field_lists(self): """ self.kept_model_keys = self.old_model_keys & self.new_model_keys self.kept_proxy_keys = self.old_proxy_keys & self.new_proxy_keys - self.kept_unmanaged_keys = self.old_unmanaged_keys & self.new_unmanaged_keys self.through_users = {} self.old_field_keys = { (app_label, model_name, field_name) @@ -656,12 +648,10 @@ def generate_created_models(self): Defer any model options that refer to collections of fields that might be deferred (e.g. unique_together). """ - old_keys = self.old_model_keys | self.old_unmanaged_keys + old_keys = self.old_model_keys added_models = self.new_model_keys - old_keys - added_unmanaged_models = self.new_unmanaged_keys - old_keys - all_added_models = chain( - sorted(added_models, key=self.swappable_first_key, reverse=True), - sorted(added_unmanaged_models, key=self.swappable_first_key, reverse=True), + all_added_models = sorted( + added_models, key=self.swappable_first_key, reverse=True ) for app_label, model_name in all_added_models: model_state = self.to_state.models[app_label, model_name] @@ -755,11 +745,6 @@ def generate_created_models(self): beginning=True, ) - # Don't add operations which modify the database for unmanaged - # models - if not model_state.options.get("managed", True): - continue - # Generate operations for each related field for name, field in sorted(related_fields.items()): dependencies = self._get_dependencies_for_foreign_key( @@ -913,20 +898,16 @@ def generate_created_proxies(self): def generate_deleted_models(self): """ - Find all deleted models (managed and unmanaged) and make delete - operations for them as well as separate operations to delete any - foreign key or M2M relationships (these are optimized later, if - possible). + Find all deleted models and make delete operations for them as well + as separate operations to delete any foreign key or M2M relationships + (these are optimized later, if possible). Also bring forward removal of any model options that refer to collections of fields - the inverse of generate_created_models(). """ - new_keys = self.new_model_keys | self.new_unmanaged_keys + new_keys = self.new_model_keys deleted_models = self.old_model_keys - new_keys - deleted_unmanaged_models = self.old_unmanaged_keys - new_keys - all_deleted_models = chain( - sorted(deleted_models), sorted(deleted_unmanaged_models) - ) + all_deleted_models = sorted(deleted_models) for app_label, model_name in all_deleted_models: model_state = self.from_state.models[app_label, model_name] # Gather related fields @@ -1840,9 +1821,7 @@ def generate_altered_unique_together(self): self._generate_altered_foo_together(operations.AlterUniqueTogether) def generate_altered_db_table(self): - models_to_check = self.kept_model_keys.union( - self.kept_proxy_keys, self.kept_unmanaged_keys - ) + models_to_check = self.kept_model_keys.union(self.kept_proxy_keys) for app_label, model_name in sorted(models_to_check): old_model_name = self.renamed_models.get( (app_label, model_name), model_name @@ -1861,9 +1840,7 @@ def generate_altered_db_table(self): ) def generate_altered_db_table_comment(self): - models_to_check = self.kept_model_keys.union( - self.kept_proxy_keys, self.kept_unmanaged_keys - ) + models_to_check = self.kept_model_keys.union(self.kept_proxy_keys) for app_label, model_name in sorted(models_to_check): old_model_name = self.renamed_models.get( (app_label, model_name), model_name @@ -1890,11 +1867,6 @@ def generate_altered_options(self): """ models_to_check = self.kept_model_keys.union( self.kept_proxy_keys, - self.kept_unmanaged_keys, - # unmanaged converted to managed - self.old_unmanaged_keys & self.new_model_keys, - # managed converted to unmanaged - self.old_model_keys & self.new_unmanaged_keys, ) for app_label, model_name in sorted(models_to_check): diff --git a/docs/releases/6.1.txt b/docs/releases/6.1.txt index f9fb779ff35e..b5689bd30df2 100644 --- a/docs/releases/6.1.txt +++ b/docs/releases/6.1.txt @@ -274,6 +274,11 @@ Management Commands :data:`~django.db.models.signals.m2m_changed` signals with ``raw=True`` when loading fixtures. +* The :djadmin:`makemigrations` command now tracks all changes to unmanaged + models, including field additions, removals, alterations, constraints and + model renames. After upgrading, you will see new migrations detected for + unmanaged models that have changed. + Models ~~~~~~ diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index 7a66e500cb89..e70c59dbb27b 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -712,6 +712,72 @@ class AutodetectorTests(BaseAutodetectorTests): author_unmanaged = ModelState( "testapp", "AuthorUnmanaged", [], {"managed": False}, ("testapp.author",) ) + author_unmanaged_empty = ModelState( + "testapp", + "Author", + [("id", models.AutoField(primary_key=True))], + {"managed": False}, + ("testapp.author",), + ) + author_unmanaged_name_check_constraint = ModelState( + "testapp", + "Author", + [ + ("id", models.AutoField(primary_key=True)), + ("name", models.CharField(max_length=200, default="Ada Lovelace")), + ], + { + "managed": False, + "constraints": [ + models.CheckConstraint( + condition=models.Q(name__contains="Bob"), name="name_contains_bob" + ) + ], + }, + ("testapp.author",), + ) + author_unmanaged_with_book = ModelState( + "testapp", + "Author", + [ + ("id", models.AutoField(primary_key=True)), + ("name", models.CharField(max_length=200)), + ("book", models.ForeignKey("otherapp.Book", models.CASCADE)), + ], + {"managed": False}, + ("testapp.author",), + ) + author_unmanaged_name_default = ModelState( + "testapp", + "Author", + [ + ("id", models.AutoField(primary_key=True)), + ("name", models.CharField(max_length=200, default="Ada Lovelace")), + ], + {"managed": False}, + ("testapp.author",), + ) + author_unmanaged_name_longer = ModelState( + "testapp", + "Author", + [ + ("id", models.AutoField(primary_key=True)), + ("name", models.CharField(max_length=400)), + ], + {"managed": False}, + ("testapp.author",), + ) + author_unmanaged_renamed_with_book = ModelState( + "testapp", + "Writer", + [ + ("id", models.AutoField(primary_key=True)), + ("name", models.CharField(max_length=200)), + ("book", models.ForeignKey("otherapp.Book", models.CASCADE)), + ], + {"managed": False}, + ("testapp.author",), + ) author_unmanaged_managed = ModelState( "testapp", "AuthorUnmanaged", [], {}, ("testapp.author",) ) @@ -829,6 +895,17 @@ class AutodetectorTests(BaseAutodetectorTests): ], {"db_table": "author_three"}, ) + book_unmanaged = ModelState( + "otherapp", + "Book", + [ + ("id", models.AutoField(primary_key=True)), + ("author", models.ForeignKey("testapp.Author", models.CASCADE)), + ("title", models.CharField(max_length=200)), + ], + {"managed": False}, + ("otherapp.book",), + ) contract = ModelState( "testapp", "Contract", @@ -968,6 +1045,17 @@ class AutodetectorTests(BaseAutodetectorTests): ("title", models.CharField(max_length=200)), ], ) + book_with_author_unmanaged_renamed = ModelState( + "otherapp", + "Book", + [ + ("id", models.AutoField(primary_key=True)), + ("author", models.ForeignKey("testapp.Writer", models.CASCADE)), + ("title", models.CharField(max_length=200)), + ], + {"managed": False}, + ("otherapp.book",), + ) book_with_field_and_author_renamed = ModelState( "otherapp", "Book", @@ -4019,8 +4107,45 @@ def test_proxy_to_mti_with_fk_to_proxy_proxy(self): ) self.assertEqual(fk_field.remote_field.model, "testapp.AAuthorProxyProxy") + def test_unmanaged_add_constraints(self): + """Test change detection of new constraints.""" + changes = self.get_changes( + [self.author_unmanaged_name_default], + [self.author_unmanaged_name_check_constraint], + ) + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["AddConstraint"]) + added_constraint = models.CheckConstraint( + condition=models.Q(name__contains="Bob"), name="name_contains_bob" + ) + self.assertOperationAttributes( + changes, "testapp", 0, 0, model_name="author", constraint=added_constraint + ) + + def test_unmanaged_add_field(self): + """Tests autodetection of new fields.""" + changes = self.get_changes( + [self.author_unmanaged_empty], [self.author_unmanaged_name_default] + ) + # Right number/type of migrations? + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["AddField"]) + self.assertOperationAttributes(changes, "testapp", 0, 0, name="name") + + def test_unmanaged_alter_field(self): + """Tests autodetection of new fields on an unmanaged model.""" + changes = self.get_changes( + [self.author_unmanaged_name_default], [self.author_unmanaged_name_longer] + ) + # Right number/type of migrations? + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["AlterField"]) + self.assertOperationAttributes( + changes, "testapp", 0, 0, name="name", preserve_default=True + ) + def test_unmanaged_create(self): - """The autodetector correctly deals with managed models.""" + """The autodetector correctly deals with unmanaged models.""" # First, we test adding an unmanaged model changes = self.get_changes( [self.author_empty], [self.author_empty, self.author_unmanaged] @@ -4032,6 +4157,50 @@ def test_unmanaged_create(self): changes, "testapp", 0, 0, name="AuthorUnmanaged", options={"managed": False} ) + def test_unmanaged_remove_field(self): + """Tests autodetection of removed fields.""" + changes = self.get_changes( + [self.author_unmanaged_name_default], [self.author_unmanaged_empty] + ) + # Right number/type of migrations? + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["RemoveField"]) + self.assertOperationAttributes( + changes, "testapp", 0, 0, name="name", model_name="author" + ) + + def test_unmanaged_remove_constraints(self): + """Test change detection of new constraints.""" + changes = self.get_changes( + [self.author_unmanaged_name_check_constraint], + [self.author_unmanaged_name_default], + ) + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["RemoveConstraint"]) + self.assertOperationAttributes( + changes, "testapp", 0, 0, model_name="author", name="name_contains_bob" + ) + + def test_unmanaged_rename_model(self): + """Tests autodetection of renamed models.""" + changes = self.get_changes( + [self.author_unmanaged_with_book, self.book_unmanaged], + [ + self.author_unmanaged_renamed_with_book, + self.book_with_author_unmanaged_renamed, + ], + MigrationQuestioner({"ask_rename_model": True}), + ) + # Right number/type of migrations? + self.assertNumberMigrations(changes, "testapp", 1) + self.assertOperationTypes(changes, "testapp", 0, ["RenameModel"]) + self.assertOperationAttributes( + changes, "testapp", 0, 0, old_name="Author", new_name="Writer" + ) + # Now that RenameModel handles related fields too, there should be + # no AlterField for the related field. + self.assertNumberMigrations(changes, "otherapp", 0) + def test_unmanaged_delete(self): changes = self.get_changes( [self.author_empty, self.author_unmanaged], [self.author_empty]