[Django] #22002: AppConfig.ready() and tests
Django
------------------------------------------------+------------------------
Reporter: mjtamlyn | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Release blocker | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
If you do something with the database in AppConfig.ready(), then during
test runs it runs against the production db as it happens before django
knows it's testing. I don't think there's any way to change this, but it
should be documented.
--
Ticket URL: <https://code.djangoproject.com/ticket/22002>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by russellm):
Is there a use case driving this? I'm having difficulty thinking of any
sort of database activity that you would need to do in a ready() method.
Of course, that's probably due to a lack of imagination on my part.
Agreed that at the very least, there needs to be docs, even if those docs
are "don't do that".
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:1>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* keywords: => app-loading
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:2>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by apollo13):
I think it is somewhat dangerous that stuff can get executed against the
production database (even though the test config shouldn't have it in
settings at all). Can't we (re)configure the databases earlier?
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:3>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
I discovered this accidentally investigating (bad) solutions for #22022. I
propose just docs. At the moment, `manage.py test` is not special cased in
the core management. We call `django.setup()` and then branch on the
command used. I think that we should:
1) recommend never interacting with the database in `ready()` (or in
startup code in general - this is what migrations are for right?)
2) mention that it would be run against the main database even in testing
I don't think it's actually that big a deal about the test/prod situation.
Such code would be run *every time* you call a management command of any
kind, so you'd soon find out if you were breaking the db. We could
theoretically explicitly error during the calls to `ready()` if we try to
obtain a connection cursor, but that would be a bit dirty in its
implementation.
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:4>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Release blocker | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by zsiciarz):
* owner: nobody => zsiciarz
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: zsiciarz
Type: Cleanup/optimization | Status: assigned
Severity: Release blocker | Resolution:
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by zsiciarz):
I added mjtamlyn's suggestions to AppConfig docs as a warning note.
Relevant PR: https://github.com/django/django/pull/2294
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: zsiciarz
Component: Documentation | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"94b5bc361aef2ae2d46a49dbfe32d9271c185800"]:
{{{
#!CommitTicketReference repository=""
revision="94b5bc361aef2ae2d46a49dbfe32d9271c185800"
Fixed #22002 -- Documented avoiding database interaction in
AppConfig.ready().
Thanks Marc Tamlyn for the suggestion.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: zsiciarz
Type: Cleanup/optimization | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by jonash):
If it's not terribly difficult to implement then I'd like to advocate the
"explicit error" way of solving this issue. These kind of things are
easily overseen and difficult to debug and/or can cause severe damage in
your database. (People will abuse the `ready()` method for all kinds of
things you'd never dream about...)
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:8>
Django
--------------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: zsiciarz
Type: Cleanup/optimization | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: app-loading | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
The real solution is to stop pretending it's a good design to switch to a
different database magically for tests... Tests should run with the
database specified in the settings, and Djangp should require specific
settings for tests...
--
Ticket URL: <https://code.djangoproject.com/ticket/22002#comment:9>