[Django] #22788: Custom migration operations are rewritten incorrectly
Django
----------------------------+--------------------
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
I've written a custom migration operation, and was in the process of
writing a management command to write a migration file, using the
MigrationWriter class.
It's almost all perfect, except that the MigrationWriter class assumes
that the migration operation will be in django.db.migrations:
https://github.com/django/django/blob/master/django/db/migrations/writer.py#L47
I suspect that the MigrationWriter will also be used for things like
squashing migrations, so this problem may appear with custom migration
operations then.
I suggest that it should inspect the migration operation, and if it's not
in the `django.db.migrations` module, add the relevant module to the
imports, and use the correct path for the operation.
--
Ticket URL: <https://code.djangoproject.com/ticket/22788>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by schinckel):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
So I've uploaded a patch that looks at the operation class, and decides if
it should add an import (and use the fully qualified path for the
operation).
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:1>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mardini):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:2>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bmispelon):
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Hi,
This seems reasonable but the patch needs some tests.
Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:3>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by schinckel):
Better patch does contain tests, but I couldn't come up with a way to make
the test fail when two classes have the same name, but a different path.
I needed to import the class using `from ... import ...` syntax, as
otherwise there were issues related to namespacing and the term
migrations, which I may try to work out.
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:4>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by schinckel):
I have a branch (almost) ready for a Pull Request.
https://github.com/schinckel/django/tree/ticket_22788
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:5>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by schinckel):
The issue I have hit with this relates only to the django test framework.
Basically, the tests for this part live in the top-level 'migrations'
package (when running the tests). Defining an operation within this
package conflicts with the import in _every_ migrations file:
from django.db import migrations
This means I've had to import the custom operation directly (i.e., not
namespaced).
Which then in turn means you cannot have two migration operations with the
same name (that are actually different classes).
That is, I can import the `foo.operations.Operation` and `bar.Operation`
in a normal situation, but can't actually write a passing test in the
normal django test system.
I could mangle the names on import "from foo.operations import Operation
as foo_operations_Operation", but this seems messy.
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:6>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by schinckel):
Perhaps code speaks louder than words.
The replacement code for adding the migration operation name to the
migration file (and ensuring it is imported):
[OperationWriter.serialize()]
# See if this operation is in django.db.migrations. If it is,
# We can just use the fact we already have that imported,
# otherwise, we need to add an import for the operation class.
if getattr(migrations, name, None) == self.operation.__class__:
self.feed('migrations.%s(' % name)
else:
# This version works for the django tests, but fails if
# you have two identical named operations.
imports.add('from %s import %s' % (
self.operation.__class__.__module__, name
))
self.feed('%s(' % (name))
# This version works normally, including with same-named
operations,
# but fails in the django tests, because
# self.operation.__class__.__module__ will start with
'migrations'
imports.add('import %s' % self.operation.__class__.__module__)
self.feed('%s.%s(' % (
self.operation.__class__.__module__, name
))
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:7>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
schinckel: I suspect you can get around this problem if you put this test
in its own test app called custom_migration_operation ?
(We already have a few things in separate apps for a variety of reasons,
they don't all need to be bundled into migrations)
Otherwise, the patch looks good. Sorry I overlooked this!
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:8>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anonymous):
I've submitted a pull request.
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:9>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:10>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"bb39037fcbe07a4c4060764533b5c03a4018bf81"]:
{{{
#!CommitTicketReference repository=""
revision="bb39037fcbe07a4c4060764533b5c03a4018bf81"
Fixed #22788 -- Ensured custom migration operations can be written.
This inspects the migration operation, and if it is not in the
django.db.migrations module, it adds the relevant imports to the
migration writer and uses the correct class name.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:11>
Django
Reporter: schinckel | Owner: nobody
Type: Bug | Status: closed
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2dba6ab767d8532df5f9ea2b58325ef823d3c6f8"]:
{{{
#!CommitTicketReference repository=""
revision="2dba6ab767d8532df5f9ea2b58325ef823d3c6f8"
[1.7.x] Fixed #22788 -- Ensured custom migration operations can be
written.
This inspects the migration operation, and if it is not in the
django.db.migrations module, it adds the relevant imports to the
migration writer and uses the correct class name.
Backport of bb39037fcb from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22788#comment:12>