From 3724717d88b45bf4fdf611364a3aee1f8fb6fa9d Mon Sep 17 00:00:00 2001 From: David Wobrock Date: Sun, 4 Feb 2024 21:02:37 +0100 Subject: [PATCH] Support Django 5.0 db_default attribute. A DB default is not considered backward incompatible, as the default value is kept on DB-level. See https://docs.djangoproject.com/en/dev/releases/5.0/#database-computed-default-values --- CHANGELOG.md | 3 ++ docs/incompatibilities.md | 3 +- .../sql_analyser/base.py | 26 ++++++++-------- tests/fixtures.py | 3 ++ tests/functional/test_migration_linter.py | 7 +++++ .../__init__.py | 0 .../migrations/0001_initial.py | 30 +++++++++++++++++++ .../0002_a_not_null_field_db_default.py | 21 +++++++++++++ .../migrations/__init__.py | 0 .../models.py | 12 ++++++++ .../models.py | 1 + tests/test_project/settings.py | 8 +++++ tests/unit/test_sql_analyser.py | 22 ++++++++++++++ 13 files changed, 123 insertions(+), 13 deletions(-) create mode 100644 tests/test_project/app_add_not_null_column_followed_by_db_default/__init__.py create mode 100644 tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/0001_initial.py create mode 100644 tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/0002_a_not_null_field_db_default.py create mode 100644 tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/__init__.py create mode 100644 tests/test_project/app_add_not_null_column_followed_by_db_default/models.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b5ef186..89cc73a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## 5.x.y +Feature: +- Support Django 5.0 `db_default` attribute (issue #275) + Bug: - Don't detect 'IS NOT NULL' as backward incompatible changes (issue #263) diff --git a/docs/incompatibilities.md b/docs/incompatibilities.md index 90a5bf06..873480d8 100644 --- a/docs/incompatibilities.md +++ b/docs/incompatibilities.md @@ -71,7 +71,7 @@ Only rolling back the code will make all new insertions crash because Django doe One would think that adding a default value in Django will prevent these errors. A common misconception is that the Django default value is translated to a database default. -But Django actually uses the default value to fill new new column on existing rows and to set an unspecified column value to its default. +But Django actually uses the default value to fill the new column on existing rows and to set an unspecified column value to its default. The latter is done at the application level, by Django and not by the database because the default value was dropped during migration. You can read more about this in the [Django and its default values blog post](https://medium.com/botify-labs/django-and-its-default-values-c21a13cff9f). @@ -90,6 +90,7 @@ You can read more about this in the [Django and its default values blog post](ht :white_check_mark: **Solutions**: - Make the column nullable +- Set a database default using the Django 5.0 attribute `db_default`. See the [Django docs](https://docs.djangoproject.com/en/dev/releases/5.0/#database-computed-default-values) - Set a database default using Django's [RunSQL](https://docs.djangoproject.com/en/dev/ref/migration-operations/#django.db.migrations.operations.RunSQL) - Set a database default using [django-add-default-value](https://github.com/3YOURMIND/django-add-default-value/) diff --git a/src/django_migration_linter/sql_analyser/base.py b/src/django_migration_linter/sql_analyser/base.py index 0dcd9b68..bc037f84 100644 --- a/src/django_migration_linter/sql_analyser/base.py +++ b/src/django_migration_linter/sql_analyser/base.py @@ -32,20 +32,22 @@ def update_migration_checks( def has_not_null_column(sql_statements: list[str], **kwargs) -> bool: # TODO: improve to detect that the same column is concerned - ends_with_default = False + not_null_column = False + has_default_value = False + for sql in sql_statements: + if re.search("(? bool: diff --git a/tests/fixtures.py b/tests/fixtures.py index 38224c4b..5432c0df 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -8,6 +8,9 @@ RENAME_TABLE = "app_rename_table" IGNORE_MIGRATION = "app_ignore_migration" ADD_NOT_NULL_COLUMN_FOLLOWED_BY_DEFAULT = "app_add_not_null_column_followed_by_default" +ADD_NOT_NULL_COLUMN_FOLLOWED_BY_DB_DEFAULT = ( + "app_add_not_null_column_followed_by_db_default" +) ALTER_COLUMN = "app_alter_column" ALTER_COLUMN_DROP_NOT_NULL = "app_alter_column_drop_not_null" DROP_UNIQUE_TOGETHER = "app_unique_together" diff --git a/tests/functional/test_migration_linter.py b/tests/functional/test_migration_linter.py index c2b15495..ca8cf5d3 100644 --- a/tests/functional/test_migration_linter.py +++ b/tests/functional/test_migration_linter.py @@ -2,7 +2,9 @@ import os import unittest +from unittest import skipIf +import django from django.conf import settings from django.core.management import call_command @@ -88,6 +90,11 @@ def test_accept_not_null_column_followed_by_adding_default(self): app = fixtures.ADD_NOT_NULL_COLUMN_FOLLOWED_BY_DEFAULT self._test_linter_finds_no_errors(app) + @skipIf(django.VERSION[0] < 5, "db_default was implemented in Django 5.0") + def test_accept_not_null_column_followed_by_adding_db_default(self): + app = fixtures.ADD_NOT_NULL_COLUMN_FOLLOWED_BY_DB_DEFAULT + self._test_linter_finds_no_errors(app) + def test_detect_alter_column(self): app = fixtures.ALTER_COLUMN self._test_linter_finds_no_errors(app) diff --git a/tests/test_project/app_add_not_null_column_followed_by_db_default/__init__.py b/tests/test_project/app_add_not_null_column_followed_by_db_default/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/0001_initial.py b/tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/0001_initial.py new file mode 100644 index 00000000..9c5bed6a --- /dev/null +++ b/tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/0001_initial.py @@ -0,0 +1,30 @@ +# Generated by Django 5.0.1 on 2024-02-04 20:07 + +from __future__ import annotations + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [] + + operations = [ + migrations.CreateModel( + name="A", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("field", models.IntegerField()), + ], + ), + ] diff --git a/tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/0002_a_not_null_field_db_default.py b/tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/0002_a_not_null_field_db_default.py new file mode 100644 index 00000000..5bd6e3be --- /dev/null +++ b/tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/0002_a_not_null_field_db_default.py @@ -0,0 +1,21 @@ +# Generated by Django 5.0.1 on 2024-02-04 20:07 + +from __future__ import annotations + +import django.db.models.functions.math +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("app_add_not_null_column_followed_by_db_default", "0001_initial"), + ] + + operations = [ + migrations.AddField( + model_name="a", + name="not_null_field_db_default", + field=models.IntegerField(db_default=django.db.models.functions.math.Pi()), + ), + ] diff --git a/tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/__init__.py b/tests/test_project/app_add_not_null_column_followed_by_db_default/migrations/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/test_project/app_add_not_null_column_followed_by_db_default/models.py b/tests/test_project/app_add_not_null_column_followed_by_db_default/models.py new file mode 100644 index 00000000..1f32388d --- /dev/null +++ b/tests/test_project/app_add_not_null_column_followed_by_db_default/models.py @@ -0,0 +1,12 @@ +from __future__ import annotations + +from django.db import models +from django.db.models.functions import Pi + + +class A(models.Model): + field = models.IntegerField() + not_null_field_db_default = models.IntegerField( + null=False, + db_default=Pi(), + ) diff --git a/tests/test_project/app_add_not_null_column_followed_by_default/models.py b/tests/test_project/app_add_not_null_column_followed_by_default/models.py index cfb119c2..629479bf 100644 --- a/tests/test_project/app_add_not_null_column_followed_by_default/models.py +++ b/tests/test_project/app_add_not_null_column_followed_by_default/models.py @@ -1,6 +1,7 @@ from __future__ import annotations from django.db import models +from django.db.models.functions import Pi class A(models.Model): diff --git a/tests/test_project/settings.py b/tests/test_project/settings.py index b23edb2a..d71663d1 100644 --- a/tests/test_project/settings.py +++ b/tests/test_project/settings.py @@ -14,6 +14,8 @@ import os +import django + # Build paths inside the project like this: os.path.join(BASE_DIR, ...) BASE_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) @@ -59,6 +61,12 @@ "tests.test_project.app_create_index_exclusive", ] +if django.VERSION[0] >= 5: + # db_default attribute was only added in Django 5.0 + INSTALLED_APPS.append( + "tests.test_project.app_add_not_null_column_followed_by_db_default" + ) + MIDDLEWARE = [ "django.middleware.security.SecurityMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", diff --git a/tests/unit/test_sql_analyser.py b/tests/unit/test_sql_analyser.py index 48daf0dd..0e903c0e 100644 --- a/tests/unit/test_sql_analyser.py +++ b/tests/unit/test_sql_analyser.py @@ -66,6 +66,15 @@ def test_add_not_null(self): ] self.assertBackwardIncompatibleSql(sql) + def test_add_not_null_without_dropping_default(self): + sql = ( + "ALTER TABLE `app_add_not_null_column_a` ADD COLUMN `new_not_null_field` integer DEFAULT 1 NOT NULL;", + ) + self.assertValidSql(sql) + + sql = "ALTER TABLE `app_add_not_null_column_followed_by_default_a` ADD COLUMN `not_null_field_db_default` integer DEFAULT (PI()) NOT NULL;" + self.assertValidSql(sql) + def test_add_not_null_followed_by_default(self): sql = [ "ALTER TABLE `app_add_not_null_column_followed_by_default_a` ADD COLUMN `new_not_null_field` integer DEFAULT 1 NOT NULL;", @@ -147,6 +156,15 @@ def test_add_not_null(self): ] self.assertBackwardIncompatibleSql(sql) + def test_add_not_null_without_dropping_default(self): + sql = [ + 'CREATE TABLE "new__app_add_not_null_column_followed_by_default_a" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "not_null_field_db_default" integer DEFAULT (PI()) NOT NULL, "field" integer NOT NULL, "not_null_field" integer NOT NULL);', + 'INSERT INTO "new__app_add_not_null_column_followed_by_default_a" ("id", "field", "not_null_field") SELECT "id", "field", "not_null_field" FROM "app_add_not_null_column_followed_by_default_a";', + 'DROP TABLE "app_add_not_null_column_followed_by_default_a";', + 'ALTER TABLE "new__app_add_not_null_column_followed_by_default_a" RENAME TO "app_add_not_null_column_followed_by_default_a";', + ] + self.assertBackwardIncompatibleSql(sql) + def test_create_table_with_not_null(self): sql = 'CREATE TABLE "app_create_table_with_not_null_column_a" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "field" varchar(150) NOT NULL);' self.assertValidSql(sql) @@ -207,6 +225,10 @@ def test_alter_column(self): sql = 'ALTER TABLE "app_alter_column_a" ALTER COLUMN "field" TYPE varchar(10) USING "field"::varchar(10);' self.assertBackwardIncompatibleSql(sql) + def test_add_not_null_without_dropping_default(self): + sql = 'ALTER TABLE "app_add_not_null_column_followed_by_default_a" ADD COLUMN "not_null_field_db_default" integer DEFAULT (PI()) NOT NULL;' + self.assertValidSql(sql) + def test_not_null_followed_by_default(self): sql = [ 'ALTER TABLE "app_add_not_null_column_followed_by_default_a" ADD COLUMN "not_null_field" integer DEFAULT 1 NOT NULL;',