Ignore soft applied migrations in consistency check

[Brian May]

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

[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"!

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

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

[Raphaël Hertzog]

Replying to 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/commits/ticket_28250

(note that I rebased and forced push that branch twice since my initial comment, now it's at least test-built)

[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?

[Marten Kenbeek]

You should be able to get the Migration object like this:

self.graph.nodes[parent]

[Raphaël Hertzog]

Replying to 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.

[Marten Kenbeek]

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.

You can check this with if migration[0] != parent[0].

I've written the following test for your 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, mock_detect_soft_applied):
        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. I've had a chance to run the test, and it seems to be correct.

Note that detect_soft_applied returns a 2-tuple of (<is_applied>, <state_after_migration>), which is non-empty and thus truthy, so you need to check for the first item in the return value.

[Marten Kenbeek]

I've created a ​PR with some additional changes.

[Raphaël Hertzog]

Replying to Marten Kenbeek:

I've created a ​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

[Marten Kenbeek]

Replying to 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.

[Raphaël Hertzog]

Replying to 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

[Carlton Gibson]

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.