Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug fixes and support several features for django migrations #97

Merged
merged 14 commits into from
Feb 26, 2022

Conversation

shimizukawa
Copy link
Member

Feature or Bugfix

  • Feature and Bugfix

Purpose

  • to support manage.py migrate for django auth, contenttypes, and etc.

Detail

Incompatible changes

  • To specify SORTKEY for Redshift, you must use django_redshift_backend.SortKey for
    Model.Meta.ordering instead of bearer string.

  • 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/type(size) changes within a transaction.
    Please refer [INFO] django-redshift-backend cannot provide change column for: PRIMARY, UNIQUE, FK, NOT NULL #96

  • 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:

Features:

  • No SQL is generated when changing a field from NOT NULL to NULL #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.

Relates

@shimizukawa shimizukawa self-assigned this Feb 26, 2022
@codecov
Copy link

codecov bot commented Feb 26, 2022

Codecov Report

Merging #97 (63a4921) into master (392a096) will increase coverage by 10.55%.
The diff coverage is 56.18%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #97       +/-   ##
===========================================
+ Coverage   52.44%   62.99%   +10.55%     
===========================================
  Files           3        4        +1     
  Lines         286      427      +141     
  Branches       80      122       +42     
===========================================
+ Hits          150      269      +119     
+ Misses        125      116        -9     
- Partials       11       42       +31     
Impacted Files Coverage Δ
django_redshift_backend/distkey.py 0.00% <0.00%> (-100.00%) ⬇️
django_redshift_backend/base.py 62.12% <56.00%> (+12.49%) ⬆️
django_redshift_backend/meta.py 58.82% <58.82%> (ø)
django_redshift_backend/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 392a096...63a4921. Read the comment docs.

@shimizukawa
Copy link
Member Author

django migrations with django-3.2.12

(.venv) @shimizukawa ➜ /workspaces/django-redshift-backend/examples/proj1 (fix/django-migrartions ✗) $ python manage.py --version
3.2.12

(.venv) @shimizukawa ➜ /workspaces/django-redshift-backend/examples/proj1 (fix/django-migrartions ✗) $ python manage.py migrate
Operations to perform:
  Apply all migrations: admin, auth, contenttypes, sessions, testapp
Running migrations:
  Applying contenttypes.0001_initial... OK
  Applying auth.0001_initial... OK
  Applying admin.0001_initial... OK
  Applying admin.0002_logentry_remove_auto_add... OK
  Applying admin.0003_logentry_add_action_flag_choices... OK
  Applying contenttypes.0002_remove_content_type_name... OK
  Applying auth.0002_alter_permission_name_max_length... OK
  Applying auth.0003_alter_user_email_max_length... OK
  Applying auth.0004_alter_user_username_opts... OK
  Applying auth.0005_alter_user_last_login_null... OK
  Applying auth.0006_require_contenttypes_0002... OK
  Applying auth.0007_alter_validators_add_error_messages... OK
  Applying auth.0008_alter_user_username_max_length... OK
  Applying auth.0009_alter_user_last_name_max_length... OK
  Applying auth.0010_alter_group_name_max_length... OK
  Applying auth.0011_update_proxy_permissions... OK
  Applying auth.0012_alter_user_first_name_max_length... OK
  Applying sessions.0001_initial... OK
  Applying testapp.0001_initial... OK

(.venv) @shimizukawa ➜ /workspaces/django-redshift-backend/examples/proj1 (fix/django-migrartions ✗) $ python manage.py showmigrations
admin
 [X] 0001_initial
 [X] 0002_logentry_remove_auto_add
 [X] 0003_logentry_add_action_flag_choices
auth
 [X] 0001_initial
 [X] 0002_alter_permission_name_max_length
 [X] 0003_alter_user_email_max_length
 [X] 0004_alter_user_username_opts
 [X] 0005_alter_user_last_login_null
 [X] 0006_require_contenttypes_0002
 [X] 0007_alter_validators_add_error_messages
 [X] 0008_alter_user_username_max_length
 [X] 0009_alter_user_last_name_max_length
 [X] 0010_alter_group_name_max_length
 [X] 0011_update_proxy_permissions
 [X] 0012_alter_user_first_name_max_length
contenttypes
 [X] 0001_initial
 [X] 0002_remove_content_type_name
sessions
 [X] 0001_initial
testapp
 [X] 0001_initial

(.venv) @shimizukawa ➜ /workspaces/django-redshift-backend/examples/proj1 (fix/django-migrartions ✗) $ python manage.py migrate contenttypes zero
Operations to perform:
  Unapply all migrations: contenttypes
Running migrations:
  Rendering model states... DONE
  Unapplying auth.0012_alter_user_first_name_max_length... OK
  Unapplying auth.0011_update_proxy_permissions... OK
  Unapplying auth.0010_alter_group_name_max_length... OK
  Unapplying auth.0009_alter_user_last_name_max_length... OK
  Unapplying auth.0008_alter_user_username_max_length... OK
  Unapplying auth.0007_alter_validators_add_error_messages... OK
  Unapplying auth.0006_require_contenttypes_0002... OK
  Unapplying contenttypes.0002_remove_content_type_name... OK
  Unapplying auth.0005_alter_user_last_login_null... OK
  Unapplying auth.0004_alter_user_username_opts... OK
  Unapplying auth.0003_alter_user_email_max_length... OK
  Unapplying auth.0002_alter_permission_name_max_length... OK
  Unapplying admin.0003_logentry_add_action_flag_choices... OK
  Unapplying admin.0002_logentry_remove_auto_add... OK
  Unapplying admin.0001_initial... OK
  Unapplying auth.0001_initial... OK
  Unapplying contenttypes.0001_initial... OK
(.venv) @shimizukawa ➜ /workspaces/django-redshift-backend/examples/proj1 (fix/django-migrartions ✗) $ python manage.py migrate sessions zero
Operations to perform:
  Unapply all migrations: sessions
Running migrations:
  Rendering model states... DONE
  Unapplying sessions.0001_initial... OK
(.venv) @shimizukawa ➜ /workspaces/django-redshift-backend/examples/proj1 (fix/django-migrartions ✗) $ python manage.py migrate testapp zero
Operations to perform:
  Unapply all migrations: testapp
Running migrations:
  Rendering model states... DONE
  Unapplying testapp.0001_initial... OK
(.venv) @shimizukawa ➜ /workspaces/django-redshift-backend/examples/proj1 (fix/django-migrartions ✗) $ python manage.py showmigrations
admin
 [ ] 0001_initial
 [ ] 0002_logentry_remove_auto_add
 [ ] 0003_logentry_add_action_flag_choices
auth
 [ ] 0001_initial
 [ ] 0002_alter_permission_name_max_length
 [ ] 0003_alter_user_email_max_length
 [ ] 0004_alter_user_username_opts
 [ ] 0005_alter_user_last_login_null
 [ ] 0006_require_contenttypes_0002
 [ ] 0007_alter_validators_add_error_messages
 [ ] 0008_alter_user_username_max_length
 [ ] 0009_alter_user_last_name_max_length
 [ ] 0010_alter_group_name_max_length
 [ ] 0011_update_proxy_permissions
 [ ] 0012_alter_user_first_name_max_length
contenttypes
 [ ] 0001_initial
 [ ] 0002_remove_content_type_name
sessions
 [ ] 0001_initial
testapp
 [ ] 0001_initial

(.venv) @shimizukawa ➜ /workspaces/django-redshift-backend/examples/proj1 (fix/django-migrartions ✗) $ python manage.py migrate
Operations to perform:
  Apply all migrations: admin, auth, contenttypes, sessions, testapp
Running migrations:
  Applying contenttypes.0001_initial... OK
  Applying auth.0001_initial... OK
  Applying admin.0001_initial... OK
  Applying admin.0002_logentry_remove_auto_add... OK
  Applying admin.0003_logentry_add_action_flag_choices... OK
  Applying contenttypes.0002_remove_content_type_name... OK
  Applying auth.0002_alter_permission_name_max_length... OK
  Applying auth.0003_alter_user_email_max_length... OK
  Applying auth.0004_alter_user_username_opts... OK
  Applying auth.0005_alter_user_last_login_null... OK
  Applying auth.0006_require_contenttypes_0002... OK
  Applying auth.0007_alter_validators_add_error_messages... OK
  Applying auth.0008_alter_user_username_max_length... OK
  Applying auth.0009_alter_user_last_name_max_length... OK
  Applying auth.0010_alter_group_name_max_length... OK
  Applying auth.0011_update_proxy_permissions... OK
  Applying auth.0012_alter_user_first_name_max_length... OK
  Applying sessions.0001_initial... OK
  Applying testapp.0001_initial... OK

Comment on lines +308 to 310
# 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):
Copy link
Member Author

@shimizukawa shimizukawa Feb 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I remove previous _alter_field once
  2. copy the newer method from https://github.com/django/django/blob/3.2.12/django/db/backends/base/schema.py#L611-L864
  3. modify for Redshift

So, the part of this diff in this PR is not important.

In this time. I note comments as "# ## original" and "# ## Redshift".

@shimizukawa
Copy link
Member Author

Glad to see Django migration working! It was worth spending 11 days of my personal time for this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant