[Django] #23273: MigrationRecorder does not obey db_router allow_migrate rules
Django
----------------------------+----------------------
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+----------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
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 jarshwah):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:1>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
#22583 is somewhat related. It deals with the inability to skip
RunSQL/RunPython operations on given database.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:2>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:3>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewgodwin):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:4>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:5>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:6>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:7>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by dperetti):
Replying to [comment:7 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:8>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7-rc-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:9>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* version: 1.7-rc-2 => 1.7
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:10>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by froomzy):
Hi guys,
Just wondering if there is any chance this will make it into 1.8? Or soon
there after?
Thanks,
Dylan
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:11>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:12>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by marfire):
* cc: k@… (added)
* version: 1.7 => master
Comment:
I wanted to chime in here to broaden the scope of the bug, as I was
affected by it recently in a different context.
The bigger issue is not just that the framework tries to create the
migrations table where it's not needed, but that it marks as applied
migrations that in fact were not. That puts the database in an
inconsistent state, at least as far as migrations are concerned. It's a
harmless inconsistency for the specific settings file used at that
specific moment in time, but it lays the seed for big problems down the
line. For example, if you later decide to change your routing scheme, or
(as happened in my case) if you have multiple projects with different
settings using the same app on the same database.
In terms of a solution, what seems straightforward to my untrained eye is
for the migration framework to simply not record migrations as applied
that it didn't apply (and as a corollary, it shouldn't try to create the
migration table on a database if it doesn't need to record any migrations
there). The fix suggested above ("to have migrations refuse to run
entirely and just exit if the migration model is not allowed to be created
on the target database") doesn't address the broader issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:13>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by marfire):
Let me amend my last comment: I think that having `migrate` blow up in
this situation would in fact solve the problem with having an inconsistent
migrations table, which is the most important thing.
My question is, since `allow_migrate()` operates at the model level and
the `migrate` command operates at the app level, wouldn't this make it
impossible to migrate some models of an app but not others?
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:14>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jorisvd):
I can confirm marfire's findings.
1/ For example even with this router applied:
{{{
class testerouter(object):
def allow_migrate(self, db, app_label, model_name=None, **hints):
return False
}}}
And then executing:
{{{
migrate --database=replica
}}}
All apps and models migrations get applied to this replica database, while
according to the router nothing should happen at all.
2/ From the documentation it is not clear whether it is possible to
isolate certain models from certain databases, or whether isolation can
only be done on the app-level. From the description in the documentation,
it seems possible to use the model_name argument for this, but by the way
django stores its migrations, I don't see how that could work.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:15>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by intgr):
* cc: marti@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:16>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ryan Vennell):
Just to further confirm: I've been wrestling with this problem for a
couple days. I have multiple databases and multiple apps. When I'm running
tests, the router allow_migrate works properly to control which migrations
can be applied to which database/app.
The actual migration portion works fine, but when it comes time to
actually write the migration history, it seems to only have a concept of
the default database. When it runs ensure_schema, it's expecting that
tables should exist on the default DB when they really only exist for
certain other apps/databases. This leads to a ProgrammingError where it
can't find tables that it shouldn't be checking for in the first place.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:17>
Django
Reporter: froomzy | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Daniel van Flymen):
Can you please have a look at http://stackoverflow.com/questions/40893868
/using-redshift-as-an-additional-django-
database?noredirect=1#comment69004673_40893868, which provides a decent
use case for this bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:18>
Django
Reporter: froomzy | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* owner: nobody => Jacob Walls
* status: new => assigned
Comment:
Since 3.1 you can set `'TEST': {'MIGRATE': False}` to avoid running
migrations on a given db connection, so that solves the test runner issue.
Still, even if you do that, apps are still synced (see fix to #32012),
Django ends up calling `migrate` to do the syncing, and this will cause
queries in `MigrationRecorder.ensure_schema()` that might create tables
(or fail with permission errors, see #27141).
I plan to open a PR to do roughly this from comment:13:
`it shouldn't try to create the migration table on a database if it
doesn't need to record any migrations there`
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:19>
Django
Reporter: froomzy | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/15252 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:20>
Django
Reporter: froomzy | Owner: Jacob Walls
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:21>
Django
Reporter: froomzy | Owner: Jacob Walls
Component: Migrations | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"92412aa94c97b5c36387fadb39d9f8edf25547fa" 92412aa]:
{{{
#!CommitTicketReference repository=""
revision="92412aa94c97b5c36387fadb39d9f8edf25547fa"
Fixed #23273 -- Avoided creation of django_migrations table when there are
no migrations to apply.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23273#comment:22>