Opened 7 months ago

Last modified 5 weeks ago

#23273 new Bug

MigrationRecorder does not obey db_router allow_migrate rules

Reported by: froomzy Owned by: nobody
Component: Migrations Version: 1.7
Severity: Normal 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

Hi,

We have a multi-db setup. We have one connection that is for the django project, and several connections that talk to other dbs for information (ie models with managed = False). Django should only create tables in the first connection, never in any of the other connections. We have a simple router that does the following:

class Router(object):

    def allow_migrate(self, db, model):
        if db == 'default':
            return True
        return False

Current Behaviour

  • We run our functional tests and the migrate command is called against each connection when the test databases are created (see django/test/runner.py, setup_databases, line 300-ish, which calls django/db/backends/creation.py, create_test_db, line 377-ish)
  • When this migrate runs, it tries to apply our migrations, which tries to record that a migration has been applied (see django/db/migrations/executor.py, apply_migration, which has several calls to self.recorder.record_applied).
  • The first thing that record_applied does is a call to self.ensure_schema() (see django/db/migrations/recorder.py, record_applied, lien 66-ish).
  • ensure_schema checks to see if the Migration model is in the tables in the connection. If it does not find the table then it tries to create the table.

I believe that this is incorrect behaviour when a db_router has been provided. If using the router above, my expectation would be that the table is not created on any connection other than the 'default' connection. Looking at the other methods on the MigrationRecorder, I would expect that there will be similar issues with applied_migrations and record_unapplied.

Change History (12)

comment:1 Changed 7 months ago by jarshwah

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

I don't think you've implemented your router correctly, but I'd need to check the router code to see if it's called multiple times (num_dbs*num_models) to be sure. This is how we implement our routers for allow_migrate:

def allow_migrate(self, db, model):
        if db == 'other':
            return model._meta.app_label == 'legacy_app' # allow migration for new django models implemented in legacy db
        elif model._meta.app_label == 'legacy_app':  # do not allow migration for legacy on any other db
            return False
        return None # this router not responsible

So, I'm not sure if there is a bug or not (I'll let someone more familiar answer that), but this is what works for us.

comment:2 Changed 7 months ago by akaariai

#22583 is somewhat related. It deals with the inability to skip RunSQL/RunPython operations on given database.

comment:3 Changed 7 months ago by froomzy

@jarshwah: I don't think it is the router. Mainly because the router is not called at this point. Which is what I believe is the bug.

@akaariai: I agree that there are similarities. Surely I should be able to manage which connections I actually run migrations against. That seems to be what the db_router is trying to do.

I thought that something like the following would solve our problem:

    from django.db import router
	
	.
	.
	.

    def ensure_schema(self):
        """
        Ensures the table exists and has the correct schema.
        """
        # If the table's there, that's fine - we've never changed its schema
        # in the codebase.
        if self.Migration._meta.db_table in self.connection.introspection.get_table_list(self.connection.cursor()):
            return
        # Make the table
	# Change below, similar to how allowed_to_migrate in django/db/migrations/operations/base.py works
	if router.allow_migrate(self.connection, self.Migration):
		with self.connection.schema_editor() as editor:
			editor.create_model(self.Migration)


But this means that changes to applied_migrations, record_applied, and record_unapplied need to be made, so that it doesn't try to write to a none existent table.

For us this is not an urgent issue, as we have appropriate permissions to those connections that are not our core connection. But I do have a concern for any time that we are using a true read-only connection, where we do not have the CREATE TABLE permission. Because this code, as it exists, will blow up in this situation. I tested that with a read-only connection in our db setup, and got an insufficient permissions error.

Thanks,
Dylan

comment:4 Changed 6 months ago by andrewgodwin

  • Triage Stage changed from Unreviewed to Accepted

This is an issue, but bear in mind that migrate must be separately executed for each database alias, so this isn't going to happen unless you specifically run migrate on a database that you know isn't supposed to have migrations on it.

I think the best fix would be to have migrations refuse to run entirely and just exit if the migration model is not allowed to be created on the target database.

comment:5 Changed 6 months ago by froomzy

I see what you mean about the needing to run migarte for each database. I noticed this with the django test runner, where it is trying to run a migration against every connection alias that we have. So that might be an issue with the test runner as much as with the migrations stuff.

Thanks,
Dylan

comment:6 Changed 6 months ago by dperetti

Just stumbled on this issue.
With a properly written router, I expected the migrations of each app to be executed in the right database by just using :

manage.py migrate

Could this be the behavior ?

Instead it's assuming I'm using the default database, OK, no, but ok :-)... and it doesn't follow the allow_migrate rule and creates in the default database the tables that are supposed to live exclusively in the another one (NOT OK !).

So for now the workaround for me is to use a shell script where the app and the database are specified.:

./manage.py migrate inapppurchase --database=iap
./manage.py migrate myapp --database=default

comment:7 follow-up: Changed 6 months ago by andrewgodwin

dperetti: Just like syncdb, migrate only runs on one database at a time, so you must execute it individually for each database, as you suggest. This is by design.

froomzy: Yes, this is likely a test runner issue, that would be doing this kind of migrate-on-everything. I think the suggested fix of refusing to migrate databases where allow_migrate on the migration model returns False will still work, as long as it errors in a way we can catch in the test runner.

comment:8 in reply to: ↑ 7 Changed 6 months ago by dperetti

Replying to andrewgodwin:

dperetti: Just like syncdb, migrate only runs on one database at a time, so you must execute it individually for each database, as you suggest. This is by design.

The question is : is it the best design ? When I run migrate, I don't want to know about how the router is configured. I just want to migrate the app.
If the router dispatches the tables of an app to different databases, then I want migrate to operate on those.
In other words, I think it would make sense to make migrate database agnostic.

comment:9 Changed 6 months ago by dperetti

Another side issue : we cannot just manage.py migrate someapp if the someapp is a "django <1.7" app without migration : the old syncdb behavior is not applied when an application is specified.
So if want the old apps to sync, I have to just run manage.py migrate, without argument... which will create unwanted tables when we have multiple databases.

comment:10 Changed 2 months ago by timgraham

  • Version changed from 1.7-rc-2 to 1.7

comment:11 Changed 5 weeks ago by froomzy

Hi guys,

Just wondering if there is any chance this will make it into 1.8? Or soon there after?

Thanks,
Dylan

comment:12 Changed 5 weeks ago by timgraham

It's not clear to me what fixing this issue involves exactly. Anyway, it doesn't appear anyone is working on it so there's no timetable for its resolution.

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