[Django] #28250: migration depending on non-existing legacy migration
Django
-----------------------------------------+------------------------
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
Hello,
As from http://bugs.debian.org/863267:
There exists a package (lava-server) that has a migration (called
`lava_scheduler_app.0001_initial`) with the following dependencies:
{{{
dependencies = [
...
('linaro_django_xmlrpc', '__first__'),
...
]
}}}
This migration was applied in prehistoric times, when Django 1.7 ruled the
earth. Unfortunately, at this stage, `linaro_django_xmlrpc` had no
migrations. Django 1.7 was fine with this. Everybody was happy.
Sometime after this, linaro_django_xmlrpc got a migration.
`0001_initial.py`. So we now have a migration installed, but its
dependencies are not installed. Or rather the database tables are there,
but the migration never has been registered in Django's table of
migrations.
Presumably Django 1.7 was happy with this, as Django 1.8 was also very
happy to deal with this. However Debian Jessie has Django 1.7 with a
version of linaro_django_xmlrpc before the migrations were introduced. Now
we are about to release Debian Stretch with Django 1.10 and the
linaro_django_xmlrpc migration.
So we have this jump from: Debian Jessie - Django 1.7 + migration not
included
to: Debian Stretch - Django 1.10 + migration included
Unfortunately Django 1.10 (and I am guessing also Django 1.11) is not so
agreeable. It immediately runs a check on all the prerequisites and aborts
with an error.
{{{
neil@jessie:~$ sudo lava-server manage migrate linaro_django_xmlrpc
--fake-initial
.....
django.db.migrations.exceptions.InconsistentMigrationHistory: Migration
lava_scheduler_app.0001_initial is applied before its dependency
linaro_django_xmlrpc.0001_initial on database 'default'.
}}}
This means that the normal avenue of creating the required fake
transaction, does not work here, as Django 1.10 will not let us do this.
I seem to recall that mixing apps with migrations with apps without
migrations is somewhat complicated. Apologies if this particular situation
is documented somewhere (references would be good).
This issue was bought to light in the middle of a heated discussion on the
backports mailing list, with some people claiming that upgrading to Django
1.10 will delete user data. Hopefully I corrected this misconception. To
me it looks like no data is touched. However there is still a release
critical bug over Django 1.10, as the upgrade of this package fails.
Is this behaviour, as I described expected?
Is this behaviour a bug in Django? Or a feature?
Is there any work around possible to get Django 1.10 to process the
migrations?
Thanks
--
Ticket URL: <https://code.djangoproject.com/ticket/28250>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
I'm not the best specialist on Django migrations, but I guess when things
go wrong this way, some manual intervention is required.
I would suggest a custom repair script along these lines (absolutely
untested) which should add the missing line in the migrations table:
{{{
from django.db.migrations.recorder import MigrationRecorder
from django.db import DEFAULT_DB_ALIAS, connections
connection = connections[DEFAULT_DB_ALIAS]
recorder = MigrationRecorder(connection)
recorder.record_applied('linaro_django_xmlrpc', '0001_initial')
}}}
I'll let others answer about whether this is a bug Django could fix, like
providing an option like "I know things are broken, I know what I'm doing,
please do not check migrations consistency"!
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:1>
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin):
1.7 introduced the new migration system but still supported apps without
migrations. This possibility must have been removed in 1.9 (I didn't
check) given that we hadn't tied the deprecation schedule to LTS releases
yet. This means the missing migration must be created and fake-applied on
Django < 1.9 before moving on to later versions of Django.
If I read the Debian bug correctly, going through 1.7 => 1.8 => 1.10 and
running `migrate` at each step works. Somewhere in the middle of the
thread, it says: "1.10 requires it and it's code in 1.8 that handles the
transition." This is consistent with my hypothesis.
We've always suggested that users should go through every x.y version when
upgrading. We considered documenting LTS to LTS upgrades but didn't do it
because we didn't feel it would be very different from concatenating the
release notes for each intermediary version; we haven't heard much
specific feedback in this area.
Non-LTS to non-LTS across three versions is not a scenario we're trying to
support and also like not a scenario for which we can reasonably provide
adequate guarantees (unless someone's willing to back that with a fat
check). The combination matrix of such upgrades is awful.
In this particular case, it doesn't help 1.7 wasn't the most polished
release, because it contained both the migrations framework and the app
loading refactor. Going through 1.8 LTS is strongly recommended.
----
Coming back to the original bug report, I agree with Claude's
recommendation.
You must only do this if the tables were created by a previous version, or
else a fresh install of the most recent version of the package will fail.
IMO the logic should be: check if a table created on Django 1.7 already
exists in the database but there's no corresponding row in the
django_migrations table (this means you're hitting the problematic upgrade
path), and in that case create a suitable row in the django_migrations
table to fake-apply the migration. This can be done in SQL directly.
I believe this is stretching the limits of what's reasonable to do with a
package manager.
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:2>
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Raphaël Hertzog):
I have not looked very closely yet, but shouldn't the check added in
#25850 be loosened for this specific case, just like it has been loosened
in #27004 for squashed migrations?
I don't see why 1.10 or 1.11 should refuse to fake a migration that 1.8
was able to fake properly.
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:3>
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
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 Raphaël Hertzog):
* cc: hertzog@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:4>
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Raphaël Hertzog):
Replying to [comment:3 Raphaël Hertzog]:
> I have not looked very closely yet, but shouldn't the check added in
#25850 be loosened for this specific case, just like it has been loosened
in #27004 for squashed migrations?
Untested patch against 1.10.x with the last two commits in
https://github.com/rhertzog/django/tree/ticket_28250
https://github.com/rhertzog/django/commit/dfbd1fb02040bdc87fbad6459c1cece1052b7799
https://github.com/rhertzog/django/commit/cd8de7945a279a0103f6043ae9f8e090e386fc13
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:5>
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Raphaël Hertzog):
FWIW my patch currently fails with this exception (tested by Senthil
Kumaran, cf debian bug):
{{{
File "/usr/lib/python2.7/dist-packages/django/db/migrations/loader.py",
line 291, in check_consistent_history
if fake_initial and self.detect_soft_applied(None, parent):
File "/usr/lib/python2.7/dist-packages/django/db/migrations/loader.py",
line 343, in detect_soft_applied
if migration.initial is None:
AttributeError: 'Node' object has no attribute 'initial'
}}}
Do you know how I can get the correct Migration object from a Node object?
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:6>
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Marten Kenbeek):
You should be able to get the `Migration` object like this:
{{{
self.graph.nodes[parent]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:7>
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Component: Migrations | Version: 1.10
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Raphaël Hertzog):
Replying to [comment:7 Marten Kenbeek]:
> You should be able to get the `Migration` object like this:
>
> {{{
> self.graph.nodes[parent]
> }}}
Thanks, I updated my branch and asked the bug submitter to retry with
updated packages.
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:8>
Django
Reporter: Brian May | Owner: nobody
Type: Uncategorized | Status: new
Severity: Normal | 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 Marten Kenbeek):
* cc: marten.knbk@… (added)
* version: 1.10 => master
* stage: Unreviewed => Accepted
Comment:
Ideally, an exception will still be raised if the initial migration that
may be faked is in the same app as an applied migration that depends on
it. So if `app1.0001` is not applied, but is an initial migration that may
be faked, and `app1.0002` is recorder as applied, it will still be an
error. The executor should never apply `app1.0002` without faking
`app1.0001`, so that state should be unreachable under normal
circumstances.
I believe you can check this with `if migration[0] != parent[0]`.
I've written the following test for you branch, which should go into
`migrations.test_loader.LoaderTests`:
{{{
@override_settings(MIGRATION_MODULES={
"migrations": "migrations.test_migrations_first",
"migrations2": "migrations2.test_migrations_2_first",
})
@modify_settings(INSTALLED_APPS={'append': 'migrations2'})
@mock.patch.object(MigrationLoader, 'detect_soft_applied',
return_value=(True, None))
def test_check_consistent_history_fake_initial(self):
loader = MigrationLoader(connection)
recorder = MigrationRecorder(connection)
recorder.record_applied('migrations2', '0001_initial')
recorder.record_applied('migrations2', '0002_second')
loader.check_consistent_history(connection, fake_initial=True)
recorder.record_applied('migrations', 'second')
msg = (
"Migration migrations.second is applied before its dependency
"
"migrations.thefirst on database 'default'."
)
with self.assertRaisesMessage(InconsistentMigrationHistory, msg):
loader.check_consistent_history(connection, fake_initial=True)
}}}
This also tests for the same-app scenario. I haven't checked if the test
is actually correct, and my mocking skills aren't too sharp, but this
should get you a long way.
Note that `detect_soft_applied` returns a 2-tuple of `(<is_applied>,
<state_after_migration>)`, so you need to check for the first item in the
return value.
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:9>
Django
Reporter: Brian May | Owner: Marten Kenbeek
Type: Uncategorized | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Marten Kenbeek):
* owner: nobody => Marten Kenbeek
* status: new => assigned
* has_patch: 0 => 1
Comment:
I've created a [https://github.com/django/django/pull/8577 PR] with some
additional changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:10>
Django
Reporter: Brian May | Owner: Marten Kenbeek
Type: Uncategorized | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Raphaël Hertzog):
Replying to [comment:10 Marten Kenbeek]:
> I've created a [https://github.com/django/django/pull/8577 PR] with some
additional changes.
Thanks Marten for that work. I asked the lava-server maintainer to test
this patch, unfortunately the "migrate" call is still failing for him,
albeit with another error further down the road:
{{{
Operations to perform:
Apply all migrations: admin, auth, contenttypes, dashboard_app,
google_analytics, lava_results_app, lava_scheduler_app,
linaro_django_xmlrpc, sessions, sites
Traceback (most recent call last):
File "/usr/bin/lava-server", line 78, in <module>
main()
File "/usr/bin/lava-server", line 74, in main
execute_from_command_line(django_options)
File "/usr/lib/python2.7/dist-
packages/django/core/management/__init__.py", line 367, in
execute_from_command_line
utility.execute()
File "/usr/lib/python2.7/dist-
packages/django/core/management/__init__.py", line 359, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/usr/lib/python2.7/dist-packages/django/core/management/base.py",
line 294, in run_from_argv
self.execute(*args, **cmd_options)
File "/usr/lib/python2.7/dist-packages/django/core/management/base.py",
line 345, in execute
output = self.handle(*args, **options)
File "/usr/lib/python2.7/dist-
packages/django/core/management/commands/migrate.py", line 164, in handle
pre_migrate_apps = pre_migrate_state.apps
File "/usr/lib/python2.7/dist-packages/django/utils/functional.py", line
35, in __get__
res = instance.__dict__[self.name] = self.func(instance)
File "/usr/lib/python2.7/dist-packages/django/db/migrations/state.py",
line 176, in apps
return StateApps(self.real_apps, self.models)
File "/usr/lib/python2.7/dist-packages/django/db/migrations/state.py",
line 249, in __init__
raise ValueError("\n".join(error.msg for error in errors))
ValueError: The field lava_scheduler_app.TestJob.submit_token was declared
with a lazy reference to 'linaro_django_xmlrpc.authtoken', but app
'linaro_django_xmlrpc' isn't installed.
}}}
But `linaro_django_xmlrpc` definitely is in the INSTALLED_APPS setting. So
I'm not sure what the problem is... if you want to have a look at the
lava-server code, it's over here:
The migrate operation is done with this version installed:
https://sources.debian.net/src/lava-server/2016.12-1/
But the application's database was created with this version:
https://sources.debian.net/src/lava-server/2014.09.1-1/
Git repo if you want fine-grained history: https://github.com/Linaro/lava-
server
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:11>
Django
Reporter: Brian May | Owner: Marten Kenbeek
Type: Uncategorized | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Marten Kenbeek):
Replying to [comment:11 Raphaël Hertzog]:
> Thanks Marten for that work. I asked the lava-server maintainer to test
this patch, unfortunately the "migrate" call is still failing for him,
albeit with another error further down the road:
That seems to be because the project state generated for the `pre_migrate`
signal only includes state changes of migrations that are applied. Since
the initial migration is still marked as not applied, its state changes
are not propagated to the project state. I've updated the PR to fix this
issue as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:12>
Django
Reporter: Brian May | Owner: Marten Kenbeek
Type: Uncategorized | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Raphaël Hertzog):
Replying to [comment:12 Marten Kenbeek]:
> That seems to be because the project state generated for the
`pre_migrate` signal only includes state changes of migrations that are
applied. Since the initial migration is still marked as not applied, its
state changes are not propagated to the project state. I've updated the PR
to fix this issue as well.
Thanks! I got the confirmation that the upgrade is now working fine with
the last version of your pull request. It would be great to get this
merged and backported to 1.10 and 1.11.
The backport for 1.10 that I'm using is available in
https://github.com/rhertzog/django/commits/ticket_28250
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Brian May | Owner: Marten
Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* type: Uncategorized => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Brian May | Owner: Marten
Type: | Kenbeek
Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
Comment:
Marten's PR looks good. It just needs minor tweaks and it should be RFC.
Comments on PR.
Please uncheck Patch needs improvement when it's ready and we can have
another look.
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:15>
Django
Reporter: Brian May | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Marten Kenbeek => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:16>
Django
--------------------------------------+------------------------------------
Reporter: Brian May | Owner: (none)
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28250#comment:17>