Opened 10 years ago

Closed 10 years ago

Last modified 3 years ago

#22983 closed Bug (fixed)

Invalid import in a squashed migration of migrations having RunPython

Reported by: Piotr Maliński Owned by: Andrew Godwin
Component: Migrations Version: 1.7-rc-1
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I've squashed migrations in one of my apps and I've noticed that every migration that had a custom function on RunPython resulted in an invalid import in the squashed migration like:

import my_app.migrations.0010_clean_propertyindex

and then it tries to use it:

migrations.RunPython(
            code=my_app.migrations.0010_clean_propertyindex.delete_indexes,
            reverse_code=None,
            atomic=True,
        ),

It's either a bug or all functions should not be in a file that starts with a digit. Either way squash should fail if it would have to make such import.

Change History (14)

comment:1 by Tim Graham, 10 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug
Version: 1.61.7-rc-1

comment:2 by Andrew Godwin, 10 years ago

Owner: changed from nobody to Andrew Godwin
Status: newassigned

Ah, this is a tricky one; we're not able to squash the custom functions, as they're not serialisable. This is possibly why I had initially planned RunPython to take strings rather than function objects.

I don't want to make squash hard fail on this kind of error, though, as the fix from the result to a working version is easy (just copy the functions over), whereas squashing manually is almost impossible to do quickly or easily. I think I'll make it leave the function references in place, but remove the valid imports and have squashmigrations output that you need to copy the functions over.

comment:3 by Claude Paroz, 10 years ago

Is the problem mainly caused by migration files starting with a number? I have already been bitten by this once. I know it's late, but is it still the time to ask if the migration files could/should be prefixed with a letter to make them importable?

comment:4 by Piotr Maliński, 10 years ago

That output will be essential as developers will be using it a lot and it's a new feature which is not yet known well (so it should be a bit verbose).

Side problem is that RunPython limits squashing efficiency of model changes etc. made after migrations with it. Can squash can be squashed after refactoring order of RunPythons in the squash?

comment:5 by Andrew Godwin, 10 years ago

claudep: No, the problem is not the number prefix; the old files are meant to be deleted anyway, not imported from. I know it's slightly wrong to have python files starting with a number but I kind of like that you can't import them; they're not meant to be a library resource, and there's no reason you should ever import one by name (rather than loading them dynamically like Django does)

You can, indeed, re-squash a squash once you've reordered RunPython operations in it, but you'll need to go through the whole squash - apply squash - delete old migrations dance first before squashing a second time. There might be room for an in-place optimize command that just optimizes a single file, but that would have to be a 1.8 feature at this point (one could use the Django migration APIs to write that command reasonably easily anyway)

comment:6 by Claude Paroz, 10 years ago

Andrew: note taken, thanks!

comment:7 by Matthew Schinckel, 10 years ago

Could we not do something within django.db.migrations.writer.MigrationWriter, that looks at the path of the operation, and rewrites the function definition into the new migration file if the function is defined in the same file as the migration was loaded from?

in reply to:  7 comment:8 by Simon Charette, 10 years ago

Replying to schinckel:

Could we not do something within django.db.migrations.writer.MigrationWriter, that looks at the path of the operation, and rewrites the function definition into the new migration file if the function is defined in the same file as the migration was loaded from?

I guess we could use inspect.getsource for this but it wouldn't take non local objects references (imports the function depends on for example) into account.

comment:9 by Andrew Godwin, 10 years ago

No, as there is no way to cleanly get a function's source code given a pointer to it. You can try using inspect, but you'd need to also take into account any custom imports in the file, any global variables, any extra functions, if the callable is actually a class, and a number of other issues - that is, if you can even read the migration file in the first place (which is likely in this case, though, as you're writing another one, but it's possible someone's compiled everything to .pyc and removed the python files)

The simplest way is to just ask the developer to port it over, as they're in the best position to be able to notice and correct for all of these things.

comment:10 by generalov, 10 years ago

I put all data migration functions in separate module migrations/datamigrations/__init__.py then import code from it to migration module.

# 0002_some_migration.py 
from . import datamigrations

class Migration(migrations.Migration):
    operations = [
        migrations.RunPython(datamigrations.fix_data),

In this case the squashmigrations builds the following result:

import project.app.migrations.datamigrations

class Migration(migrations.Migration):
    operations = [
        migrations.RunPython(
            code=project.app.migrations.datamigrations.fill_serial,
            reverse_code=None,
            atomic=True,
        ),

The code looks bit ugly but it works without any modifications.

I create datamigration as package instead of just file module beacause otherwise manage.py squashmigrations .. fails with exception django.db.migrations.loader.BadMigrationError: Migration datamigrations in app app has no Migration class

comment:11 by Andrew Godwin, 10 years ago

Yes, I guess that works. The reason you get the error is because any python file in the migrations directory is assumed to be a migration. Not sure I'd recommend that pattern for people's general use, though, I like encouraging having all the implementation in one file for easier review.

comment:12 by Andrew Godwin <andrew@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In ebb13bbd884d8c3053d1d342ef0423240feb05e6:

Fixed #22983: Alert when squashing RunPython operations with referred functions.

comment:13 by Andrew Godwin <andrew@…>, 10 years ago

In 563046a7de24d5e1eac1d455ec5530d0479186e0:

[1.7.x] Fixed #22983: Alert when squashing RunPython operations with referred functions.

comment:14 by GitHub <noreply@…>, 3 years ago

In 2d07e1aa:

Refs #22983 -- Added tests for squashing migrations with functions from migration files.

Follow up to ebb13bbd884d8c3053d1d342ef0423240feb05e6.

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