Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#22788 closed Bug (fixed)

Custom migration operations are rewritten incorrectly

Reported by: schinckel Owned by: nobody
Component: Migrations Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

22788.diff (1.1 KB) - added by schinckel 2 years ago.
Better patch, containing tests.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 2 years ago by schinckel

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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).

comment:2 Changed 2 years ago by mardini

  • Has patch set

comment:3 Changed 2 years ago by bmispelon

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

Hi,

This seems reasonable but the patch needs some tests.

Thanks.

Changed 2 years ago by schinckel

Better patch, containing tests.

comment:4 Changed 2 years ago 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.

comment:5 Changed 2 years ago by schinckel

I have a branch (almost) ready for a Pull Request.

https://github.com/schinckel/django/tree/ticket_22788

comment:6 Changed 2 years ago 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.

comment:7 Changed 2 years ago 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

))

comment:8 Changed 2 years ago 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!

comment:9 Changed 2 years ago by anonymous

I've submitted a pull request.

Last edited 2 years ago by timo (previous) (diff)

comment:10 Changed 2 years ago by timo

  • Needs tests unset

comment:11 Changed 2 years ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 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.

comment:12 Changed 2 years ago by Tim Graham <timograham@…>

In 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

Note: See TracTickets for help on using tickets.
Back to Top