Opened 5 months ago

Last modified 5 months ago

#34988 assigned Cleanup/optimization

Makemigrations shouldn't prompt for default values for non-nullable fields of other apps.

Reported by: Sarah Boyce Owned by: Bhuvnesh
Component: Migrations Version: dev
Severity: Normal Keywords: makemigrations
Cc: Bhuvnesh Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Have more than one app (app_1 and app_2). In app_1 have a model that has it's migrations applied then add a field that doesn't have a default and is not nullable (do not run makemigrations).
Then add a new model (or anything that would require a migration) to app_2.
Run manage.py makemigrations app_2

You get prompted for every app that has unapplied migrations missing defaults even though you're running migrations for a different app.
I would expect it not to care, as you can input temporary defaults to all these other apps and it doesn't do anything with them.

I think this might be a test case:

diff --git a/tests/migrations/test_commands.py b/tests/migrations/test_commands.py
index a9c1cdf893..518ebef872 100644
--- a/tests/migrations/test_commands.py
+++ b/tests/migrations/test_commands.py
@@ -1979,6 +1979,39 @@ class MakeMigrationsTests(MigrationTestBase):
         self.assertIn("Remove field silly_field from sillymodel", out.getvalue())
         self.assertIn("Add field silly_rename to sillymodel", out.getvalue())

+    @override_settings(
+        INSTALLED_APPS=[
+            "migrations",
+            "migrations.migrations_test_apps.migrated_app",
+        ]
+    )
+    def test_makemigrations_interactive_not_null_addition_multiple_apps_single_call(self):
+        class Author(models.Model):
+            silly_author_field = models.BooleanField(null=False)
+
+            class Meta:
+                app_label = "migrations"
+
+        class NewModel1(models.Model):
+            class Meta:
+                app_label = "migrated_app"
+
+        input_msg = (
+            "It is impossible to add a non-nullable field 'silly_author_field' to "
+            "author without specifying a default. This is because the "
+            "database needs something to populate existing rows.\n"
+            "Please select a fix:\n"
+            " 1) Provide a one-off default now (will be set on all existing "
+            "rows with a null value for this column)\n"
+            " 2) Quit and manually define a default value in models.py."
+        )
+        with self.temporary_migration_module(module="migrations.test_migrations"):
+            with mock.patch("builtins.input", return_value="1"):
+                with captured_stdout() as out:
+                    call_command("makemigrations", "migrated_app", interactive=True)
+            output = out.getvalue()
+            self.assertNotIn(input_msg, output)
+
     @mock.patch("builtins.input", return_value="Y")
     def test_makemigrations_model_rename_interactive(self, mock_input):
         class RenamedModel(models.Model):

Change History (6)

comment:1 by David Sanders, 5 months ago

Triage Stage: UnreviewedAccepted

Thanks again for another report 🏆🏆

Confirmed this is indeed happening for apps with mutually independent migrations ✓

What's more is that if you run makemigrations a second time it asks for a default but then reports "No changes detected in app <app-specified>"

django-sample % dj makemigrations ticket_34988_app_1
It is impossible to add a non-nullable field 'app_1_field' to app1model without specifying a default. This is because the database needs something to populate existing rows.
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Quit and manually define a default value in models.py.
Select an option: 1
Please enter the default value as valid Python.
The datetime and django.utils.timezone modules are available, so it is possible to provide e.g. timezone.now as a value.
Type 'exit' to exit this prompt
>>> 1
It is impossible to add a non-nullable field 'app_2_field' to app2model without specifying a default. This is because the database needs something to populate existing rows.
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Quit and manually define a default value in models.py.
Select an option: 1
Please enter the default value as valid Python.
The datetime and django.utils.timezone modules are available, so it is possible to provide e.g. timezone.now as a value.
Type 'exit' to exit this prompt
>>> 1
Migrations for 'ticket_34988_app_1':
  ticket_34988_app_1/migrations/0002_app1model_app_1_field.py
    - Add field app_1_field to app1model

django-sample % dj makemigrations ticket_34988_app_1
It is impossible to add a non-nullable field 'app_2_field' to app2model without specifying a default. This is because the database needs something to populate existing rows.
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
 2) Quit and manually define a default value in models.py.
Select an option: 1
Please enter the default value as valid Python.
The datetime and django.utils.timezone modules are available, so it is possible to provide e.g. timezone.now as a value.
Type 'exit' to exit this prompt
>>> 1
No changes detected in app 'ticket_34988_app_1'
Last edited 5 months ago by David Sanders (previous) (diff)

comment:2 by David Sanders, 5 months ago

Just thinking about whether this is feasible to do: The autodetector is documented as being designed to run on entire projects but I wonder if it's possible to delay prompting the questioner for various things it requests until after _trim_changes() is called.

comment:3 by Mariusz Felisiak, 5 months ago

Follow up to #22791.

comment:4 by Sarah Boyce, 5 months ago

Cc: Bhuvnesh added

comment:5 by Mariusz Felisiak, 5 months ago

Type: UncategorizedCleanup/optimization

comment:6 by Bhuvnesh, 5 months ago

Owner: changed from nobody to Bhuvnesh
Status: newassigned

The autodetector is documented as being designed to run on entire projects but I wonder if it's possible to delay prompting the questioner for various things it requests until after _trim_changes() is called.

Hi, David ! I cannot fully understand what you are trying the propose, it will be helpful if you could please elaborate this a bit. 😅

I'm working on one more approach - passing app_label from auto-detector and restricting questioner for apps other than specified apps. This is working for independent apps but not for dependent apps. Need to look more into this.

Note: See TracTickets for help on using tickets.
Back to Top