From 43289707809c814a70f0db38ca4f82f35f43dbfd Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Wed, 15 Dec 2021 19:36:08 -0500 Subject: [PATCH] Fixed #33366 -- Fixed case handling with swappable setting detection in migrations autodetector. The migration framework uniquely identifies models by case insensitive labels composed of their app label and model names and so does the app registry in most of its methods (e.g. AppConfig.get_model) but it wasn't the case for get_swappable_settings_name() until this change. This likely slipped under the radar for so long and only regressed in b9df2b74b98b4d63933e8061d3cfc1f6f39eb747 because prior to the changes related to the usage of model states instead of rendered models in the auto-detector the exact value settings value was never going through a case folding hoop. Thanks Andrew Chen Wang for the report and Keryn Knight for the investigation. --- django/apps/registry.py | 5 +++-- docs/releases/4.0.1.txt | 4 ++++ tests/field_deconstruction/tests.py | 20 ++++++++++++++++++++ tests/migrations/test_autodetector.py | 16 ++++++++++++++++ 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/django/apps/registry.py b/django/apps/registry.py index 62650ca4bc..268e1a6af7 100644 --- a/django/apps/registry.py +++ b/django/apps/registry.py @@ -286,13 +286,14 @@ class Apps: change after Django has loaded the settings, there is no reason to get the respective settings attribute over and over again. """ + to_string = to_string.lower() for model in self.get_models(include_swapped=True): swapped = model._meta.swapped # Is this model swapped out for the model given by to_string? - if swapped and swapped == to_string: + if swapped and swapped.lower() == to_string: return model._meta.swappable # Is this model swappable and the one given by to_string? - if model._meta.swappable and model._meta.label == to_string: + if model._meta.swappable and model._meta.label_lower == to_string: return model._meta.swappable return None diff --git a/docs/releases/4.0.1.txt b/docs/releases/4.0.1.txt index 50c84bfe17..5fea7aa8df 100644 --- a/docs/releases/4.0.1.txt +++ b/docs/releases/4.0.1.txt @@ -19,3 +19,7 @@ Bugfixes * Relaxed the check added in Django 4.0 to reallow use of a duck-typed ``HttpRequest`` in ``django.views.decorators.cache.cache_control()`` and ``never_cache()`` decorators (:ticket:`33350`). + +* Fixed a regression in Django 4.0 that caused creating bogus migrations for + models that reference swappable models such as ``auth.User`` + (:ticket:`33366`). diff --git a/tests/field_deconstruction/tests.py b/tests/field_deconstruction/tests.py index b2fb097100..70846f23d3 100644 --- a/tests/field_deconstruction/tests.py +++ b/tests/field_deconstruction/tests.py @@ -211,6 +211,13 @@ class FieldDeconstructionTests(SimpleTestCase): self.assertEqual(args, []) self.assertEqual(kwargs, {"to": "auth.user", "on_delete": models.CASCADE}) self.assertEqual(kwargs['to'].setting_name, "AUTH_USER_MODEL") + # Swap detection for lowercase swappable model. + field = models.ForeignKey('auth.user', models.CASCADE) + name, path, args, kwargs = field.deconstruct() + self.assertEqual(path, 'django.db.models.ForeignKey') + self.assertEqual(args, []) + self.assertEqual(kwargs, {'to': 'auth.user', 'on_delete': models.CASCADE}) + self.assertEqual(kwargs['to'].setting_name, 'AUTH_USER_MODEL') # Test nonexistent (for now) model field = models.ForeignKey("something.Else", models.CASCADE) name, path, args, kwargs = field.deconstruct() @@ -273,6 +280,19 @@ class FieldDeconstructionTests(SimpleTestCase): self.assertEqual(kwargs, {"to": "auth.permission", "on_delete": models.CASCADE}) self.assertEqual(kwargs['to'].setting_name, "AUTH_USER_MODEL") + # Model names are case-insensitive. + with isolate_lru_cache(apps.get_swappable_settings_name): + # It doesn't matter that we swapped out user for permission; + # there's no validation. We just want to check the setting stuff + # works. + field = models.ForeignKey('auth.permission', models.CASCADE) + name, path, args, kwargs = field.deconstruct() + + self.assertEqual(path, 'django.db.models.ForeignKey') + self.assertEqual(args, []) + self.assertEqual(kwargs, {'to': 'auth.permission', 'on_delete': models.CASCADE}) + self.assertEqual(kwargs['to'].setting_name, 'AUTH_USER_MODEL') + def test_one_to_one(self): # Test basic pointing from django.contrib.auth.models import Permission diff --git a/tests/migrations/test_autodetector.py b/tests/migrations/test_autodetector.py index 18055f242b..d25b14cedb 100644 --- a/tests/migrations/test_autodetector.py +++ b/tests/migrations/test_autodetector.py @@ -1965,6 +1965,22 @@ class AutodetectorTests(TestCase): self.assertOperationAttributes(changes, 'testapp', 0, 0, name="Author") self.assertMigrationDependencies(changes, 'testapp', 0, [("__setting__", "AUTH_USER_MODEL")]) + def test_swappable_lowercase(self): + model_state = ModelState('testapp', 'Document', [ + ('id', models.AutoField(primary_key=True)), + ('owner', models.ForeignKey( + settings.AUTH_USER_MODEL.lower(), models.CASCADE, + )), + ]) + with isolate_lru_cache(apps.get_swappable_settings_name): + changes = self.get_changes([], [model_state]) + self.assertNumberMigrations(changes, 'testapp', 1) + self.assertOperationTypes(changes, 'testapp', 0, ['CreateModel']) + self.assertOperationAttributes(changes, 'testapp', 0, 0, name='Document') + self.assertMigrationDependencies( + changes, 'testapp', 0, [('__setting__', 'AUTH_USER_MODEL')], + ) + def test_swappable_changed(self): with isolate_lru_cache(apps.get_swappable_settings_name): before = self.make_project_state([self.custom_user, self.author_with_user])