[Django] #35044: Accessing a deferred field clears reverse relations
Django
-------------------------------------+-------------------------------------
Reporter: Adam | Owner: nobody
Johnson |
Type: | Status: assigned
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
`DeferredAttribute.__get__` calls `Model.refresh_from_db()` to load the
deferred field. Whilst `refresh_from_db()` does load the intended field,
it also clears a lot of cached data, including reverse relations. This can
causes extra queries when those relations are accessed before and after a
deferred field.
For example, take these models:
{{{
class Book(models.Model):
title = models.TextField()
...
class BookText(models.Model):
book = models.OneToOneField(Book, on_delete=models.CASCADE)
text = models.TextField()
}}}
With query debug logging on, we can see that a second access to `booktext`
after accessing the deferred `title` causes an unnecessary extra query:
{{{
In [1]: from example.models import *
In [2]: b=Book.objects.defer('title').earliest('id')
(0.000) SELECT "example_book"."id", "example_book"."author_id" FROM
"example_book" ORDER BY "example_book"."id" ASC LIMIT 1; args=();
alias=default
In [3]: b.booktext
(0.000) SELECT "example_booktext"."id", "example_booktext"."book_id",
"example_booktext"."text" FROM "example_booktext" WHERE
"example_booktext"."book_id" = 1 LIMIT 21; args=(1,); alias=default
Out[3]: <BookText: BookText object (1)>
In [4]: b.title
(0.000) SELECT "example_book"."id", "example_book"."title" FROM
"example_book" WHERE "example_book"."id" = 1 LIMIT 21; args=(1,);
alias=default
Out[4]: 'A 1'
In [5]: b.booktext
(0.000) SELECT "example_booktext"."id", "example_booktext"."book_id",
"example_booktext"."text" FROM "example_booktext" WHERE
"example_booktext"."book_id" = 1 LIMIT 21; args=(1,); alias=default
Out[5]: <BookText: BookText object (1)>
}}}
This is due to `refresh_from_db()` clearing the reverse-related object
cache.
Spotted whilst working on #28586. My implementation for that will probably
fix this issue, but I thought it best to report this separately.
--
Ticket URL: <https://code.djangoproject.com/ticket/35044>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Cleanup/optimization => Bug
* stage: Unreviewed => Accepted
Comment:
Feels like a bug to me.
When `refresh_from_db(fields)` is specified it should likely not clear
reverse relationships and I think the same can be said about
Regression in a7b5ad8b19a08d7d57302ece74f6e26d2887fd9f #27846 for reverse
relations and possibly 123b1d3fcf79f091573c40be6da7113a6ef35b62 #34137 for
private fields. The private field situation is more complex though as they
might be composed of other fields (e.g. `GenericForeignKey`) but the field
APIs doesn't expose a generic way of introspecting that so I would assume
we'd still want to clear .
An alternative here would be to accept another kwarg to denote field cache
clearing that would default to `True` and that `DeferredAttribute.__get__`
would pass `False` to.
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:1>
Django
-------------------------------------+-------------------------------------
| Terzopoulos
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Giannis Terzopoulos
Comment:
Hey, I'm interested in taking this over unless @Adam, you might want to
fix this as part of #28586?
> Spotted whilst working on #28586. My implementation for that will
probably fix this issue
What I'm thinking to do is follow Simon's plan above and not clear cached
relations when `refresh_from_db(fields)` is specified, unless a relation
is part of `fields` explicitly. Not sure how I would deal with the
private fields though to be honest, or if we actually need to handle that.
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Giannis
| Terzopoulos
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
> Not sure how I would deal with the private fields though to be honest,
or if we actually need to handle that.
yeah not sure either, I suggest trying the same approach that avoids
clearing if `fields` is specified and the private field name is not part
of them. My only concern is that I'm not sure how `.only` will behave when
passed private field names. In all cases the only regression test existing
for #34137
[https://github.com/django/django/commit/123b1d3fcf79f091573c40be6da7113a6ef35b62
#diff-0e842fa4e4a689b120379780e62dc71a1476a409e21ccd09eac181ec5a2314d8R50
doesn't make use] of `fields` so it's in a grey area.
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Giannis
| Terzopoulos
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Giannis, feel free to take this ticket. It’s now looking like my approach
to #28586 will go in a different direction.
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Giannis
| Terzopoulos
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Thank you both! I opened a PR for this
[https://github.com/django/django/pull/17831 PR]
I've added tests for `defer()` and `only()` for relations/private fields
and updated the `refresh_from_db()` accordingly. I also added a couple of
tests with the `fields` kwarg specified.
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Giannis
| Terzopoulos
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Giannis
| Terzopoulos
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Giannis
| Terzopoulos
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In [changeset:"0c690c60012e6a2f0cde0fc564c55054c465d02f" 0c690c6]:
{{{#!CommitTicketReference repository=""
revision="0c690c60012e6a2f0cde0fc564c55054c465d02f"
Refs #35044 -- Added Model.refresh_from_db(fields=...) test for clearing
reverse relations.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Giannis
| Terzopoulos
(models, ORM) |
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"73df8b54a2fab53bec4c7573cda5ad8c869c2fd8" 73df8b54]:
{{{#!CommitTicketReference repository=""
revision="73df8b54a2fab53bec4c7573cda5ad8c869c2fd8"
Fixed #35044 -- Avoided clearing reverse relations and private fields when
accessing deferred fields.
Regression in a7b5ad8b19a08d7d57302ece74f6e26d2887fd9f for reverse
relations and possibly in 123b1d3fcf79f091573c40be6da7113a6ef35b62 for
private fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35044#comment:9>