Re: [Django] #36282: Prefetched relations are not used from parent classes in all cases

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: mti | Triage Stage: Accepted
prefetch_related |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* component: Uncategorized => Database layer (models, ORM)
* keywords: => mti prefetch_related
* stage: Unreviewed => Accepted
* type: Uncategorized => Bug

Comment:

Interestingly I ran into the same issue [https://github.com/charettes
/django-
polymodels/pull/45/commits/cdb4c54f402619ff91f0559f4e5af14810e434fa
implementing similar logic 5 years ago].

If you're interested in solving this issue I'd have a few pointers

1. Avoid copy'ing `_prefetched_objects_cache` related cache between model
instances as it will make cache invalidation a nightmare (the workaround I
linked above is flawed in this regard). The logic should be adapted to
fully walk the ancestor chain. In other words, the cached value should
remain solely on the model hosting the field and subclasses accessors
should retrieve them from there.
2. When adding tests to `tests/prefetch_related` reuse the existing
models. It seems that `AuthorWithAge` is a good candidate for subclassing.
3. The logic should always ensure that ancestor links are cached before
accessing their parent (it's not always the case when field deferral is
used)

Here's a little patch I used to validate your report if it can be handy

{{{#!diff
diff --git a/django/db/models/fields/related_descriptors.py
b/django/db/models/fields/related_descriptors.py
index 77ebe798b4..36e8798340 100644
--- a/django/db/models/fields/related_descriptors.py
+++ b/django/db/models/fields/related_descriptors.py
@@ -226,20 +226,23 @@ def __get__(self, instance, cls=None):
rel_obj = self.field.get_cached_value(instance)
except KeyError:
has_value = None not in
self.field.get_local_related_value(instance)
+ field_model = self.field.model
ancestor_link = (
- instance._meta.get_ancestor_link(self.field.model)
- if has_value
- else None
+ instance._meta.get_ancestor_link(field_model) if
has_value else None
)
- if ancestor_link and ancestor_link.is_cached(instance):
- # An ancestor link will exist if this field is defined on
a
- # multi-table inheritance parent of the instance's class.
- ancestor = ancestor_link.get_cached_value(instance)
- # The value might be cached on an ancestor if the
instance
- # originated from walking down the inheritance chain.
- rel_obj = self.field.get_cached_value(ancestor,
default=None)
- else:
- rel_obj = None
+ rel_obj = None
+ # An ancestor link will exist if this field is defined on a
+ # multi-table inheritance parent of the instance's class.
+ if ancestor_link:
+ ancestor = instance
+ while ancestor_link.is_cached(ancestor):
+ ancestor = ancestor_link.get_cached_value(ancestor)
+ if type(ancestor) is field_model:
+ # The value might be cached on an ancestor if the
instance
+ # originated from walking down the inheritance
chain.
+ rel_obj = self.field.get_cached_value(ancestor,
default=None)
+ else:
+ ancestor_link =
ancestor._meta.get_ancestor_link(field_model)
if rel_obj is None and has_value:
rel_obj = self.get_object(instance)
remote_field = self.field.remote_field
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:2>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: mti | Triage Stage: Accepted
prefetch_related |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Adya):

Replying to [comment:2 Simon Charette]:
Hi, can I work on it?
--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:3>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: mti | Triage Stage: Accepted
prefetch_related |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

The reporter explicitly stated

> I would be willing to work on a patch to make this work out of the box
in Django.

So I would give it a few more days before assigning the ticket to you
Adya.
--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:4>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: mti | Triage Stage: Accepted
prefetch_related |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Adya):

Replying to [comment:4 Simon Charette]:

Ok, it makes sense.

Could you suggest me any other available ticket, and I can work on it? It
will be my pleasure
--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:5>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: (none)
Type: Bug | Status: new
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: mti | Triage Stage: Accepted
prefetch_related |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Take Weiland):

Replying to [comment:2 Simon Charette]:

Thank you for your pointers. I have already started working on this here:
https://github.com/django/django/compare/main...diesieben07:django:prefetch_related_inheritance

I will work on polishing it to get it ready for a contribution. One thing
to think about is indeed cache invalidation. If you do
`parent.child.m2m_relation.set(...)` should that invalidate
`parent.m2m_relation`? Currently it doesn't, but if
`parent.child.m2m_relation` and `parent.m2m_relation` now point to the
same cache, because the child one just looks in its parent...?
Probably the clearing also needs to happen recursively and walk up to the
parents, but that is a potentially more invasive change.
--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:6>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------

Reporter: Take Weiland | Owner: Take
| Weiland
Type: Bug | Status: assigned

Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: mti | Triage Stage: Accepted
prefetch_related |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* owner: (none) => Take Weiland
* status: new => assigned

Comment:

Good point about cache invalidation.

All layers of inherited caching should effectively be busted when
`parent.child.rel` is altered.
--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:7>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: Take
| Weiland
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: mti | Triage Stage: Accepted
prefetch_related |

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Take Weiland):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:8>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: Take
| Weiland
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: mti | Triage Stage: Accepted
prefetch_related |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:9>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: Take
| Weiland
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: mti | Triage Stage: Ready for
prefetch_related | checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):

* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:10>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: Take
| Weiland

Type: Bug | Status: closed

Component: Database layer | Version: 5.1
(models, ORM) |

Severity: Normal | Resolution: fixed

Keywords: mti | Triage Stage: Ready for
prefetch_related | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce <42296566+sarahboyce@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"e709301000edddc48ebfe9535e2e2177d3bcedb1" e709301]:
{{{#!CommitTicketReference repository=""
revision="e709301000edddc48ebfe9535e2e2177d3bcedb1"
Fixed #36282 -- Used prefetched values in ForwardManyToOneDescriptor from
indirect ancestors.

When looking for cached values in ManyRelatedManager and
ForwardManyToOneDescriptor walk up the whole chain of ancestors
(as long as they are cached) to find the prefetched relation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:11>

[Django]

#36282: Prefetched relations are not used from parent classes in all cases
-------------------------------------+-------------------------------------
Reporter: Take Weiland | Owner: Take
| Weiland
Type: Bug | Status: closed
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: mti | Triage Stage: Ready for
prefetch_related | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by GitHub <noreply@…>):

In [changeset:"e664c5afa934be5a7b57576de12620071077c4fb" e664c5a]:
{{{#!CommitTicketReference repository=""
revision="e664c5afa934be5a7b57576de12620071077c4fb"
Refs #36282 -- Fixed PrefetchRelatedMTICacheTests test ordering
expectations.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36282#comment:12>