Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#22002 closed Cleanup/optimization (fixed)

AppConfig.ready() and tests

Reported by: Marc Tamlyn Owned by: Zbigniew Siciarz
Component: Documentation Version: master
Severity: Release blocker Keywords: app-loading
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (9)

comment:1 Changed 3 years ago by Russell Keith-Magee

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

comment:2 Changed 3 years ago by Aymeric Augustin

Keywords: app-loading added

comment:3 Changed 3 years ago by Florian Apolloner

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?

comment:4 Changed 3 years ago by Marc Tamlyn

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.

comment:5 Changed 3 years ago by Zbigniew Siciarz

Owner: changed from nobody to Zbigniew Siciarz
Status: newassigned

comment:6 Changed 3 years ago by Zbigniew Siciarz

I added mjtamlyn's suggestions to AppConfig docs as a warning note. Relevant PR: https://github.com/django/django/pull/2294

comment:7 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 94b5bc361aef2ae2d46a49dbfe32d9271c185800:

Fixed #22002 -- Documented avoiding database interaction in AppConfig.ready().

Thanks Marc Tamlyn for the suggestion.

comment:8 Changed 3 years ago by Jonas H.

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

comment:9 Changed 3 years ago by Aymeric Augustin

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 Django should require specific settings for tests...

Last edited 3 years ago by Aymeric Augustin (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top