Closed Archmonger closed 1 year ago
@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.
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
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?
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?
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 https://github.com/idom-team/django-idom/issues/104 since you only need to use prefetch if you're returning models or querysets.
I just realized that prefetch
could also be used as a decorator:
@prefetch
def my_query(): ...
@prefetch(related=True)
def my_query(): ...
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.
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.
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.
I attempted to generate an option that is extensible, with the knowledge that use_query
may some day be merged into core.
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: https://github.com/idom-team/django-idom/issues/113
LMK if you're okay with this implementation, if so I'll start documentation.
I'll have time to take a look at this tomorrow.
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.
Having that be the default seems alright.
Do you want to review the docs or should I merge?
Besides that one little nit it looks good.
Postprocessor
to now require a return value.
use_query
to database calls, we have no way of ensuring that data
is always mutable (such as a Model
object).Since the changes are non-trivial, I will wait for one more approval.
Pushing this forward, we can do any requested changes in the version bump PR.
Description
Attempt at auto fetching ManyToMany fields
Checklist:
Please update this checklist as you complete each item:
changelog.rst
has been updated with any significant changes, if necessary.