Adding support for importing models through the app registry while it is loading

[Aymeric Augustin]

Hello,

Since Django 1.7, `get_user_model()` cannot be called at import time. Django 
contains a lot of methods that start with `UserModel = get_user_model()` while 
it would be more natural to declare it as a global variable in the module. This 
trips up users and the reason why it’s forbidden isn’t obvious.


It's the consequence of two design decisions (which I’m responsible for) in the 
app-loading refactor.

1. Before Django 1.7, the sequence in which models were loaded could depend on 
which views were hit in what order after a WSGI process started. That allowed 
interesting failures mode. For example, a project could randomly fail to load, 
only in production, because of a circular import error. Throw multithreading 
into the mix and it could get really exciting. To solve this, the new 
app-loading system added an explicit `django.setup()` step that imports apps 
and models in a deterministic order and doesn’t let code interact with the app 
registry until it’s fully loaded.

2. Django must ensure that relationships between models are set up correctly. 
Specifically it must guarantee that reverse accessors are added to each model 
targeted by a foreign key. This guarantee can only be provided once all other 
models have been imported. There’s been a long string of bugs in this area 
around 2008 (if memory serves). The new app-loading system took this 
requirement into account at the design stage and, thanks to the explicit setup, 
is structurally more robust against such bugs.

Because of 2. apps.get_models() raises an exception before all models are 
imported rather than return an incomplete model. Because of 1. Django gives 
very little control on what happens up to this point.


However, in many cases, it doesn’t matter whether the model is fully 
constructed or not. In the use case I described in my introduction, the code 
only needs a reference to the model class at import time; that reference won’t 
be used until run time, after the app-loading process has completed. 
(Performing SQL queries through the ORM at import time doesn’t work anyway.)

For this reason, I think it would make sense to add an API similar to 
`apps.get_models()`, but that would work while app-loading is still in 
progress. It would make no guarantees about the functionality of the class it 
returns until app-loading has completed.

I implemented a proof of concept here: 
https://github.com/django/django/pull/6547.


I would appreciate feedback on the following questions:

1. Have you been missing this feature? If it’s needed outside of Django, I must 
make it a public API.

2. Is it reasonable to expose it as a public API? There’s a risk that 
StackOverflow will consider it as superior to get_models() because it’s more 
lax; they'll assume that I put checks in get_models() simply because I like 
messing with users.

3. Would you prefer adding a kwarg to get_models e.g. 
apps.get_models(settings.AUTH_USER_MODEL, unsafe=True) or adding a new method 
like the PR currently does? I’m thinking the former is more user friendly as 
the functionality is very similar.


Thanks,

-- 
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 https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/67450ED7-88CD-49CE-9B63-180EDC033D55%40polytechnique.org.
For more options, visit https://groups.google.com/d/optout.

Re: unpleasant experience trying to bisect a change or regression

[Tim Graham]

Thanks, Paulo, and I'm sorry, this was my fault. My app was indeed using 
has_page_change_permission(). It wasn't a problem with django-cms. Another 
change in django-cms caused the exception in my app to change to something 
else, but ultimately I resolved my problems by removing usage of 
has_page_change_permission() and fixing usage of remove features 
(cms_app.py to cms_apps.py and cms_toolbar.py to cms_toolbars.py).

One possible improvement for django-cms could be the release notes 
regarding how to upgrade for the permissions changes: 
http://docs.django-cms.org/en/latest/upgrade/3.4.html#permissions

I've attached a diff of the change I made to my app and maybe you can tell 
me if it's correct. In particular, I couldn't tell the reason for both the 
Page.has_change_permission() method and the 
cms.utils.page_permissions.user_can_change_page() function. Since the 
former calls the latter, I can't see why the latter is needed.

page.has_change_permission(self.request.user)

seems simpler to me than (equivalent I think):

page_permissions.user_can_change_page(request.user, page=page)

Thanks!

On Friday, September 2, 2016 at 8:19:08 AM UTC-4, czpython wrote:
>
> Hello Tim,
>
> Sorry for trouble.
> Following Django practices, we've started to only do *one* commit per PR 
> which means that we no longer have a change with commits that work and 
> others that don't.
> The big "Bumped version to 3.4.0rc1" commit contains release *only* 
> changes like PO file changes from Transifex,  compilemessages, etc.. it's 
> why it shows as 5k lines +.
>
> Regarding the ImportError cannot import name 'has_page_change_permission' 
> error, this is caused by our refactor of the permissions logic.
> has_page_change_permission was not a public API and as such got removed 
> without a deprecation period, you can see the list of functions that were 
> removed in the release notes 
> .
>
> Most of the functions there were replaced by more specific functions in 
> cms.utils.page_permissions, if you still rely on the CMS permissions system 
> feel free to ping me to get more information
> on the new API. Like the old one, it's still private but now it's very 
> unlikely to change for a bit.
>
>
> On Thursday, September 1, 2016 at 9:41:08 PM UTC-4, Tim Graham wrote:
>>
>> Hi,
>>
>> I wanted to share my experience trying to update to django-cms 3.4. When 
>> I launch my app, I get a NoReverseMatch exception related to URL reversing, 
>> so I tried to use git bisect to find the commit where the behavior changed 
>> in the django-cms. This wasn't so pleasant as there were a long string of 
>> commits where I couldn't run my app due to "ImportError cannot import name 
>> 'has_page_change_permission'" coming from django-cms. It seems that 
>> django-cms may be broken in its history for quite a few commits (maybe 
>> during the massive "Bumped version to 3.4.0rc1" merge commit?) which makes 
>> it quite difficult to pin exactly why my app is failing. I'm not sure if 
>> this is a common issue anyone else has seen or if it could have been 
>> avoided, but I just wanted to emphasize that it's really helpful if 
>> django-cms is working after each commit to help in a case like this. I'll 
>> admit that I don't really understand how git merging works but I find 
>> Django's cherry picking of commits to make backports to be very reliable in 
>> terms of making history bisectable. Anyway, just wanted to share in case 
>> some process improvement could come of it. I'll try to debug the actual 
>> problem with my app later.
>>
>>
>> There are only skipped commits left to test.
>> The first bad commit could be any of:
>> 035de0310f54ed4b2ddacf9bd715c3ab4e9de072
>> 046dc3e937b18a4f16526257320aa25a7a143754
>> 1c92e02c8bfaba544663c91c6d7efa30e40e0de0
>> 70a5c61500d7d3c7f09f7844bb64db4beeff7a7c
>> 4989c294bc21dd725690983a0034300d5c1770e5
>> f96c80357c7152ef46045842ad0edb9b27dddae7
>> 0136232ded0b97605d88df3da0f42a7fa13fcda5
>> bc1ae40a84592436ef56ac93a21e3f5320324174
>> 59d303bb0b07beecbbe3f18a2498e382d6022430
>> 8e2e049eaaf85b01a1e1949ce6dec6366fca577e
>> 153b3d5625117769354918fb28ae73c48b417a4d
>> c6e7ccd9c1e8fab5bc257abb6d9e6b268fb93f9a
>> c7617f3150e3fddc37ed2345aa2d38b6f0956ee1
>> 365e503f7e21e924cfae13bf7a0f8364b2e98d4c
>> db2b69ba800033515aa8cdf907895cb7d7fb19be
>> aabeb755ee6d711f403370f1b4149b3de2e985fc
>> 355b7671b60a43b53f60ff3f23be57f460e920b2
>> 4fd0d66ecdc44fa081b1d19e8550320a00b59b76
>> a4c0cb5d99bee250b6f0c6e33f50503a8248ef7c
>> d41b42316ebacb236ca116a4b2aa324291da673e
>> e686f74542b08757a8c996df034909d158573780
>> 08c51d30a00727fae9c47ba650cb088e7646f47b
>> 9b4af053b1f3d90e52d4c054b83682aef05c57d2
>> af21e0c352df6a07d98587c1e8a31f072a6a43e8
>> 16e3bbecb7c9bf5d77c56e49832f473b177de6cf
>> 5c09f1d731aa644fb091941654a37843ff9eab7a
>> b4ca4580aaa7531644ccc6661614d21417d3ee7b
>> We cannot bisect more!
>>
>>

-- 
Message URL: 

Re: Exceptions caught by the "include" template tag make it hard to rely on tests

[Jon Dufresne]

> Has anyone relied on the exception silencing behavior as a "feature" when
using {% include %}?

Not here.

+1 on deprecating the silencing behavior.

I have created a ticket [0] and PR [1] so others may get a sense of the
bigger picture change and how it could affect Django. Feedback welcome.

More generally, I find the template engine's behavior of replacing
exceptions with an empty string to cause more bugs than it prevents. In my
experience, loud errors will more likely be caught during development, unit
tests, and manual testing.

[0] https://code.djangoproject.com/ticket/27175
[1] https://github.com/django/django/pull/7208

-- 
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 https://groups.google.com/group/django-developers.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-developers/CADhq2b7%3Daxpcduaas68ihWugLhiffTPmAx0bQoyTy%3Dsa8Sq7GA%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.