Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#30493 closed Bug (fixed)

GenericRelation and prefetch_related: wrong caching with cyclic prefetching.

Reported by: Finn Stutzenstein Owned by: Can Sarıgöl
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: GenericRelation prefetch_related
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hello @all!

I encountered an issue with GenericRelations. Here is an example to reproduce this issue: https://github.com/FinnStutzenstein/GenericRelatedPrefetch
Just do a migrate and runserver. The code showing the error is started automatically in main/apps.py. Please start with --noreload not to have the output twice.

Whats the problem?
I have a generic model (Tag) that have a content_object. Then there are multiple (in the example 2) models, Book and CD, that have exactly one tag assigned. In the real application (OpenSlides) this invariant is ensured in other places; in the example the objects are created in a way, that this invariant holds.

All these content objects have a property tag, that should return the one assigned tag. This is done by adding a helper field tags=GenericRelation(Tag) and the property accesses self.tags.all()[0]. The .all()[0] instead of a simple .get() is required for the prefetching to work. See main/models.py in the example.

Now all tags should be loaded because in OpenSlides they would be serialized. For each tag the content_object is accessed as well as content_object.tag. Because this would result in many DB queries (in the real application about 10000 Tags, and in sum 10000 content objects) the models are prefetched with: Tag.objects.prefetch_related("content_object", "content_object__tag") (Note: The executed code is in main/apps.py). This results in a constant amount of queries (4 in this case) instead of something proportional to the amount of objects. In the example you can set N, the amount of objects created, to a higher amount to verify, that the amount of queries stays constant.

What is expected: If I have a tag tag, tag.content_object.tag should be equal to tag.
Output from the example (with N=1):

Got 'Tag to book0':
    -the content object: Book0
    -the content objects tag (should be the same as 'Tag to book0'!):Tag to book0
Got 'Tag to cd0':
    -the content object: CD0
    -the content objects tag (should be the same as 'Tag to cd0'!):Tag to book0

This is not the case: 'Tag to cd1' -> 'cd1' -> 'Tag to book1'.

I tracked this a bit showing, that _prefetched_objects_cache holds the wrong value, which is accessed through .all() -> .get_queryset() where the cached/prefetched result is taken.

Thanks!

Change History (9)

comment:1 by Mariusz Felisiak, 6 years ago

Component: contrib.contenttypesDatabase layer (models, ORM)
Summary: GenericRelation and prefetch_related: wrong caching with cyclic prefetchingGenericRelation and prefetch_related: wrong caching with cyclic prefetching.
Triage Stage: UnreviewedAccepted
Version: 2.2master

Thanks for the report. This is definitely related with prefetch_related(), because get() and first() works properly.

Reproduced at 519016e5f25d7c0a040015724f9920581551cab0.

comment:2 by Can Sarıgöl, 5 years ago

Has patch: set

comment:3 by Can Sarıgöl, 5 years ago

Owner: changed from nobody to Can Sarıgöl
Status: newassigned

comment:4 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:5 by Mariusz Felisiak, 5 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:6 by Mariusz Felisiak, 5 years ago

Triage Stage: Ready for checkinAccepted

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In f66021f3:

Refs #30493 -- Added GenericRelatedObjectManager.get_content_type() hook.

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In dffa3e19:

Fixed #30493 -- Fixed prefetch_related() for GenericRelation with different content types.

Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@…>

Thanks Simon Charette for the review.

comment:9 by Finn Stutzenstein, 5 years ago

Thanks to Can Sarıgöl, felixxm and charettes for the fix! Great work!

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