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

Use django-safemigrate for migrations #11087

Merged
merged 9 commits into from
Feb 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion common
1 change: 1 addition & 0 deletions docs/dev/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
133 changes: 133 additions & 0 deletions docs/dev/migrations.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
Database migrations
===================

We use `Django migrations <https://docs.djangoproject.com/en/4.2/topics/migrations/>`__ to manage database schema changes,
and the `django-safemigrate <https://github.com/aspiredu/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``.
Comment on lines +75 to +76
Copy link
Member

Choose a reason for hiding this comment

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

Can't this migration be created in the same deploy and marked as after_deploy to make everything in the same deploy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if that will work, but we still need to another deploy to remove all handling of the null case in the code.

- #. 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.
stsewd marked this conversation as resolved.
Show resolved Hide resolved

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``.
32 changes: 32 additions & 0 deletions readthedocs/builds/migrations/0057_migrate_timestamp_fields.py
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we use a date that it's obvious it's wrong to avoid confusions here? Like 1999-01-01 or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that would add more confusion, for us and users (if we expose this field).

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),
]
Original file line number Diff line number Diff line change
@@ -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,
),
]
14 changes: 0 additions & 14 deletions readthedocs/builds/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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'),
Expand Down
Original file line number Diff line number Diff line change
@@ -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
stsewd marked this conversation as resolved.
Show resolved Hide resolved

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,
),
]
14 changes: 0 additions & 14 deletions readthedocs/integrations/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down
28 changes: 14 additions & 14 deletions readthedocs/projects/fixtures/test_data.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down
Loading