From 78b666a6537693a389421610b91790451b54b451 Mon Sep 17 00:00:00 2001 From: Kim Gustyr Date: Wed, 6 Nov 2024 17:55:19 +0000 Subject: [PATCH] fix: Avoid a long table lock for the `first_name` migration --- api/tests/unit/users/test_migrations.py | 53 ++++++++++++++++++ .../0039_alter_ffadminuser_first_name.py | 18 ------ .../0039_ffadminuser_first_name_v2.py | 55 +++++++++++++++++++ .../sql/0039_ffadminuser_first_name_v2.sql | 8 +++ ...0039_ffadminuser_first_name_v2_reverse.sql | 8 +++ api/users/models.py | 6 +- 6 files changed, 129 insertions(+), 19 deletions(-) create mode 100644 api/tests/unit/users/test_migrations.py delete mode 100644 api/users/migrations/0039_alter_ffadminuser_first_name.py create mode 100644 api/users/migrations/0039_ffadminuser_first_name_v2.py create mode 100644 api/users/migrations/sql/0039_ffadminuser_first_name_v2.sql create mode 100644 api/users/migrations/sql/0039_ffadminuser_first_name_v2_reverse.sql diff --git a/api/tests/unit/users/test_migrations.py b/api/tests/unit/users/test_migrations.py new file mode 100644 index 000000000000..6a7d72d3d1b5 --- /dev/null +++ b/api/tests/unit/users/test_migrations.py @@ -0,0 +1,53 @@ +import pytest +from django.conf import settings +from django_test_migrations.migrator import Migrator + +pytestmark = pytest.mark.skipif( + settings.SKIP_MIGRATION_TESTS is True, + reason="Skip migration tests to speed up tests where necessary", +) + + +def test_0039_ffadminuser_first_name_v2__values_expected(migrator: Migrator) -> None: + # Given + old_state = migrator.apply_initial_migration( + ("users", "0038_create_hubspot_tracker") + ) + OldFFAdminUser = old_state.apps.get_model("users", "FFAdminUser") + + user = OldFFAdminUser.objects.create(first_name="Testfirstname") + + # When + new_state = migrator.apply_tested_migration( + ("users", "0039_ffadminuser_first_name_v2") + ) + NewFFAdminUser = new_state.apps.get_model("users", "FFAdminUser") + + # Then + assert NewFFAdminUser.objects.get(id=user.id).first_name == user.first_name + + +def test_0039_ffadminuser_first_name_v2__reverse__values_expected( + migrator: Migrator, +) -> None: + # Given + old_state = migrator.apply_initial_migration( + ("users", "0039_ffadminuser_first_name_v2") + ) + NewFFAdminUser = old_state.apps.get_model("users", "FFAdminUser") + + user = NewFFAdminUser.objects.create( + first_name="TestfirstnameTestfirstnameTestfirstnameTestfirstname" + ) + + # When + new_state = migrator.apply_tested_migration( + ("users", "0038_create_hubspot_tracker") + ) + OldFFAdminUser = new_state.apps.get_model("users", "FFAdminUser") + + # Then + assert ( + OldFFAdminUser.objects.get(id=user.id).first_name + == "TestfirstnameTestfirstnameTest" + ) diff --git a/api/users/migrations/0039_alter_ffadminuser_first_name.py b/api/users/migrations/0039_alter_ffadminuser_first_name.py deleted file mode 100644 index 11406f6da6e9..000000000000 --- a/api/users/migrations/0039_alter_ffadminuser_first_name.py +++ /dev/null @@ -1,18 +0,0 @@ -# Generated by Django 4.2.16 on 2024-11-04 17:09 - -from django.db import migrations, models - - -class Migration(migrations.Migration): - - dependencies = [ - ("users", "0038_create_hubspot_tracker"), - ] - - operations = [ - migrations.AlterField( - model_name="ffadminuser", - name="first_name", - field=models.CharField(max_length=150, verbose_name="first name"), - ), - ] diff --git a/api/users/migrations/0039_ffadminuser_first_name_v2.py b/api/users/migrations/0039_ffadminuser_first_name_v2.py new file mode 100644 index 000000000000..a6729c138215 --- /dev/null +++ b/api/users/migrations/0039_ffadminuser_first_name_v2.py @@ -0,0 +1,55 @@ +# Generated by Django 4.2.16 on 2024-11-05 10:49 + +from pathlib import Path +from django.db import migrations, models + + +def _get_ffadminuser_table_name() -> str: + from users.models import FFAdminUser + + return FFAdminUser._meta.db_table + + +_ffadminuser_table_name = _get_ffadminuser_table_name() + + +parent_dir = Path(__file__).parent.resolve() + +with open(parent_dir / "sql/0039_ffadminuser_first_name_v2.sql") as f: + sql_forwards = f.read().format(ffadminuser_table_name=_ffadminuser_table_name) + +with open(parent_dir / "sql/0039_ffadminuser_first_name_v2_reverse.sql") as f: + sql_reverse = f.read().format(ffadminuser_table_name=_ffadminuser_table_name) + + +class Migration(migrations.Migration): + + dependencies = [ + ("users", "0038_create_hubspot_tracker"), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.RemoveField( + model_name="ffadminuser", + name="first_name", + ), + migrations.AddField( + model_name="ffadminuser", + name="first_name", + field=models.CharField( + db_column="first_name_v2", + max_length=150, + verbose_name="first name", + ), + ), + ], + database_operations=[ + migrations.RunSQL( + sql=sql_forwards, + reverse_sql=sql_reverse, + ) + ], + ) + ] diff --git a/api/users/migrations/sql/0039_ffadminuser_first_name_v2.sql b/api/users/migrations/sql/0039_ffadminuser_first_name_v2.sql new file mode 100644 index 000000000000..a9e307cdb79c --- /dev/null +++ b/api/users/migrations/sql/0039_ffadminuser_first_name_v2.sql @@ -0,0 +1,8 @@ +ALTER TABLE {ffadminuser_table_name} +ADD COLUMN first_name_v2 varchar(150) NULL; +UPDATE {ffadminuser_table_name} +SET first_name_v2 = {ffadminuser_table_name}.first_name; +ALTER TABLE {ffadminuser_table_name} +ALTER COLUMN first_name_v2 SET NOT NULL; +ALTER TABLE {ffadminuser_table_name} +DROP COLUMN first_name; \ No newline at end of file diff --git a/api/users/migrations/sql/0039_ffadminuser_first_name_v2_reverse.sql b/api/users/migrations/sql/0039_ffadminuser_first_name_v2_reverse.sql new file mode 100644 index 000000000000..bd9e33405003 --- /dev/null +++ b/api/users/migrations/sql/0039_ffadminuser_first_name_v2_reverse.sql @@ -0,0 +1,8 @@ +ALTER TABLE {ffadminuser_table_name} +ADD COLUMN first_name varchar(30) NULL; +UPDATE {ffadminuser_table_name} +SET first_name = left({ffadminuser_table_name}.first_name_v2, 30); +ALTER TABLE {ffadminuser_table_name} +ALTER COLUMN first_name SET NOT NULL; +ALTER TABLE {ffadminuser_table_name} +DROP COLUMN first_name_v2; diff --git a/api/users/models.py b/api/users/models.py index 2030ad05c3f7..6b86161c09a0 100644 --- a/api/users/models.py +++ b/api/users/models.py @@ -99,7 +99,11 @@ class FFAdminUser(LifecycleModel, AbstractUser): email = models.EmailField(unique=True, null=False) objects = UserManager() username = models.CharField(unique=True, max_length=150, null=True, blank=True) - first_name = models.CharField("first name", max_length=150) + first_name = models.CharField( + "first name", + max_length=150, + db_column="first_name_v2", + ) last_name = models.CharField("last name", max_length=150) google_user_id = models.CharField(max_length=50, null=True, blank=True) github_user_id = models.CharField(max_length=50, null=True, blank=True)