From 3f4ff0f7bb17d29e4d18dfb70623e92f8a4d69f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Ml=C3=A1dek?= Date: Sat, 10 May 2025 19:49:13 +0200 Subject: [PATCH 1/5] Aded regresion tests for #29177. 1) On creation of a new managed=False model in migration with FK. 2) On addition of a FK field to already created managed=False model. 3) On removing of a FK field from already created managed=False model. --- .../unmanaged_models/migrations/__init__.py | 0 .../unmanaged_models/models.py | 13 + .../migrations/test_autodetector_unmanaged.py | 227 ++++++++++++++++++ 3 files changed, 240 insertions(+) create mode 100644 tests/migrations/migrations_test_apps/unmanaged_models/migrations/__init__.py create mode 100644 tests/migrations/migrations_test_apps/unmanaged_models/models.py create mode 100644 tests/migrations/test_autodetector_unmanaged.py diff --git a/tests/migrations/migrations_test_apps/unmanaged_models/migrations/__init__.py b/tests/migrations/migrations_test_apps/unmanaged_models/migrations/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/migrations/migrations_test_apps/unmanaged_models/models.py b/tests/migrations/migrations_test_apps/unmanaged_models/models.py new file mode 100644 index 000000000000..6afa6ad888a3 --- /dev/null +++ b/tests/migrations/migrations_test_apps/unmanaged_models/models.py @@ -0,0 +1,13 @@ +from django.db import models + + +class Foo(models.Model): + class Meta: + managed = True + + +class Boo(models.Model): + foo = models.ForeignKey("Foo", on_delete=models.CASCADE) + + class Meta: + managed = False diff --git a/tests/migrations/test_autodetector_unmanaged.py b/tests/migrations/test_autodetector_unmanaged.py new file mode 100644 index 000000000000..feec59ce6289 --- /dev/null +++ b/tests/migrations/test_autodetector_unmanaged.py @@ -0,0 +1,227 @@ +import io +import textwrap +from pathlib import Path + +from migrations.test_base import MigrationTestBase + +from django.core.exceptions import FieldError +from django.core.management import call_command +from django.test import TransactionTestCase +from django.test.utils import override_settings + + +class AutodetectorUnmanagedModelTest(MigrationTestBase, TransactionTestCase): + """Regression test for bug in autodetector with FK to managed=False model.""" + + # Consider also other cases to test, but the + # 'test_create_model_migrate_crashes_on_missing_fk' is the proove of a bug + + @override_settings( + DEFAULT_AUTO_FIELD="django.db.models.AutoField", + INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_models"], + ) + def test_create_model_migrate_crashes_on_missing_fk(self): + out = io.StringIO() + with self.temporary_migration_module("unmanaged_models") as tmp_dir: + call_command("makemigrations", "unmanaged_models", verbosity=0) + call_command("migrate", "unmanaged_models", verbosity=0) + with open(Path(tmp_dir) / "0002_custom.py", "w") as custom_migration_file: + custom_migration_content = textwrap.dedent( + """ + from django.db import migrations + + + def forwards_func(apps, schema_editor): + klass_Boo = apps.get_model("unmanaged_models", "Boo") + klass_Boo.objects.filter(foo=1) + + + class Migration(migrations.Migration): + + dependencies = [ + ('unmanaged_models', '0001_initial'), + ] + + operations = [ + migrations.RunPython( + forwards_func, + reverse_code=migrations.RunPython.noop + ), + ] + """ + ) + custom_migration_file.write(custom_migration_content) + try: + call_command("migrate", "unmanaged_models", stdout=out) + except FieldError: + # this is the bug from #29177, it can not find FK in managed=False model + pass + self.assertIn("OK", out.getvalue()) + + @override_settings( + DEFAULT_AUTO_FIELD="django.db.models.AutoField", + INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_models"], + ) + def test_add_field_operation_is_detected(self): + out = io.StringIO() + initial_migration_content = textwrap.dedent( + """ + from django.db import migrations, models + + + class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ] + + operations = [ + migrations.CreateModel( + name='Foo', + fields=[ + ( + 'id', + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name='ID' + ) + ), + ], + options={ + 'managed': True, + }, + ), + migrations.CreateModel( + name='Boo', + fields=[ + ( + 'id', + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name='ID' + ) + ), + ], + options={ + 'managed': False, + }, + ), + ] + """ + ) + with self.temporary_migration_module("unmanaged_models") as tmp_dir: + with open(Path(tmp_dir) / "0001_initial.py", "w") as initial_migration_file: + initial_migration_file.write(initial_migration_content) + call_command( + "makemigrations", + "unmanaged_models", + dry_run=True, + verbosity=3, + stdout=out, + ) + # explanation: currently as a side effect of a bug in #29177, + # there is: "No changes detected in app...", but + # AddField( ... name='foo' ...) should be there + migration_content = out.getvalue() + add_field_content = migration_content[migration_content.find("AddField") :] + add_field_content = add_field_content[: add_field_content.find(")") + 1] + self.assertIn("AddField", add_field_content) + self.assertIn("name='foo'", add_field_content) + + @override_settings( + DEFAULT_AUTO_FIELD="django.db.models.AutoField", + INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_models"], + ) + def test_remove_field_operation_is_detected(self): + out = io.StringIO() + initial_migration_content = textwrap.dedent( + """ + from django.db import migrations, models + + + class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ] + + operations = [ + migrations.CreateModel( + name='Foo', + fields=[ + ( + 'id', + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name='ID' + ) + ), + ], + options={ + 'managed': True, + }, + ), + migrations.CreateModel( + name='Boo', + fields=[ + ( + 'id', + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name='ID' + ) + ), + ( + 'foo', + models.ForeignKey( + on_delete=models.deletion.CASCADE, + to='unmanaged_models.foo', + ) + ), + ( + 'fk_foo', + models.ForeignKey( + on_delete=models.deletion.CASCADE, + to='unmanaged_models.foo', + ) + ), + ], + options={ + 'managed': False, + }, + ), + ] + """ + ) + with self.temporary_migration_module("unmanaged_models") as tmp_dir: + with open(Path(tmp_dir) / "0001_initial.py", "w") as initial_migration_file: + initial_migration_file.write(initial_migration_content) + call_command( + "makemigrations", + "unmanaged_models", + dry_run=True, + verbosity=3, + stdout=out, + ) + # explanation: currently as a side effect of a bug in #29177, + # there is: "No changes detected in app...", but + # RemoveField( ... name='fk_foo' ...) should be there + migration_content = out.getvalue() + remove_field_content = migration_content[ + migration_content.find("RemoveField") : + ] + remove_field_content = remove_field_content[ + : remove_field_content.find(")") + 1 + ] + self.assertIn("RemoveField", remove_field_content) + self.assertIn("name='fk_foo'", remove_field_content) From 0578a4e88657bfbab9a10b537bdb06f518d54c5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Ml=C3=A1dek?= Date: Mon, 12 May 2025 13:35:10 +0200 Subject: [PATCH 2/5] Fixed #29177. Unmanaged models missed FKs in MigrationAutodetector --- django/db/migrations/autodetector.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/django/db/migrations/autodetector.py b/django/db/migrations/autodetector.py index 13fe8f01a49a..9867340c6515 100644 --- a/django/db/migrations/autodetector.py +++ b/django/db/migrations/autodetector.py @@ -147,7 +147,7 @@ def _detect_changes(self, convert_apps=None, graph=None): self.renamed_fields = {} # Prepare some old/new state and model lists, separating - # proxy models and ignoring unmigrated apps. + # proxy models and unmanaged models(for later usage?). self.old_model_keys = set() self.old_proxy_keys = set() self.old_unmanaged_keys = set() @@ -240,14 +240,14 @@ def _prepare_field_lists(self): self.through_users = {} self.old_field_keys = { (app_label, model_name, field_name) - for app_label, model_name in self.kept_model_keys + for app_label, model_name in self.kept_model_keys | self.kept_unmanaged_keys for field_name in self.from_state.models[ app_label, self.renamed_models.get((app_label, model_name), model_name) ].fields } self.new_field_keys = { (app_label, model_name, field_name) - for app_label, model_name in self.kept_model_keys + for app_label, model_name in self.kept_model_keys | self.kept_unmanaged_keys for field_name in self.to_state.models[app_label, model_name].fields } @@ -741,10 +741,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( From 77e627861e904f2434b56cfc660a5aa6c69e5484 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Ml=C3=A1dek?= Date: Sun, 18 May 2025 15:24:40 +0200 Subject: [PATCH 3/5] It fixed unittests for autodetector. They had good results before this fix and they have got the same good results after it, but now the tests are testing what they should IMHO. --- tests/migrations/test_autodetector.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index ac725d317e01..ba76cb103eaf 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -716,7 +716,10 @@ class AutodetectorTests(BaseAutodetectorTests): "testapp", "AuthorUnmanaged", [], {}, ("testapp.author",) ) author_unmanaged_default_pk = ModelState( - "testapp", "Author", [("id", models.AutoField(primary_key=True))] + "testapp", + "Author", + [("id", models.AutoField(primary_key=True))], + {"managed": False}, ) author_unmanaged_custom_pk = ModelState( "testapp", @@ -724,6 +727,7 @@ class AutodetectorTests(BaseAutodetectorTests): [ ("pk_field", models.IntegerField(primary_key=True)), ], + {"managed": False}, ) author_with_m2m = ModelState( "testapp", From 60fefdbbcb13f9386d85a3588ca3162e53bce1c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Ml=C3=A1dek?= Date: Sun, 18 May 2025 17:38:56 +0200 Subject: [PATCH 4/5] Added unit tests for Autodetector and unmanaged models for #29177. --- tests/migrations/test_autodetector.py | 85 +++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index ba76cb103eaf..6344047776e7 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -1075,6 +1075,34 @@ class AutodetectorTests(BaseAutodetectorTests): "unique_together": {("title", "newfield2")}, }, ) + book_unmanaged_with_fk = ModelState( + "otherapp", + "BookUnmanaged", + [ + ("author", models.ForeignKey("testapp.Author", models.CASCADE)), + ("name", models.CharField(max_length=200)), + ("published", models.PositiveSmallIntegerField()), + ], + {"managed": False}, + ) + book_unmanaged_without_fk = ModelState( + "otherapp", + "BookUnmanaged", + [ + ("name", models.CharField(max_length=200)), + ], + {"managed": False}, + ) + book_unmanaged_rename_fk = ModelState( + "otherapp", + "BookUnmanaged", + [ + ("author_old", models.ForeignKey("testapp.Author", models.CASCADE)), + ("name_old", models.CharField(max_length=200)), + ("published", models.PositiveSmallIntegerField()), + ], + {"managed": False}, + ) attribution = ModelState( "otherapp", "Attribution", @@ -3715,6 +3743,63 @@ def test_unmanaged_custom_pk(self): fk_field = changes["otherapp"][0].operations[0].fields[2][1] self.assertEqual(fk_field.remote_field.model, "testapp.Author") + def test_unmanaged_with_fk(self): + """ + #29177 - The autodetector must correctly deal with FK of unmanaged + on managed models - add FK to unmanaged model + fields(and other fields also) + """ + changes = self.get_changes([], [self.author_empty, self.book_unmanaged_with_fk]) + fk_field = changes["otherapp"][0].operations[0].fields[2][1] + name_field = changes["otherapp"][0].operations[0].fields[0] + published_field = changes["otherapp"][0].operations[0].fields[1] + self.assertEqual(fk_field.remote_field.model, "testapp.Author") + self.assertEqual(name_field[0], "name") + self.assertEqual(name_field[1].get_internal_type(), "CharField") + self.assertEqual(published_field[0], "published") + self.assertEqual( + published_field[1].get_internal_type(), "PositiveSmallIntegerField" + ) + + def test_unmanaged_removing_fk(self): + """ + #29177 - The autodetector must correctly deal with FK of unmanaged + on managed models - remove FK from unmanaged model + fields(and other fields also) + """ + changes = self.get_changes( + [self.author_empty, self.book_unmanaged_with_fk], + [self.author_empty, self.book_unmanaged_without_fk], + ) + remove_fk = changes["otherapp"][0].operations[0] + remove_published = changes["otherapp"][0].operations[1] + self.assertEqual(remove_fk.name, "author") + self.assertTrue("REMOVAL" in remove_fk.category.__str__()) + self.assertEqual(remove_published.name, "published") + self.assertTrue("REMOVAL" in remove_published.category.__str__()) + + def test_unmanaged_rename_fk(self): + """ + #29177 - The autodetector must correctly deal with FK of unmanaged + on managed models - rename FK(and other field also) + """ + changes = self.get_changes( + [self.author_empty, self.book_unmanaged_with_fk], + [self.author_empty, self.book_unmanaged_rename_fk], + ) + remove_author_fk = changes["otherapp"][0].operations[0] + remove_name = changes["otherapp"][0].operations[1] + add_author_old_fk = changes["otherapp"][0].operations[2] + add_name_old = changes["otherapp"][0].operations[3] + self.assertEqual(remove_author_fk.name, "author") + self.assertTrue("REMOVAL" in remove_author_fk.category.__str__()) + self.assertEqual(remove_name.name, "name") + self.assertTrue("REMOVAL" in remove_name.category.__str__()) + self.assertEqual(add_author_old_fk.name, "author_old") + self.assertTrue("ADDITION" in add_author_old_fk.category.__str__()) + self.assertEqual(add_name_old.name, "name_old") + self.assertTrue("ADDITION" in add_name_old.category.__str__()) + @override_settings(AUTH_USER_MODEL="thirdapp.CustomUser") def test_swappable(self): with isolate_lru_cache(apps.get_swappable_settings_name): From 28366c24330347a76208760caba04168ea635042 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Ml=C3=A1dek?= Date: Mon, 19 May 2025 14:02:14 +0200 Subject: [PATCH 5/5] fixup! Aded regresion tests for #29177. --- .../__init__.py | 0 .../migrations/__init__.py | 0 .../models.py | 0 .../migrations/test_autodetector_unmanaged.py | 30 +++++++++---------- 4 files changed, 15 insertions(+), 15 deletions(-) rename tests/migrations/migrations_test_apps/{unmanaged_models/migrations => unmanaged_model_with_fk}/__init__.py (100%) create mode 100644 tests/migrations/migrations_test_apps/unmanaged_model_with_fk/migrations/__init__.py rename tests/migrations/migrations_test_apps/{unmanaged_models => unmanaged_model_with_fk}/models.py (100%) diff --git a/tests/migrations/migrations_test_apps/unmanaged_models/migrations/__init__.py b/tests/migrations/migrations_test_apps/unmanaged_model_with_fk/__init__.py similarity index 100% rename from tests/migrations/migrations_test_apps/unmanaged_models/migrations/__init__.py rename to tests/migrations/migrations_test_apps/unmanaged_model_with_fk/__init__.py diff --git a/tests/migrations/migrations_test_apps/unmanaged_model_with_fk/migrations/__init__.py b/tests/migrations/migrations_test_apps/unmanaged_model_with_fk/migrations/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/tests/migrations/migrations_test_apps/unmanaged_models/models.py b/tests/migrations/migrations_test_apps/unmanaged_model_with_fk/models.py similarity index 100% rename from tests/migrations/migrations_test_apps/unmanaged_models/models.py rename to tests/migrations/migrations_test_apps/unmanaged_model_with_fk/models.py diff --git a/tests/migrations/test_autodetector_unmanaged.py b/tests/migrations/test_autodetector_unmanaged.py index feec59ce6289..b45cd7517e1d 100644 --- a/tests/migrations/test_autodetector_unmanaged.py +++ b/tests/migrations/test_autodetector_unmanaged.py @@ -18,13 +18,13 @@ class AutodetectorUnmanagedModelTest(MigrationTestBase, TransactionTestCase): @override_settings( DEFAULT_AUTO_FIELD="django.db.models.AutoField", - INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_models"], + INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_model_with_fk"], ) def test_create_model_migrate_crashes_on_missing_fk(self): out = io.StringIO() - with self.temporary_migration_module("unmanaged_models") as tmp_dir: - call_command("makemigrations", "unmanaged_models", verbosity=0) - call_command("migrate", "unmanaged_models", verbosity=0) + with self.temporary_migration_module("unmanaged_model_with_fk") as tmp_dir: + call_command("makemigrations", "unmanaged_model_with_fk", verbosity=0) + call_command("migrate", "unmanaged_model_with_fk", verbosity=0) with open(Path(tmp_dir) / "0002_custom.py", "w") as custom_migration_file: custom_migration_content = textwrap.dedent( """ @@ -32,14 +32,14 @@ def test_create_model_migrate_crashes_on_missing_fk(self): def forwards_func(apps, schema_editor): - klass_Boo = apps.get_model("unmanaged_models", "Boo") + klass_Boo = apps.get_model("unmanaged_model_with_fk", "Boo") klass_Boo.objects.filter(foo=1) class Migration(migrations.Migration): dependencies = [ - ('unmanaged_models', '0001_initial'), + ('unmanaged_model_with_fk', '0001_initial'), ] operations = [ @@ -52,7 +52,7 @@ class Migration(migrations.Migration): ) custom_migration_file.write(custom_migration_content) try: - call_command("migrate", "unmanaged_models", stdout=out) + call_command("migrate", "unmanaged_model_with_fk", stdout=out) except FieldError: # this is the bug from #29177, it can not find FK in managed=False model pass @@ -60,7 +60,7 @@ class Migration(migrations.Migration): @override_settings( DEFAULT_AUTO_FIELD="django.db.models.AutoField", - INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_models"], + INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_model_with_fk"], ) def test_add_field_operation_is_detected(self): out = io.StringIO() @@ -114,12 +114,12 @@ class Migration(migrations.Migration): ] """ ) - with self.temporary_migration_module("unmanaged_models") as tmp_dir: + with self.temporary_migration_module("unmanaged_model_with_fk") as tmp_dir: with open(Path(tmp_dir) / "0001_initial.py", "w") as initial_migration_file: initial_migration_file.write(initial_migration_content) call_command( "makemigrations", - "unmanaged_models", + "unmanaged_model_with_fk", dry_run=True, verbosity=3, stdout=out, @@ -135,7 +135,7 @@ class Migration(migrations.Migration): @override_settings( DEFAULT_AUTO_FIELD="django.db.models.AutoField", - INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_models"], + INSTALLED_APPS=["migrations.migrations_test_apps.unmanaged_model_with_fk"], ) def test_remove_field_operation_is_detected(self): out = io.StringIO() @@ -185,14 +185,14 @@ class Migration(migrations.Migration): 'foo', models.ForeignKey( on_delete=models.deletion.CASCADE, - to='unmanaged_models.foo', + to='unmanaged_model_with_fk.foo', ) ), ( 'fk_foo', models.ForeignKey( on_delete=models.deletion.CASCADE, - to='unmanaged_models.foo', + to='unmanaged_model_with_fk.foo', ) ), ], @@ -203,12 +203,12 @@ class Migration(migrations.Migration): ] """ ) - with self.temporary_migration_module("unmanaged_models") as tmp_dir: + with self.temporary_migration_module("unmanaged_model_with_fk") as tmp_dir: with open(Path(tmp_dir) / "0001_initial.py", "w") as initial_migration_file: initial_migration_file.write(initial_migration_content) call_command( "makemigrations", - "unmanaged_models", + "unmanaged_model_with_fk", dry_run=True, verbosity=3, stdout=out,