[Django] #26481: Add a "strict mode" for defer()/only()
Django
----------------------------------------------+-------------------------
Reporter: adamchainz | Owner: nobody
Type: New feature | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords: only, defer
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+-------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26481>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: adamchainz | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: only, defer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26481#comment:1>
Django
field access
-------------------------------------+-------------------------------------
Reporter: adamchainz | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: only, defer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26481#comment:2>
Django
field access
-------------------------------------+-------------------------------------
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: only, defer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
FWIW Adrian proposed adding another keyword argument to `only()`/`defer()`
that [https://groups.google.com/forum/#!searchin/django-developers/defer
/django-developers/C3qoHEfeeUg/cwBNV5snQPMJ 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26481#comment:3>
Django
field access
-------------------------------------+-------------------------------------
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: only, defer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by akaariai):
Overriding Modelrefresh_from_db allows raising failure when deferred field
is accessed - is this enough for you use case?
--
Ticket URL: <https://code.djangoproject.com/ticket/26481#comment:4>
Django
field access
-------------------------------------+-------------------------------------
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: only, defer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by apollo13):
Replying to [comment:3 charettes]:
> FWIW Adrian proposed adding another keyword argument to
`only()`/`defer()` that [https://groups.google.com/forum/#!searchin
/django-developers/defer/django-developers/C3qoHEfeeUg/cwBNV5snQPMJ
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26481#comment:5>
Django
field access
-------------------------------------+-------------------------------------
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: only, defer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by apollo13):
* cc: apollo13 (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26481#comment:6>
Django
field access
-------------------------------------+-------------------------------------
Johnson |
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: only, defer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
This has been implemented [https://github.com/charettes/django-seal as a
third-party application].
Not sure if this ticket should be closed as a duplicate of #22492 like
#30874 was.
--
Ticket URL: <https://code.djangoproject.com/ticket/26481#comment:7>
Django
field access
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Component: Database layer | Version: master
(models, ORM) |
Keywords: only, defer | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => duplicate
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/26481#comment:8>