Opened 2 years ago

Closed 4 months ago

#33143 closed Cleanup/optimization (fixed)

Block import-time queries

Reported by: Adam Johnson Owned by: Florian Zimmermann
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It's possible to make ORM queries at import time. For example:

class Form(forms.Form):
    country = forms.ChoiceField(choices=[c.name for c in Country.objects.all()])

I see import time queries fairly frequently, and every time I have encountered them they were a mistake. They end up querying the database once when the web server starts and caching the data forever, which is never desired.

Python allows imports to happen at any time, so there's no way to detect when "import time" is over, and the app is really running. An inner import may cause a module to be first loaded during a web request.

I therefore propose we block queries until *after* the AppConfig.ready() phase. This would protect against most problems.

We could block until *before* AppConfig.ready(). Queries inside a ready() method kind-of imply the user knows that the query is one-off at startup. But we do encourage users to perform some imports within ready() methods such as for signal registration.

Change History (15)

comment:2 Changed 2 years ago by Simon Charette

I'd be in favor of blocking until *after* AppConfig.ready() phase as any queries performed at .ready() time has the potential to crash due to still unapplied migrations (missing table, schema mismatch) and can get users in a pickle where they can't even run migrate. This is something we've been warning against for a while and that I've had to point to a fair amount of time when reviewing Django project changes.

comment:3 Changed 2 years ago by Adam Johnson

Thanks for the developers discussion link Tim. I like the proposal of warning and then if there are no serious reports, deprecating from the next version. A RuntimeWarning sounds appropriate.

And I see you have some history with this Simon.

Aymeric was previously "+0" but he may have changed his mind. I will post again on the thread with the link to this ticket just to see if there are any dissenting opinions.

comment:4 Changed 2 years ago by Carlton Gibson

Triage Stage: UnreviewedAccepted

I think there's enough consensus to Accept.

Right or wrong, I suspect there are plenty of folks using ready() as a home for running some kind of query at startup. We should have some advice for them.

comment:5 Changed 6 months ago by Florian Zimmermann

Owner: changed from nobody to Florian Zimmermann
Status: newassigned

comment:6 Changed 6 months ago by Florian Zimmermann

Has patch: set

comment:7 Changed 6 months ago by Natalia Bidart

Patch needs improvement: set

comment:8 Changed 6 months ago by Florian Zimmermann

Patch needs improvement: unset

comment:9 Changed 5 months ago by Mariusz Felisiak

Needs tests: set
Patch needs improvement: set

comment:10 Changed 5 months ago by Florian Zimmermann

Needs tests: unset
Patch needs improvement: unset

comment:11 Changed 4 months ago by Mariusz Felisiak

Patch needs improvement: set

Marking as need improvement per Adam's comment.

comment:12 Changed 4 months ago by Florian Zimmermann

Patch needs improvement: unset

comment:13 Changed 4 months ago by Mariusz Felisiak

Patch needs improvement: set

comment:14 Changed 4 months ago by Mariusz Felisiak

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:15 Changed 4 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In fbd16438:

Fixed #33143 -- Raised RuntimeWarning when performing import-time queries.

Note: See TracTickets for help on using tickets.
Back to Top