"migrate --plan" outputs "IRREVERSIBLE" on RunPython operations without docstrings.

[Kyle Dickerson]

Given a migration like:

from django.db import migrations

def forward(apps, schema_editor):
    pass

def reverse(apps, schema_editor):
    pass

class Migration(migrations.Migration):
    operations = [
        migrations.RunPython(forward, reverse)
    ]

manage.py migrate --plan will output:

Planned operations:
example.0001_initial
    Raw Python operation -> IRREVERSIBLE

The migration should not be described as "irreversible".

This error is in the definition of describe_operation in django/django/core/management/commands/migrate.py, reproduced below with line numbers from 2.2.6 tag.

343    @staticmethod
344    def describe_operation(operation, backwards):
345        """Return a string that describes a migration operation for --plan."""
346        prefix = ''
347        if hasattr(operation, 'code'):
348            code = operation.reverse_code if backwards else operation.code
349            action = code.__doc__ if code else ''
350        elif hasattr(operation, 'sql'):
351            action = operation.reverse_sql if backwards else operation.sql
352        else:
353            action = ''
354            if backwards:
355                prefix = 'Undo '
356        if action is None:
357            action = 'IRREVERSIBLE'
358            is_error = True
359        else:
360            action = str(action).replace('\n', '')
361            is_error = False
362        if action:
363            action = ' -> ' + action
364        truncated = Truncator(action)
365    return prefix + operation.describe() + truncated.chars(40), is_error

Line 349 uses the docstring as the output string.
Line 356 tests that value and sets action = 'IRREVERSIBLE' on line 357 because the dosctring is None.

It would appear that the intention is to use a docstring to describe the operation, if available, and leave it blank otherwise. However, because it tests against code instead of code.__doc__ it actually sets action = None resulting in 'IRREVERSIBLE' being displayed.

Proposed Solutions below

For a localized fix, I believe line 349 should be replaced by

        if code:
            action = code.__doc__ if code.__doc__ else ''
        else:
            action = None

However, a more holistic view suggests that displaying "IRREVERSIBLE" isn't really the correct thing to do. "IRREVERSIBLE" is set when is_error is also set to True and seems to be trying to indicate that the migration operation is invalid rather than irreversible. That is, if code/reverse_code is None (line 348) or sql/reverse_sql is None (line 351) the migration can't run.

Since sql and code are required parameters for their respective Operations, action should only possibly be None in the reverse case, which seems to be what this code is trying to capture and explain.

Given that, a better approach would probably make use of the reversible property defined on RunSQL and RunPython operations. This is a little verbose and could probably be pared down, but I think it has the right logic:

@staticmethod
def describe_operation(operation, backwards):
    """Return a string that describes a migration operation for --plan."""
    prefix = ''
    action = ''
    is_error = False

    if backwards:
        prefix = 'Undo '
        if hasattr(operation, 'reversible') and not operation.reversible:
            action = 'INVALID'
            is_error = True
        elif hasattr(operation, 'reverse_code'):
            action = operation.reverse_code.__doc__ if operation.reverse_code.__doc__ else ''
        elif hasattr(operation, 'reverse_sql'):
            action = operation.reverse_sql.__doc__ if operation.reverse_sql.__doc__ else ''
    else:
        if hasattr(operation, 'code'):
            action = operation.code.__doc__ if operation.code.__doc__ else ''
        elif hasattr(operation, 'sql'):
            action = operation.sql.__doc__ if operation.sql.__doc__ else ''

    action = ' -> ' + str(action).replace('\n', '')
    truncated = Truncator(action)
    return prefix + operation.describe() + truncated.chars(40), is_error

[Mariusz Felisiak]

Thanks for this report.

However, a more holistic view suggests that displaying "IRREVERSIBLE" isn't really the correct thing to do. "IRREVERSIBLE" is set when is_error is also set to True and seems to be trying to indicate that the migration operation is invalid rather than irreversible. That is, if code/reverse_code is None (line 348) or sql/reverse_sql is None (line 351) the migration can't run.

IRREVERSIBLE doesn't mean that migrations are invalid, it means that you cannot reverse them. is_error is to emphasize this as a warning (probably name is misleading). IMO a one line fix is acceptable for backporting

diff --git a/django/core/management/commands/migrate.py b/django/core/management/commands/migrate.py
index 37914e2622..5b5b96d1da 100644
--- a/django/core/management/commands/migrate.py
+++ b/django/core/management/commands/migrate.py
@@ -345,7 +345,7 @@ class Command(BaseCommand):
         prefix = ''
         if hasattr(operation, 'code'):
             code = operation.reverse_code if backwards else operation.code
-            action = code.__doc__ if code else ''
+            action = (code.__doc__ or '') if code else ''
         elif hasattr(operation, 'sql'):
             action = operation.reverse_sql if backwards else operation.sql
         else:

We could accept some refactoring with a reversible property but this should be done as a cleanup only on master.

[Hasan Ramezani]

@Kyle Dickerson, if you want to prepare a patch just reassign the ticket to yourself.

[Mariusz Felisiak]

After reconsideration I think we should display IRREVERSIBLE only for reverse migrations (backwards is True). As a cleanup we could also handle migrations.RunPython.noop and migrations.RunSQL.noop as a special case.

[Kyle Dickerson]

I believe the patch should be

-            action = code.__doc__ if code else ''
+            action = (code.__doc__ or '') if code else None

rather than

-            action = code.__doc__ if code else ''
+            action = (code.__doc__ or '') if code else ''

Or a RunPython operation will never show IRREVERSIBLE as action could never be None at line 356.

After reconsideration I think we should display IRREVERSIBLE only for reverse migrations (backwards is True).

The current code should have that behavior because, with this bug fix, action should only be None on line 356 if we set it based on either reverse_code or reverse_sql (since code and sql are required attributes of their respective operations), but it's not an explicit restriction in the current form. It would certainly be more readable to make that restriction explicit (e.g., in my proposed broader refactor).

[Mariusz Felisiak]

I'm going to push this forward today. Hasan, thanks for the initial patch.

[Hasan Ramezani]

The patch is ready. I am going to clean it and add test for it. I can push it today.

[Mariusz Felisiak]

This is a release blocker so it's quite urgent, I'm going to work on it now, and prepare Django 3.0b1 after that.

[Mariusz Felisiak]

Replying to Kyle Dickerson:

The current code should have that behavior because, with this bug fix, action should only be None on line 356 if we set it based on either reverse_code or reverse_sql (since code and sql are required attributes of their respective operations), but it's not an explicit restriction in the current form. It would certainly be more readable to make that restriction explicit (e.g., in my proposed broader refactor).

code and sql are required attributes but they can be empty, nevertheless I agree that IRREVERSIBLE should be displayed on for backwards.

[Mariusz Felisiak]

​PR

[Mariusz Felisiak <felisiak.mariusz@…>]

In 06d34aab:

Fixed #30870 -- Fixed showing that RunPython operations are irreversible by migrate --plan.

Thanks Hasan Ramezani for the initial patch and Kyle Dickerson for the
report.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4a756cb:

[3.0.x] Fixed #30870 -- Fixed showing that RunPython operations are irreversible by migrate --plan.

Thanks Hasan Ramezani for the initial patch and Kyle Dickerson for the
report.

Backport of 06d34aab7cfb1632a1538a243db81f24498525ff from master

[Mariusz Felisiak <felisiak.mariusz@…>]

In d2f02aa5:

[2.2.x] Fixed #30870 -- Fixed showing that RunPython operations are irreversible by migrate --plan.

Thanks Hasan Ramezani for the initial patch and Kyle Dickerson for the
report.

Backport of 06d34aab7cfb1632a1538a243db81f24498525ff from master.