[Django] #22983: Invalid import in a squashed migration of migrations having RunPython
Django
-------------------------------+--------------------
Reporter: riklaunim | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22983>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: riklaunim | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* severity: Normal => Release blocker
* needs_better_patch: => 0
* needs_tests: => 0
* version: 1.6 => 1.7-rc-1
* needs_docs: => 0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:1>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewgodwin):
* status: new => assigned
* owner: nobody => andrewgodwin
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:2>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:3>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by riklaunim):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:4>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
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)
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:5>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
Andrew: note taken, thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:6>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:7>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
Replying to [comment:7 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:8>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:9>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by generalov):
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`
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:10>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:11>
Django
Reporter: riklaunim | Owner: andrewgodwin
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andrew Godwin <andrew@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ebb13bbd884d8c3053d1d342ef0423240feb05e6"]:
{{{
#!CommitTicketReference repository=""
revision="ebb13bbd884d8c3053d1d342ef0423240feb05e6"
Fixed #22983: Alert when squashing RunPython operations with referred
functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:12>
Django
Reporter: riklaunim | Owner: andrewgodwin
Type: Bug | Status: closed
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Godwin <andrew@…>):
In [changeset:"563046a7de24d5e1eac1d455ec5530d0479186e0"]:
{{{
#!CommitTicketReference repository=""
revision="563046a7de24d5e1eac1d455ec5530d0479186e0"
[1.7.x] Fixed #22983: Alert when squashing RunPython operations with
referred functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:13>
Django
Reporter: Piotr Maliński | Owner: Andrew Godwin
Type: Bug | Status: closed
Component: Migrations | Version: 1.7-rc-1
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"2d07e1aaebadbd576ee2a055760a5866d4408a87" 2d07e1aa]:
{{{
#!CommitTicketReference repository=""
revision="2d07e1aaebadbd576ee2a055760a5866d4408a87"
Refs #22983 -- Added tests for squashing migrations with functions from
migration files.
Follow up to ebb13bbd884d8c3053d1d342ef0423240feb05e6.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22983#comment:14>