Signature of the allow_migrate database router.

[Loïc Bistuer]

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.

Re: status of 1.8 release blockers

[Markus Holtermann]

So, it turns out that #24291 (Migrations fail with unused swappable model) 
 only occurs when a swapped 
out model doesn't define an explicit model manager. Since auth.User is the 
only model that is officially supported for "swappiness" and it defines an 
explicit UserManager this isn't really a problem in my opinion. Since the 
whole swappable model API is internal API anyway, I don't want to rule this 
issue a release blocker. If projects in the wild use the swappable model 
API they can get around the issue by adding `objects = models.Manager()` to 
that model.

Tim, Russ, thoughts 'bout that?

/Markus

On Tuesday, February 17, 2015 at 1:05:37 AM UTC+1, Tim Graham wrote:
>
> Here's the status on blockers. Given the work that remains, I don't 
> foresee the beta release happening earlier than Wednesday, but the next 
> update of this thread will be tomorrow.
>
> #24324  Crashes when project 
> path or path to Django install contains a non-ascii character 
> 
> Owner: Tim
> Status: Patch needs review
>
>
> #24351    RunPython/RunSQL 
> operations in contrib migrations and multi-db routers. 
> Owner: Loic
> Status: API design decision needed. Time permitting, Loic will write to 
> the mailing list on this tomorrow.
>
> #24328  The new 
> Options._get_fields() method needs to be cleaned up 
> 
> Owner: Anssi
> Status: Anssi still reviewing the patch; decision may be needed on 
> backwards compatibility of get_fields().
>
> #24343  UUID foreign key 
> accessor returns inconsistent type 
> Owner: Marc/Josh/Shai
> Status: Josh working on cleaning up the patch. Review/feedback from Anssi 
> requested.
>
> #24291 
> 
>  Migrations 
> fail with unused swappable model 
> 
> Owner: Markus
> Status: Patch looks good to me; Markus to review & commit tomorrow.
>
> On Monday, February 16, 2015 at 11:12:46 AM UTC-5, Tim Graham wrote:
>>
>> There are still quite a few unresolved issues, so the beta release won't 
>> happen today. I'll send an update at the end of the day with the status of 
>> the remaining issues.
>>
>> On Friday, February 13, 2015 at 1:30:44 PM UTC-5, Markus Holtermann wrote:
>>>
>>> Hey folks,
>>>
>>> I looked into the issues #24225, #24264 and #24282 and have a working 
>>> pull request ready for review: 
>>> https://github.com/django/django/pull/4097
>>>
>>> The essential change in the pull request is the way how the set of 
>>> models that needs to be rerendered is constructed. Instead of naively only 
>>> resolving one level of relations (Only rerender B and C if C changed: A --> 
>>> B --> C) the new approach recursively checks for relational fields 
>>> (forwards and reverse relations (e.g. ForeignKey and ManyToManyRel)) as 
>>> well as inheritance.
>>>
>>> This approach boils down to the following behavior:
>>>
>>> If a completely standalone model (no incoming or outgoing relations and 
>>> no subclasses and only a directed subclass of models.Model) changes, only 
>>> this model will be rerendered, which is the best case scenario. If every 
>>> model is somehow related (directly or through other models) to every other 
>>> model, e.g. through the user model, changing one model from that set will 
>>> require *all* models to be rerendered, which is the worst case scenario 
>>> and results in the similar behavior as 1.7.
>>>
>>> To get that pull request into the 1.8 beta I ask everybody to take a 
>>> look and try it out on their projects. Especially if 1.8 alpha 1 didn't 
>>> work for you.
>>>
>>> Thanks
>>>
>>> /Markus
>>>
>>> On Saturday, December 20, 2014 at 8:40:33 PM UTC+1, Tim Graham wrote:

 As we approach the date for 1.8 alpha, I plan to send a weekly update 
 on the status of release blocking issues.

 We currently have 3 release blockers affecting master. You can use this 
 Trac filter to see them:

 https://code.djangoproject.com/query?status=assigned&status=new&version=master&severity=Release+blocker

 You can also see other tickets we are targeting for 1.8 with this 
 filter. This includes some of the remaining large features as well as a 
 couple code reorganizations we want to make closer to when cut the 1.8 
 branch to avoid creating conflicts with the large features that are in 
 progress.

 https://code.djangoproject.com/query?status=assigned&status=new&keywords=~1.8-alpha

 Here is a summary of where we stand with each release blo

Re: Signature of the allow_migrate database router.

[Tim Graham]

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.

Re: status of 1.8 release blockers

[Tim Graham]

If it's the only release blocker left and the fix is difficult, I think we 
could issue a beta release without it. I think the worst case is that 
you'll get some duplicate bug reports from testers.

On Tuesday, February 17, 2015 at 10:06:27 AM UTC-5, Markus Holtermann wrote:
>
> So, it turns out that #24291 (Migrations fail with unused swappable model) 
>  only occurs when a swapped 
> out model doesn't define an explicit model manager. Since auth.User is the 
> only model that is officially supported for "swappiness" and it defines an 
> explicit UserManager this isn't really a problem in my opinion. Since the 
> whole swappable model API is internal API anyway, I don't want to rule this 
> issue a release blocker. If projects in the wild use the swappable model 
> API they can get around the issue by adding `objects = models.Manager()` to 
> that model.
>
> Tim, Russ, thoughts 'bout that?
>
> /Markus
>
> On Tuesday, February 17, 2015 at 1:05:37 AM UTC+1, Tim Graham wrote:
>>
>> Here's the status on blockers. Given the work that remains, I don't 
>> foresee the beta release happening earlier than Wednesday, but the next 
>> update of this thread will be tomorrow.
>>
>> #24324  Crashes when 
>> project path or path to Django install contains a non-ascii character 
>> 
>> Owner: Tim
>> Status: Patch needs review
>>
>>
>> #24351    RunPython/RunSQL 
>> operations in contrib migrations and multi-db routers. 
>> Owner: Loic
>> Status: API design decision needed. Time permitting, Loic will write to 
>> the mailing list on this tomorrow.
>>
>> #24328  The new 
>> Options._get_fields() method needs to be cleaned up 
>> 
>> Owner: Anssi
>> Status: Anssi still reviewing the patch; decision may be needed on 
>> backwards compatibility of get_fields().
>>
>> #24343  UUID foreign key 
>> accessor returns inconsistent type 
>> Owner: Marc/Josh/Shai
>> Status: Josh working on cleaning up the patch. Review/feedback from Anssi 
>> requested.
>>
>> #24291 
>> 
>>  Migrations 
>> fail with unused swappable model 
>> 
>> Owner: Markus
>> Status: Patch looks good to me; Markus to review & commit tomorrow.
>>
>> On Monday, February 16, 2015 at 11:12:46 AM UTC-5, Tim Graham wrote:
>>>
>>> There are still quite a few unresolved issues, so the beta release won't 
>>> happen today. I'll send an update at the end of the day with the status of 
>>> the remaining issues.
>>>
>>> On Friday, February 13, 2015 at 1:30:44 PM UTC-5, Markus Holtermann 
>>> wrote:

 Hey folks,

 I looked into the issues #24225, #24264 and #24282 and have a working 
 pull request ready for review: 
 https://github.com/django/django/pull/4097

 The essential change in the pull request is the way how the set of 
 models that needs to be rerendered is constructed. Instead of naively only 
 resolving one level of relations (Only rerender B and C if C changed: A 
 --> 
 B --> C) the new approach recursively checks for relational fields 
 (forwards and reverse relations (e.g. ForeignKey and ManyToManyRel)) as 
 well as inheritance.

 This approach boils down to the following behavior:

 If a completely standalone model (no incoming or outgoing relations and 
 no subclasses and only a directed subclass of models.Model) changes, only 
 this model will be rerendered, which is the best case scenario. If every 
 model is somehow related (directly or through other models) to every other 
 model, e.g. through the user model, changing one model from that set will 
 require *all* models to be rerendered, which is the worst case 
 scenario and results in the similar behavior as 1.7.

 To get that pull request into the 1.8 beta I ask everybody to take a 
 look and try it out on their projects. Especially if 1.8 alpha 1 didn't 
 work for you.

 Thanks

 /Markus

 On Saturday, December 20, 2014 at 8:40:33 PM UTC+1, Tim Graham wrote:
>
> As we approach the date for 1.8 alpha, I plan to send a weekly update 
> on the status of release blocking issues.
>
> We currently have 3 release blockers affecting master. You can use 
> this Trac filter to see them:
>
> https://code.djangoproject.com/query?status=assigned&status=new&version=master&severity=Release+blocker
>
> You can also see other tickets we are targeting for 1.8 with this 
>>

Re: status of 1.8 release blockers

[Marten Kenbeek]

@Tim I believe it's quite easy actually, I've fixed my original patch and 
it's just waiting to be reviewed. It's available at 
https://github.com/django/django/pull/4071. However, it's a very rare bug 
and I only encountered it in the tests when working on another issue, not 
in any real project. Not really worth delaying the beta I think, I'm 
getting the feeling you guys are busy enough as it is. 

Op dinsdag 17 februari 2015 18:20:22 UTC+1 schreef Tim Graham:
>
> If it's the only release blocker left and the fix is difficult, I think we 
> could issue a beta release without it. I think the worst case is that 
> you'll get some duplicate bug reports from testers.
>
> On Tuesday, February 17, 2015 at 10:06:27 AM UTC-5, Markus Holtermann 
> wrote:
>>
>> So, it turns out that #24291 (Migrations fail with unused swappable 
>> model)  only occurs when a 
>> swapped out model doesn't define an explicit model manager. Since auth.User 
>> is the only model that is officially supported for "swappiness" and it 
>> defines an explicit UserManager this isn't really a problem in my opinion. 
>> Since the whole swappable model API is internal API anyway, I don't want to 
>> rule this issue a release blocker. If projects in the wild use the 
>> swappable model API they can get around the issue by adding `objects = 
>> models.Manager()` to that model.
>>
>> Tim, Russ, thoughts 'bout that?
>>
>> /Markus
>>
>> On Tuesday, February 17, 2015 at 1:05:37 AM UTC+1, Tim Graham wrote:
>>>
>>> Here's the status on blockers. Given the work that remains, I don't 
>>> foresee the beta release happening earlier than Wednesday, but the next 
>>> update of this thread will be tomorrow.
>>>
>>> #24324  Crashes when 
>>> project path or path to Django install contains a non-ascii character 
>>> 
>>> Owner: Tim
>>> Status: Patch needs review
>>>
>>>
>>> #24351    RunPython/RunSQL 
>>> operations in contrib migrations and multi-db routers. 
>>> Owner: Loic
>>> Status: API design decision needed. Time permitting, Loic will write to 
>>> the mailing list on this tomorrow.
>>>
>>> #24328  The new 
>>> Options._get_fields() method needs to be cleaned up 
>>> 
>>> Owner: Anssi
>>> Status: Anssi still reviewing the patch; decision may be needed on 
>>> backwards compatibility of get_fields().
>>>
>>> #24343  UUID foreign key 
>>> accessor returns inconsistent type 
>>> Owner: Marc/Josh/Shai
>>> Status: Josh working on cleaning up the patch. Review/feedback from 
>>> Anssi requested.
>>>
>>> #24291 
>>> 
>>>  Migrations 
>>> fail with unused swappable model 
>>> 
>>> Owner: Markus
>>> Status: Patch looks good to me; Markus to review & commit tomorrow.
>>>
>>> On Monday, February 16, 2015 at 11:12:46 AM UTC-5, Tim Graham wrote:

 There are still quite a few unresolved issues, so the beta release 
 won't happen today. I'll send an update at the end of the day with the 
 status of the remaining issues.

 On Friday, February 13, 2015 at 1:30:44 PM UTC-5, Markus Holtermann 
 wrote:
>
> Hey folks,
>
> I looked into the issues #24225, #24264 and #24282 and have a working 
> pull request ready for review: 
> https://github.com/django/django/pull/4097
>
> The essential change in the pull request is the way how the set of 
> models that needs to be rerendered is constructed. Instead of naively 
> only 
> resolving one level of relations (Only rerender B and C if C changed: A 
> --> 
> B --> C) the new approach recursively checks for relational fields 
> (forwards and reverse relations (e.g. ForeignKey and ManyToManyRel)) as 
> well as inheritance.
>
> This approach boils down to the following behavior:
>
> If a completely standalone model (no incoming or outgoing relations 
> and no subclasses and only a directed subclass of models.Model) changes, 
> only this model will be rerendered, which is the best case scenario. If 
> every model is somehow related (directly or through other models) to 
> every 
> other model, e.g. through the user model, changing one model from that 
> set 
> will require *all* models to be rerendered, which is the worst case 
> scenario and results in the similar behavior as 1.7.
>
> To get that pull request into the 1.8 beta I ask everybody to take a 
> look and try it out on their projects. Especially if 1.8 a

Re: Signature of the allow_migrate database router.

[Loïc Bistuer]

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'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 
> b

Re: Feature proposal: Conditional block tags

[Preston Timmons]

After looking at this, there's nothing special about blocks compared to any 
other node. They could just as well be evaluated at render time.

Here's a branch that implements this change:

https://github.com/prestontimmons/django/compare/conditional-blocks?expand=1

Before returning blocks into block_context to be used as overrides, it 
loops through any if nodes and checks the conditions.

The one case that's kinda funny is if you do something like:

{% if var %}
{% block content %}...{% endblock %}
{% else %}
{% block content %}...{% endblock %}
{% endif %}

Currently, the second block node will raise a TemplateSyntaxError because 
it's defined twice. Does that matter?

-- 
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/71aa2d20-d162-4620-b4c6-8cb915a1da5d%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Signature of the allow_migrate database router.

[Aymeric Augustin]

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: Formalizing template loader and debug api's

[Aymeric Augustin]

Hi Preston,

Thanks for your continued work on this and sorry for my slow answers.


On 30 janv. 2015, at 03:24, Preston Timmons  wrote:

> The cache algorithm I added uses the origin object as a cache key. The "name"
> attribute is used as part of the key value. Therefore, the underlying loader
> must be sure to set name to a unique value per template.

Indeed, since you added skipping, you can’t use template_name (the value
passed to get_template()) as the cache key.


> Can you think of a loader where we wouldn't want to require a unique name?

Well, given what I just said above, we don’t have a choice here. We must
make this a requirement of the template loader API.


> Does that makes sense, or should I look for a way to incorporate that
> into LoaderOrigin?

Looking at the implementation, it feels weird to add attributes to the origin
that are only used by the loader… If origins were dumb bags of data this
would be all right but they have a reload() method that calls the loader.

Such mutual dependencies never end well :-/ That’s a broader design
problem not directly related to your refactoring, but while you’re breaking
all the things, perhaps we could fix it too.


> LoaderOrigin vs StringOrigin
> 
> I looked at combining these, but didn't yet since it doesn't impact the 
> feature
> this patch is implementing. If they're combined to a DTLOrigin, should that
> live in django.template.base? It can't live in django.template.loader since
> that module isn't safe for django.template.base to import.

Yes, it can go into django.template.base or django.template.origin, a new
module.

By the way, I would like to check if we can deprecate the dirs argument of
LoaderOrigin. It may only be used by deprecated code as of Django 1.8.


> context.origin
> 
> In order for the origin to be available to the ExtendsNode, I also used the
> context.origin = self.origin hack in the Context.render method. I saw the 
> logic
> you used to only set engine on toplevel_render. When I moved this assignment
> into the if statement or not, it didn't seem to make a difference.
> 
> Is that a hack we can live with for now?

Well this isn’t an example you’re supposed to follow… ;-)

context.origin doesn’t make a lot of sense. Wouldn’t context.template be better?
Then you could walk to context.template.origin.

In fact context.template.engine would be much better than context.engine. I’ll 
try
to fix that in 1.8 before it’s too late.


> There's one case where the debug postmortem isn't optimal. If multiple engines
> are configured and a template is found by the second engine that fails
> extending, the postmortem won't include the templates that were tried by the
> initial get_template call to the first engine. I'm not seeing an obvious way
> to persist that information at the moment.

This isn’t a big deal. Using multiple Django template engines is an edge case.

-- 
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/1D3E35AF-675D-4C00-B463-85D968D39A43%40polytechnique.org.
For more options, visit https://groups.google.com/d/optout.

Re: Formalizing template loader and debug api's

[Aymeric Augustin]

On 17 févr. 2015, at 05:46, Preston Timmons  wrote:

> Even so, I think that can be handled with documentation.

And an option to disable it. This is very easy to implement.

> Do you think it's worth making an attempt to formalize this?

Yes, I do.

This is an independent follow-up to the big refactoring we’re talking about, 
isn’t it?

-- 
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/E1DBEF63-97B6-46CD-A59A-6EF08EADB42C%40polytechnique.org.
For more options, visit https://groups.google.com/d/optout.

Re: Formalizing template loader and debug api's

[Preston Timmons]

>
> Thanks for your continued work on this and sorry for my slow answers. 
>

No worries. I've been taking the time to get a better grasp of Django 
templates as a whole.
 

> > Does that makes sense, or should I look for a way to incorporate that 
> > into LoaderOrigin? 
>
> Looking at the implementation, it feels weird to add attributes to the 
> origin 
> that are only used by the loader… If origins were dumb bags of data this 
> would be all right but they have a reload() method that calls the loader. 
>
> Such mutual dependencies never end well :-/ That’s a broader design 
> problem not directly related to your refactoring, but while you’re 
> breaking 
> all the things, perhaps we could fix it too.
>

Yes. I'll look at this. The reload method isn't documented and I think it 
can
go away if the debug view is updated.
 

> By the way, I would like to check if we can deprecate the dirs argument of 
> LoaderOrigin. It may only be used by deprecated code as of Django 1.8. 
>

Deprecate or remove? :)

This argument is one I don't think is documented anywhere. 
 

> In fact context.template.engine would be much better than context.engine. 
> I’ll try 
> to fix that in 1.8 before it’s too late. 


Sure. I forgot to mention it but my latest patch actually solves this by 
passing origin
in context.render_context. This makes sense because render_context is 
available
only in the current render scope, rather than modifying the global context. 
I'm just
as happy with accessing context.template, though.


-- 
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/a9a02345-feb9-4d5e-b14c-09e61ae386e4%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: Signature of the allow_migrate database router.

[Andrew Godwin]

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: Formalizing template loader and debug api's

[Preston Timmons]

>
> Yes, I do. 
>
> This is an independent follow-up to the big refactoring we’re talking 
> about, isn’t it?


Cool.

It depends. The cached loader is what I'm least happy with in my 
refactoring. This 
internal cache idea I think is simpler, more performant, and easier to 
understand.
Adding it makes the cached loader changes unnecessary.

Because the changes to support it are minimal, I'll at least make a 
side-branch that
replaces the cached loader changes with an internal cache. If it pans out 
and doesn't
balloon into further unrelated changes, it might be easier to include it in 
this refactoring.

Preston

-- 
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/3f0f9cca-ef69-4528-acef-2c4067035606%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: status of 1.8 release blockers

[Tim Graham]

Today's update:

Rrelease tomorrow seems unlikely -- let's target Friday and if we finish 
earlier, that's a bonus.

#24307  Oracle Syncdb breaks 
trying to set NULL to column that already is NULL 

Owner: Shai
Status: Fix written; working on a test.

#24324  Crashes when project 
path or path to Django install contains a non-ascii character 

Owner: Tim
Status: Mostly committed. Waiting for reply from Berker regarding a couple 
of his review comments.


#24351    RunPython/RunSQL 
operations in contrib migrations and multi-db routers. 
Owner: Loic
Status: Waiting for feedback on API design (see mailing list thread).

#24328  The new 
Options._get_fields() method needs to be cleaned up 

Owner: Anssi
Status: Patch still in progress.

#24343  UUID foreign key 
accessor returns inconsistent type 
Owner: Marc/Josh/Shai
Status: Josh thinks the patch is RFC. Awaiting final review from anyone 
interested.

#24291  Migrations fail with 
unused swappable model 
Owner: Markus
Status: Patch looks good to me (again).

On Tuesday, February 17, 2015 at 12:40:40 PM UTC-5, Marten Kenbeek wrote:
>
> @Tim I believe it's quite easy actually, I've fixed my original patch and 
> it's just waiting to be reviewed. It's available at 
> https://github.com/django/django/pull/4071. However, it's a very rare bug 
> and I only encountered it in the tests when working on another issue, not 
> in any real project. Not really worth delaying the beta I think, I'm 
> getting the feeling you guys are busy enough as it is. 
>
> Op dinsdag 17 februari 2015 18:20:22 UTC+1 schreef Tim Graham:
>>
>> If it's the only release blocker left and the fix is difficult, I think 
>> we could issue a beta release without it. I think the worst case is that 
>> you'll get some duplicate bug reports from testers.
>>
>> On Tuesday, February 17, 2015 at 10:06:27 AM UTC-5, Markus Holtermann 
>> wrote:
>>>
>>> So, it turns out that #24291 (Migrations fail with unused swappable 
>>> model)  only occurs when a 
>>> swapped out model doesn't define an explicit model manager. Since auth.User 
>>> is the only model that is officially supported for "swappiness" and it 
>>> defines an explicit UserManager this isn't really a problem in my opinion. 
>>> Since the whole swappable model API is internal API anyway, I don't want to 
>>> rule this issue a release blocker. If projects in the wild use the 
>>> swappable model API they can get around the issue by adding `objects = 
>>> models.Manager()` to that model.
>>>
>>> Tim, Russ, thoughts 'bout that?
>>>
>>> /Markus
>>>
>>> On Tuesday, February 17, 2015 at 1:05:37 AM UTC+1, Tim Graham wrote:

 Here's the status on blockers. Given the work that remains, I don't 
 foresee the beta release happening earlier than Wednesday, but the next 
 update of this thread will be tomorrow.

 #24324  Crashes when 
 project path or path to Django install contains a non-ascii character 
 
 Owner: Tim
 Status: Patch needs review


 #24351    RunPython/RunSQL 
 operations in contrib migrations and multi-db routers. 
 Owner: Loic
 Status: API design decision needed. Time permitting, Loic will write to 
 the mailing list on this tomorrow.

 #24328  The new 
 Options._get_fields() method needs to be cleaned up 
 
 Owner: Anssi
 Status: Anssi still reviewing the patch; decision may be needed on 
 backwards compatibility of get_fields().

 #24343  UUID foreign key 
 accessor returns inconsistent type 
 Owner: Marc/Josh/Shai
 Status: Josh working on cleaning up the patch. Review/feedback from 
 Anssi requested.

 #24291 
 
  Migrations 
 fail with unused swappable model 
 
 Owner: Markus
 Status: Patch looks good to me; Markus to review & commit tomorrow.

 On Monday, February 16, 2015 

Re: Signature of the allow_migrate database router.

[Loïc Bistuer]

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

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

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.