Opened 9 years ago
Closed 5 years ago
#26481 closed New feature (duplicate)
Add a "strict mode" for defer()/only() to prevent queries on unfetched field access
Reported by: | Adam Johnson | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | only, defer |
Cc: | me@…, Florian Apolloner | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
In our project at YPlan we had some highly optimized queries using only()
to restrict the fields being fetched.
Some time later the downstream code of these queries was changed to access extra fields not in the only()
list. This triggered the DeferredAttribute
implicit queries to go fill in those fields with single queries, a classic N+1 queries problem.
Whilst better testing on the queries would have revealed this, I can't help but feel that it's a somewhat surprising behaviour that only()
(and by extension, defer()
) can bite back this badly.
I'd like some way of making only()
and defer()
strict, and to raise an exception on deferred fields.
Currently we've implemented this with a patchy.patch
to insert a raise
into DeferredAttribute
:
from django.db.models.query_utils import DeferredAttribute patchy.patch(DeferredAttribute.__get__, """\ @@ -20,6 +20,7 @@ def __get__(self, instance, owner): # might be able to reuse the already loaded value. Refs #18343. val = self._check_parent_chain(instance, name) if val is None: + raise AttributeError("Deferred field '{}' fetched from database!".format(self.field_name)) instance.refresh_from_db(fields=[self.field_name]) val = getattr(instance, self.field_name) data[self.field_name] = val """)
I can imagine this being a Django-wide setting, or maybe per-model so it wouldn't affect third party apps so badly.
Change History (8)
comment:1 by , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 9 years ago
Summary: | Add a "strict mode" for defer()/only() → Add a "strict mode" for defer()/only() to prevent queries on unfetched field access |
---|
follow-up: 5 comment:3 by , 9 years ago
FWIW Adrian proposed adding another keyword argument to only()
/defer()
that results in loading all fields.
Since we might want to add different kind of actions when a deferred field is accessed I suggest to use another name for the kwarg.
For example, we could issue a warning (warnings.warn
) or log one (logging.warning
) instead of raising an exception to prevent the code from crashing while exposing a way to monitor such instances.
comment:4 by , 9 years ago
Overriding Model.refresh_from_db()
allows raising failure when deferred field is accessed - is this enough for you use case?.
comment:5 by , 9 years ago
Replying to charettes:
FWIW Adrian proposed adding another keyword argument to
only()
/defer()
that results in loading all fields.
Loading all, or loading in groups as suggested by me in that thread and done in sqlalchemy (http://docs.sqlalchemy.org/en/latest/orm/loading_columns.html#deferred-loading-with-multiple-entities) seems like a good idea.
For example, we could issue a warning (
warnings.warn
) or log one (logging.warning
) instead of raising an exception to prevent the code from crashing while exposing a way to monitor such instances.
Since this feature would be optional for backwards compat anyways I do not see the need for logging.
comment:6 by , 9 years ago
Cc: | added |
---|
comment:7 by , 5 years ago
This has been implemented as a third-party application.
Not sure if this ticket should be closed as a duplicate of #22492 like #30874 was.
comment:8 by , 5 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Yes, it's the same ball-park certainly.
Since #22492 is Accepted, I wonder if bringing django-seal into core is on your agenda Simon?
The API that comes to mind to me would be something like
only(..., strict=True)
. Possible APIs should be discussed on the DevelopersMailingList before doing the implementation.