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

[cansarigol]

Ticket: #30493

[felixxm]

@cansarigol Thanks for this patch 👍 I left some comments.

[cansarigol]

@felixxm thanks for your review. I applied your comments, could you review it?

[felixxm]

I rebased from master, moved GenericRelatedObjectManager.get_content_type() to a separate commit, edited tests and grouped related objects by content types (it is more optimal than adding separate condition for each object).

[cansarigol]

Should we use subTest for forloop test?

[felixxm]

I don't think so because it is not a "sub-test" (we're not testing different conditions here), all three will work or not.

[cansarigol]

I got it thanks

[charettes]

I think it might be worth adding some form of memoization to get_content_type calls by (obj._state.db, obj.__class__) in get_prefetch_queryset as there will potentially be lot of them and they'll result in a non negligible amount of work.

[felixxm]

@charettes Thanks for the review 👍 get_for_model() is already cached.

[charettes]

@felixxm I know but each ContentType.objects.db_manager(obj._state.db) results in three attributes lookup and a Manager instance. That's not even counting the function call and the cache lookup in get_for_models. That can grow to a large amount of time if instances is large.

[felixxm]

I will add it tomorrow.

[felixxm]

@charettes I added basic memoization. What do you think?

[cansarigol]

What if we add self.manager in __init__()to use the get_for_models cache?
self.manager = ContentType.objects.db_manager(instance._state.db)

[charettes]

This memoize_content_type seems to be working but is not kept in sync with ContentType.objects.clear_cache().

I like @cansarigol's idea of caching it at __init__ though which should be doable given all objects should be on the same database.

e.g. in __init__

self.get_content_type = partial(
    ContentType.objects.db_manager(instance._state.db).get_for_model,
    for_concrete_model=rel.field.for_concrete_model,
)
self.content_type = self.get_content_type(instance)

[felixxm]

@charettes Thanks 🏆 I updated PR.

[ngnpope]

This seems rather inefficient... Doesn't self.get_content_type(obj) execute a query?

[charettes]

@pope1ni no, it's heavily cached as it's using ContentType.objects.get_for_model.