From 7c4aa201e3a00722abfef29284cec391d20f16e6 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Tue, 15 Feb 2022 23:47:29 +0000 Subject: [PATCH 01/14] hotfix: django contenttypes migration --- django_redshift_backend/base.py | 38 ++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 33cf1d8..4539bf6 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -25,7 +25,7 @@ DatabaseIntrospection as BasePGDatabaseIntrospection, ) from django.db.models import Index -from django.db.utils import NotSupportedError +from django.db.utils import NotSupportedError, ProgrammingError from django_redshift_backend.distkey import DistKey @@ -552,6 +552,42 @@ def quoted_column_name(field_name): return " ".join(create_options) + def remove_field(self, model, field): + """ + This customization will drop the SORTKEY if the `ProgrammingError` exception + with 'cannot drop sortkey' is raised for the 'django_content_type' table + migration. + + Django's ContentType.name field was specified as ordering and was used for + SORTKEY on Redshift. A columns used for SORTKEY could not be dropped, so the + ProgrammingError exception was raised. This customization will allow us to drop + Django's ContentType.name. + + This is not strictly correct, but since Django's migration does not keep track + of ordering changes, there is no other way to unconditionally remove SORTKEY. + """ + try: + super().remove_field(model, field) + except ProgrammingError as e: + if 'cannot drop sortkey' not in str(e): + raise + if model._meta.db_table != 'django_content_type': + # https://github.com/jazzband/django-redshift-backend/issues/37 + raise + + # Reset connection if required + if self.connection.errors_occurred: + self.connection.close() + self.connection.connect() + + # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html + self.execute( + 'ALTER TABLE %(table)s ALTER SORTKEY NONE;' % { + "table": self.quote_name(model._meta.db_table), + } + ) + super().remove_field(model, field) + redshift_data_types = { "AutoField": "integer identity(1, 1)", From c6a7b50db952e1b2bde81215820123c70fe59fa5 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Wed, 16 Feb 2022 10:45:09 +0000 Subject: [PATCH 02/14] add SortKey class for sortkey --- CHANGES.rst | 16 +++++++++ django_redshift_backend/__init__.py | 2 ++ django_redshift_backend/base.py | 27 +++++++------- django_redshift_backend/distkey.py | 20 ++--------- django_redshift_backend/meta.py | 45 ++++++++++++++++++++++++ doc/refs.rst | 13 ++++--- examples/proj1/testapp/models.py | 4 +-- tests/testapp/migrations/0001_initial.py | 6 ++-- tests/testapp/models.py | 4 +-- 9 files changed, 94 insertions(+), 43 deletions(-) create mode 100644 django_redshift_backend/meta.py diff --git a/CHANGES.rst b/CHANGES.rst index 85beb92..88142ac 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -9,11 +9,27 @@ General: * #87 Drop py2 wheel tag from release package file. * Add `CODE_OF_CONDUCT.rst` The linked text which has been referred to from CONTRIBUTING.rst is now included. +Incompatible Changes: + +* #??: To specify SORTKEY for Redshift, you must use `django_redshift_backend.SortKey` for + `Model.Meta.ordering` instead of bearer string. + + **IMPORTANT**: + With this change, existing migration files that specify ordering are not affected. + If you want to apply SortKey to your migration files, please comment out the ordering option once and run + makemigrations, then comment in the ordering option and run makemigrations again. + +* #??: `django_redshift_backend.distkey.DistKey` is moved to `django_redshift_backend.DistKey`. + However old name is still supported for a compatibility. Bug Fixes: * #92, #93: since django-3.0 sqlmigrate (and migrate) does not work. * #90, #13: fix database inspection capability with `manage.py inspectdb`. Thanks to Matt Fisher. +* #37: fix Django `contenttype` migration that cause `ProgrammingError: cannot drop sortkey column + "name"` exception. +* #64: fix Django `auth` migration that cause `NotSupportedError: column "content_type__app_label" + specified as distkey/sortkey is not in the table "auth_permission"` exception. Features: diff --git a/django_redshift_backend/__init__.py b/django_redshift_backend/__init__.py index 8debadf..3a419b8 100644 --- a/django_redshift_backend/__init__.py +++ b/django_redshift_backend/__init__.py @@ -12,3 +12,5 @@ except DistributionNotFound: # package is not installed pass + +from django_redshift_backend.meta import DistKey, SortKey # noqa diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 4539bf6..bdcd48d 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -27,7 +27,7 @@ from django.db.models import Index from django.db.utils import NotSupportedError, ProgrammingError -from django_redshift_backend.distkey import DistKey +from django_redshift_backend.meta import DistKey, SortKey logger = logging.getLogger('django.db.backends') @@ -542,13 +542,14 @@ def quoted_column_name(field_name): create_options.append("DISTKEY({})".format(normalized_field)) # TODO: Support DISTSTYLE ALL. - if model._meta.ordering: - normalized_fields = [ - quoted_column_name(field) - for field in model._meta.ordering - ] + sortkeys = [ + quoted_column_name(field) + for field in model._meta.ordering + if isinstance(field, SortKey) + ] + if sortkeys: create_options.append("SORTKEY({fields})".format( - fields=', '.join(normalized_fields))) + fields=', '.join(sortkeys))) return " ".join(create_options) @@ -558,10 +559,10 @@ def remove_field(self, model, field): with 'cannot drop sortkey' is raised for the 'django_content_type' table migration. - Django's ContentType.name field was specified as ordering and was used for - SORTKEY on Redshift. A columns used for SORTKEY could not be dropped, so the - ProgrammingError exception was raised. This customization will allow us to drop - Django's ContentType.name. + Especially, django's ContentType.name field was specified as ordering and was + used for SORTKEY on Redshift. A columns used for SORTKEY could not be dropped, + so the ProgrammingError exception was raised. This customization will allow us + to drop Django's ContentType.name. This is not strictly correct, but since Django's migration does not keep track of ordering changes, there is no other way to unconditionally remove SORTKEY. @@ -569,11 +570,9 @@ def remove_field(self, model, field): try: super().remove_field(model, field) except ProgrammingError as e: + # https://github.com/jazzband/django-redshift-backend/issues/37 if 'cannot drop sortkey' not in str(e): raise - if model._meta.db_table != 'django_content_type': - # https://github.com/jazzband/django-redshift-backend/issues/37 - raise # Reset connection if required if self.connection.errors_occurred: diff --git a/django_redshift_backend/distkey.py b/django_redshift_backend/distkey.py index ed89e6e..241409c 100644 --- a/django_redshift_backend/distkey.py +++ b/django_redshift_backend/distkey.py @@ -1,17 +1,3 @@ -from __future__ import absolute_import - -from django.db.models import Index - - -class DistKey(Index): - """A single-field index denoting the distkey for a model. - - Use as follows: - - class MyModel(models.Model): - ... - - class Meta: - indexes = [DistKey(fields=['customer_id'])] - """ - pass +# flake8: noqa +# for backward compatibility before 3.0.0 +from .meta import DistKey diff --git a/django_redshift_backend/meta.py b/django_redshift_backend/meta.py new file mode 100644 index 0000000..b0744dc --- /dev/null +++ b/django_redshift_backend/meta.py @@ -0,0 +1,45 @@ +from django.db.models import Index + + +class DistKey(Index): + """A single-field index denoting the distkey for a model. + + Use as follows: + + class MyModel(models.Model): + ... + + class Meta: + indexes = [DistKey(fields=['customer_id'])] + """ + def deconstruct(self): + path, expressions, kwargs = super().deconstruct() + path = path.replace('django_redshift_backend.meta', 'django_redshift_backend') + return (path, expressions, kwargs) + + +class SortKey(str): + """A SORTKEY in Redshift, also valid as ordering in Django. + + https://docs.djangoproject.com/en/dev/ref/models/options/#django.db.models.Options.ordering + + Use as follows: + + class MyModel(models.Model): + ... + + class Meta: + ordering = [SortKey('created_at'), SortKey('-id')] + """ + def __hash__(self): + return hash(str(self)) + + def deconstruct(self): + path = '%s.%s' % (self.__class__.__module__, self.__class__.__name__) + path = path.replace('django_redshift_backend.meta', 'django_redshift_backend') + return (path, [str(self)], {}) + + def __eq__(self, other): + if self.__class__ == other.__class__: + return self.deconstruct() == other.deconstruct() + return NotImplemented diff --git a/doc/refs.rst b/doc/refs.rst index 225e549..51ae9bb 100644 --- a/doc/refs.rst +++ b/doc/refs.rst @@ -92,13 +92,16 @@ Django Models Using sortkey ------------- -There is built-in support for this option for Django >= 1.9. To use `sortkey`, simply define an `ordering` on the model meta as follow:: +There is built-in support for this option for Django >= 1.9. To use `sortkey`, define an `ordering` on the model +meta with the custom sortkey type `django_redshift_backend.SortKey` as follow:: class MyModel(models.Model): ... class Meta: - ordering = ['col2'] + ordering = [SortKey('col2')] + +`SortKey` in `ordering` are also valid as ordering in Django. N.B.: there is no validation of this option, instead we let Redshift validate it for you. Be sure to refer to the `documentation `_. @@ -106,7 +109,7 @@ Using distkey ------------- There is built-in support for this option for Django >= 1.11. To use `distkey`, define an index on the model -meta with the custom index type `django_redshift_backend.distkey.DistKey` with `fields` naming a single field:: +meta with the custom index type `django_redshift_backend.DistKey` with `fields` naming a single field:: class MyModel(models.Model): ... @@ -143,7 +146,7 @@ That is, to go from:: ... migrations.AddIndex( model_name='facttable', - index=django_redshift_backend.distkey.DistKey(fields=['distkeycol'], name='...'), + index=django_redshift_backend.DistKey(fields=['distkeycol'], name='...'), ), ] @@ -160,7 +163,7 @@ To:: ... ], options={ - 'indexes': [django_redshift_backend.distkey.DistKey(fields=['distkeycol'], name='...')], + 'indexes': [django_redshift_backend.DistKey(fields=['distkeycol'], name='...')], }, ), ... diff --git a/examples/proj1/testapp/models.py b/examples/proj1/testapp/models.py index cda3683..5e3bf3b 100644 --- a/examples/proj1/testapp/models.py +++ b/examples/proj1/testapp/models.py @@ -2,7 +2,7 @@ from django.db import models -from django_redshift_backend.base import DistKey +from django_redshift_backend.base import DistKey, SortKey class TestModel(models.Model): @@ -23,7 +23,7 @@ class TestModelWithMetaKeys(models.Model): class Meta: indexes = [DistKey(fields=['fk'])] - ordering = ['created_at', '-id'] + ordering = [SortKey('created_at'), SortKey('-id')] class TestParentModel(models.Model): diff --git a/tests/testapp/migrations/0001_initial.py b/tests/testapp/migrations/0001_initial.py index 6e3d97d..7cd105a 100644 --- a/tests/testapp/migrations/0001_initial.py +++ b/tests/testapp/migrations/0001_initial.py @@ -2,7 +2,7 @@ from django.db import migrations, models import django.db.models.deletion -import django_redshift_backend.distkey +import django_redshift_backend class Migration(migrations.Migration): @@ -45,7 +45,7 @@ class Migration(migrations.Migration): ('fk', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, to='testapp.testreferencedmodel')), ], options={ - 'ordering': ['created_at', '-id'], + 'ordering': [django_redshift_backend.SortKey('created_at'), django_redshift_backend.SortKey('-id')], }, ), migrations.CreateModel( @@ -58,6 +58,6 @@ class Migration(migrations.Migration): ), migrations.AddIndex( model_name='testmodelwithmetakeys', - index=django_redshift_backend.distkey.DistKey(fields=['fk'], name='testapp_tes_fk_id_cd99f5_idx'), + index=django_redshift_backend.DistKey(fields=['fk'], name='testapp_tes_fk_id_cd99f5_idx'), ), ] diff --git a/tests/testapp/models.py b/tests/testapp/models.py index 42dd59f..945c0fe 100644 --- a/tests/testapp/models.py +++ b/tests/testapp/models.py @@ -2,7 +2,7 @@ from django.db import models -from django_redshift_backend.base import DistKey +from django_redshift_backend.base import DistKey, SortKey class TestModel(models.Model): @@ -23,7 +23,7 @@ class TestModelWithMetaKeys(models.Model): class Meta: indexes = [DistKey(fields=['fk'])] - ordering = ['created_at', '-id'] + ordering = [SortKey('created_at'), SortKey('-id')] class TestParentModel(models.Model): From 9670bb324ca697fc0f76fba0208d2e81d31fb7d0 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Sat, 19 Feb 2022 21:07:29 +0000 Subject: [PATCH 03/14] add tests. there are wrong behaviors --- django_redshift_backend/base.py | 86 ++++++++++++++++++++++- tests/test_migrations.py | 116 ++++++++++++++++++++++++++++++++ 2 files changed, 199 insertions(+), 3 deletions(-) create mode 100644 tests/test_migrations.py diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index bdcd48d..b6dd1e7 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -7,6 +7,7 @@ from copy import deepcopy import re +import sys import uuid import logging @@ -44,6 +45,7 @@ class DatabaseFeatures(BasePGDatabaseFeatures): allows_group_by_selected_pks = False has_native_uuid_field = False supports_aggregate_filter_clause = False + can_rollback_ddl = False class DatabaseOperations(BasePGDatabaseOperations): @@ -245,8 +247,22 @@ def add_field(self, model, field): return self.create_model(field.remote_field.through) # Get the column's definition - # ## To add column to existent table on Redshift, field.null must be allowed - field.null = True + if not field.null: + # ## WARNING: Redshift Can't ADD COLUMN with NOT NULL. + # https://github.com/jazzband/django-redshift-backend/issues/96 + # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html + # To add column to existent table on Redshift, field.null must be allowed + msg = '\n'.join([ + "!!! WARNING: ADD COLUMN WITH NULLABLE INSTEAD OF NOT NULL !!!", + "ref: https://github.com/jazzband/django-redshift-backend/issues/96", + "Field {n}:", + " expect: null={n.null}, type={ntype}", + " actual: null=True, type={ntype}", + ]) + msgfmt = dict(n=field, ntype=type(field)) + logger.warning(msg.format(**msgfmt)) + sys.stdout.write('\n\n' + msg.format(**msgfmt) + '\n\n') + field.null = True # Redshift can't ADD COLUMN with NOT NULL definition, params = self.column_sql(model, field, include_default=True) # It might not actually have a column behind it if definition is None: @@ -364,8 +380,55 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, new_default is not None and not self.skip_default(new_field) ) + # column type changed (NG pattern) + if (old_type != new_type and ( + old_field.unique or new_field.unique or + old_field.primary_key or new_field.primary_key or + old_field.is_relation or new_field.is_relation)): + # ## WARNING: Redshift Can't ADD/ALTER/DROP COLUMN with UNIQUE, PRIMARY KEY or FOREIGN KEY. + # https://github.com/jazzband/django-redshift-backend/issues/96 + # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html + fragment, other_actions = self._alter_column_type_sql( + model, old_field, new_field, new_type + ) + sql = self.sql_alter_column % { + "table": self.quote_name(model._meta.db_table), + "changes": fragment[0], + } + msg = '\n'.join([ + "!!! WARNING: SKIPPED ALTER COLUMN SIZE WITH PRIMARY KEY, UNIQUE, FOREIGN KEY !!!", + "ref: https://github.com/jazzband/django-redshift-backend/issues/96", + "Field {n}:", + " from: primary_key={o.primary_key}, unique={o.unique}, is_relation={o.is_relation}, type={otype}", + " to: primary_key={n.primary_key}, unique={n.unique}, is_relation={n.is_relation}, type={ntype}", + "SQL:", + " expect: {sql!r}", + " actual: SKIPPED", + ]) + msgfmt = dict(sql=sql, o=old_field, n=new_field, otype=old_type, ntype=new_type) + logger.warning(msg.format(**msgfmt)) + sys.stdout.write('\n\n' + msg.format(**msgfmt) + '\n\n') + + # Size is changed + elif (type(old_field) == type(new_field) and + old_field.max_length is not None and + new_field.max_length is not None): + # if shrink size as `old_field.max_length > new_field.max_length` and + # larger data in database, this change will raise exception. + fragment, other_actions = self._alter_column_type_sql( + model, old_field, new_field, new_type + ) + actions.append(( + self.sql_alter_column % { + "table": self.quote_name(model._meta.db_table), + "changes": fragment[0], + }, + fragment[1] + )) + # Type or default is changed? - if (old_type != new_type) or needs_database_default: + elif (old_type != new_type) or needs_database_default: + breakpoint() # ## To change column type or default, We need this migration sequence: # ## # ## 1. Add new column with temporary name @@ -374,6 +437,23 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, # ## 4. Rename temporary column name to original column name # ## ALTER TABLE ADD COLUMN 'tmp' DEFAULT + if (not new_field.null and new_default is None): + # ## WARNING: Redshift Can't ADD COLUMN with NOT NULL. + # https://github.com/jazzband/django-redshift-backend/issues/96 + # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html + # To add column to existent table on Redshift, field.null must be allowed + msg = '\n'.join([ + "!!! WARNING: ADD COLUMN WITH NULLABLE INSTEAD OF NOT NULL !!!", + "ref: https://github.com/jazzband/django-redshift-backend/issues/96", + "Field {n}:", + " expect: null={n.null}, type={ntype}", + " actual: null=True, type={ntype}", + ]) + msgfmt = dict(o=old_field, n=new_field, otype=old_type, ntype=new_type) + logger.warning(msg.format(**msgfmt)) + sys.stdout.write('\n\n' + msg.format(**msgfmt) + '\n\n') + new_field.null = True # Redshift can't ADD COLUMN with NOT NUL without default + definition, params = self.column_sql(model, new_field, include_default=True) new_defaults = [new_default] if new_default is not None else [] actions.append(( diff --git a/tests/test_migrations.py b/tests/test_migrations.py new file mode 100644 index 0000000..3a0a712 --- /dev/null +++ b/tests/test_migrations.py @@ -0,0 +1,116 @@ +import os +from unittest import mock + +from django.db import connection, migrations, models +import pytest + +from django_redshift_backend.base import BasePGDatabaseWrapper +from test_base import OperationTestBase + + +@pytest.mark.skipif(not os.environ.get('TEST_WITH_POSTGRES'), + reason='to run, TEST_WITH_POSTGRES=1 tox') +class MigrationTests(OperationTestBase): + available_apps = ["testapp"] + databases = {'default'} + + def tearDown(self): + self.cleanup_test_tables() + # super().tearDown() # disabled: DELETE from django_migrations + + @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: '') + def test_alter_size(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'), + ), + migrations.AlterField( + model_name='Pony', + name='name', + field=models.CharField(max_length=20, verbose_name='name'), + ), + migrations.AlterField( + model_name='Pony', + name='name', + field=models.CharField(max_length=10, verbose_name='name'), + ), + ] + sqls = self.apply_operations_and_collect_sql('test', new_state, operations) + print('\n'.join(sqls)) + sqls = [s for s in sqls if not s.startswith('--')] + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) NULL;''', + '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', + '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(10);''', + ], sqls) + + @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: '') + # def test_add_notnull_without_default_change_to_nullable(self): + def test_add_notnull_without_default_raise_exception(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=False), + ), + ] + # sqls = self.apply_operations_and_collect_sql('test', new_state, operations) + # print('\n'.join(sqls)) + # sqls = [s for s in sqls if not s.startswith('--')] + # self.assertIn('''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) NULL;''', sqls) + with self.assertRaises(RuntimeError): + self.apply_operations_and_collect_sql('test', new_state, operations) + + @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: '') + def test_add_notnull_with_default(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=False, default=''), + ), + ] + sqls = self.apply_operations_and_collect_sql('test', new_state, operations) + print('\n'.join(sqls)) + sqls = [s for s in sqls if not s.startswith('--')] + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', + ], sqls) + + @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: '') + def test_alter_notnull_to_nullable(self): + # https://github.com/jazzband/django-redshift-backend/issues/63 + new_state = self.set_up_test_model('test') + operations = [ + migrations.AlterField( + model_name='Pony', + name='weight', + field=models.FloatField(null=True), + ), + ] + sqls = self.apply_operations_and_collect_sql('test', new_state, operations) + print('\n'.join(sqls)) + sqls = [s for s in sqls if not s.startswith('--')] + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "name_tmp" varchar(10) NULL;''', + '''UPDATE "test_pony" SET "name_tmp"="name";''', + '''ALTER TABLE "test_pony" DROP COLUMN "name";''', + '''ALTER TABLE "test_pony" RENAME COLUMN "name_tmp" TO "name";''', + ], sqls) + + def apply_operations_and_collect_sql(self, app_label, project_state, operations, atomic=True): + from django.db.migrations.migration import Migration + migration = Migration('name', app_label) + migration.operations = operations + with connection.schema_editor(collect_sql=True, atomic=atomic) as editor: + migration.apply(project_state, editor, collect_sql=True) + return editor.collected_sql From 7eee8f15c5573fafbc656dfe94158bf69276abe9 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Sat, 19 Feb 2022 22:31:13 +0000 Subject: [PATCH 04/14] remove: NOT NULL column add NULLABLE implicitly. and now doesn't support can_rollback_ddl. --- CHANGES.rst | 8 +++++ django_redshift_backend/base.py | 43 +++++----------------- tests/test_migrations.py | 63 ++++++++++++++++++++++++++++----- 3 files changed, 71 insertions(+), 43 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 88142ac..0c2eb3d 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -22,6 +22,14 @@ Incompatible Changes: * #??: `django_redshift_backend.distkey.DistKey` is moved to `django_redshift_backend.DistKey`. However old name is still supported for a compatibility. +* #??: Now django-redshift-backend doesn't support `can_rollback_ddl`. + Originally, Redshift did not support column name changes within a transaction. + Please refer https://github.com/jazzband/django-redshift-backend/issues/96 + +* #??: Changed the behavior of implicit NOT NULL column addition: When adding a NOT NULL column, it was + implicitly changed to allow NULL, but it is now fixed to raise a ProgrammingError exception without + implicitly changing it. Adding NOT NULL with no default should be an error. + Bug Fixes: * #92, #93: since django-3.0 sqlmigrate (and migrate) does not work. diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index b6dd1e7..67fa279 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -247,22 +247,6 @@ def add_field(self, model, field): return self.create_model(field.remote_field.through) # Get the column's definition - if not field.null: - # ## WARNING: Redshift Can't ADD COLUMN with NOT NULL. - # https://github.com/jazzband/django-redshift-backend/issues/96 - # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html - # To add column to existent table on Redshift, field.null must be allowed - msg = '\n'.join([ - "!!! WARNING: ADD COLUMN WITH NULLABLE INSTEAD OF NOT NULL !!!", - "ref: https://github.com/jazzband/django-redshift-backend/issues/96", - "Field {n}:", - " expect: null={n.null}, type={ntype}", - " actual: null=True, type={ntype}", - ]) - msgfmt = dict(n=field, ntype=type(field)) - logger.warning(msg.format(**msgfmt)) - sys.stdout.write('\n\n' + msg.format(**msgfmt) + '\n\n') - field.null = True # Redshift can't ADD COLUMN with NOT NULL definition, params = self.column_sql(model, field, include_default=True) # It might not actually have a column behind it if definition is None: @@ -277,6 +261,12 @@ def add_field(self, model, field): "column": self.quote_name(field.column), "definition": definition, } + # ## Redshift + if not field.null and self.effective_default(field) is None: + # Redshift Can't add NOT NULL column without DEFAULT. + # https://github.com/jazzband/django-redshift-backend/issues/96 + # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html + raise ProgrammingError(sql % params) self.execute(sql, params) # ## original BasePGDatabaseSchemaEditor.add_field drop default here @@ -412,7 +402,8 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, # Size is changed elif (type(old_field) == type(new_field) and old_field.max_length is not None and - new_field.max_length is not None): + new_field.max_length is not None and + old_field.max_length != new_field.max_length): # if shrink size as `old_field.max_length > new_field.max_length` and # larger data in database, this change will raise exception. fragment, other_actions = self._alter_column_type_sql( @@ -428,7 +419,6 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, # Type or default is changed? elif (old_type != new_type) or needs_database_default: - breakpoint() # ## To change column type or default, We need this migration sequence: # ## # ## 1. Add new column with temporary name @@ -437,23 +427,6 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, # ## 4. Rename temporary column name to original column name # ## ALTER TABLE
ADD COLUMN 'tmp' DEFAULT - if (not new_field.null and new_default is None): - # ## WARNING: Redshift Can't ADD COLUMN with NOT NULL. - # https://github.com/jazzband/django-redshift-backend/issues/96 - # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html - # To add column to existent table on Redshift, field.null must be allowed - msg = '\n'.join([ - "!!! WARNING: ADD COLUMN WITH NULLABLE INSTEAD OF NOT NULL !!!", - "ref: https://github.com/jazzband/django-redshift-backend/issues/96", - "Field {n}:", - " expect: null={n.null}, type={ntype}", - " actual: null=True, type={ntype}", - ]) - msgfmt = dict(o=old_field, n=new_field, otype=old_type, ntype=new_type) - logger.warning(msg.format(**msgfmt)) - sys.stdout.write('\n\n' + msg.format(**msgfmt) + '\n\n') - new_field.null = True # Redshift can't ADD COLUMN with NOT NUL without default - definition, params = self.column_sql(model, new_field, include_default=True) new_defaults = [new_default] if new_default is not None else [] actions.append(( diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 3a0a712..9ae89e8 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -26,7 +26,7 @@ def test_alter_size(self): migrations.AddField( model_name='Pony', name='name', - field=models.CharField(max_length=10, verbose_name='name'), + field=models.CharField(max_length=10, verbose_name='name', default=''), ), migrations.AlterField( model_name='Pony', @@ -43,7 +43,7 @@ def test_alter_size(self): print('\n'.join(sqls)) sqls = [s for s in sqls if not s.startswith('--')] self.assertEqual([ - '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) NULL;''', + '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(10);''', ], sqls) @@ -52,6 +52,7 @@ def test_alter_size(self): @mock.patch('django_redshift_backend.base.DatabaseSchemaEditor._get_create_options', lambda self, model: '') # def test_add_notnull_without_default_change_to_nullable(self): def test_add_notnull_without_default_raise_exception(self): + from django.db.utils import ProgrammingError new_state = self.set_up_test_model('test') operations = [ migrations.AddField( @@ -60,12 +61,9 @@ def test_add_notnull_without_default_raise_exception(self): field=models.CharField(max_length=10, verbose_name='name', null=False), ), ] - # sqls = self.apply_operations_and_collect_sql('test', new_state, operations) - # print('\n'.join(sqls)) - # sqls = [s for s in sqls if not s.startswith('--')] - # self.assertIn('''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) NULL;''', sqls) - with self.assertRaises(RuntimeError): - self.apply_operations_and_collect_sql('test', new_state, operations) + with self.assertRaises(ProgrammingError): + sqls = self.apply_operations_and_collect_sql('test', new_state, operations) + print(sqls) # for debug @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: '') @@ -85,6 +83,55 @@ def test_add_notnull_with_default(self): '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', ], sqls) + @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: '') + def test_alter_type(self): + new_state = self.set_up_test_model('test') + operations = [ + migrations.AlterField( + model_name='Pony', + name='weight', + field=models.CharField(max_length=10, null=False, default=''), + ), + ] + sqls = self.apply_operations_and_collect_sql('test', new_state, operations) + print('\n'.join(sqls)) + sqls = [s for s in sqls if not s.startswith('--')] + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "weight_tmp" varchar(10) DEFAULT '' NOT NULL;''', + '''UPDATE test_pony SET "weight_tmp" = "weight";''', + '''ALTER TABLE test_pony DROP COLUMN "weight" CASCADE;''', + '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', + ], sqls) + + @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: '') + def test_change_default(self): + # https://github.com/jazzband/django-redshift-backend/issues/63 + 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=False, default=''), + ), + migrations.AlterField( + model_name='Pony', + name='name', + field=models.CharField(max_length=10, verbose_name='name', null=False, default='blink'), + ), + ] + sqls = self.apply_operations_and_collect_sql('test', new_state, operations) + print('\n'.join(sqls)) + sqls = [s for s in sqls if not s.startswith('--')] + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', + '''ALTER TABLE "test_pony" ADD COLUMN "name_tmp" varchar(10) DEFAULT 'blink' NOT NULL;''', + '''UPDATE test_pony SET "name_tmp" = "name";''', + '''ALTER TABLE test_pony DROP COLUMN "name" CASCADE;''', + '''ALTER TABLE test_pony RENAME COLUMN "name_tmp" TO "name";''', + ], sqls) + @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: '') def test_alter_notnull_to_nullable(self): From a5d976ecbcd82724a4ad843e5ef6bf7efd658dc9 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Tue, 22 Feb 2022 14:26:54 +0000 Subject: [PATCH 05/14] update DatabaseSchemaEditor._alter_field bsaed from 3.2.x --- CHANGES.rst | 1 + django_redshift_backend/base.py | 537 +++++++++++++++++++++----------- tests/test_migrations.py | 39 ++- 3 files changed, 391 insertions(+), 186 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 0c2eb3d..0ca0d91 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -43,6 +43,7 @@ Features: * #82 Add Python-3.10 support. * #82 Drop Django-3.0 support. +* #63 Support changing a field from NOT NULL to NULL on migrate / sqlmigrate. 2.1.0 (2021/09/23) ------------------ diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 67fa279..8bf0dba 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -15,7 +15,9 @@ from django.conf import settings from django.core.exceptions import FieldDoesNotExist from django.db.backends.base.introspection import FieldInfo +from django.db.backends.base.schema import _is_relevant_relation from django.db.backends.base.validation import BaseDatabaseValidation +from django.db.backends.ddl_references import Statement from django.db.backends.postgresql.base import ( DatabaseFeatures as BasePGDatabaseFeatures, DatabaseWrapper as BasePGDatabaseWrapper, @@ -46,6 +48,7 @@ class DatabaseFeatures(BasePGDatabaseFeatures): has_native_uuid_field = False supports_aggregate_filter_clause = False can_rollback_ddl = False + supports_combined_alters = False # since django-1.8 class DatabaseOperations(BasePGDatabaseOperations): @@ -122,6 +125,9 @@ def _related_non_m2m_objects(old_field, new_field): class DatabaseSchemaEditor(BasePGDatabaseSchemaEditor): sql_create_table = "CREATE TABLE %(table)s (%(definition)s) %(options)s" + if django.VERSION < (3,): + # to remove "USING %(column)s::%(type)s" + sql_alter_column_type = "ALTER COLUMN %(column)s TYPE %(type)s" @property def multiply_varchar_length(self): @@ -285,39 +291,52 @@ def add_field(self, model, field): if self.connection.features.connection_persists_old_columns: self.connection.close() + # BASED FROM https://github.com/django/django/blob/3.2.12/django/db/backends/base/schema.py#L611-L864 def _alter_field(self, model, old_field, new_field, old_type, new_type, old_db_params, new_db_params, strict=False): - """Actually perform a "physical" (non-ManyToMany) field update.""" - + """Perform a "physical" (non-ManyToMany) field update.""" # Drop any FK constraints, we'll remake them later fks_dropped = set() - if old_field.remote_field and old_field.db_constraint: + if ( + self.connection.features.supports_foreign_keys and + old_field.remote_field and + old_field.db_constraint + ): fk_names = self._constraint_names(model, [old_field.column], foreign_key=True) if strict and len(fk_names) != 1: - raise ValueError( - "Found wrong number (%s) of foreign key constraints for %s.%s" % - (len(fk_names), model._meta.db_table, old_field.column)) + raise ValueError("Found wrong number (%s) of foreign key constraints for %s.%s" % ( + len(fk_names), + model._meta.db_table, + old_field.column, + )) for fk_name in fk_names: fks_dropped.add((old_field.column,)) - self.execute(self._delete_constraint_sql( - self.sql_delete_fk, model, fk_name)) + self.execute(self._delete_fk_sql(model, fk_name)) # Has unique been removed? - if (old_field.unique and - (not new_field.unique or - (not old_field.primary_key and new_field.primary_key))): + if old_field.unique and (not new_field.unique or self._field_became_primary_key(old_field, new_field)): # Find the unique constraint for this field + meta_constraint_names = {constraint.name for constraint in model._meta.constraints} constraint_names = self._constraint_names( - model, [old_field.column], unique=True) + model, [old_field.column], unique=True, primary_key=False, + exclude=meta_constraint_names, + ) if strict and len(constraint_names) != 1: - raise ValueError( - "Found wrong number (%s) of unique constraints for %s.%s" % - (len(constraint_names), model._meta.db_table, old_field.column)) + raise ValueError("Found wrong number (%s) of unique constraints for %s.%s" % ( + len(constraint_names), + model._meta.db_table, + old_field.column, + )) for constraint_name in constraint_names: - self.execute(self._delete_constraint_sql( - self.sql_delete_unique, model, constraint_name)) - # Drop incoming FK constraints if we're a primary key and things are going - # to change. - if old_field.primary_key and new_field.primary_key and old_type != new_type: + self.execute(self._delete_unique_sql(model, constraint_name)) + # Drop incoming FK constraints if the field is a primary key or unique, + # which might be a to_field target, and things are going to change. + drop_foreign_keys = ( + self.connection.features.supports_foreign_keys and ( + (old_field.primary_key and new_field.primary_key) or + (old_field.unique and new_field.unique) + ) and old_type != new_type + ) + if drop_foreign_keys: # '_meta.related_field' also contains M2M reverse fields, these # will be filtered out for _old_rel, new_rel in _related_non_m2m_objects(old_field, new_field): @@ -325,43 +344,319 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, new_rel.related_model, [new_rel.field.column], foreign_key=True ) for fk_name in rel_fk_names: - self.execute(self._delete_constraint_sql( - self.sql_delete_fk, new_rel.related_model, fk_name)) + self.execute(self._delete_fk_sql(new_rel.related_model, fk_name)) # Removed an index? (no strict check, as multiple indexes are possible) - if (old_field.db_index and - not new_field.db_index and - not old_field.unique and - not (not new_field.unique and old_field.unique)): + # Remove indexes if db_index switched to False or a unique constraint + # will now be used in lieu of an index. The following lines from the + # truth table show all True cases; the rest are False: + # + # old_field.db_index | old_field.unique | new_field.db_index | new_field.unique + # ------------------------------------------------------------------------------ + # True | False | False | False + # True | False | False | True + # True | False | True | True + if old_field.db_index and not old_field.unique and (not new_field.db_index or new_field.unique): # Find the index for this field - index_names = self._constraint_names(model, [old_field.column], index=True) + meta_index_names = {index.name for index in model._meta.indexes} + # Retrieve only BTREE indexes since this is what's created with + # db_index=True. + index_names = self._constraint_names( + model, [old_field.column], index=True, type_=Index.suffix, + exclude=meta_index_names, + ) for index_name in index_names: - self.execute(self._delete_constraint_sql( - self.sql_delete_index, model, index_name)) + # The only way to check if an index was created with + # db_index=True or with Index(['field'], name='foo') + # is to look at its name (refs #28053). + self.execute(self._delete_index_sql(model, index_name)) # Change check constraints? if old_db_params['check'] != new_db_params['check'] and old_db_params['check']: + meta_constraint_names = {constraint.name for constraint in model._meta.constraints} constraint_names = self._constraint_names( - model, [old_field.column], check=True) + model, [old_field.column], check=True, + exclude=meta_constraint_names, + ) if strict and len(constraint_names) != 1: - raise ValueError( - "Found wrong number (%s) of check constraints for %s.%s" % - (len(constraint_names), model._meta.db_table, old_field.column)) + raise ValueError("Found wrong number (%s) of check constraints for %s.%s" % ( + len(constraint_names), + model._meta.db_table, + old_field.column, + )) for constraint_name in constraint_names: - self.execute(self._delete_constraint_sql( - self.sql_delete_check, model, constraint_name)) + self.execute(self._delete_check_sql(model, constraint_name)) # Have they renamed the column? if old_field.column != new_field.column: - self.execute(self._rename_field_sql( - model._meta.db_table, old_field, new_field, new_type)) + self.execute(self._rename_field_sql(model._meta.db_table, old_field, new_field, new_type)) + # Rename all references to the renamed column. + for sql in self.deferred_sql: + if isinstance(sql, Statement): + sql.rename_column_references(model._meta.db_table, old_field.column, new_field.column) # Next, start accumulating actions to do actions = [] + null_actions = [] post_actions = [] - + # Collation change? + old_collation = getattr(old_field, 'db_collation', None) + new_collation = getattr(new_field, 'db_collation', None) + if old_collation != new_collation: + # Collation change handles also a type change. + fragment = self._alter_column_collation_sql(model, new_field, new_type, new_collation) + actions.append(fragment) + # Type change? + elif old_type != new_type: + fragment, other_actions = self._alter_column_type_sql(model, old_field, new_field, new_type) + if fragment: # ## Redshift: In some case, fragment will be empty. + actions.append(fragment) + post_actions.extend(other_actions) # When changing a column NULL constraint to NOT NULL with a given # default value, we need to perform 4 steps: # 1. Add a default for new incoming writes # 2. Update existing NULL rows with new default # 3. Replace NULL constraint with NOT NULL # 4. Drop the default again. + # Default change? + # ## original do alter ALTER COLUMN SET/DROP DEFAULT. + # ## Redshift Can't: https://github.com/jazzband/django-redshift-backend/issues/96 + # needs_database_default = False + # if old_field.null and not new_field.null: + # old_default = self.effective_default(old_field) + # new_default = self.effective_default(new_field) + # if ( + # not self.skip_default_on_alter(new_field) and + # old_default != new_default and + # new_default is not None + # ): + # needs_database_default = True + # actions.append(self._alter_column_default_sql(model, old_field, new_field)) + + # Nullability change? + if old_field.null != new_field.null: + # ## original BaseDatabaseSchemaEditor._alter_column_null_sql return only a fragment + # fragment = self._alter_column_null_sql(model, old_field, new_field) + # if fragment: + # null_actions.append(fragment) + # ## Redshift use 4 steps null alternation + fragment, other_actions = self._alter_column_null_sqls(model, old_field, new_field) + actions.append(fragment) + null_actions.extend(other_actions) + + # Only if we have a default and there is a change from NULL to NOT NULL + # ## original BaseDatabaseSchemaEditor._alter_table four_way_default_alteration + # four_way_default_alteration = ( + # new_field.has_default() and + # (old_field.null and not new_field.null) + # ) + # ## Redshift is always 4-way, this flag should be False because the null_actions handles it. + four_way_default_alteration = False + new_default = self.effective_default(new_field) # not used, but for flake8 + + if actions or null_actions: + if not four_way_default_alteration: + # If we don't have to do a 4-way default alteration we can + # directly run a (NOT) NULL alteration + actions = actions + null_actions + # Combine actions together if we can (e.g. postgres) + if self.connection.features.supports_combined_alters and actions: + sql, params = tuple(zip(*actions)) + actions = [(", ".join(sql), sum(params, []))] + # Apply those actions + for sql, params in actions: + self.execute( + # ## original assumes only alters, so adding an ALTER TABLE clause + # self.sql_alter_column % { + # "table": self.quote_name(model._meta.db_table), + # "changes": sql, + # }, + # ## Redshift executes ADD and UPDATE on alter, so the sql is a complete sentence + sql, + params, + ) + if four_way_default_alteration: + # Update existing rows with default value + self.execute( + self.sql_update_with_default % { + "table": self.quote_name(model._meta.db_table), + "column": self.quote_name(new_field.column), + "default": "%s", + }, + [new_default], + ) + # Since we didn't run a NOT NULL change before we need to do it + # now + for sql, params in null_actions: + self.execute( + self.sql_alter_column % { + "table": self.quote_name(model._meta.db_table), + "changes": sql, + }, + params, + ) + if post_actions: + for sql, params in post_actions: + self.execute(sql, params) + # If primary_key changed to False, delete the primary key constraint. + if old_field.primary_key and not new_field.primary_key: + self._delete_primary_key(model, strict) + # Added a unique? + if self._unique_should_be_added(old_field, new_field): + self.execute(self._create_unique_sql(model, [new_field.column])) + # Added an index? Add an index if db_index switched to True or a unique + # constraint will no longer be used in lieu of an index. The following + # lines from the truth table show all True cases; the rest are False: + # + # old_field.db_index | old_field.unique | new_field.db_index | new_field.unique + # ------------------------------------------------------------------------------ + # False | False | True | False + # False | True | True | False + # True | True | True | False + + # ## original BasePGDatabaseSchemaEditor._alter_field has CREATE INDEX + # ## Redshift doesn't support it. + # https://docs.aws.amazon.com/redshift/latest/dg/c_unsupported-postgresql-features.html + # if (not old_field.db_index or old_field.unique) and new_field.db_index and not new_field.unique: + # self.execute(self._create_index_sql(model, fields=[new_field])) + + # Type alteration on primary key? Then we need to alter the column + # referring to us. + rels_to_update = [] + if drop_foreign_keys: + rels_to_update.extend(_related_non_m2m_objects(old_field, new_field)) + # Changed to become primary key? + if self._field_became_primary_key(old_field, new_field): + # Make the new one + self.execute(self._create_primary_key_sql(model, new_field)) + # Update all referencing columns + rels_to_update.extend(_related_non_m2m_objects(old_field, new_field)) + # Handle our type alters on the other end of rels from the PK stuff above + for old_rel, new_rel in rels_to_update: + rel_db_params = new_rel.field.db_parameters(connection=self.connection) + rel_type = rel_db_params['type'] + fragment, other_actions = self._alter_column_type_sql( + new_rel.related_model, old_rel.field, new_rel.field, rel_type + ) + self.execute( + self.sql_alter_column % { + "table": self.quote_name(new_rel.related_model._meta.db_table), + "changes": fragment[0], + }, + fragment[1], + ) + for sql, params in other_actions: + self.execute(sql, params) + # Does it have a foreign key? + if (self.connection.features.supports_foreign_keys and new_field.remote_field and + (fks_dropped or not old_field.remote_field or not old_field.db_constraint) and + new_field.db_constraint): + self.execute(self._create_fk_sql(model, new_field, "_fk_%(to_table)s_%(to_column)s")) + # Rebuild FKs that pointed to us if we previously had to drop them + if drop_foreign_keys: + for rel in new_field.model._meta.related_objects: + if _is_relevant_relation(rel, new_field) and rel.field.db_constraint: + self.execute(self._create_fk_sql(rel.related_model, rel.field, "_fk")) + # Does it have check constraints we need to add? + if old_db_params['check'] != new_db_params['check'] and new_db_params['check']: + constraint_name = self._create_index_name(model._meta.db_table, [new_field.column], suffix='_check') + self.execute(self._create_check_sql(model, constraint_name, new_db_params['check'])) + + # ## original BasePGDatabaseSchemaEditor._alter_field drop default here + # ## Redshift doesn't support DROP DEFAULT. + # Drop the default if we need to + # (Django usually does not use in-database defaults) + # if needs_database_default: + # changes_sql, params = self._alter_column_default_sql(model, old_field, new_field, drop=True) + # sql = self.sql_alter_column % { + # "table": self.quote_name(model._meta.db_table), + # "changes": changes_sql, + # } + # self.execute(sql, params) + + # Reset connection if required + if self.connection.features.connection_persists_old_columns: + self.connection.close() + + def _alter_column_with_recreate(self, model, old_field, new_field): + """ + To change column type or default, We need this migration sequence: + + 1. Add new column with temporary name + 2. Migrate values from original column to temprary column + 3. Drop old column + 4. Rename temporary column name to original column name + """ + new_default = self.effective_default(new_field) + + fragment = ('', []) + actions = [] + + # ## ALTER TABLE
ADD COLUMN 'tmp' DEFAULT + definition, params = self.column_sql(model, new_field, include_default=True) + new_defaults = [new_default] if new_default is not None else [] + fragment = ( + self.sql_create_column % { + "table": self.quote_name(model._meta.db_table), + "column": self.quote_name(new_field.column + "_tmp"), + "definition": definition, + }, + new_defaults + ) + # ## UPDATE
SET 'tmp' = + actions.append(( + "UPDATE %(table)s SET %(new_column)s = %(old_column)s" % { + "table": model._meta.db_table, + "new_column": self.quote_name(new_field.column + "_tmp"), + "old_column": self.quote_name(new_field.column), + }, [] + )) + # ## ALTER TABLE
DROP COLUMN + actions.append(( + self.sql_delete_column % { + "table": model._meta.db_table, + "column": self.quote_name(new_field.column), + }, [], + )) + # ## ALTER TABLE
RENAME COLUMN 'tmp' + actions.append(( + self.sql_rename_column % { + "table": model._meta.db_table, + "old_column": self.quote_name(new_field.column + "_tmp"), + "new_column": self.quote_name(new_field.column), + }, [] + )) + + return fragment, actions + + # BASED FROM https://github.com/django/django/blob/3.2.12/django/db/backends/base/schema.py#L866-L886 + # postgres/schema.py doesn't have `_alter_column_null_sql` method. + def _alter_column_null_sqls(self, model, old_field, new_field): + """ + Hook to specialize column null alteration. + + Return a [(sql, params), ...] fragment to set a column to null or non-null + as required by new_field, or None if no changes are required. + """ + # ## original BaseDatabaseSchemaEditor._alter_column_null_sql return a sql fragment + # ## Redshift needs 4 step alter + return self._alter_column_with_recreate(model, old_field, new_field) + + # override to avoid `USING %(column)s::%(type)s` on postgres/schema.py + def _alter_column_type_sql(self, model, old_field, new_field, new_type): + # """ + # Hook to specialize column type alteration for different backends, + # for cases when a creation type is different to an alteration type + # (e.g. SERIAL in PostgreSQL, PostGIS fields). + + # Return a two-tuple of: an SQL fragment of (sql, params) to insert into + # an ALTER TABLE statement and a list of extra (sql, params) tuples to + # run once the field is altered. + # """ + old_db_params = old_field.db_parameters(connection=self.connection) + old_type = old_db_params['type'] + old_default = self.effective_default(old_field) + new_default = self.effective_default(new_field) + + fragment = ('', []) + actions = [] + # Default change? old_default = self.effective_default(old_field) new_default = self.effective_default(new_field) @@ -371,6 +666,10 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, not self.skip_default(new_field) ) # column type changed (NG pattern) + # MEMO: for a future, another procedure to ALTER: + # 1. once remove UNIQUE, PKEY, FK + # 2. alter column + # 3. add UNIQUE, PKEY, FK again if (old_type != new_type and ( old_field.unique or new_field.unique or old_field.primary_key or new_field.primary_key or @@ -378,12 +677,13 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, # ## WARNING: Redshift Can't ADD/ALTER/DROP COLUMN with UNIQUE, PRIMARY KEY or FOREIGN KEY. # https://github.com/jazzband/django-redshift-backend/issues/96 # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html - fragment, other_actions = self._alter_column_type_sql( - model, old_field, new_field, new_type - ) + _fragment = self.sql_alter_column_type % { + "column": self.quote_name(new_field.column), + "type": new_type, + } sql = self.sql_alter_column % { "table": self.quote_name(model._meta.db_table), - "changes": fragment[0], + "changes": _fragment, } msg = '\n'.join([ "!!! WARNING: SKIPPED ALTER COLUMN SIZE WITH PRIMARY KEY, UNIQUE, FOREIGN KEY !!!", @@ -398,6 +698,10 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, msgfmt = dict(sql=sql, o=old_field, n=new_field, otype=old_type, ntype=new_type) logger.warning(msg.format(**msgfmt)) sys.stdout.write('\n\n' + msg.format(**msgfmt) + '\n\n') + # actions.append(['', []]) + # self.sql_alter_column = '' # cancel # FIXME comment and impl + # FIXME: need UnitTest + breakpoint() # Size is changed elif (type(old_field) == type(new_field) and @@ -406,153 +710,22 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, old_field.max_length != new_field.max_length): # if shrink size as `old_field.max_length > new_field.max_length` and # larger data in database, this change will raise exception. - fragment, other_actions = self._alter_column_type_sql( - model, old_field, new_field, new_type - ) - actions.append(( + fragment = ( self.sql_alter_column % { "table": self.quote_name(model._meta.db_table), - "changes": fragment[0], + "changes": self.sql_alter_column_type % { + "column": self.quote_name(new_field.column), + "type": new_type, + } }, - fragment[1] - )) + [] + ) # Type or default is changed? elif (old_type != new_type) or needs_database_default: - # ## To change column type or default, We need this migration sequence: - # ## - # ## 1. Add new column with temporary name - # ## 2. Migrate values from original column to temprary column - # ## 3. Drop old column - # ## 4. Rename temporary column name to original column name - - # ## ALTER TABLE
ADD COLUMN 'tmp' DEFAULT - definition, params = self.column_sql(model, new_field, include_default=True) - new_defaults = [new_default] if new_default is not None else [] - actions.append(( - self.sql_create_column % { - "table": self.quote_name(model._meta.db_table), - "column": self.quote_name(new_field.column + "_tmp"), - "definition": definition, - }, - new_defaults - )) - # ## UPDATE
SET 'tmp' = - actions.append(( - "UPDATE %(table)s SET %(new_column)s = %(old_column)s" % { - "table": model._meta.db_table, - "new_column": self.quote_name(new_field.column + "_tmp"), - "old_column": self.quote_name(new_field.column), - }, [] - )) - # ## ALTER TABLE
DROP COLUMN - actions.append(( - self.sql_delete_column % { - "table": model._meta.db_table, - "column": self.quote_name(new_field.column), - }, [], - )) - # ## ALTER TABLE
RENAME COLUMN 'tmp' - actions.append(( - self.sql_rename_column % { - "table": model._meta.db_table, - "old_column": self.quote_name(new_field.column + "_tmp"), - "new_column": self.quote_name(new_field.column), - }, [] - )) + fragment, actions = self._alter_column_with_recreate(model, old_field, new_field) - # Apply those actions - for sql, params in actions: - self.execute(sql, params) - if post_actions: - for sql, params in post_actions: - self.execute(sql, params) - # Added a unique? - if ((not old_field.unique and new_field.unique) or - (old_field.primary_key and not new_field.primary_key and new_field.unique)): - self.execute(self._create_unique_sql(model, [new_field.column])) - - # ## original BasePGDatabaseSchemaEditor._alter_field has CREATE INDEX - # ## Redshift doesn't support it. - - # Type alteration on primary key? Then we need to alter the column - # referring to us. - rels_to_update = [] - if old_field.primary_key and new_field.primary_key and old_type != new_type: - rels_to_update.extend(_related_non_m2m_objects(old_field, new_field)) - # Changed to become primary key? - # Note that we don't detect unsetting of a PK, as we assume another field - # will always come along and replace it. - if not old_field.primary_key and new_field.primary_key: - # First, drop the old PK - constraint_names = self._constraint_names(model, primary_key=True) - if strict and len(constraint_names) != 1: - raise ValueError("Found wrong number (%s) of PK constraints for %s" % ( - len(constraint_names), - model._meta.db_table, - )) - for constraint_name in constraint_names: - self.execute(self._delete_constraint_sql( - self.sql_delete_pk, model, constraint_name)) - # Make the new one - self.execute( - self.sql_create_pk % { - "table": self.quote_name(model._meta.db_table), - "name": self.quote_name(self._create_index_name( - model, [new_field.column], suffix="_pk")), - "columns": self.quote_name(new_field.column), - } - ) - # Update all referencing columns - rels_to_update.extend(_related_non_m2m_objects(old_field, new_field)) - # Handle our type alters on the other end of rels from the PK stuff above - for old_rel, new_rel in rels_to_update: - rel_db_params = new_rel.field.db_parameters(connection=self.connection) - rel_type = rel_db_params['type'] - fragment, other_actions = self._alter_column_type_sql( - new_rel.related_model._meta.db_table, old_rel.field, new_rel.field, - rel_type - ) - self.execute( - self.sql_alter_column % { - "table": self.quote_name(new_rel.related_model._meta.db_table), - "changes": fragment[0], - }, - fragment[1]) - for sql, params in other_actions: - self.execute(sql, params) - # Does it have a foreign key? - if (new_field.remote_field and - (fks_dropped or - not old_field.remote_field or - not old_field.db_constraint) and - new_field.db_constraint): - self.execute(self._create_fk_sql( - model, new_field, "_fk_%(to_table)s_%(to_column)s")) - # Rebuild FKs that pointed to us if we previously had to drop them - if old_field.primary_key and new_field.primary_key and old_type != new_type: - for rel in new_field.model._meta.related_objects: - if not rel.many_to_many: - self.execute(self._create_fk_sql( - rel.related_model, rel.field, "_fk")) - # Does it have check constraints we need to add? - if old_db_params['check'] != new_db_params['check'] and new_db_params['check']: - self.execute( - self.sql_create_check % { - "table": self.quote_name(model._meta.db_table), - "name": self.quote_name(self._create_index_name( - model, [new_field.column], suffix="_check")), - "column": self.quote_name(new_field.column), - "check": new_db_params['check'], - } - ) - - # ## original BasePGDatabaseSchemaEditor._alter_field drop default here - # ## Redshift doesn't support DROP DEFAULT. - - # Reset connection if required - if self.connection.features.connection_persists_old_columns: - self.connection.close() + return fragment, actions def _get_create_options(self, model): """ diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 9ae89e8..2253af7 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -104,6 +104,37 @@ def test_alter_type(self): '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', ], sqls) + @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: '') + def test_alter_notnull_with_default(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=10, verbose_name='name', null=False, default=''), + ), + ] + sqls = self.apply_operations_and_collect_sql('test', new_state, operations) + print('\n'.join(sqls)) + sqls = [s for s in sqls if not s.startswith('--')] + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) NULL;''', + '''ALTER TABLE "test_pony" ADD COLUMN "name_tmp" varchar(10) DEFAULT '' NOT NULL;''', + '''UPDATE test_pony SET "name_tmp" = "name";''', + '''ALTER TABLE test_pony DROP COLUMN "name" CASCADE;''', + '''ALTER TABLE test_pony RENAME COLUMN "name_tmp" TO "name";''', + ], sqls) + + # ## Django usually does not use in-database defaults + # ## 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') @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: '') def test_change_default(self): @@ -148,10 +179,10 @@ def test_alter_notnull_to_nullable(self): print('\n'.join(sqls)) sqls = [s for s in sqls if not s.startswith('--')] self.assertEqual([ - '''ALTER TABLE "test_pony" ADD COLUMN "name_tmp" varchar(10) NULL;''', - '''UPDATE "test_pony" SET "name_tmp"="name";''', - '''ALTER TABLE "test_pony" DROP COLUMN "name";''', - '''ALTER TABLE "test_pony" RENAME COLUMN "name_tmp" TO "name";''', + '''ALTER TABLE "test_pony" ADD COLUMN "weight_tmp" double precision NULL;''', + '''UPDATE test_pony SET "weight_tmp" = "weight";''', + '''ALTER TABLE test_pony DROP COLUMN "weight" CASCADE;''', + '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', ], sqls) def apply_operations_and_collect_sql(self, app_label, project_state, operations, atomic=True): From 5f9053a3903ef91a52f38298bd41d11e115e24f1 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Wed, 23 Feb 2022 21:34:40 +0000 Subject: [PATCH 06/14] feature: alter type size of unique --- django_redshift_backend/base.py | 134 ++++++++++++++----------- tests/test_migrations.py | 170 ++++++++++++++++++++++++++------ 2 files changed, 219 insertions(+), 85 deletions(-) diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 8bf0dba..9f3e24b 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -7,7 +7,6 @@ from copy import deepcopy import re -import sys import uuid import logging @@ -125,6 +124,8 @@ def _related_non_m2m_objects(old_field, new_field): class DatabaseSchemaEditor(BasePGDatabaseSchemaEditor): sql_create_table = "CREATE TABLE %(table)s (%(definition)s) %(options)s" + sql_delete_fk = "ALTER TABLE %(table)s DROP CONSTRAINT %(name)s" + if django.VERSION < (3,): # to remove "USING %(column)s::%(type)s" sql_alter_column_type = "ALTER COLUMN %(column)s TYPE %(type)s" @@ -534,13 +535,16 @@ def _alter_field(self, model, old_field, new_field, old_type, new_type, fragment, other_actions = self._alter_column_type_sql( new_rel.related_model, old_rel.field, new_rel.field, rel_type ) - self.execute( - self.sql_alter_column % { - "table": self.quote_name(new_rel.related_model._meta.db_table), - "changes": fragment[0], - }, - fragment[1], - ) + # ## original assumes only alters, so adding an ALTER TABLE clause + # self.execute( + # self.sql_alter_column % { + # "table": self.quote_name(new_rel.related_model._meta.db_table), + # "changes": fragment[0], + # }, + # fragment[1], + # ) + # ## Redshift executes ADD and UPDATE on alter, so the fragment[0] is a complete sentence + self.execute(fragment[0], fragment[1]) for sql, params in other_actions: self.execute(sql, params) # Does it have a foreign key? @@ -649,13 +653,11 @@ def _alter_column_type_sql(self, model, old_field, new_field, new_type): # an ALTER TABLE statement and a list of extra (sql, params) tuples to # run once the field is altered. # """ + fragment = None # ('', []) + actions = [] + old_db_params = old_field.db_parameters(connection=self.connection) old_type = old_db_params['type'] - old_default = self.effective_default(old_field) - new_default = self.effective_default(new_field) - - fragment = ('', []) - actions = [] # Default change? old_default = self.effective_default(old_field) @@ -665,52 +667,60 @@ def _alter_column_type_sql(self, model, old_field, new_field, new_type): new_default is not None and not self.skip_default(new_field) ) - # column type changed (NG pattern) - # MEMO: for a future, another procedure to ALTER: - # 1. once remove UNIQUE, PKEY, FK - # 2. alter column - # 3. add UNIQUE, PKEY, FK again - if (old_type != new_type and ( - old_field.unique or new_field.unique or - old_field.primary_key or new_field.primary_key or - old_field.is_relation or new_field.is_relation)): - # ## WARNING: Redshift Can't ADD/ALTER/DROP COLUMN with UNIQUE, PRIMARY KEY or FOREIGN KEY. - # https://github.com/jazzband/django-redshift-backend/issues/96 - # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html - _fragment = self.sql_alter_column_type % { - "column": self.quote_name(new_field.column), - "type": new_type, - } - sql = self.sql_alter_column % { - "table": self.quote_name(model._meta.db_table), - "changes": _fragment, - } - msg = '\n'.join([ - "!!! WARNING: SKIPPED ALTER COLUMN SIZE WITH PRIMARY KEY, UNIQUE, FOREIGN KEY !!!", - "ref: https://github.com/jazzband/django-redshift-backend/issues/96", - "Field {n}:", - " from: primary_key={o.primary_key}, unique={o.unique}, is_relation={o.is_relation}, type={otype}", - " to: primary_key={n.primary_key}, unique={n.unique}, is_relation={n.is_relation}, type={ntype}", - "SQL:", - " expect: {sql!r}", - " actual: SKIPPED", - ]) - msgfmt = dict(sql=sql, o=old_field, n=new_field, otype=old_type, ntype=new_type) - logger.warning(msg.format(**msgfmt)) - sys.stdout.write('\n\n' + msg.format(**msgfmt) + '\n\n') - # actions.append(['', []]) - # self.sql_alter_column = '' # cancel # FIXME comment and impl - # FIXME: need UnitTest - breakpoint() # Size is changed - elif (type(old_field) == type(new_field) and - old_field.max_length is not None and - new_field.max_length is not None and - old_field.max_length != new_field.max_length): + if (type(old_field) == type(new_field) and + old_field.max_length is not None and + new_field.max_length is not None and + old_field.max_length != new_field.max_length): # if shrink size as `old_field.max_length > new_field.max_length` and # larger data in database, this change will raise exception. - fragment = ( + + # ## Redshift can't alter column size for primary key, unique, foreign key + # https://github.com/jazzband/django-redshift-backend/issues/96 + # https://docs.aws.amazon.com/en_us/redshift/latest/dg/r_ALTER_TABLE.html + # another procedure to ALTER: + # 1. once remove UNIQUE (, PKEY, FK) + # 2. alter column + # 3. add UNIQUE (, PKEY, FK) again + + # 0. Find the unique constraint for this field + def get_constraint(model, field, unique=None, primary_key=None, foreign_key=None): + meta_constraint_names = {constraint.name for constraint in model._meta.constraints} + constraint_names = self._constraint_names( + model, [field.column], unique=unique, primary_key=primary_key, foreign_key=foreign_key, + exclude=meta_constraint_names, + ) + if not constraint_names: + constraint_name = None + elif len(constraint_names) == 1: + constraint_name = constraint_names[0] + else: + raise ValueError("Found wrong number (%s) of unique constraints for %s.%s" % ( + len(constraint_names), + model._meta.db_table, + old_field.column, + )) + return constraint_name + + unique_constraint = pk_constraint = fk_constraint = None + if old_field.unique and new_field.unique: + unique_constraint = get_constraint(model, old_field, unique=True, primary_key=False) + elif old_field.primary_key and new_field.primary_key: + pk_constraint = get_constraint(model, old_field, primary_key=True) + elif old_field.is_relation and new_field.is_relation: + fk_constraint = get_constraint(model, old_field, foreign_key=True) + + # 1. once remove UNIQUE, PKEY, FK + if unique_constraint: + actions.append((self._delete_unique_sql(model, unique_constraint), [])) + elif pk_constraint: + actions.append((self._delete_primary_key_sql(model, pk_constraint), [])) + elif fk_constraint: + actions.append((self._delete_fk_sql(model, fk_constraint), [])) + + # 2. alter column + actions.append(( self.sql_alter_column % { "table": self.quote_name(model._meta.db_table), "changes": self.sql_alter_column_type % { @@ -719,12 +729,24 @@ def _alter_column_type_sql(self, model, old_field, new_field, new_type): } }, [] - ) + )) + # 3. add UNIQUE, PKEY, FK again + if unique_constraint: + actions.append((self._create_unique_sql(model, [new_field.column], unique_constraint), [])) + elif pk_constraint: + actions.append((self._create_primary_key_sql(model, pk_constraint), [])) # FIXME + elif fk_constraint: + actions.append((self._create_fk_sql(model, fk_constraint), [])) # FIXME + fragment = actions.pop(0) # Type or default is changed? elif (old_type != new_type) or needs_database_default: fragment, actions = self._alter_column_with_recreate(model, old_field, new_field) + # other case + else: + raise ValueError('django-redshift-backend doesnt support this alter case.') + return fragment, actions def _get_create_options(self, model): diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 2253af7..6cf74c3 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -1,7 +1,9 @@ import os +import contextlib from unittest import mock -from django.db import connection, migrations, models +from django.db import migrations, models +from django.db.migrations.state import ProjectState import pytest from django_redshift_backend.base import BasePGDatabaseWrapper @@ -18,6 +20,24 @@ def tearDown(self): self.cleanup_test_tables() # super().tearDown() # disabled: DELETE from django_migrations + @contextlib.contextmanager + def collect_sql(self): + collected_sql = [] + + def execute(self, sql, params=()): + sql = str(sql) + ending = "" if sql.endswith(";") else ";" + if params is not None: + collected_sql.append((sql % tuple(map(self.quote_value, params))) + ending) + else: + collected_sql.append(sql + ending) + return super(type(self), self).execute(sql, params) + + with mock.patch('django_redshift_backend.base.DatabaseSchemaEditor.execute', execute): + yield collected_sql + + print('\n'.join(collected_sql)) + @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: '') def test_alter_size(self): @@ -39,15 +59,110 @@ def test_alter_size(self): field=models.CharField(max_length=10, verbose_name='name'), ), ] - sqls = self.apply_operations_and_collect_sql('test', new_state, operations) - print('\n'.join(sqls)) - sqls = [s for s in sqls if not s.startswith('--')] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(10);''', ], sqls) + @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: '') + def test_alter_size_for_unique(self): + new_state = self.set_up_test_model('test') + operations = [ + migrations.AddField( + model_name='Pony', + name='name', + field=models.CharField(max_length=10, default='', unique=True), + ), + migrations.AlterField( + model_name='Pony', + name='name', + field=models.CharField(max_length=20, default='', unique=True), + ), + ] + + with self.collect_sql() as sqls: + 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" ALTER COLUMN "name" TYPE varchar(20);''', + '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_key" UNIQUE ("name");''' + ], sqls) + + @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: '') + def test_alter_size_for_pk(self): + setup_operations = [migrations.CreateModel( + 'Pony', + [ + ('name', models.CharField(max_length=10, default='', primary_key=True)), + ], + )] + new_state = self.apply_operations('test', ProjectState(), setup_operations) + + operations = [ + migrations.AlterField( + model_name='Pony', + name='name', + field=models.CharField(max_length=20, default='', primary_key=True), + ), + ] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + + self.assertEqual([ + '''ALTER TABLE "test_pony" DROP CONSTRAINT "test_pony_name_key";''', + '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', + '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_key" UNIQUE ("name");''' + ], sqls) + + @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: '') + def test_alter_size_for_fk(self): + setup_operations = [ + migrations.CreateModel( + 'Pony', + [ + # ('id', models.AutoField(primary_key=True)), + # ('name', models.CharField(max_length=10, unique=True)), + ('id', models.CharField(max_length=10, primary_key=True)), + ], + ), + migrations.CreateModel( + 'Rider', + [ + ('id', models.AutoField(primary_key=True)), + ('pony', models.ForeignKey('Pony', models.CASCADE)), + ], + ), + ] + new_state = self.apply_operations('test', ProjectState(), setup_operations) + + operations = [ + migrations.AlterField( + model_name='Pony', + name='id', + field=models.CharField(max_length=20, primary_key=True), + ), + ] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + + self.assertEqual([ + '''ALTER TABLE "test_pony" DROP CONSTRAINT "test_pony_name_key";''', + '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', + '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_key" UNIQUE ("name");''' + ], sqls) + @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: '') # def test_add_notnull_without_default_change_to_nullable(self): @@ -62,8 +177,8 @@ def test_add_notnull_without_default_raise_exception(self): ), ] with self.assertRaises(ProgrammingError): - sqls = self.apply_operations_and_collect_sql('test', new_state, operations) - print(sqls) # for debug + with self.collect_sql(): + self.apply_operations('test', new_state, operations) @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: '') @@ -76,9 +191,10 @@ def test_add_notnull_with_default(self): field=models.CharField(max_length=10, verbose_name='name', null=False, default=''), ), ] - sqls = self.apply_operations_and_collect_sql('test', new_state, operations) - print('\n'.join(sqls)) - sqls = [s for s in sqls if not s.startswith('--')] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', ], sqls) @@ -94,9 +210,10 @@ def test_alter_type(self): field=models.CharField(max_length=10, null=False, default=''), ), ] - sqls = self.apply_operations_and_collect_sql('test', new_state, operations) - print('\n'.join(sqls)) - sqls = [s for s in sqls if not s.startswith('--')] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "weight_tmp" varchar(10) DEFAULT '' NOT NULL;''', '''UPDATE test_pony SET "weight_tmp" = "weight";''', @@ -120,9 +237,10 @@ def test_alter_notnull_with_default(self): field=models.CharField(max_length=10, verbose_name='name', null=False, default=''), ), ] - sqls = self.apply_operations_and_collect_sql('test', new_state, operations) - print('\n'.join(sqls)) - sqls = [s for s in sqls if not s.startswith('--')] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) NULL;''', '''ALTER TABLE "test_pony" ADD COLUMN "name_tmp" varchar(10) DEFAULT '' NOT NULL;''', @@ -152,9 +270,10 @@ def test_change_default(self): field=models.CharField(max_length=10, verbose_name='name', null=False, default='blink'), ), ] - sqls = self.apply_operations_and_collect_sql('test', new_state, operations) - print('\n'.join(sqls)) - sqls = [s for s in sqls if not s.startswith('--')] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', '''ALTER TABLE "test_pony" ADD COLUMN "name_tmp" varchar(10) DEFAULT 'blink' NOT NULL;''', @@ -175,20 +294,13 @@ def test_alter_notnull_to_nullable(self): field=models.FloatField(null=True), ), ] - sqls = self.apply_operations_and_collect_sql('test', new_state, operations) - print('\n'.join(sqls)) - sqls = [s for s in sqls if not s.startswith('--')] + + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "weight_tmp" double precision NULL;''', '''UPDATE test_pony SET "weight_tmp" = "weight";''', '''ALTER TABLE test_pony DROP COLUMN "weight" CASCADE;''', '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', ], sqls) - - def apply_operations_and_collect_sql(self, app_label, project_state, operations, atomic=True): - from django.db.migrations.migration import Migration - migration = Migration('name', app_label) - migration.operations = operations - with connection.schema_editor(collect_sql=True, atomic=atomic) as editor: - migration.apply(project_state, editor, collect_sql=True) - return editor.collected_sql From da95b91b56827d1643dd609b8613638cda9772e0 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Thu, 24 Feb 2022 12:53:59 +0000 Subject: [PATCH 07/14] refactoring: test_migrations.py --- tests/test_migrations.py | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 6cf74c3..ba04a68 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -12,6 +12,8 @@ @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: '') class MigrationTests(OperationTestBase): available_apps = ["testapp"] databases = {'default'} @@ -38,8 +40,6 @@ def execute(self, sql, params=()): print('\n'.join(collected_sql)) - @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: '') def test_alter_size(self): new_state = self.set_up_test_model('test') operations = [ @@ -69,8 +69,6 @@ def test_alter_size(self): '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(10);''', ], sqls) - @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: '') def test_alter_size_for_unique(self): new_state = self.set_up_test_model('test') operations = [ @@ -96,8 +94,6 @@ def test_alter_size_for_unique(self): '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_key" UNIQUE ("name");''' ], sqls) - @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: '') def test_alter_size_for_pk(self): setup_operations = [migrations.CreateModel( 'Pony', @@ -124,8 +120,6 @@ def test_alter_size_for_pk(self): '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_key" UNIQUE ("name");''' ], sqls) - @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: '') def test_alter_size_for_fk(self): setup_operations = [ migrations.CreateModel( @@ -163,9 +157,6 @@ def test_alter_size_for_fk(self): '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_key" UNIQUE ("name");''' ], sqls) - @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: '') - # def test_add_notnull_without_default_change_to_nullable(self): def test_add_notnull_without_default_raise_exception(self): from django.db.utils import ProgrammingError new_state = self.set_up_test_model('test') @@ -180,8 +171,6 @@ def test_add_notnull_without_default_raise_exception(self): with self.collect_sql(): self.apply_operations('test', new_state, operations) - @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: '') def test_add_notnull_with_default(self): new_state = self.set_up_test_model('test') operations = [ @@ -199,8 +188,6 @@ def test_add_notnull_with_default(self): '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) DEFAULT '' NOT NULL;''', ], sqls) - @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: '') def test_alter_type(self): new_state = self.set_up_test_model('test') operations = [ @@ -221,8 +208,6 @@ def test_alter_type(self): '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', ], sqls) - @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: '') def test_alter_notnull_with_default(self): new_state = self.set_up_test_model('test') operations = [ @@ -253,8 +238,6 @@ 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') - @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: '') def test_change_default(self): # https://github.com/jazzband/django-redshift-backend/issues/63 new_state = self.set_up_test_model('test') @@ -282,8 +265,6 @@ def test_change_default(self): '''ALTER TABLE test_pony RENAME COLUMN "name_tmp" TO "name";''', ], sqls) - @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: '') def test_alter_notnull_to_nullable(self): # https://github.com/jazzband/django-redshift-backend/issues/63 new_state = self.set_up_test_model('test') From 0177e2559e3cbb6378569630e34161675f9f3bfb Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Thu, 24 Feb 2022 20:12:59 +0000 Subject: [PATCH 08/14] feature: alter type size of primary key --- CHANGES.rst | 1 + django_redshift_backend/base.py | 47 +++++++++++++++++---------------- tests/test_migrations.py | 11 ++++---- 3 files changed, 31 insertions(+), 28 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 0ca0d91..612ec8c 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -44,6 +44,7 @@ Features: * #82 Add Python-3.10 support. * #82 Drop Django-3.0 support. * #63 Support changing a field from NOT NULL to NULL on migrate / sqlmigrate. +* #?? Support VARCHAR size changing for UNIQUE, PRIMARY KEY. 2.1.0 (2021/09/23) ------------------ diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 9f3e24b..127f791 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -642,6 +642,24 @@ def _alter_column_null_sqls(self, model, old_field, new_field): # ## Redshift needs 4 step alter return self._alter_column_with_recreate(model, old_field, new_field) + def _get_constraint(self, model, field, unique=None, primary_key=None, foreign_key=None): + meta_constraint_names = {constraint.name for constraint in model._meta.constraints} + constraint_names = self._constraint_names( + model, [field.column], unique=unique, primary_key=primary_key, foreign_key=foreign_key, + exclude=meta_constraint_names, + ) + if not constraint_names: + constraint_name = None + elif len(constraint_names) == 1: + constraint_name = constraint_names[0] + else: + raise ValueError("Found wrong number (%s) of unique constraints for %s.%s" % ( + len(constraint_names), + model._meta.db_table, + field.column, + )) + return constraint_name + # override to avoid `USING %(column)s::%(type)s` on postgres/schema.py def _alter_column_type_sql(self, model, old_field, new_field, new_type): # """ @@ -685,31 +703,14 @@ def _alter_column_type_sql(self, model, old_field, new_field, new_type): # 3. add UNIQUE (, PKEY, FK) again # 0. Find the unique constraint for this field - def get_constraint(model, field, unique=None, primary_key=None, foreign_key=None): - meta_constraint_names = {constraint.name for constraint in model._meta.constraints} - constraint_names = self._constraint_names( - model, [field.column], unique=unique, primary_key=primary_key, foreign_key=foreign_key, - exclude=meta_constraint_names, - ) - if not constraint_names: - constraint_name = None - elif len(constraint_names) == 1: - constraint_name = constraint_names[0] - else: - raise ValueError("Found wrong number (%s) of unique constraints for %s.%s" % ( - len(constraint_names), - model._meta.db_table, - old_field.column, - )) - return constraint_name unique_constraint = pk_constraint = fk_constraint = None - if old_field.unique and new_field.unique: - unique_constraint = get_constraint(model, old_field, unique=True, primary_key=False) + if old_field.unique and new_field.unique and not(old_field.primary_key or new_field.primary_key): + unique_constraint = self._get_constraint(model, old_field, unique=True, primary_key=False) elif old_field.primary_key and new_field.primary_key: - pk_constraint = get_constraint(model, old_field, primary_key=True) + pk_constraint = self._get_constraint(model, old_field, primary_key=True) elif old_field.is_relation and new_field.is_relation: - fk_constraint = get_constraint(model, old_field, foreign_key=True) + fk_constraint = self._get_constraint(model, old_field, foreign_key=True) # 1. once remove UNIQUE, PKEY, FK if unique_constraint: @@ -734,9 +735,9 @@ def get_constraint(model, field, unique=None, primary_key=None, foreign_key=None if unique_constraint: actions.append((self._create_unique_sql(model, [new_field.column], unique_constraint), [])) elif pk_constraint: - actions.append((self._create_primary_key_sql(model, pk_constraint), [])) # FIXME + actions.append((self._create_primary_key_sql(model, new_field), [])) elif fk_constraint: - actions.append((self._create_fk_sql(model, fk_constraint), [])) # FIXME + actions.append((self._create_fk_sql(model, new_field, fk_constraint), [])) # FIXME: fk_constraint fragment = actions.pop(0) # Type or default is changed? diff --git a/tests/test_migrations.py b/tests/test_migrations.py index ba04a68..23ecdd0 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -112,12 +112,15 @@ def test_alter_size_for_pk(self): ] with self.collect_sql() as sqls: - self.apply_operations('test', new_state, operations) + with mock.patch( + 'django_redshift_backend.base.DatabaseSchemaEditor._create_index_name', + lambda self, table_name, column_names, suffix="": f"{table_name}_pkey"): + self.apply_operations('test', new_state, operations) self.assertEqual([ - '''ALTER TABLE "test_pony" DROP CONSTRAINT "test_pony_name_key";''', + '''ALTER TABLE "test_pony" DROP CONSTRAINT "test_pony_pkey";''', '''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_pkey" PRIMARY KEY ("name");''' ], sqls) def test_alter_size_for_fk(self): @@ -125,8 +128,6 @@ def test_alter_size_for_fk(self): migrations.CreateModel( 'Pony', [ - # ('id', models.AutoField(primary_key=True)), - # ('name', models.CharField(max_length=10, unique=True)), ('id', models.CharField(max_length=10, primary_key=True)), ], ), From 00e12cf41509dc6dd69cfd4c976cd26e3cba0b94 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Thu, 24 Feb 2022 20:15:11 +0000 Subject: [PATCH 09/14] refactor CHANGES --- CHANGES.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 612ec8c..26f85e9 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -26,14 +26,13 @@ Incompatible Changes: Originally, Redshift did not support column name changes within a transaction. Please refer https://github.com/jazzband/django-redshift-backend/issues/96 -* #??: Changed the behavior of implicit NOT NULL column addition: When adding a NOT NULL column, it was - implicitly changed to allow NULL, but it is now fixed to raise a ProgrammingError exception without - implicitly changing it. Adding NOT NULL with no default should be an error. +* #??: changed the behavior of implicit not null column addition. + previously, adding a not null column was implicitly changed to allow null. + now adding not null without default raises a programmingerror exception. Bug Fixes: * #92, #93: since django-3.0 sqlmigrate (and migrate) does not work. -* #90, #13: fix database inspection capability with `manage.py inspectdb`. Thanks to Matt Fisher. * #37: fix Django `contenttype` migration that cause `ProgrammingError: cannot drop sortkey column "name"` exception. * #64: fix Django `auth` migration that cause `NotSupportedError: column "content_type__app_label" @@ -43,6 +42,8 @@ Features: * #82 Add Python-3.10 support. * #82 Drop Django-3.0 support. +* #90, #13: Support `manage.py inspectdb`, also support working with the django-sql-explorer package. + Thanks to Matt Fisher. * #63 Support changing a field from NOT NULL to NULL on migrate / sqlmigrate. * #?? Support VARCHAR size changing for UNIQUE, PRIMARY KEY. From 494549f89aa6a7c23f850929c3c13c9209e0f80a Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Thu, 24 Feb 2022 20:46:52 +0000 Subject: [PATCH 10/14] feature: alter type size of foreign key --- CHANGES.rst | 2 +- django_redshift_backend/base.py | 16 +++++++++++++--- tests/test_migrations.py | 17 +++++++++-------- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 26f85e9..ee692bb 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -45,7 +45,7 @@ Features: * #90, #13: Support `manage.py inspectdb`, also support working with the django-sql-explorer package. Thanks to Matt Fisher. * #63 Support changing a field from NOT NULL to NULL on migrate / sqlmigrate. -* #?? Support VARCHAR size changing for UNIQUE, PRIMARY KEY. +* #?? Support VARCHAR size changing for UNIQUE, PRIMARY KEY, FOREIGN KEY. 2.1.0 (2021/09/23) ------------------ diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 127f791..8113e7d 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -686,11 +686,21 @@ def _alter_column_type_sql(self, model, old_field, new_field, new_type): not self.skip_default(new_field) ) + # Size change? + def _get_max_length(field): + if field.is_relation: + max_length = field.foreign_related_fields[0].max_length + else: + max_length = field.max_length + return max_length + old_max_length = _get_max_length(old_field) + new_max_length = _get_max_length(new_field) + # Size is changed if (type(old_field) == type(new_field) and - old_field.max_length is not None and - new_field.max_length is not None and - old_field.max_length != new_field.max_length): + old_max_length is not None and + new_max_length is not None and + old_max_length != new_max_length): # if shrink size as `old_field.max_length > new_field.max_length` and # larger data in database, this change will raise exception. diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 23ecdd0..5333fa5 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -112,15 +112,12 @@ def test_alter_size_for_pk(self): ] with self.collect_sql() as sqls: - with mock.patch( - 'django_redshift_backend.base.DatabaseSchemaEditor._create_index_name', - lambda self, table_name, column_names, suffix="": f"{table_name}_pkey"): - self.apply_operations('test', new_state, operations) + self.apply_operations('test', new_state, operations) self.assertEqual([ '''ALTER TABLE "test_pony" DROP CONSTRAINT "test_pony_pkey";''', '''ALTER TABLE "test_pony" ALTER COLUMN "name" TYPE varchar(20);''', - '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_pkey" PRIMARY KEY ("name");''' + '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_name_2c070d2a_pk" PRIMARY KEY ("name");''', ], sqls) def test_alter_size_for_fk(self): @@ -153,9 +150,13 @@ def test_alter_size_for_fk(self): self.apply_operations('test', new_state, operations) self.assertEqual([ - '''ALTER TABLE "test_pony" DROP CONSTRAINT "test_pony_name_key";''', - '''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_rider" DROP CONSTRAINT "test_rider_pony_id_3c028c84_fk_test_pony_id";''', + '''ALTER TABLE "test_pony" DROP CONSTRAINT "test_pony_pkey";''', + '''ALTER TABLE "test_pony" ALTER COLUMN "id" TYPE varchar(20);''', + '''ALTER TABLE "test_pony" ADD CONSTRAINT "test_pony_id_f5124350_pk" PRIMARY KEY ("id");''', + '''ALTER TABLE "test_rider" ALTER COLUMN "pony_id" TYPE varchar(20);''', + ('''ALTER TABLE "test_rider" ADD CONSTRAINT "test_rider_pony_id_3c028c84_fk"''' + ''' FOREIGN KEY ("pony_id") REFERENCES "test_pony" ("id");'''), ], sqls) def test_add_notnull_without_default_raise_exception(self): From 6ce89ba43ccb317caf9c93a1176d3194c6f4f0f7 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Thu, 24 Feb 2022 21:32:06 +0000 Subject: [PATCH 11/14] fix django migration error --- django_redshift_backend/base.py | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 8113e7d..8820eef 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -14,7 +14,7 @@ from django.conf import settings from django.core.exceptions import FieldDoesNotExist from django.db.backends.base.introspection import FieldInfo -from django.db.backends.base.schema import _is_relevant_relation +from django.db.backends.base.schema import _is_relevant_relation, _related_non_m2m_objects from django.db.backends.base.validation import BaseDatabaseValidation from django.db.backends.ddl_references import Statement from django.db.backends.postgresql.base import ( @@ -108,19 +108,6 @@ def distinct_sql(self, fields, *args): return super(DatabaseOperations, self).distinct_sql(fields, *args) -def _related_non_m2m_objects(old_field, new_field): - # Filters out m2m objects from reverse relations. - # Returns (old_relation, new_relation) tuples. - return zip( - (obj - for obj in old_field.model._meta.related_objects - if not obj.field.many_to_many), - (obj - for obj in new_field.model._meta.related_objects - if not obj.field.many_to_many) - ) - - class DatabaseSchemaEditor(BasePGDatabaseSchemaEditor): sql_create_table = "CREATE TABLE %(table)s (%(definition)s) %(options)s" From 00e04e36b2753f077abdd08e564c88f2cf8d3bc4 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Fri, 25 Feb 2022 21:32:04 +0000 Subject: [PATCH 12/14] update reason for 'can_rollback_ddl=False' --- CHANGES.rst | 2 +- django_redshift_backend/base.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index ee692bb..b605bb5 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -23,7 +23,7 @@ Incompatible Changes: However old name is still supported for a compatibility. * #??: Now django-redshift-backend doesn't support `can_rollback_ddl`. - Originally, Redshift did not support column name changes within a transaction. + Originally, Redshift did not support column name/type(size) changes within a transaction. Please refer https://github.com/jazzband/django-redshift-backend/issues/96 * #??: changed the behavior of implicit not null column addition. diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 8820eef..5ecf668 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -46,7 +46,7 @@ class DatabaseFeatures(BasePGDatabaseFeatures): allows_group_by_selected_pks = False has_native_uuid_field = False supports_aggregate_filter_clause = False - can_rollback_ddl = False + can_rollback_ddl = False # django-redshift-backend #96 supports_combined_alters = False # since django-1.8 From e07e77ffc94a66b65a149d1c3bfe3241c2c0e700 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Sat, 26 Feb 2022 20:15:07 +0000 Subject: [PATCH 13/14] Support backward migration for DROP NOT NULL column wituout DEFAULT --- CHANGES.rst | 3 +++ django_redshift_backend/base.py | 46 ++++++++++++++++++++++++++++----- tests/test_migrations.py | 36 +++++++++++++++++++++++--- 3 files changed, 76 insertions(+), 9 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index b605bb5..6c01712 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -46,6 +46,9 @@ Features: Thanks to Matt Fisher. * #63 Support changing a field from NOT NULL to NULL on migrate / sqlmigrate. * #?? Support VARCHAR size changing for UNIQUE, PRIMARY KEY, FOREIGN KEY. +* #?? Support backward migration for DROP NOT NULL column wituout DEFAULT. + One limitation is that the DEFAULT value is set to match the type. This is because the only way for + Redshift to add NOT NULL without default is to recreate the table. 2.1.0 (2021/09/23) ------------------ diff --git a/django_redshift_backend/base.py b/django_redshift_backend/base.py index 5ecf668..6a9a7f7 100644 --- a/django_redshift_backend/base.py +++ b/django_redshift_backend/base.py @@ -11,6 +11,7 @@ import logging import django +from django.utils import timezone from django.conf import settings from django.core.exceptions import FieldDoesNotExist from django.db.backends.base.introspection import FieldInfo @@ -108,6 +109,31 @@ def distinct_sql(self, fields, *args): return super(DatabaseOperations, self).distinct_sql(fields, *args) +def _get_type_default(field): + internal_type = field.get_internal_type() + if internal_type in ('CharField', 'SlugField'): + default = '' + elif internal_type == 'BinaryField': + default = b'' + elif internal_type == 'FloatField': + default = 0.0 + elif internal_type in ( + 'BigAutoField', 'IntegerField', 'BigIntegerField', 'PositiveBigIntegerField', 'PositiveIntegerField', + 'PositiveSmallIntegerField', 'SmallAutoField', 'SmallIntegerField', 'DecimalField'): + default = 0 + elif internal_type == 'BooleanField': + default = False + elif internal_type == 'DateField': + default = timezone.date() + elif internal_type == 'TimeField': + default = timezone.time() + elif internal_type == 'DateTimeField': + default = timezone.now() + else: + default = None + return default + + class DatabaseSchemaEditor(BasePGDatabaseSchemaEditor): sql_create_table = "CREATE TABLE %(table)s (%(definition)s) %(options)s" @@ -574,25 +600,33 @@ def _alter_column_with_recreate(self, model, old_field, new_field): 3. Drop old column 4. Rename temporary column name to original column name """ - new_default = self.effective_default(new_field) - fragment = ('', []) actions = [] # ## ALTER TABLE
ADD COLUMN 'tmp' DEFAULT + if not new_field.null and not new_field.has_default(): + # Redshift can't add NOT NULL or DROP NOT NULL, then DEFAULT value is needed. + # Note that only backwards migration is in here. + default = _get_type_default(new_field) + if default is None: + raise ValueError( + "django-redshift-backend doesn't know default for the type: {}".format( + new_field.get_internal_type() + )) + new_field.default = default + definition, params = self.column_sql(model, new_field, include_default=True) - new_defaults = [new_default] if new_default is not None else [] fragment = ( self.sql_create_column % { "table": self.quote_name(model._meta.db_table), "column": self.quote_name(new_field.column + "_tmp"), "definition": definition, }, - new_defaults + params ) # ## UPDATE
SET 'tmp' = actions.append(( - "UPDATE %(table)s SET %(new_column)s = %(old_column)s" % { + "UPDATE %(table)s SET %(new_column)s = %(old_column)s WHERE %(old_column)s IS NOT NULL" % { "table": model._meta.db_table, "new_column": self.quote_name(new_field.column + "_tmp"), "old_column": self.quote_name(new_field.column), @@ -734,7 +768,7 @@ def _get_max_length(field): elif pk_constraint: actions.append((self._create_primary_key_sql(model, new_field), [])) elif fk_constraint: - actions.append((self._create_fk_sql(model, new_field, fk_constraint), [])) # FIXME: fk_constraint + actions.append((self._create_fk_sql(model, new_field, fk_constraint), [])) fragment = actions.pop(0) # Type or default is changed? diff --git a/tests/test_migrations.py b/tests/test_migrations.py index 5333fa5..cb76f7a 100644 --- a/tests/test_migrations.py +++ b/tests/test_migrations.py @@ -173,6 +173,36 @@ def test_add_notnull_without_default_raise_exception(self): with self.collect_sql(): self.apply_operations('test', new_state, operations) + def test_add_notnull_without_default_on_backwards(self): + project_state = self.set_up_test_model('test') + operations = [ + migrations.AlterField( + model_name='Pony', + name='weight', + field=models.FloatField(null=True), + ), + ] + new_state = project_state.clone() + with self.collect_sql() as sqls: + self.apply_operations('test', new_state, operations) + + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "weight_tmp" double precision NULL;''', + '''UPDATE test_pony SET "weight_tmp" = "weight" WHERE "weight" IS NOT NULL;''', + '''ALTER TABLE test_pony DROP COLUMN "weight" CASCADE;''', + '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', + ], sqls) + + with self.collect_sql() as sqls: + self.unapply_operations('test', project_state, operations) + + self.assertEqual([ + '''ALTER TABLE "test_pony" ADD COLUMN "weight_tmp" double precision DEFAULT 0.0 NOT NULL;''', + '''UPDATE test_pony SET "weight_tmp" = "weight" WHERE "weight" IS NOT NULL;''', + '''ALTER TABLE test_pony DROP COLUMN "weight" CASCADE;''', + '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', + ], sqls) + def test_add_notnull_with_default(self): new_state = self.set_up_test_model('test') operations = [ @@ -205,7 +235,7 @@ def test_alter_type(self): self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "weight_tmp" varchar(10) DEFAULT '' NOT NULL;''', - '''UPDATE test_pony SET "weight_tmp" = "weight";''', + '''UPDATE test_pony SET "weight_tmp" = "weight" WHERE "weight" IS NOT NULL;''', '''ALTER TABLE test_pony DROP COLUMN "weight" CASCADE;''', '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', ], sqls) @@ -231,7 +261,7 @@ def test_alter_notnull_with_default(self): self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "name" varchar(10) NULL;''', '''ALTER TABLE "test_pony" ADD COLUMN "name_tmp" varchar(10) DEFAULT '' NOT NULL;''', - '''UPDATE test_pony SET "name_tmp" = "name";''', + '''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) @@ -283,7 +313,7 @@ def test_alter_notnull_to_nullable(self): self.assertEqual([ '''ALTER TABLE "test_pony" ADD COLUMN "weight_tmp" double precision NULL;''', - '''UPDATE test_pony SET "weight_tmp" = "weight";''', + '''UPDATE test_pony SET "weight_tmp" = "weight" WHERE "weight" IS NOT NULL;''', '''ALTER TABLE test_pony DROP COLUMN "weight" CASCADE;''', '''ALTER TABLE test_pony RENAME COLUMN "weight_tmp" TO "weight";''', ], sqls) From 63a49219cf7484245da005977111001607183461 Mon Sep 17 00:00:00 2001 From: Takayuki SHIMIZUKAWA Date: Sat, 26 Feb 2022 20:48:35 +0000 Subject: [PATCH 14/14] update CHANGES --- CHANGES.rst | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 6c01712..b81c5fc 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -11,7 +11,7 @@ General: Incompatible Changes: -* #??: To specify SORTKEY for Redshift, you must use `django_redshift_backend.SortKey` for +* #97 To specify SORTKEY for Redshift, you must use `django_redshift_backend.SortKey` for `Model.Meta.ordering` instead of bearer string. **IMPORTANT**: @@ -19,14 +19,14 @@ Incompatible Changes: If you want to apply SortKey to your migration files, please comment out the ordering option once and run makemigrations, then comment in the ordering option and run makemigrations again. -* #??: `django_redshift_backend.distkey.DistKey` is moved to `django_redshift_backend.DistKey`. +* #97 `django_redshift_backend.distkey.DistKey` is moved to `django_redshift_backend.DistKey`. However old name is still supported for a compatibility. -* #??: Now django-redshift-backend doesn't support `can_rollback_ddl`. +* #97 Now django-redshift-backend doesn't support `can_rollback_ddl`. Originally, Redshift did not support column name/type(size) changes within a transaction. Please refer https://github.com/jazzband/django-redshift-backend/issues/96 -* #??: changed the behavior of implicit not null column addition. +* #97 changed the behavior of implicit not null column addition. previously, adding a not null column was implicitly changed to allow null. now adding not null without default raises a programmingerror exception. @@ -45,8 +45,8 @@ Features: * #90, #13: Support `manage.py inspectdb`, also support working with the django-sql-explorer package. Thanks to Matt Fisher. * #63 Support changing a field from NOT NULL to NULL on migrate / sqlmigrate. -* #?? Support VARCHAR size changing for UNIQUE, PRIMARY KEY, FOREIGN KEY. -* #?? Support backward migration for DROP NOT NULL column wituout DEFAULT. +* #97 Support VARCHAR size changing for UNIQUE, PRIMARY KEY, FOREIGN KEY. +* #97 Support backward migration for DROP NOT NULL column wituout DEFAULT. One limitation is that the DEFAULT value is set to match the type. This is because the only way for Redshift to add NOT NULL without default is to recreate the table.