Add warning or prohibit module level queries?

[Tim Graham]

Simon proposed [0]: "I wonder if we should prevent django.db from executing queries until django.apps.apps.ready or at least issue a RuntimeWarning. We would have to go through deprecation but I'm pretty sure this would uncover a lot of existing application bugs and prevent future ones. This is related to #25454 [1] and probably a large number of closed tickets."

We have this restriction in some places, for example: "Executing database queries with the ORM at import time in models modules will also trigger this exception. The ORM cannot function properly until all models are available."

We also have a warning about using the ORM in AppConfig.ready(): "Although you can access model classes as described above, avoid interacting with the database in your ready() implementation. This includes model methods that execute queries (save(), delete(), manager methods etc.), and also raw SQL queries via django.db.connection. Your ready() method will run during startup of every management command. For example, even though the test database configuration is separate from the production settings, manage.py test would still execute some queries against your production database!"

There's also a warning in the testing docs: "Finding data from your production database when running tests? If your code attempts to access the database when its modules are compiled, this will occur before the test database is set up, with potentially unexpected results. For example, if you have a database query in module-level code and a real database exists, production data could pollute your tests. It is a bad idea to have such import-time database queries in your code anyway - rewrite your code so that it doesn’t do this. This also applies to customized implementations of ready()."

What do you think? Prohibiting such queries might be too strict at this point as I guess some users might rely on them. I suppose warnings could be selectively silenced as/if needed. We could start with a warning and ask users to let us know if they believe they have a legitimate usage. If we don't hear anything, we could proceed with the deprecation.

Related tickets:

[0] https://code.djangoproject.com/ticket/26273
[1] https://code.djangoproject.com/ticket/25454

[Aymeric Augustin]

Hello,

To the best of my knowledge, in Django 1.10, the window for such problematic database queries will shrink to the interval between the moment `apps.models_ready` becomes `True` and the moment `apps.ready` becomes `True`. That’s when AppConfig.ready() methods execute.

In Django 1.9, if a developer references their views with “Django string magic" in their URLconf, database queries can also easily happen during the first request, when modules containing views get imported. (URLconfs are lazily loaded, aren’t they?) This will go away in 1.10, though.

Going back to Django 1.10, I’m wondering if forbidding database queries in AppConfig.ready() methods would restrict their usefulness significantly. From Django’s perspective, it’s safe to use the ORM at that point (except when running tests). It’s pretty clear AppConfig.ready() executes exactly once at startup. Developers should expect querysets fetched in AppConfig.ready() not to change over the application’s lifetime.

One might consider the ORM not to be safe until `apps.ready` becomes `True` e.g. if a developer alters models dynamically in an AppConfig.ready(). Well, in that case, it’s up to them to ensure their project remains consistent. There’s only so much safety Django can provide in the presence of monkey patching.

I think additional efforts should focus primarily on importing all modules during the app loading process, to avoid importing additional modules on the first request (the most common case) or on later requests (a possible case, if imports are put inside functions instead of at module level.) Currently that’s the most likely way import-time database queries and the corresponding bugs can still occur in reasonable Django projects.

The problem of running queries before switching to the test database is a real one, though, and sways me from -0 to +0 on at least warning and perhaps erroring out when queries are run before `apps.ready` becomes `True`. Tim’s proposal looks reasonable and consistent with the current documentation.

Best regards,

-- 

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 https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/7f41ba58-09ed-4f78-b5ce-be6d7d5a6fc7%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Curtis Maloney]

+1

At the very least we need to provide better feedback or errors to help
isolate _where_ this is happening.

I've helped quite a number of people on IRC with this problem...
firstly, people aren't understanding _what_ the problem is, but also
discerning where it's happening is often quite difficult, even with a
full traceback.

--
C

On 26/02/16 04:29, Tim Graham wrote:
> Simon proposed [0]: "I wonder if we should prevent django.db from
> executing queries until django.apps.apps.ready or at least issue a
> RuntimeWarning. We would have to go through deprecation but I'm pretty
> sure this would uncover a lot of existing application bugs and prevent
> future ones. This is related to #25454

> <https://code.djangoproject.com/ticket/25454> [1] and probably a large

> number of closed tickets."
>
>
> We have this restriction in some places, for example: "Executing
> database queries with the ORM at import time in models modules will also
> trigger this exception. The ORM cannot function properly until all
> models are available."
>
> We also have a warning about using the ORM in AppConfig.ready():
> "Although you can access model classes as described above, avoid
> interacting with the database in your |ready()| implementation. This
> includes model methods that execute queries (|save()|, |delete()|,
> manager methods etc.), and also raw SQL queries via

> |django.db.connection|. Your |ready()| method will run during startup of

> every management command. For example, even though the test database
> configuration is separate from the production settings, |manage.py test|

> would still execute some queries against your *production* database!"

>
> There's also a warning in the testing docs: "Finding data from your
> production database when running tests? If your code attempts to access

> the database when its modules are compiled, this will occur /before/ the

> test database is set up, with potentially unexpected results. For
> example, if you have a database query in module-level code and a real

> database exists, production data could pollute your tests. /It is a bad
> idea to have such import-time database queries in your code/ anyway -

> rewrite your code so that it doesn’t do this. This also applies to
> customized implementations of |ready()|."
>
> What do you think? Prohibiting such queries might be too strict at this
> point as I guess some users might rely on them. I suppose warnings could
> be selectively silenced as/if needed. We could start with a warning and
> ask users to let us know if they believe they have a legitimate usage.
> If we don't hear anything, we could proceed with the deprecation.
>
> Related tickets:
>
> [0] https://code.djangoproject.com/ticket/26273
> [1] https://code.djangoproject.com/ticket/25454
>

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

> <mailto:django-develop...@googlegroups.com>.

> To post to this group, send email to django-d...@googlegroups.com

> <mailto:django-d...@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/7f41ba58-09ed-4f78-b5ce-be6d7d5a6fc7%40googlegroups.com

> <https://groups.google.com/d/msgid/django-developers/7f41ba58-09ed-4f78-b5ce-be6d7d5a6fc7%40googlegroups.com?utm_medium=email&utm_source=footer>.

[Adam Johnson]

I noticed I've continued to encounter this problem in code review relatively frequently, so I made a ticket: https://groups.google.com/g/django-developers/c/7JwWatLfP44/ . Tim then pointed me back to this discussion.

I'm happy to implement Tim's suggestion of a RuntimeWarning followed by deprecation if it doesn't cause many problems. Just checking here if anyone would be against the idea.

[laym...@gmail.com]

+1, I've had people raising issues that were caused by this. They were using a query to provide a model field's default value.