From 874f6f91b6492c8e414abec9803906d3fc67d827 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Mon, 15 Jul 2024 12:59:04 +0000 Subject: [PATCH 1/3] refs #134: fix test_inspectdb; inspectdb should suppress output 'id = AutoField(primary_key=True)' --- CHANGES.rst | 2 ++ django_redshift_backend/base.py | 15 +++++++++- doc/dev.rst | 6 ++++ pyproject.toml | 2 +- tests/conftest.py | 52 +++++++++++++++++++++++++++++++++ tests/settings.py | 17 ++++++----- tests/test_inspectdb.py | 11 +++---- tests/test_migrations.py | 19 ++++++++---- tests/test_redshift_backend.py | 7 ++--- tox.ini | 2 ++ 10 files changed, 106 insertions(+), 27 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index e61d29e..6abb469 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -16,6 +16,8 @@ Features: Bug Fixes: +* #134 inspectdb should suppress output 'id = AutoField(primary_key=True)' + 3.0.0 (2022/02/27) ------------------ diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index c397ba6..589db43 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -841,7 +841,7 @@ def _get_max_length(field): # Size is changed if ( - type(old_field) == type(new_field) + type(old_field) is type(new_field) and old_max_length is not None and new_max_length is not None and old_max_length != new_max_length @@ -1069,6 +1069,19 @@ class DatabaseCreation(BasePGDatabaseCreation): class DatabaseIntrospection(BasePGDatabaseIntrospection): + # to avoid output 'id = meta.AutoField(primary_key=True)', + # return 'AutoField' for 'identity'. + def get_field_type(self, data_type, description): + field_type = super().get_field_type(data_type, description) + if description.default and "identity" in description.default: + if field_type == "IntegerField": + return "AutoField" + elif field_type == "BigIntegerField": + return "BigAutoField" + elif field_type == "SmallIntegerField": + return "SmallAutoField" + return field_type + def get_table_description(self, cursor, table_name): """ Return a description of the table with the DB-API cursor.description diff --git a/doc/dev.rst b/doc/dev.rst index 1e18832..ec55894 100644 --- a/doc/dev.rst +++ b/doc/dev.rst @@ -41,6 +41,12 @@ To test the database migration as well, start postgres and test it as follows:: $ docker-compose up -d $ TEST_WITH_POSTGRES=1 tox +To test migrations with Redshift, do it as follows: + +1. Create your redshift cruster on AWS +2. Get a redshift endpoint URI +3. run tox as: `TEST_WITH_REDSHIFT=redshift://user:password@...redshift.amazonaws.com:5439/?DISABLE_SERVER_SIDE_CURSORS=True tox` + CI (Continuous Integration) ---------------------------- diff --git a/pyproject.toml b/pyproject.toml index 0cc2bb4..474dfb6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -32,7 +32,7 @@ classifiers = [ "Topic :: Software Development :: Libraries :: Python Modules", ] dependencies = [ - "django", + "django<4.2", ] [project.optional-dependencies] diff --git a/tests/conftest.py b/tests/conftest.py index f43a069..7c6d614 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -5,3 +5,55 @@ from django.apps import apps # noqa E402 apps.populate(['testapp']) + + +import contextlib +from unittest import mock + +import pytest + +from django_redshift_backend.base import BasePGDatabaseWrapper + +TEST_WITH_POSTGRES = os.environ.get('TEST_WITH_POSTGRES') +TEST_WITH_REDSHIFT = os.environ.get('TEST_WITH_REDSHIFT') + +skipif_no_database = pytest.mark.skipif( + not TEST_WITH_POSTGRES and not TEST_WITH_REDSHIFT, + reason="no TEST_WITH_POSTGRES/TEST_WITH_REDSHIFT are found", +) +run_only_postgres = pytest.mark.skipif( + not TEST_WITH_POSTGRES, + reason="Test only for postgres", +) +run_only_redshift = pytest.mark.skipif( + not TEST_WITH_REDSHIFT, + reason="Test only for redshift", +) + +@contextlib.contextmanager +def postgres_fixture(): + """A context manager that patches the database backend to use PostgreSQL + for local testing. + + The purpose of the postgres_fixture context manager is to conditionally + patch the database backend to use PostgreSQL for testing, but only if the + TEST_WITH_POSTGRES variable is set to True. + + The reason for not using pytest.fixture in the current setup is due to the + use of classes that inherit from TestCase. pytest fixtures do not directly + integrate with Django's TestCase based tests. + """ + if TEST_WITH_POSTGRES: + with \ + mock.patch( + 'django_redshift_backend.base.DatabaseWrapper.data_types', + BasePGDatabaseWrapper.data_types, + ), \ + mock.patch( + 'django_redshift_backend.base.DatabaseSchemaEditor._get_create_options', + lambda self, model: '', + ): + yield + + else: + yield diff --git a/tests/settings.py b/tests/settings.py index 73816ee..60d4580 100644 --- a/tests/settings.py +++ b/tests/settings.py @@ -1,15 +1,16 @@ # -*- coding: utf-8 -*- +import os +import environ +if uri := os.environ.get("TEST_WITH_REDSHIFT"): + # use URI if it has least one charactor. + os.environ["DATABASE_URL"] = uri +else: + os.environ["DATABASE_URL"] = "redshift://user:password@localhost:5439/testing" +env = environ.Env() DATABASES = { - 'default': { - 'ENGINE': 'django_redshift_backend', - 'NAME': 'testing', - 'USER': 'user', - 'PASSWORD': 'password', - 'HOST': 'localhost', - 'PORT': '5439', - } + 'default': env.db() } SECRET_KEY = '' diff --git a/tests/test_inspectdb.py b/tests/test_inspectdb.py index a278803..34aec73 100644 --- a/tests/test_inspectdb.py +++ b/tests/test_inspectdb.py @@ -6,11 +6,10 @@ from django.db import connections from django.core.management import call_command -import pytest -from django_redshift_backend.base import BasePGDatabaseWrapper from test_base import OperationTestBase +from conftest import skipif_no_database, postgres_fixture def norm_sql(sql): return ' '.join(sql.split()).replace('( ', '(').replace(' )', ')').replace(' ;', ';') @@ -188,8 +187,7 @@ def test_get_get_constraints_does_not_use_unsupported_functions(self): self.assertEqual(self.expected_indexes_query, executed_sql) -@pytest.mark.skipif(not os.environ.get('TEST_WITH_POSTGRES'), - reason='to run, TEST_WITH_POSTGRES=1 tox') +@skipif_no_database class InspectDbTests(OperationTestBase): available_apps = [] databases = {'default'} @@ -210,11 +208,10 @@ class Meta: def tearDown(self): self.cleanup_test_tables() - @mock.patch('django_redshift_backend.base.DatabaseWrapper.data_types', BasePGDatabaseWrapper.data_types) - @mock.patch('django_redshift_backend.base.DatabaseSchemaEditor._get_create_options', lambda self, model: '') + @postgres_fixture() def test_inspectdb(self): self.set_up_test_model('test') out = StringIO() - call_command('inspectdb', stdout=out) + call_command('inspectdb', 'test_pony', stdout=out) print(out.getvalue()) self.assertIn(self.expected_pony_model, out.getvalue()) diff --git a/tests/test_migrations.py b/tests/test_migrations.py index cb76f7a..06ba95f 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -1,4 +1,3 @@ -import os import contextlib from unittest import mock @@ -6,14 +5,11 @@ from django.db.migrations.state import ProjectState import pytest -from django_redshift_backend.base import BasePGDatabaseWrapper from test_base import OperationTestBase +from conftest import skipif_no_database, postgres_fixture -@pytest.mark.skipif(not os.environ.get('TEST_WITH_POSTGRES'), - reason='to run, TEST_WITH_POSTGRES=1 tox') -@mock.patch('django_redshift_backend.base.DatabaseWrapper.data_types', BasePGDatabaseWrapper.data_types) -@mock.patch('django_redshift_backend.base.DatabaseSchemaEditor._get_create_options', lambda self, model: '') +@skipif_no_database class MigrationTests(OperationTestBase): available_apps = ["testapp"] databases = {'default'} @@ -40,6 +36,7 @@ def execute(self, sql, params=()): print('\n'.join(collected_sql)) + @postgres_fixture() def test_alter_size(self): new_state = self.set_up_test_model('test') operations = [ @@ -69,6 +66,7 @@ def test_alter_size(self): '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(10);''', ], sqls) + @postgres_fixture() def test_alter_size_for_unique(self): new_state = self.set_up_test_model('test') operations = [ @@ -94,6 +92,7 @@ def test_alter_size_for_unique(self): '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_key" UNIQUE ("name");''' ], sqls) + @postgres_fixture() def test_alter_size_for_pk(self): setup_operations = [migrations.CreateModel( 'Pony', @@ -120,6 +119,7 @@ def test_alter_size_for_pk(self): '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_2c070d2a_pk" PRIMARY KEY ("name");''', ], sqls) + @postgres_fixture() def test_alter_size_for_fk(self): setup_operations = [ migrations.CreateModel( @@ -159,6 +159,7 @@ def test_alter_size_for_fk(self): ''' FOREIGN KEY ("pony_id") REFERENCES "test_pony" ("id");'''), ], sqls) + @postgres_fixture() def test_add_notnull_without_default_raise_exception(self): from django.db.utils import ProgrammingError new_state = self.set_up_test_model('test') @@ -173,6 +174,7 @@ def test_add_notnull_without_default_raise_exception(self): with self.collect_sql(): self.apply_operations('test', new_state, operations) + @postgres_fixture() def test_add_notnull_without_default_on_backwards(self): project_state = self.set_up_test_model('test') operations = [ @@ -203,6 +205,7 @@ def test_add_notnull_without_default_on_backwards(self): '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', ], sqls) + @postgres_fixture() def test_add_notnull_with_default(self): new_state = self.set_up_test_model('test') operations = [ @@ -220,6 +223,7 @@ def test_add_notnull_with_default(self): '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', ], sqls) + @postgres_fixture() def test_alter_type(self): new_state = self.set_up_test_model('test') operations = [ @@ -240,6 +244,7 @@ def test_alter_type(self): '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', ], sqls) + @postgres_fixture() def test_alter_notnull_with_default(self): new_state = self.set_up_test_model('test') operations = [ @@ -270,6 +275,7 @@ def test_alter_notnull_with_default(self): # ## ref: https://github.com/django/django/blob/3.2.12/django/db/backends/base/schema.py#L524 # ## django-redshift-backend also does not support in-database defaults @pytest.mark.skip('django-redshift-backend does not support in-database defaults') + @postgres_fixture() def test_change_default(self): # https://github.com/jazzband/django-redshift-backend/issues/63 new_state = self.set_up_test_model('test') @@ -297,6 +303,7 @@ def test_change_default(self): '''ALTER TABLE test_pony RENAME COLUMN "name_tmp" TO "name";''', ], sqls) + @postgres_fixture() def test_alter_notnull_to_nullable(self): # https://github.com/jazzband/django-redshift-backend/issues/63 new_state = self.set_up_test_model('test') diff --git a/tests/test_redshift_backend.py b/tests/test_redshift_backend.py index 3b49adf..60e6f81 100644 --- a/tests/test_redshift_backend.py +++ b/tests/test_redshift_backend.py @@ -1,12 +1,12 @@ # -*- coding: utf-8 -*- -import os import unittest from django.db import connections from django.db.utils import NotSupportedError from django.core.management.color import no_style -import pytest + +from conftest import skipif_no_database def norm_sql(sql): @@ -153,8 +153,7 @@ def test_create_table_meta_keys(self): from testapp.models import TestModelWithMetaKeys self.check_model_creation(TestModelWithMetaKeys, expected_ddl_meta_keys) - @pytest.mark.skipif(not os.environ.get('TEST_WITH_POSTGRES'), - reason='to run, TEST_WITH_POSTGRES=1 tox') + @skipif_no_database def test_sqlmigrate(self): from django.db import connection from django.db.migrations.loader import MigrationLoader diff --git a/tox.ini b/tox.ini index 5b5400b..83e3d82 100644 --- a/tox.ini +++ b/tox.ini @@ -24,6 +24,7 @@ deps = pytest pytest-cov mock>=2.0 + django-environ dj32: Django>=3.2,<3.3 dj40: Django>=4.0,<4.1 djmain: https://github.com/django/django/archive/main.tar.gz @@ -31,6 +32,7 @@ setenv = DJANGO_SETTINGS_MODULE = settings PYTHONPATH = {toxinidir} TEST_WITH_POSTGRES = {env:TEST_WITH_POSTGRES:} + TEST_WITH_REDSHIFT = {env:TEST_WITH_REDSHIFT:} pip_pre = True commands = pytest -v --cov django_redshift_backend --cov-append --cov-report term-missing --cov-report=xml {posargs} From fc6faa0da70fc2882b5cb0a2223459f96c4e3459 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Tue, 16 Jul 2024 21:21:58 +0000 Subject: [PATCH 2/3] refs #134: Support adding COLUMN with UNIQUE; adding column without UNIQUE then add UNIQUE CONSTRAINT. This change also fixes test_alter_size_for_unique with redshift. --- CHANGES.rst | 1 + django_redshift_backend/base.py | 11 +++++++++++ tests/test_migrations.py | 33 ++++++++++++++++++++++++++++++--- 3 files changed, 42 insertions(+), 3 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 6abb469..1aa3c02 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -13,6 +13,7 @@ Features: * #83 Drop Python-3.6 support. * #127 Drop Python-3.7 support. * #83 Drop Django-2.2 support. +* #134 Support adding COLUMN with UNIQUE; adding column without UNIQUE then add UNIQUE CONSTRAINT. Bug Fixes: diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 589db43..c0657c8 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -307,6 +307,13 @@ def add_field(self, model, field): "column": self.quote_name(field.column), "definition": definition, } + + # ## Redshift + if field.unique: + # temporarily remove UNIQUE constraint from sql + # because Redshift can't add column with UNIQUE constraint. + sql = sql.rstrip(" UNIQUE") + # ## Redshift if not field.null and self.effective_default(field) is None: # Redshift Can't add NOT NULL column without DEFAULT. @@ -321,6 +328,10 @@ def add_field(self, model, field): # ## original BasePGDatabaseSchemaEditor.add_field has CREATE INDEX. # ## Redshift doesn't support INDEX. + # Add UNIQUE constraints later + if field.unique: + self.deferred_sql.append(self._create_unique_sql(model, [field])) + # Add any FK constraints later if ( field.remote_field diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 06ba95f..70a18bd 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -66,6 +66,32 @@ def test_alter_size(self): '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(10);''', ], sqls) + @postgres_fixture() + def test_add_unique_column(self): + new_state = self.set_up_test_model('test') + operations = [ + migrations.AddField( + model_name='Pony', + name='name_with_default', + field=models.CharField(max_length=10, default='', unique=True), + ), + migrations.AddField( + model_name='Pony', + name='name_with_nullable', + field=models.CharField(max_length=10, null=True, unique=True), + ), + ] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "name_with_default" varchar(10) DEFAULT '' NOT NULL;''', + '''ALTER TABLE "test_pony" ADD COLUMN "name_with_nullable" varchar(10) NULL;''', + '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_with_default_b3620670_uniq" UNIQUE ("name_with_default");''', + '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_with_nullable_d1043f78_uniq" UNIQUE ("name_with_nullable");''', + ], sqls) + @postgres_fixture() def test_alter_size_for_unique(self): new_state = self.set_up_test_model('test') @@ -86,10 +112,11 @@ def test_alter_size_for_unique(self): self.apply_operations('test', new_state, operations) self.assertEqual([ - '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL UNIQUE;''', - '''ALTER TABLE "test_pony" DROP CONSTRAINT "test_pony_name_key";''', + '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', + # ADD UNIQUE "name" + # DROP UNIQUE "name" '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', - '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_key" UNIQUE ("name");''' + '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_2c070d2a_uniq" UNIQUE ("name");''' ], sqls) @postgres_fixture() From 2a108f581d1d68a80cc9527ed02610f987956166 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Tue, 16 Jul 2024 21:43:01 +0000 Subject: [PATCH 3/3] refs #134: fix test_alter_size with redshift. Redshift can not decrease the size of a column that have default. --- CHANGES.rst | 1 + django_redshift_backend/base.py | 10 ++++++- tests/test_migrations.py | 47 ++++++++++++++++++++++++++++++--- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 1aa3c02..f7ee583 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -18,6 +18,7 @@ Features: Bug Fixes: * #134 inspectdb should suppress output 'id = AutoField(primary_key=True)' +* #134 fix for decreasing size of column with default by create-copy-drop-rename strategy. 3.0.0 (2022/02/27) ------------------ diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index c0657c8..508a8c7 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -849,6 +849,9 @@ def _get_max_length(field): old_max_length = _get_max_length(old_field) new_max_length = _get_max_length(new_field) + decrease_size_with_default = old_default is not None and ( + old_max_length > new_max_length + ) # Size is changed if ( @@ -856,6 +859,7 @@ def _get_max_length(field): and old_max_length is not None and new_max_length is not None and old_max_length != new_max_length + and not decrease_size_with_default ): # if shrink size as `old_field.max_length > new_field.max_length` and # larger data in database, this change will raise exception. @@ -921,7 +925,11 @@ def _get_max_length(field): fragment = actions.pop(0) # Type or default is changed? - elif (old_type != new_type) or needs_database_default: + elif ( + (old_type != new_type) + or needs_database_default + or decrease_size_with_default + ): fragment, actions = self._alter_column_with_recreate( model, old_field, new_field ) diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 70a18bd..0c34ece 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -37,7 +37,40 @@ def execute(self, sql, params=()): print('\n'.join(collected_sql)) @postgres_fixture() - def test_alter_size(self): + def test_alter_size_with_nullable(self): + new_state = self.set_up_test_model('test') + operations = [ + migrations.AddField( + model_name='Pony', + name='name', + field=models.CharField(max_length=10, verbose_name='name', null=True), + ), + migrations.AlterField( + model_name='Pony', + name='name', + field=models.CharField(max_length=20, verbose_name='name', null=True), + ), + migrations.AlterField( + model_name='Pony', + name='name', + field=models.CharField(max_length=10, verbose_name='name', null=True), + ), + ] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + + self.assertEqual([ + # add column + '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) NULL;''', + # increase size + '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', + # decrease size + '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(10);''', + ], sqls) + + @postgres_fixture() + def test_alter_size_with_default(self): new_state = self.set_up_test_model('test') operations = [ migrations.AddField( @@ -48,12 +81,12 @@ def test_alter_size(self): migrations.AlterField( model_name='Pony', name='name', - field=models.CharField(max_length=20, verbose_name='name'), + field=models.CharField(max_length=20, verbose_name='name', default=''), ), migrations.AlterField( model_name='Pony', name='name', - field=models.CharField(max_length=10, verbose_name='name'), + field=models.CharField(max_length=10, verbose_name='name', default=''), ), ] @@ -61,9 +94,15 @@ def test_alter_size(self): self.apply_operations('test', new_state, operations) self.assertEqual([ + # add column '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', + # increase size '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', - '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(10);''', + # decrease size + '''ALTER TABLE "test_pony" ADD COLUMN "name_tmp" varchar(10) DEFAULT '' NOT NULL;''', + '''UPDATE test_pony SET "name_tmp" = "name" WHERE "name" IS NOT NULL;''', + '''ALTER TABLE test_pony DROP COLUMN "name" CASCADE;''', + '''ALTER TABLE test_pony RENAME COLUMN "name_tmp" TO "name";''', ], sqls) @postgres_fixture()