Signature of the allow_migrate database router.
528 views
Skip to first unread message
Loïc Bistuer
Feb 17, 2015, 5:06:24 AM2/17/15
to django-d...@googlegroups.com
Hi all,
The proposed fix for the release blocker https://code.djangoproject.com/ticket/24351 suggests changing the signature of the `allow_migrate` database router.
From:
def allow_migrate(self, db, model):
To:
def allow_migrate(self, db, app_label, model, **hints):
We need to make a design decision.
Some background:
1. Django 1.7 doesn't call the `allow_migrate` router for RunPython and RunSQL operations. If you control all migrations you can somewhat work around the issue inside RunPython using https://github.com/django/django/blob/1.7.4/docs/topics/migrations.txt#L472-L500, but you still have to stay away from RunSQL, and you are out of luck if these operations live in django.contrib or in a third-party app.
2. Behavior from 1. changed in Django 1.8 (#22583) and the RunPython and RunSQL operations now call `allow_migrate` with `model=None`. These operations also accept a `hints` kwarg that is passed as `**hints` to `allow_migrate`. Unless hints are provided `allow_migrate` can only make a blanket yes/no decision because it doesn't have anything other than the `db_alias` to work with.
3. The change from 2. is backwards incompatible in a sense that routers will blow up if they don't expect None as a possible value for `model`. The `hints` part is backwards compatible as long as no migrations make use of them.
4. Django 1.8 introduced a migration for contrib.contenttypes that contains a RunPython operation (https://github.com/django/django/blob/b4ac23290772e0c11379eb2dfb81c750b7052b66/django/contrib/contenttypes/migrations/0002_remove_content_type_name.py#L33-L36). This is a problem because it won't work in many multi-db setups. The problem already existed for third-party apps but now it becomes a core issue.
5. We could take advantage of 2. to fix 4. (e.g. `RunPython(noop, add_legacy_name, hints={'app_label': 'contenttypes'})`) which would allow multi-db users to make a routing decision, but then we introduce a backwards incompatibility (see point 3.) that requires changing the signature of `allow_migrate` in user code (to expect either the `app_label` kwarg or `**hints`). While this would fix the problem for contrib.contenttypes, the problem would still exist in third-party apps unless they also cooperate by providing similar hints (note that those that still want to support 1.7 projects won't be able to do so).
6. `app_label` applies to all operations including RunPython and RunSQL, `model` only applies to some model operations (CreateModel, AddField, etc.). Our docs and tests only use the `model` argument to dig out the `app_label`, and I suspect this is the most common usage (at least it certainly matches my usage).
My opinion is that `app_label` is always useful in `allow_migrate`, even if only as a first pass before inspecting the model, so since we are introducing a change in the signature of `allow_migrate`, we might as well bite the bullet and make it a required argument. The latest version of my branch https://github.com/django/django/pull/4152 does this while still supporting the old signature during a deprecation period.
An alternative that was proposed by Markus is to pass `app_label` as a hint instead of arg from within `allowed_to_migrate`. I don't think it's a good idea because it's just as invasive (the signature of existing routers still need to change) and we make it less obvious to people calling `router.allow_migrate()` directly that they should supply an `app_label`. It's IMO more error prone if we can't reliably expect that `app_label` will be provided because it requires writing additional defensive code.
Thoughts?
--
Loïc
The proposed fix for the release blocker https://code.djangoproject.com/ticket/24351 suggests changing the signature of the `allow_migrate` database router.
From:
def allow_migrate(self, db, model):
To:
def allow_migrate(self, db, app_label, model, **hints):
We need to make a design decision.
Some background:
1. Django 1.7 doesn't call the `allow_migrate` router for RunPython and RunSQL operations. If you control all migrations you can somewhat work around the issue inside RunPython using https://github.com/django/django/blob/1.7.4/docs/topics/migrations.txt#L472-L500, but you still have to stay away from RunSQL, and you are out of luck if these operations live in django.contrib or in a third-party app.
2. Behavior from 1. changed in Django 1.8 (#22583) and the RunPython and RunSQL operations now call `allow_migrate` with `model=None`. These operations also accept a `hints` kwarg that is passed as `**hints` to `allow_migrate`. Unless hints are provided `allow_migrate` can only make a blanket yes/no decision because it doesn't have anything other than the `db_alias` to work with.
3. The change from 2. is backwards incompatible in a sense that routers will blow up if they don't expect None as a possible value for `model`. The `hints` part is backwards compatible as long as no migrations make use of them.
4. Django 1.8 introduced a migration for contrib.contenttypes that contains a RunPython operation (https://github.com/django/django/blob/b4ac23290772e0c11379eb2dfb81c750b7052b66/django/contrib/contenttypes/migrations/0002_remove_content_type_name.py#L33-L36). This is a problem because it won't work in many multi-db setups. The problem already existed for third-party apps but now it becomes a core issue.
5. We could take advantage of 2. to fix 4. (e.g. `RunPython(noop, add_legacy_name, hints={'app_label': 'contenttypes'})`) which would allow multi-db users to make a routing decision, but then we introduce a backwards incompatibility (see point 3.) that requires changing the signature of `allow_migrate` in user code (to expect either the `app_label` kwarg or `**hints`). While this would fix the problem for contrib.contenttypes, the problem would still exist in third-party apps unless they also cooperate by providing similar hints (note that those that still want to support 1.7 projects won't be able to do so).
6. `app_label` applies to all operations including RunPython and RunSQL, `model` only applies to some model operations (CreateModel, AddField, etc.). Our docs and tests only use the `model` argument to dig out the `app_label`, and I suspect this is the most common usage (at least it certainly matches my usage).
My opinion is that `app_label` is always useful in `allow_migrate`, even if only as a first pass before inspecting the model, so since we are introducing a change in the signature of `allow_migrate`, we might as well bite the bullet and make it a required argument. The latest version of my branch https://github.com/django/django/pull/4152 does this while still supporting the old signature during a deprecation period.
An alternative that was proposed by Markus is to pass `app_label` as a hint instead of arg from within `allowed_to_migrate`. I don't think it's a good idea because it's just as invasive (the signature of existing routers still need to change) and we make it less obvious to people calling `router.allow_migrate()` directly that they should supply an `app_label`. It's IMO more error prone if we can't reliably expect that `app_label` will be provided because it requires writing additional defensive code.
Thoughts?
--
Loïc
Tim Graham
Feb 17, 2015, 12:16:15 PM2/17/15
to django-d...@googlegroups.com
I'm not a big user of multi-db so I could be wrong here, but it seems like this API seems to assume that all models in a given app are stored in the same database. Have you thought through what happens if this isn't true? This question seems to come into play when we allowed model=None in RunPython/SQL.
Loïc Bistuer
Feb 17, 2015, 12:48:50 PM2/17/15
to django-d...@googlegroups.com
Hi Tim,
The API doesn't assume anything, it just gives to the router all the information that's available, which is limited to connection_alias and app_label in the case of RunPython/RunSQL.
Previously all RunPython/RunSQL operations would run on every db no matter what, this is obviously broken for many multi-db setups.
With this branch at the very least you'd get the app_label which is sufficient for routing in most cases.
If you need even more granularity then you need the operations to give you some hints. For example a RunPython operation could provide the list of affected models with `hints={'affected_models': [ModelA, ModelB]}`, but when you need this level of control over routing, you hopefully also control the app so you can easily provide the hints that you need.
--
Loïc
> --
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/fbaf40ce-f2de-41e5-9bf5-3c15a411f012%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
The API doesn't assume anything, it just gives to the router all the information that's available, which is limited to connection_alias and app_label in the case of RunPython/RunSQL.
Previously all RunPython/RunSQL operations would run on every db no matter what, this is obviously broken for many multi-db setups.
With this branch at the very least you'd get the app_label which is sufficient for routing in most cases.
If you need even more granularity then you need the operations to give you some hints. For example a RunPython operation could provide the list of affected models with `hints={'affected_models': [ModelA, ModelB]}`, but when you need this level of control over routing, you hopefully also control the app so you can easily provide the hints that you need.
--
Loïc
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/fbaf40ce-f2de-41e5-9bf5-3c15a411f012%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
Aymeric Augustin
Feb 17, 2015, 3:58:34 PM2/17/15
to django-d...@googlegroups.com
On 17 févr. 2015, at 11:06, Loïc Bistuer <loic.b...@gmail.com> wrote:
> The proposed fix for the release blocker https://code.djangoproject.com/ticket/24351 suggests changing the signature of the `allow_migrate` database router.
>
> From:
> def allow_migrate(self, db, model):
>
> To:
> def allow_migrate(self, db, app_label, model, **hints):
>
> We need to make a design decision.
It appears that we have the change the signature of this function one way or another and that inspect.getargspec is our only option to preserve compatibility.
> The proposed fix for the release blocker https://code.djangoproject.com/ticket/24351 suggests changing the signature of the `allow_migrate` database router.
>
> From:
> def allow_migrate(self, db, model):
>
> To:
> def allow_migrate(self, db, app_label, model, **hints):
>
> We need to make a design decision.
Did you consider the following signature?
def allow_migrate(self, db, app_label, model_name=None, **hints):
While it’s a slightly larger change, it has a several advantages:
- passing (app_label, model_name) as strings is an established convention in many other APIs.
- it removes the possibility that app_label != model._meta.app_label — which doesn’t make much sense anyway.
- our examples only ever use app_label — and keep repeating model._meta.app_label to get it.
If the model class is needed, it can be fetched with apps.get_model(app_label, model_name).
--
Aymeric.
PS: we chose to make 1.8 the next LTS instead of 1.7 precisely so we could make such adjustments.
Andrew Godwin
Feb 17, 2015, 5:19:12 PM2/17/15
to django-d...@googlegroups.com
I am fine with the proposed change to allow better routing of RunSQL/RunPython (though in the RunPython case people can easily do routing inside the python code provided anyway, as you can see the DB aliases there).
Aymeric: The problem with your proposal is that there's no easy way to get the versioned model that the migration is using from just (app_label, model_name); we'd need to pass the Apps instance in there as well. At least if the whole model instance is around it'll be the correctly-versioned one from the right point in time (I'm not sure if that makes a big difference to most routers but it will stop lookups failing for models that have since been deleted, for example)
Andrew
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/B42BA226-16FE-43D5-BE12-8FFD3DCD3094%40polytechnique.org.
Loïc Bistuer
Feb 17, 2015, 11:46:01 PM2/17/15
to django-d...@googlegroups.com
> On Feb 18, 2015, at 05:18, Andrew Godwin <and...@aeracode.org> wrote:
>
> I am fine with the proposed change to allow better routing of RunSQL/RunPython (though in the RunPython case people can easily do routing inside the python code provided anyway, as you can see the DB aliases there).
> On Tue, Feb 17, 2015 at 12:58 PM, Aymeric Augustin <aymeric....@polytechnique.org> wrote:
>
> Did you consider the following signature?
> def allow_migrate(self, db, app_label, model_name=None, **hints):
> While it’s a slightly larger change, it has a several advantages:
> - passing (app_label, model_name) as strings is an established convention in many other APIs.
> - it removes the possibility that app_label != model._meta.app_label — which doesn’t make much sense anyway.
> - our examples only ever use app_label — and keep repeating model._meta.app_label to get it.
>
> If the model class is needed, it can be fetched with apps.get_model(app_label, model_name).
The only (slightly convoluted) use-case I can think of is pushing the routing decision onto a model or manager method. For model methods you'd have to be a bit crafty by using non-model base classes, and for manager methods you'd have to use Django 1.8 since custom managers weren't supported before. In both cases versioning is bypassed and there is the requirement that classes still exist in the codebase, but at least it's not coupled to the existence of a specific model like `apps.get_model(app_label, model_name)` would.
It's a tangent but seeing this, we should definitely turn the `model` (or `model_name`) arg into kwarg even if only in the docs, that'll make it more obvious that it's not always there.
> PS: we chose to make 1.8 the next LTS instead of 1.7 precisely so we could make such adjustments.
--
Loïc
Loïc Bistuer
Feb 18, 2015, 12:34:42 AM2/18/15
to django-d...@googlegroups.com
Another option would be to make the signature `allow_migrate(self, db, app_label, model_name=None, **hints)` and to put the model class in the hints dict, the same way we pass `instance` as hint to the other routers.
That would make the 80% use-case less fiddly without any loss of functionality.
--
Loïc
That would make the 80% use-case less fiddly without any loss of functionality.
--
Loïc
Aymeric Augustin
Feb 18, 2015, 3:41:51 AM2/18/15
to django-d...@googlegroups.com
2015-02-18 6:34 GMT+01:00 Loïc Bistuer <loic.b...@gmail.com>:
Another option would be to make the signature `allow_migrate(self, db, app_label, model_name=None, **hints)` and to put the model class in the hints dict, the same way we pass `instance` as hint to the other routers.
Yes, that's what I wanted to suggest after Andrew confirmed my suspicion that we have to account for historical models.
Perhaps the following convenience function can help factoring out the arguments in the common case:
def allow_migrate_model(router, db, model):
return router.allow_migrate(db, model._meta.app_label, model._meta.model_name, model=model)
(This isn't a router method because there isn't a base class for routers.)
--
Aymeric.
Loïc Bistuer
Feb 18, 2015, 8:07:28 AM2/18/15
to django-d...@googlegroups.com
Individual routers don't have a base class but we can add it to the master router `django.db.router`. I've updated the PR with a `router.allow_migrate_model` method. I don't know if we should document this just yet.
Internally we now treat anything other than `app_label` as a hint, but I guess we should still document `model_name=None` as part of the official signature.
Speaking about `model_name`, it seems to be the lowercase version of `_meta.object_name` (which is basically __name__). I didn't investigate further, can you confirm that `_meta.model_name` is what we want?
Internally we now treat anything other than `app_label` as a hint, but I guess we should still document `model_name=None` as part of the official signature.
Speaking about `model_name`, it seems to be the lowercase version of `_meta.object_name` (which is basically __name__). I didn't investigate further, can you confirm that `_meta.model_name` is what we want?
> --
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CANE-7mVJpobigSdmEQt1NdkXOyjDWdPoEZJz4TVdVDeG9g%2BS2w%40mail.gmail.com.
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at http://groups.google.com/group/django-developers.
Marten Kenbeek
Feb 18, 2015, 8:44:07 AM2/18/15
to django-d...@googlegroups.com
Loïc, the model_name is indeed the lower-case version of the object_name: https://github.com/django/django/blob/master/django/db/models/options.py#L203
Aymeric Augustin
Feb 18, 2015, 8:46:50 AM2/18/15
to django-d...@googlegroups.com
2015-02-18 14:07 GMT+01:00 Loïc Bistuer <loic.b...@gmail.com>:
Individual routers don't have a base class but we can add it to the master router `django.db.router`. I've updated the PR with a `router.allow_migrate_model` method. I don't know if we should document this just yet.
Good. Unless I missed something that's an implementation detail. There's no need to document it.
Internally we now treat anything other than `app_label` as a hint, but I guess we should still document `model_name=None` as part of the official signature.
Yes.
Speaking about `model_name`, it seems to be the lowercase version of `_meta.object_name` (which is basically __name__). I didn't investigate further, can you confirm that `_meta.model_name` is what we want?
The alternative is model._meta.object_name. object_name is __name__ and model_name is object_name.lower().
Since model_name should be treated as case-insensitive — that's how Django works, most likely for no good reason — passing the lowercase version is less error-prone. If the developer doesn't account for case insensitivity, their code will fail immediately, since model class names are usually written in CamelCase.
--
Aymeric.
Reply all
Reply to author
Forward
0 new messages