diff --git a/django_migration_linter/migration_linter.py b/django_migration_linter/migration_linter.py index 0262967..ac83aa1 100644 --- a/django_migration_linter/migration_linter.py +++ b/django_migration_linter/migration_linter.py @@ -20,6 +20,7 @@ import os import re from subprocess import Popen, PIPE +from django.conf import settings from django.core.management import call_command from django.db import DEFAULT_DB_ALIAS, connections @@ -122,7 +123,9 @@ class MigrationLinter(object): sql_statements = self.get_sql(app_label, migration_name) exclude_migration_tests = self.exclude_migration_tests or [] errors, ignored = analyse_sql_statements( - sql_statements, exclude_migration_tests + sql_statements, + settings.DATABASES[self.database]["ENGINE"], + exclude_migration_tests, ) if not errors: diff --git a/django_migration_linter/sql_analyser.py b/django_migration_linter/sql_analyser.py index aff2c20..d5fa251 100644 --- a/django_migration_linter/sql_analyser.py +++ b/django_migration_linter/sql_analyser.py @@ -14,63 +14,141 @@ import re import logging +from copy import deepcopy logger = logging.getLogger(__name__) -def has_default(sql, **kwargs): - if re.search("SET DEFAULT", sql) and kwargs["errors"]: - err = next((err for err in kwargs["errors"] if err["code"] == "NOT_NULL"), None) - if err: - logger.info( - ( - "Found a NOT_NULL error in migration, " - "but it has a default value added: {}" - ).format(err) - ) - kwargs["errors"].remove(err) - - return False # Never fails +def find_error_dict_with_code(tests, code): + return next((test_dict for test_dict in tests if test_dict["code"] == code), None) -migration_tests = ( +def has_not_null_column(sql_statements, **kwargs): + # TODO: improve to detect that the same column is concerned + return any( + re.search("(? should use cache now. @@ -116,15 +116,15 @@ class CacheTestCase(unittest.TestCase): self.assertEqual("OK", cache["4a3770a405738d457e2d23e17fb1f3aa"]["result"]) self.assertEqual("ERR", cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["result"]) self.assertListEqual( - cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["errors"], [ { - "err_msg": "RENAMING tables", - "code": "RENAME_TABLE", + "err_msg": "NOT NULL constraint on columns", + "code": "NOT_NULL", "table": None, "column": None, } ], + cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["errors"], ) # Start the Linter again but with different database, should not be the same cache @@ -143,15 +143,15 @@ class CacheTestCase(unittest.TestCase): self.assertEqual("OK", cache["4a3770a405738d457e2d23e17fb1f3aa"]["result"]) self.assertEqual("ERR", cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["result"]) self.assertListEqual( - cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["errors"], [ { "err_msg": "NOT NULL constraint on columns", "code": "NOT_NULL", - "table": "app_add_not_null_column_a", - "column": "new_not_null_field", + "table": None, + "column": None, } ], + cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["errors"], ) self.assertTrue(linter.has_errors) @@ -256,15 +256,15 @@ class CacheTestCase(unittest.TestCase): self.assertEqual("OK", cache["4a3770a405738d457e2d23e17fb1f3aa"]["result"]) self.assertEqual("ERR", cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["result"]) self.assertListEqual( - cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["errors"], [ { - "err_msg": "RENAMING tables", - "code": "RENAME_TABLE", + "err_msg": "NOT NULL constraint on columns", + "code": "NOT_NULL", "table": None, "column": None, } ], + cache["19fd3ea688fc05e2cc2a6e67c0b7aa17"]["errors"], ) # Start the Linter again -> should use cache now but ignore the erroneous diff --git a/tests/functional/test_migration_linter.py b/tests/functional/test_migration_linter.py index 68660a6..79172a2 100644 --- a/tests/functional/test_migration_linter.py +++ b/tests/functional/test_migration_linter.py @@ -79,7 +79,7 @@ class BaseBackwardCompatibilityDetection(object): def test_detect_alter_column(self): app = fixtures.ALTER_COLUMN - self._test_linter_finds_errors(app) + self._test_linter_finds_no_errors(app) def test_accept_alter_column_drop_not_null(self): app = fixtures.ALTER_COLUMN_DROP_NOT_NULL @@ -109,3 +109,7 @@ class PostgresqlBackwardCompatibilityDetectionTestCase( BaseBackwardCompatibilityDetection, unittest.TestCase ): databases = ["postgresql"] + + def test_detect_alter_column(self): + app = fixtures.ALTER_COLUMN + self._test_linter_finds_errors(app) diff --git a/tests/unit/test_linter.py b/tests/unit/test_linter.py index 80a7751..35ff13a 100644 --- a/tests/unit/test_linter.py +++ b/tests/unit/test_linter.py @@ -28,7 +28,7 @@ class LinterFunctionsTestCase(unittest.TestCase): self.assertEqual(sql_statements[-1], "COMMIT;") def test_has_errors(self): - linter = MigrationLinter() + linter = MigrationLinter(database="mysql") self.assertFalse(linter.has_errors) m = Migration("0001_create_table", "app_add_not_null_column") diff --git a/tests/unit/test_sql_analyser.py b/tests/unit/test_sql_analyser.py new file mode 100644 index 0000000..f911578 --- /dev/null +++ b/tests/unit/test_sql_analyser.py @@ -0,0 +1,122 @@ +# Copyright 2019 3YOURMIND GmbH + +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at + +# http://www.apache.org/licenses/LICENSE-2.0 + +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import unittest + +from django_migration_linter.sql_analyser import analyse_sql_statements + + +class SqlAnalyserTestCase(unittest.TestCase): + database_vendor = "default" + + def analyse_sql(self, sql): + if isinstance(sql, str): + sql = sql.splitlines() + return analyse_sql_statements( + sql_statements=sql, database_vendor=self.database_vendor + ) + + def assertValidSql(self, sql): + errors, _ = self.analyse_sql(sql) + self.assertEqual(0, len(errors), "Found errors in sql: {}".format(errors)) + + def assertBackwardIncompatibleSql(self, sql): + errors, _ = self.analyse_sql(sql) + self.assertNotEqual(0, len(errors), "Found no errors in sql") + + +class MySqlAnalyserTestCase(SqlAnalyserTestCase): + database_vendor = "mysql" + + def test_alter_column(self): + sql = "ALTER TABLE `app_alter_column_a` MODIFY `field` varchar(10) NULL;" + self.assertValidSql(sql) + + def test_drop_not_null(self): + sql = "ALTER TABLE `app_alter_column_drop_not_null_a` MODIFY `not_null_field` integer NULL;" + self.assertValidSql(sql) + + def test_add_not_null(self): + sql = [ + "ALTER TABLE `app_add_not_null_column_a` ADD COLUMN `new_not_null_field` integer DEFAULT 1 NOT NULL;", + "ALTER TABLE `app_add_not_null_column_a` ALTER COLUMN `new_not_null_field` DROP DEFAULT;", + ] + self.assertBackwardIncompatibleSql(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;", + "ALTER TABLE `app_add_not_null_column_followed_by_default_a` ALTER COLUMN `new_not_null_field` DROP DEFAULT;", + "ALTER TABLE `app_add_not_null_column_followed_by_default_a` ALTER COLUMN `new_not_null_field` SET DEFAULT '1';", + ] + self.assertValidSql(sql) + + +class SqliteAnalyserTestCase(SqlAnalyserTestCase): + database_vendor = "sqlite" + + def test_drop_not_null(self): + sql = [ + 'ALTER TABLE "app_alter_column_drop_not_null_a" RENAME TO "app_alter_column_drop_not_null_a__old";', + 'CREATE TABLE "app_alter_column_drop_not_null_a" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "not_null_field" integer NULL);', + 'INSERT INTO "app_alter_column_drop_not_null_a" ("id", "not_null_field") SELECT "id", "not_null_field" FROM "app_alter_column_drop_not_null_a__old";', + 'DROP TABLE "app_alter_column_drop_not_null_a__old";', + ] + self.assertValidSql(sql) + + def test_add_not_null(self): + sql = [ + 'ALTER TABLE "app_add_not_null_column_a" RENAME TO "app_add_not_null_column_a__old";', + 'CREATE TABLE "app_add_not_null_column_a" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "new_not_null_field" integer NOT NULL, "null_field" integer NULL);', + 'INSERT INTO "app_add_not_null_column_a" ("id", "null_field", "new_not_null_field") SELECT "id", "null_field", 1 FROM "app_add_not_null_column_a__old";', + 'DROP TABLE "app_add_not_null_column_a__old";', + ] + 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) + + def test_rename_table(self): + sql = 'ALTER TABLE "app_rename_table_a" RENAME TO "app_rename_table_b";' + self.assertBackwardIncompatibleSql(sql) + + def test_alter_column(self): + sql = [ + 'ALTER TABLE "app_alter_column_a" RENAME TO "app_alter_column_a__old";', + 'CREATE TABLE "app_alter_column_a" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "field" varchar(10) NULL);', + 'INSERT INTO "app_alter_column_a" ("id", "field") SELECT "id", "field" FROM "app_alter_column_a__old";', + 'DROP TABLE "app_alter_column_a__old";', + ] + self.assertValidSql(sql) + + +class PostgresqlAnalyserTestCase(SqlAnalyserTestCase): + database_vendor = "postgresql" + + 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_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;', + 'ALTER TABLE "app_add_not_null_column_followed_by_default_a" ALTER COLUMN "not_null_field" DROP DEFAULT;', + 'ALTER TABLE "app_add_not_null_column_followed_by_default_a" ALTER COLUMN "not_null_field" SET DEFAULT \'1\';', + ] + self.assertValidSql(sql) + + def test_drop_not_null(self): + sql = 'ALTER TABLE "app_alter_column_drop_not_null_a" ALTER COLUMN "not_null_field" DROP NOT NULL;' + self.assertValidSql(sql)