From 501ecfa20bae6554be30d1b52cd1cd102fc776fe Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 14 Feb 2024 09:10:49 -0500 Subject: [PATCH] Use django-safemigrate for migrations (#11087) * Use django-safemigrate for migrations Closes https://github.com/readthedocs/readthedocs.org/issues/10964 * Add to toc * Fix migration * Fix fixtures * Update common * Fix package name * Updates from review * Update common --- docs/dev/index.rst | 1 + docs/dev/migrations.rst | 133 ++++++++++++++++++ .../0057_migrate_timestamp_fields.py | 32 +++++ ..._version_created_alter_version_modified.py | 37 +++++ readthedocs/builds/models.py | 14 -- .../0013_set_timestamp_fields_as_no_null.py | 37 +++++ readthedocs/integrations/models.py | 14 -- readthedocs/projects/fixtures/test_data.json | 28 ++-- .../0114_set_timestamp_fields_as_no_null.py | 33 +++++ readthedocs/projects/models.py | 4 - readthedocs/settings/base.py | 1 + requirements/deploy.txt | 8 +- requirements/docker.txt | 8 +- requirements/pip.in | 2 + requirements/pip.txt | 7 +- requirements/testing.txt | 12 +- 16 files changed, 307 insertions(+), 64 deletions(-) create mode 100644 docs/dev/migrations.rst create mode 100644 readthedocs/builds/migrations/0057_migrate_timestamp_fields.py create mode 100644 readthedocs/builds/migrations/0058_alter_version_created_alter_version_modified.py create mode 100644 readthedocs/integrations/migrations/0013_set_timestamp_fields_as_no_null.py create mode 100644 readthedocs/projects/migrations/0114_set_timestamp_fields_as_no_null.py diff --git a/docs/dev/index.rst b/docs/dev/index.rst index 99ebeaca194..4e7f70d6d55 100644 --- a/docs/dev/index.rst +++ b/docs/dev/index.rst @@ -25,6 +25,7 @@ or taking the open source Read the Docs codebase for your own custom installatio style-guide front-end i18n + migrations server-side-search search-integration subscriptions diff --git a/docs/dev/migrations.rst b/docs/dev/migrations.rst new file mode 100644 index 00000000000..3c4117286e4 --- /dev/null +++ b/docs/dev/migrations.rst @@ -0,0 +1,133 @@ +Database migrations +=================== + +We use `Django migrations `__ to manage database schema changes, +and the `django-safemigrate `__ package to ensure that migrations are run in a given order to avoid downtime. + +To make sure that migrations don't cause downtime, +the following rules should be followed for each case. + +Adding a new field +------------------ + +**When adding a new field to a model, it should be nullable.** +This way, the database can be migrated without downtime, and the field can be populated later. +Don't forget to make the field non-nullable in a separate migration after the data has been populated. +You can achieve this by following these steps: + +- #. Set the new field as ``null=True`` and ``blank=True`` in the model. + + .. code-block:: python + + class MyModel(models.Model): + new_field = models.CharField( + max_length=100, null=True, blank=True, default="default" + ) + +- #. Make sure that the field is always populated with a proper value in the new code, + and the code handles the case where the field is null. + + .. code-block:: python + + if my_model.new_field in [None, "default"]: + pass + + + # If it's a boolean field, make sure that the null option is removed from the form. + class MyModelForm(forms.ModelForm): + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.fields["new_field"].widget = forms.CheckboxInput() + self.fields["new_field"].empty_value = False + +- #. Create the migration file (let's call this migration ``app 0001``), + and mark it as ``Safe.before_deploy``. + + .. code-block:: python + + from django.db import migrations, models + from django_safemigrate import Safe + + + class Migration(migrations.Migration): + safe = Safe.before_deploy + +- #. Create a data migration to populate all null values of the new field with a proper value (let's call this migration ``app 0002``), + and mark it as ``Safe.after_deploy``. + + .. code-block:: python + + from django.db import migrations + + + def migrate(apps, schema_editor): + MyModel = apps.get_model("app", "MyModel") + MyModel.objects.filter(new_field=None).update(new_field="default") + + + class Migration(migrations.Migration): + safe = Safe.after_deploy + + operations = [ + migrations.RunPython(migrate), + ] + +- #. After the deploy has been completed, create a new migration to set the field as non-nullable (let's call this migration ``app 0003``). + Run this migration on a new deploy, you can mark it as ``Safe.before_deploy`` or ``Safe.always``. +- #. Remove any handling of the null case from the code. + +At the end, the deploy should look like this: + +- Deploy web-extra. +- Run ``django-admin safemigrate`` to run the migration ``app 0001``. +- Deploy the webs +- Run ``django-admin migrate`` to run the migration ``app 0002``. +- Create a new migration to set the field as non-nullable, + and apply it on the next deploy. + +Removing a field +---------------- + +**When removing a field from a model, +all usages of the field should be removed from the code before the field is removed from the model, +and the field should be nullable.** +You can achieve this by following these steps: + +- #. Remove all usages of the field from the code. +- #. Set the field as ``null=True`` and ``blank=True`` in the model. + + .. code-block:: python + + class MyModel(models.Model): + field_to_delete = models.CharField(max_length=100, null=True, blank=True) + +- #. Create the migration file (let's call this migration ``app 0001``), + and mark it as ``Safe.before_deploy``. + + .. code-block:: python + + from django.db import migrations, models + from django_safemigrate import Safe + + + class Migration(migrations.Migration): + safe = Safe.before_deploy + +- #. Create a migration to remove the field from the database (let's call this migration ``app 0002``), + and mark it as ``Safe.after_deploy``. + + .. code-block:: python + + from django.db import migrations, models + from django_safemigrate import Safe + + + class Migration(migrations.Migration): + safe = Safe.after_deploy + +At the end, the deploy should look like this: + +- Deploy web-extra. +- Run ``django-admin safemigrate`` to run the migration ``app 0001``. +- Deploy the webs +- Run ``django-admin migrate`` to run the migration ``app 0002``. diff --git a/readthedocs/builds/migrations/0057_migrate_timestamp_fields.py b/readthedocs/builds/migrations/0057_migrate_timestamp_fields.py new file mode 100644 index 00000000000..5192f283882 --- /dev/null +++ b/readthedocs/builds/migrations/0057_migrate_timestamp_fields.py @@ -0,0 +1,32 @@ +# Generated by Django 4.2.9 on 2024-02-01 20:29 + +import datetime + +from django.db import migrations +from django_safemigrate import Safe + + +def migrate(apps, schema_editor): + """ + Migrate the created and modified fields of the Version model to have a non-null value. + + This date corresponds to the release date of 5.6.5, + when the created field was added to the Version model + at https://github.com/readthedocs/readthedocs.org/commit/d72ee6e27dc398b97e884ccec8a8cf135134faac. + """ + Version = apps.get_model("builds", "Version") + date = datetime.datetime(2020, 11, 23, tzinfo=datetime.timezone.utc) + Version.objects.filter(created=None).update(created=date) + Version.objects.filter(modified=None).update(modified=date) + + +class Migration(migrations.Migration): + safe = Safe.before_deploy + + dependencies = [ + ("builds", "0056_alter_versionautomationrule_priority"), + ] + + operations = [ + migrations.RunPython(migrate), + ] diff --git a/readthedocs/builds/migrations/0058_alter_version_created_alter_version_modified.py b/readthedocs/builds/migrations/0058_alter_version_created_alter_version_modified.py new file mode 100644 index 00000000000..f66dea80bbe --- /dev/null +++ b/readthedocs/builds/migrations/0058_alter_version_created_alter_version_modified.py @@ -0,0 +1,37 @@ +# Generated by Django 4.2.9 on 2024-02-01 20:38 + +import django.utils.timezone +import django_extensions.db.fields +from django.db import migrations +from django_safemigrate import Safe + + +class Migration(migrations.Migration): + safe = Safe.after_deploy + + dependencies = [ + ("builds", "0057_migrate_timestamp_fields"), + ] + + operations = [ + migrations.AlterField( + model_name="version", + name="created", + field=django_extensions.db.fields.CreationDateTimeField( + auto_now_add=True, + default=django.utils.timezone.now, + verbose_name="created", + ), + preserve_default=False, + ), + migrations.AlterField( + model_name="version", + name="modified", + field=django_extensions.db.fields.ModificationDateTimeField( + auto_now=True, + default=django.utils.timezone.now, + verbose_name="modified", + ), + preserve_default=False, + ), + ] diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 926984456f7..5947c9d7a0d 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -13,7 +13,6 @@ from django.utils import timezone from django.utils.translation import gettext from django.utils.translation import gettext_lazy as _ -from django_extensions.db.fields import CreationDateTimeField, ModificationDateTimeField from django_extensions.db.models import TimeStampedModel from polymorphic.models import PolymorphicModel @@ -92,19 +91,6 @@ class Version(TimeStampedModel): """Version of a ``Project``.""" - # Overridden from TimeStampedModel just to allow null values. - # TODO: remove after deploy. - created = CreationDateTimeField( - _('created'), - null=True, - blank=True, - ) - modified = ModificationDateTimeField( - _('modified'), - null=True, - blank=True, - ) - project = models.ForeignKey( Project, verbose_name=_('Project'), diff --git a/readthedocs/integrations/migrations/0013_set_timestamp_fields_as_no_null.py b/readthedocs/integrations/migrations/0013_set_timestamp_fields_as_no_null.py new file mode 100644 index 00000000000..81113f6a7bc --- /dev/null +++ b/readthedocs/integrations/migrations/0013_set_timestamp_fields_as_no_null.py @@ -0,0 +1,37 @@ +# Generated by Django 4.2.9 on 2024-02-01 20:10 + +import django.utils.timezone +import django_extensions.db.fields +from django.db import migrations +from django_safemigrate import Safe + + +class Migration(migrations.Migration): + safe = Safe.always + + dependencies = [ + ("integrations", "0012_migrate_timestamp_fields"), + ] + + operations = [ + migrations.AlterField( + model_name="integration", + name="created", + field=django_extensions.db.fields.CreationDateTimeField( + auto_now_add=True, + default=django.utils.timezone.now, + verbose_name="created", + ), + preserve_default=False, + ), + migrations.AlterField( + model_name="integration", + name="modified", + field=django_extensions.db.fields.ModificationDateTimeField( + auto_now=True, + default=django.utils.timezone.now, + verbose_name="modified", + ), + preserve_default=False, + ), + ] diff --git a/readthedocs/integrations/models.py b/readthedocs/integrations/models.py index ea812aee9fd..06a1d153f73 100644 --- a/readthedocs/integrations/models.py +++ b/readthedocs/integrations/models.py @@ -9,7 +9,6 @@ from django.utils.crypto import get_random_string from django.utils.safestring import mark_safe from django.utils.translation import gettext_lazy as _ -from django_extensions.db.fields import CreationDateTimeField, ModificationDateTimeField from django_extensions.db.models import TimeStampedModel from pygments import highlight from pygments.formatters import HtmlFormatter @@ -278,19 +277,6 @@ class Integration(TimeStampedModel): INTEGRATIONS = WEBHOOK_INTEGRATIONS - # Overridden from TimeStampedModel just to allow null values. - # TODO: remove after deploy. - created = CreationDateTimeField( - _("created"), - null=True, - blank=True, - ) - modified = ModificationDateTimeField( - _("modified"), - null=True, - blank=True, - ) - project = models.ForeignKey( Project, related_name="integrations", diff --git a/readthedocs/projects/fixtures/test_data.json b/readthedocs/projects/fixtures/test_data.json index c3f28045f75..cfc3284f9d6 100644 --- a/readthedocs/projects/fixtures/test_data.json +++ b/readthedocs/projects/fixtures/test_data.json @@ -888,8 +888,8 @@ "model": "builds.version", "pk": 1, "fields": { - "created": null, - "modified": null, + "created": "2022-02-22T10:55:46.784Z", + "modified": "2022-02-22T10:55:46.784Z", "project": 1, "type": "unknown", "identifier": "2ff3d36340fa4d3d39424e8464864ca37c5f191c", @@ -912,8 +912,8 @@ "model": "builds.version", "pk": 2, "fields": { - "created": null, - "modified": null, + "created": "2022-02-22T10:55:46.784Z", + "modified": "2022-02-22T10:55:46.784Z", "project": 1, "type": "unknown", "identifier": "354456a7dba2a75888e2fe91f6d921e5fe492bcd", @@ -936,8 +936,8 @@ "model": "builds.version", "pk": 3, "fields": { - "created": null, - "modified": null, + "created": "2022-02-22T10:55:46.784Z", + "modified": "2022-02-22T10:55:46.784Z", "project": 1, "type": "unknown", "identifier": "master", @@ -960,8 +960,8 @@ "model": "builds.version", "pk": 4, "fields": { - "created": null, - "modified": null, + "created": "2022-02-22T10:55:46.784Z", + "modified": "2022-02-22T10:55:46.784Z", "project": 1, "type": "unknown", "identifier": "not_ok", @@ -984,8 +984,8 @@ "model": "builds.version", "pk": 8, "fields": { - "created": null, - "modified": null, + "created": "2022-02-22T10:55:46.784Z", + "modified": "2022-02-22T10:55:46.784Z", "project": 1, "type": "unknown", "identifier": "awesome", @@ -1008,8 +1008,8 @@ "model": "builds.version", "pk": 18, "fields": { - "created": null, - "modified": null, + "created": "2022-02-22T10:55:46.784Z", + "modified": "2022-02-22T10:55:46.784Z", "project": 6, "type": "tag", "identifier": "2404a34eba4ee9c48cc8bc4055b99a48354f4950", @@ -1032,8 +1032,8 @@ "model": "builds.version", "pk": 19, "fields": { - "created": null, - "modified": null, + "created": "2022-02-22T10:55:46.784Z", + "modified": "2022-02-22T10:55:46.784Z", "project": 6, "type": "tag", "identifier": "f55c28e560c92cafb6e6451f8084232b6d717603", diff --git a/readthedocs/projects/migrations/0114_set_timestamp_fields_as_no_null.py b/readthedocs/projects/migrations/0114_set_timestamp_fields_as_no_null.py new file mode 100644 index 00000000000..ce4b1c5b426 --- /dev/null +++ b/readthedocs/projects/migrations/0114_set_timestamp_fields_as_no_null.py @@ -0,0 +1,33 @@ +# Generated by Django 4.2.9 on 2024-02-01 20:10 + +import django.utils.timezone +from django.db import migrations, models +from django_safemigrate import Safe + + +class Migration(migrations.Migration): + safe = Safe.always + + dependencies = [ + ("projects", "0113_disable_analytics_addons"), + ] + + operations = [ + migrations.AlterField( + model_name="domain", + name="skip_validation", + field=models.BooleanField( + default=False, verbose_name="Skip validation process." + ), + ), + migrations.AlterField( + model_name="domain", + name="validation_process_start", + field=models.DateTimeField( + auto_now_add=True, + default=django.utils.timezone.now, + verbose_name="Start date of the validation process.", + ), + preserve_default=False, + ), + ] diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index 82ded4eb40f..f4f708ee31f 100644 --- a/readthedocs/projects/models.py +++ b/readthedocs/projects/models.py @@ -1830,14 +1830,10 @@ class Domain(TimeStampedModel): skip_validation = models.BooleanField( _("Skip validation process."), default=False, - # TODO: remove after deploy. - null=True, ) validation_process_start = models.DateTimeField( _("Start date of the validation process."), auto_now_add=True, - # TODO: remove after deploy. - null=True, ) # Strict-Transport-Security header options diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 453ad881ec4..37bd7a1a631 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -237,6 +237,7 @@ def INSTALLED_APPS(self): # noqa 'simple_history', 'djstripe', 'django_celery_beat', + "django_safemigrate.apps.SafeMigrateConfig", # our apps 'readthedocs.projects', diff --git a/requirements/deploy.txt b/requirements/deploy.txt index d0a0a7512a6..e51079ae4a4 100644 --- a/requirements/deploy.txt +++ b/requirements/deploy.txt @@ -110,6 +110,7 @@ django==4.2.10 # django-filter # django-formtools # django-polymorphic + # django-safemigrate # django-storages # django-structlog # django-taggit @@ -150,12 +151,12 @@ django-ipware==5.0.2 # django-structlog django-polymorphic==3.1.0 # via -r requirements/pip.txt +django-safemigrate==4.2 + # via -r requirements/pip.txt django-simple-history==3.0.0 # via -r requirements/pip.txt django-storages[boto3]==1.14.2 - # via - # -r requirements/pip.txt - # django-storages + # via -r requirements/pip.txt django-structlog==2.2.0 # via -r requirements/pip.txt django-taggit==5.0.1 @@ -286,7 +287,6 @@ pyjwt[crypto]==2.8.0 # via # -r requirements/pip.txt # django-allauth - # pyjwt pyquery==2.0.0 # via -r requirements/pip.txt python-crontab==3.0.0 diff --git a/requirements/docker.txt b/requirements/docker.txt index f6754acb783..8a3aee0802e 100644 --- a/requirements/docker.txt +++ b/requirements/docker.txt @@ -121,6 +121,7 @@ django==4.2.10 # django-filter # django-formtools # django-polymorphic + # django-safemigrate # django-storages # django-structlog # django-taggit @@ -161,12 +162,12 @@ django-ipware==5.0.2 # django-structlog django-polymorphic==3.1.0 # via -r requirements/pip.txt +django-safemigrate==4.2 + # via -r requirements/pip.txt django-simple-history==3.0.0 # via -r requirements/pip.txt django-storages[boto3]==1.14.2 - # via - # -r requirements/pip.txt - # django-storages + # via -r requirements/pip.txt django-structlog==2.2.0 # via -r requirements/pip.txt django-taggit==5.0.1 @@ -315,7 +316,6 @@ pyjwt[crypto]==2.8.0 # via # -r requirements/pip.txt # django-allauth - # pyjwt pyproject-api==1.6.1 # via tox pyquery==2.0.0 diff --git a/requirements/pip.in b/requirements/pip.in index 26026dc12d0..01ea60f618c 100644 --- a/requirements/pip.in +++ b/requirements/pip.in @@ -36,6 +36,8 @@ django-vanilla-views # dependency as well. jsonfield +django-safemigrate + requests requests-toolbelt slumber diff --git a/requirements/pip.txt b/requirements/pip.txt index 9da7e37b52e..702faca37fc 100644 --- a/requirements/pip.txt +++ b/requirements/pip.txt @@ -73,6 +73,7 @@ django==4.2.10 # django-filter # django-formtools # django-polymorphic + # django-safemigrate # django-storages # django-structlog # django-taggit @@ -113,6 +114,8 @@ django-ipware==5.0.2 # django-structlog django-polymorphic==3.1.0 # via -r requirements/pip.in +django-safemigrate==4.2 + # via -r requirements/pip.in django-simple-history==3.0.0 # via -r requirements/pip.in django-storages[boto3]==1.14.2 @@ -198,9 +201,7 @@ pycparser==2.21 pygments==2.17.2 # via -r requirements/pip.in pyjwt[crypto]==2.8.0 - # via - # django-allauth - # pyjwt + # via django-allauth pyquery==2.0.0 # via -r requirements/pip.in python-crontab==3.0.0 diff --git a/requirements/testing.txt b/requirements/testing.txt index 346e41cef2c..87f2cab76b0 100644 --- a/requirements/testing.txt +++ b/requirements/testing.txt @@ -71,9 +71,7 @@ click-repl==0.3.0 # -r requirements/pip.txt # celery coverage[toml]==7.4.1 - # via - # coverage - # pytest-cov + # via pytest-cov cron-descriptor==1.4.3 # via # -r requirements/pip.txt @@ -113,6 +111,7 @@ django==4.2.10 # django-filter # django-formtools # django-polymorphic + # django-safemigrate # django-storages # django-structlog # django-taggit @@ -155,12 +154,12 @@ django-ipware==5.0.2 # django-structlog django-polymorphic==3.1.0 # via -r requirements/pip.txt +django-safemigrate==4.2 + # via -r requirements/pip.txt django-simple-history==3.0.0 # via -r requirements/pip.txt django-storages[boto3]==1.14.2 - # via - # -r requirements/pip.txt - # django-storages + # via -r requirements/pip.txt django-structlog==2.2.0 # via -r requirements/pip.txt django-taggit==5.0.1 @@ -288,7 +287,6 @@ pyjwt[crypto]==2.8.0 # via # -r requirements/pip.txt # django-allauth - # pyjwt pyquery==2.0.0 # via -r requirements/pip.txt pytest==8.0.0