From 9659efa52630bd24fda3c4920cab6de5dee338b7 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Tue, 22 Apr 2025 22:59:29 -0400 Subject: [PATCH] Fix #60 -- Adjust pre-AddField(db_default) for ForeignKey with callable defaults. ForeignKey.get_default() cannot be safely called on instances that are not bound to a model yet; this will have to be adjusted if Django's schema editor is ever adjusted to operate from model states. Thanks @jzmiller1 for the report. --- CHANGELOG.rst | 2 ++ syzygy/operations.py | 18 +++++++++- tests/models.py | 7 ++++ tests/test_autodetector.py | 34 +++++++++++++++++-- .../null_field_removal/0001_initial.py | 16 +++++++++ 5 files changed, 73 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3bc8df6..6ab2e3e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ - Avoid unnecessary prompting for a default value on `ManyToManyField` removals. (#59) +- Address a ``makemigration`` crash when adding a ``ForeignKey`` with a + callable ``default``. (#60) 1.2.0 ===== diff --git a/syzygy/operations.py b/syzygy/operations.py index 74b2e6a..c84e4f9 100644 --- a/syzygy/operations.py +++ b/syzygy/operations.py @@ -1,5 +1,6 @@ from contextlib import contextmanager +from django.db import models from django.db.migrations import operations from django.db.models.fields import NOT_PROVIDED from django.utils.functional import cached_property @@ -201,7 +202,22 @@ def get_pre_add_field_operation(model_name, name, field, preserve_default=True): "Fields with a db_default don't require a pre-deployment operation." ) field = field.clone() - field.db_default = field.get_default() + if isinstance(field, models.ForeignKey): + # XXX: Replicate ForeignKey.get_default() logic in a way that + # doesn't require the field references to be pre-emptively + # resolved. This will need to be fixed upstream if we ever + # implement model state schema alterations. + # See https://code.djangoproject.com/ticket/29898 + field_default = super(models.ForeignKey, field).get_default() + if ( + isinstance(field_default, models.Model) + and field_default._meta.label_lower == field.related_model.lower() + ): + target_field = field.to_fields[0] or "pk" + field_default = getattr(field_default, target_field) + else: + field_default = field.get_default() + field.db_default = field_default operation = operations.AddField(model_name, name, field, preserve_default) return operation diff --git a/tests/models.py b/tests/models.py index 7f89123..eaefee9 100644 --- a/tests/models.py +++ b/tests/models.py @@ -3,3 +3,10 @@ class Foo(models.Model): pass + + +class Bar(models.Model): + name = models.CharField(max_length=100, unique=True) + + class Meta: + managed = False diff --git a/tests/test_autodetector.py b/tests/test_autodetector.py index 762c235..263a919 100644 --- a/tests/test_autodetector.py +++ b/tests/test_autodetector.py @@ -25,6 +25,8 @@ ) from syzygy.plan import get_migration_stage +from .models import Bar + class AutodetectorTestCase(TestCase): style = color_style() @@ -53,7 +55,7 @@ def get_changes( class AutodetectorTests(AutodetectorTestCase): - def _test_field_addition(self, field): + def _test_field_addition(self, field, expected_db_default=None): from_model = ModelState("tests", "Model", []) to_model = ModelState("tests", "Model", [("field", field)]) changes = self.get_changes([from_model], [to_model])["tests"] @@ -64,7 +66,10 @@ def _test_field_addition(self, field): pre_operation = changes[0].operations[0] if field_db_default_supported: self.assertIsInstance(pre_operation, migrations.AddField) - self.assertEqual(pre_operation.field.db_default, field.default) + self.assertEqual( + pre_operation.field.db_default, + expected_db_default or field.get_default(), + ) else: self.assertIsInstance(pre_operation, AddField) self.assertEqual(get_migration_stage(changes[1]), Stage.POST_DEPLOY) @@ -81,10 +86,33 @@ def test_field_addition(self): fields = [ models.IntegerField(default=42), models.IntegerField(null=True, default=42), + models.IntegerField(default=lambda: 42), + # Foreign keys with callable defaults should have their associated + # db_default generated with care. + (models.ForeignKey("tests.Model", models.CASCADE, default=42), 42), + ( + models.ForeignKey( + "tests.Bar", models.CASCADE, default=lambda: Bar(id=42) + ), + 42, + ), + ( + models.ForeignKey( + "tests.bar", + models.CASCADE, + to_field="name", + default=lambda: Bar(id=123, name="bar"), + ), + "bar", + ), ] for field in fields: + if isinstance(field, tuple): + field, expected_db_default = field + else: + expected_db_default = None with self.subTest(field=field): - self._test_field_addition(field) + self._test_field_addition(field, expected_db_default) def test_many_to_many_addition(self): from_model = ModelState("tests", "Model", []) diff --git a/tests/test_migrations/null_field_removal/0001_initial.py b/tests/test_migrations/null_field_removal/0001_initial.py index 5a91212..fd7483d 100644 --- a/tests/test_migrations/null_field_removal/0001_initial.py +++ b/tests/test_migrations/null_field_removal/0001_initial.py @@ -19,4 +19,20 @@ class Migration(migrations.Migration): ("bar", models.IntegerField()), ], ), + migrations.CreateModel( + "Bar", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("name", models.CharField(unique=True)), + ], + options={"managed": False}, + ), ]