[Django] #30870: "migrate --plan" outputs "IRREVERSIBLE" on RunPython operation when no docstring present
Django
docstring present
-------------------------------------+-------------------------------------
Reporter: | Owner: nobody
kdickerson |
Type: Bug | Status: new
Component: Core | Version: 2.2
(Management commands) |
Severity: Normal | Keywords: migrate
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
Given a migration like:
{{{#!python
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.
{{{#!python
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
{{{#!python
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:
{{{#!python
@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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30870>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: nobody
Type: Bug | Status: new
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:1>
Django
docstrings.
-------------------------------------+-------------------------------------
| Ramezani
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Hasan Ramezani
Comment:
@Kyle Dickerson, if you want to prepare a patch just reassign the ticket
to yourself.
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:2>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:3>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by felixxm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:4>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:5>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by Kyle Dickerson):
I believe the patch should be
{{{#!python
- action = code.__doc__ if code else ''
+ action = (code.__doc__ or '') if code else None
}}}
rather than
{{{#!python
- 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:6>
Django
docstrings.
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
* owner: Hasan Ramezani => felixxm
Comment:
I'm going to push this forward today. Hasan, thanks for the initial patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:7>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: felixxm
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by Hasan Ramezani):
The patch is ready. I am going to clean it and add test for it. I can push
it today.
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:8>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: felixxm
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by felixxm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:9>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: felixxm
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by felixxm):
Replying to [comment:6 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:10>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: felixxm
Type: Bug | Status: assigned
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution:
Keywords: migrate | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by felixxm):
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* easy: 1 => 0
* needs_docs: 1 => 0
Comment:
[https://github.com/django/django/pull/11911 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:11>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: felixxm
Component: Core (Management | Version: 2.2
commands) |
Keywords: migrate | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"06d34aab7cfb1632a1538a243db81f24498525ff" 06d34aab]:
{{{
#!CommitTicketReference repository=""
revision="06d34aab7cfb1632a1538a243db81f24498525ff"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:12>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: felixxm
Type: Bug | Status: closed
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution: fixed
Keywords: migrate | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"4a756cbc383bdefb683f03a0d40c0d0630ba3f60" 4a756cb]:
{{{
#!CommitTicketReference repository=""
revision="4a756cbc383bdefb683f03a0d40c0d0630ba3f60"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:13>
Django
docstrings.
-------------------------------------+-------------------------------------
Reporter: Kyle Dickerson | Owner: felixxm
Type: Bug | Status: closed
Component: Core (Management | Version: 2.2
commands) |
Severity: Release blocker | Resolution: fixed
Keywords: migrate | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"d2f02aa56b57f8b3de1b624f8b1a909c67b6823a" d2f02aa5]:
{{{
#!CommitTicketReference repository=""
revision="d2f02aa56b57f8b3de1b624f8b1a909c67b6823a"
[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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30870#comment:14>