Re: Signature of the allow_migrate database router.
2015-02-18 14:07 GMT+01:00 Loïc Bistuer : > 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. -- 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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-7mUM-SEzfBR0S_CtCvhVNbd45%3DfC2F2aoDcim%3D2hX_GkoA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
Re: Signature of the allow_migrate database router.
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 On Wednesday, February 18, 2015 at 2:07:28 PM UTC+1, Loïc Bistuer wrote: > > 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? > > > On Feb 18, 2015, at 15:41, Aymeric Augustin < > aymeric@polytechnique.org > wrote: > > > > 2015-02-18 6:34 GMT+01:00 Loïc Bistuer >: > > 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. > > > > -- > > 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. > > > > For more options, visit https://groups.google.com/d/optout. > > -- 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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/9e07c5d3-7706-4873-bb7a-9224af94c35f%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
Re: Signature of the allow_migrate database router.
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? > On Feb 18, 2015, at 15:41, Aymeric Augustin > wrote: > > 2015-02-18 6:34 GMT+01:00 Loïc Bistuer : > 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. > > -- > 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-developers+unsubscr...@googlegroups.com. > To post to this group, send email to django-developers@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. > For more options, visit https://groups.google.com/d/optout. -- 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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/A38EDB7A-9099-49A1-B4EA-86BBF393B792%40gmail.com. For more options, visit https://groups.google.com/d/optout.
Re: Signature of the allow_migrate database router.
2015-02-18 6:34 GMT+01:00 Loïc Bistuer : > 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. -- 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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. For more options, visit https://groups.google.com/d/optout.
Re: Signature of the allow_migrate database router.
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 > On Feb 18, 2015, at 11:45, Loïc Bistuer wrote: > > >> On Feb 18, 2015, at 05:18, Andrew Godwin 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). > > True although that tightly couples the host migration and the routing step. > It is fine if you control both but you are out of luck if the migration is in > a reusable or third-party app. > >> On Tue, Feb 17, 2015 at 12:58 PM, Aymeric Augustin >> 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). > > It's a fair proposal but it's hard to predict what people currently do with > their routers. I've only ever needed to "identify" models, so it would > suffice for my needs. > > 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. > > +1 > > -- > 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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/0749ADD0-3402-4131-A4BF-AC4BC12DB6B0%40gmail.com. For more options, visit https://groups.google.com/d/optout.
Re: Signature of the allow_migrate database router.
> On Feb 18, 2015, at 05:18, Andrew Godwin 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). True although that tightly couples the host migration and the routing step. It is fine if you control both but you are out of luck if the migration is in a reusable or third-party app. > On Tue, Feb 17, 2015 at 12:58 PM, Aymeric Augustin > 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). It's a fair proposal but it's hard to predict what people currently do with their routers. I've only ever needed to "identify" models, so it would suffice for my needs. 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. +1 -- 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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/E5E8EEBA-CA5B-43CF-8D0E-B044CE20B5D8%40gmail.com. For more options, visit https://groups.google.com/d/optout.
Re: Signature of the allow_migrate database router.
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 On Tue, Feb 17, 2015 at 12:58 PM, Aymeric Augustin < aymeric.augus...@polytechnique.org> wrote: > On 17 févr. 2015, at 11:06, Loïc Bistuer 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. > > 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. > > -- > 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-developers+unsubscr...@googlegroups.com. > To post to this group, send email to django-developers@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 > . > For more options, visit https://groups.google.com/d/optout. > -- 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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/CAFwN1uo2tSTOUdZy0nAAftcEXGGdcZmezWCThkM9ePYe3_brFw%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
Re: Signature of the allow_migrate database router.
On 17 févr. 2015, at 11:06, Loïc Bistuer 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. 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. -- 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-developers+unsubscr...@googlegroups.com. To post to this group, send email to django-developers@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. For more options, visit https://groups.google.com/d/optout.
Re: Signature of the allow_migrate database router.
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
On Feb 18, 2015, at 00:16, Tim Graham wrote:
>
> 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.
>
> On Tuesday, February 17, 2015 at 5:06:24 AM UTC-5, Loïc Bistuer wrote:
> 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'
Re: Signature of the allow_migrate database router.
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.
On Tuesday, February 17, 2015 at 5:06:24 AM UTC-5, Loïc Bistuer wrote:
>
> 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
>
>
--
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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.
Signature of the allow_migrate database router.
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
--
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-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@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/4384E3A1-9454-4A34-9583-892430D89A60%40gmail.com.
For more options, visit https://groups.google.com/d/optout.
