use_query prefetching for ManyToMany and ManyToOne fields

[Archmonger]

Description

Attempt at auto fetching ManyToMany fields

Checklist:

Please update this checklist as you complete each item:

• Tests have been included for all bug fixes or added functionality.
• The changelog.rst has been updated with any significant changes, if necessary.
• GitHub Issues which may be closed by this PR have been linked.

[Archmonger]

@rmorshea This may not be possible. get_queryset does not solve the problem, and when calling all() on the results they don't get cached.

I think the solution may be documenting that ManyToMany field fetches require a separate use_query hook call.

[rmorshea]

On a side note, I'm observing the following traceback. It seems like render_view isn't actually returning an HttpResponse object, but rather an unawaited coroutine.

Traceback (most recent call last):
  File "/home/ryan/Repositories/idom-team/django-idom/venv/lib/python3.10/site-packages/asgiref/sync.py", line 472, in thread_handler
    raise exc_info[1]
  File "/home/ryan/Repositories/idom-team/django-idom/venv/lib/python3.10/site-packages/django/core/handlers/exception.py", line 42, in inner
    response = await get_response(request)
  File "/home/ryan/Repositories/idom-team/django-idom/venv/lib/python3.10/site-packages/django/core/handlers/base.py", line 253, in _get_response_async
    response = await wrapped_callback(
  File "/home/ryan/Repositories/idom-team/django-idom/src/django_idom/http/views.py", line 52, in view_to_component_iframe
    response["X-Frame-Options"] = "SAMEORIGIN"
TypeError: 'coroutine' object does not support item assignment

[Archmonger]

Weird, I haven't seen that in my testing environment. Not happening on GH actions either.

We'll have to narrow that one down, might be OS-dependent?

[Archmonger]

I needed to add a kwarg to allow users to turn off many_to_many or many_to_one prefetch behavior. It may undesirable and memory intensive if using deep nested database structures. However, this has broken the *args: _Params.args, **kwargs: _Params.kwargs, type hints.

Is ParamSpec not compatible with functions with predefined kwargs?

[rmorshea]

Unfortunately, ParamSpec does not support this. It was explicitly left out of the original PEP because it was deemed to be of sufficient complexity to be considered out of scope.

Here's one way we could work around this...

Instead of implementing this caching logic in use_query itself, this could be done in a wrapper function:

# default prefetches fields
use_query(prefetch(my_query), *args, **kwargs)

# prefetch related too
use_query(prefetch(my_query, related=True), *args, **kwargs)

Where prefetch is of the form:

P = ParamSpec("P")
R = TypeVar("R")

def prefetch(query: Callable[P, R], fields: bool = True, related: bool = False) -> Callable[P, R]: ...

I actually quite like this since it doesn't impinge on the *args, **kwargs of the query function. It could also be used to address #104 since you only need to use prefetch if you're returning models or querysets.

[rmorshea]

I just realized that prefetch could also be used as a decorator:

@prefetch
def my_query(): ...

@prefetch(related=True)
def my_query(): ...

[Archmonger]

Some form of prefetching would still need to occur within use_query to ensure all the basic fields are accessible.

I personally think it feels the least awkward for them to be parameters within use_query, truly unfortunate on that ParamSpec limitation... I'll try wrappers out.

[Archmonger]

Having it be a decorator is also kind of unfortunate, since I was hoping the default behavior would be to prefetch.

Now that I'm thinking about it, it's not unrealistic to ask Django users to add a prefetch parameter to their models.py. Would fit Django's "fat models" architecture.

Downside is they would have to add this parameter for every model they want prefetching behaviors in.

[rmorshea]

The type hints for prefetch would be:

from typing import ParamSpec, TypeVar, overload

_Params = ParamSpec("_Params")
_Result = TypeVar("_Result")

@overload
def prefetch(
    query: None = ..., *, fields: bool = ..., related: bool = ...
) -> Callable[[Callable[_Params, _Result]], Callable[_Params, _Result]]:
    ...

@overload
def prefetch(
    query: Callable[_Params, _Result], *, fields: bool = ..., related: bool = ...
) -> Callable[_Params, _Result]:
    ...

def prefetch(
    query: Callable[_Params, _Result] | None = None, *, fields: bool = True, related: bool = False
) -> Callable[_Params, _Result] | Callable[[Callable[_Params, _Result]], Callable[_Params, _Result]]:
    
    def wrapper(query: Callable[_Params, _Result]) -> Callable[_Params, _Result]:
        ...

    if query is None:
        return wrapper
    else:
        return wrapper(query)

This allows prefetch to be used as a decorator and with an inline call.

[Archmonger]

I attempted to generate an option that is extensible, with the knowledge that use_query may some day be merged into core.

[rmorshea]

I think I'd like to have some more discussion before implementing anything. I've gathered relevant issues that require discussion into a meta-issue: #113

[rmorshea]

We can merge this as-is. However, when it comes time to get this into core I'd like to completely rethink the API. In doing so, I think the user experience can be greatly improved.

[Archmonger]

How would you feel about defaulting {"many_to_many": True, "many_to_one": True} to simplify the user experience?

I think it's better to have things "just work" by default, and then have users turn it off if performance becomes an issue.

I would imagine the vast majority of Django users don't have deeply nested trees of data.

[rmorshea]

Having that be the default seems alright.

[Archmonger]

Do you want to review the docs or should I merge?

[rmorshea]

Besides that one little nit it looks good.

[Archmonger]

• Fixed the type hint
• Modified the Postprocessor to now require a return value.
  • The idea behind this change is, since we aren't trying to fully limit use_query to database calls, we have no way of ensuring that data is always mutable (such as a Model object).
• Made the default postprocessor configurable via Django settings.
  • Helps lay the groundwork for merging this hook into core

Since the changes are non-trivial, I will wait for one more approval.

[Archmonger]

Pushing this forward, we can do any requested changes in the version bump PR.