From 18a39cb3fca0322a4a3366fb19ac07e6346580fc 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 | 12 ++++++++ .../__init__.py | 0 .../migrations/0001_initial.py | 30 +++++++++++++++++++ .../0002_a_not_null_field_db_default.py | 21 +++++++++++++ .../migrations/__init__.py | 0 .../models.py | 12 ++++++++ tests/test_project/settings.py | 8 +++++ tests/unit/test_sql_analyser.py | 22 ++++++++++++++ 12 files changed, 127 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..6c3cf2fc 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) @@ -128,6 +135,11 @@ def test_detect_make_column_not_null_with_django_default(self): app = fixtures.MAKE_NOT_NULL_WITH_DJANGO_DEFAULT self._test_linter_finds_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_errors(app) + def test_detect_make_column_not_null_with_lib_default(self): # The 'django-add-default-value' doesn't handle sqlite correctly app = fixtures.MAKE_NOT_NULL_WITH_LIB_DEFAULT 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/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;',