Improving use_query

[rmorshea]

Current Situation

There are a number of usability problems with use_query:

1. Prefetch logic is not perfect and can have significant performance costs: use_query does not properly pre-fetch some model fields #110
2. The interface does not lend itself to future extensions: Add ability to disable Model/QuerySet restrictions on use_query #104, Add fetch_policy=... to use_query #103
3. And that, when a user accesses a field that has not been pre-fetched in the scope of a render function, the SynchronousOnlyOperation error they get from Django does not communicate this fact and how to resolve it.

Proposed Actions

Discuss and arrive at a solution to each of the above problems. I'll update this section as we reach consensus on how to proceed.

[rmorshea]

In my opinion, (1) and (3) are linked. Our present attempts to pre-fetch as many fields and relationships as possible are related to the fact that we're trying to protect users from experiencing a SynchronousOnlyOperation error because, when they do, they probably aren't going to know how to fix it. If we can catch SynchronousOnlyOperation errors and re-raise them with a more helpful error message, we may be able to relax how much default pre-fetching we need to do to maintain a smooth user experience.

[Archmonger]

The issue is those exceptions are happening in the context of component renders.

We could possibly mutate all ORM objects that pass through IDOM to provide custom exception handling. But there's no realistic way to fix this for users who attempt to directly perform ORM queries in a component without use_query.

[rmorshea]

Since errors that happen in the context of a render function get logged. I think the best option might be to use a custom log handler to amend the reported log record to include a more helpful message. Another option might be to change Layout to make it easier to customize error reporting in a subclass.

[Archmonger]

Something for us to keep in mind is that these Django exceptions may not be a permanent feature of Django. In fact, with the last revision of Django the exceptions technically are no longer necessary.

With every update Django has been adding more async ORM compatibility, I would expect these exceptions to be removed from Django core at some point.

However, this may not change what we need to do today. But it is likely to complicate our solution further down the line.

[rmorshea]

As for (2), I think there may be a way to get rid of the *args, **kwargs from use_query that make it difficult to add options. The idea would allow people to parametrize query functions using lambdas similarly to how use_memo works:

use_query(lambda: my_query(*a, **kw), other_options=...)

The problem here is that since the identity of the query function will change on every render, we'll need to reach into the lambda to find my_query so that you can still do use_mutation(..., refetch=my_query). It turns out this is possible:

def f(x, y):
    return x + y

g = lambda: f(1, 2)

globals_referenced_by_g = [g.__globals__[name] for name in g.__code__.co_names]
assert f in globals_referenced_by_g

There's probably a bit of work to do here in terms of hardening this heuristic, but it seems like a plausible solution to avoiding *args, **kwargs in use_query.

[Archmonger]

I honestly dislike the idea of heuristics, sounds like it's going to be a fairly error prone process.

I'd rather concede to a slightly uglier API (for example, the fetch_options API I just developed) over something that might require a lot of dev hours to maintain.

[rmorshea]

sounds like it's going to be a fairly error prone process.

On thinking further, I agree, cases like lambda: some_module.my_query(...) would make this difficult.

[rmorshea]

There's two more things I'd like to nail down:

1. Given that we're going to find some way to report a more helpful error message than the current SynchronousOnlyOperation, what should the default pre-fetch behavior of use_query be?
2. The exact interface of fetch_options. In particular, the object it should return.

In the case of (1), my intuition is that use_query should only try to prefetch a QuerySet. From what I understand, field pre-fetching only really matters if the user has explicitly done something like User.objects.all().only("name"). In which case, pre-fetching the other fields actively works against the user's intentions. If we're able to explain the SynchronousOnlyOperation error, I'm not really sure pre-fetching fields makese sense at all.

In the case of (2), I think fetch_options should return a callable object since this allows normal usage of the query function (e.g. in tests). I also was not clear on what the purpose of the evaluator was in your PR. Perhaps that comes from the fact that OrmFetch wasn't itself callable?

[Archmonger]

evaluator was my proposal to allow for supporting other ORMs within use_query. The default evaluator for Django IDOM is just a function that handles lazy fields. This might be better renamed to postprocessor.

Also I agree, if we can find a graceful way to handle error messages for SynchronousOnlyOperation exceptions, then prefetching should be disabled by default.

However, I do think fetch_options should return an object, rather than a callable to allow for future feature expansion, such as cache_policy. Additionally, this allows us to keep all fetching logic encapsulated within the use_query hook.

[Archmonger]

The proposal I submitted for use_query revolves around a few things

1. The only "Django specific" code within use_query was related to post-processing.
   • I've allowed this to be configurable via evaluator, and when moved into IDOM Core there likely won't be a default evaluator.
   • evaluator can also be renamed to postprocessor
2. All configuration, including configuration values which we haven't thought of yet, need to be accomplished externally to the use_query hook.
   • To accomplish this, I generated an OrmFetch type to encapsulate all current/future configuration values to the use_query hook.
   • Currently, the only configurable values are the evaluator and options, but will likely be expanded soon to contain cache_policy.
     • options are configuration values that are used by the evaluator.
     • options could possibly be renamed to evaluator_options
     • When moved into IDOM Core, the default values for evaluator and options will likely be set via configuration globals.
3. If more "backend specific" code shows itself as necessary, we can add them as Callable attributes to OrmFetch.
   • For example, if we configuration values such as preprocessor or executor, this is now contained within OrmFetch.
4. All actions must be performed within the context of the use_query hook.
   • By this, I am saying that anything Callable should be performed directly and explicitly within the use_query function definition.
   • This is mostly to help with debugging and ease code refactoring
   • This is also the main reason why I opted for fetch_options to return a dataclass rather than a callable.

[rmorshea]

Ok, I have another idea. What if this is the interface:

# default options
use_query(my_query, *args, **kwargs)
# explicit options
use_query(Options(fetch_policy="cache-first"), my_query, *args, **kwargs)

MyPy seems to be ok with this:

from typing import overload, ParamSpec, TypeVar, Callable, Any

from django_idom import Query

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

@overload
def use_query(options: Options, query: Callable[P, R], *args: P.args, **kwargs: P.kwargs) -> Query[R]: ...

@overload
def use_query(query: Callable[P, R], *args: P.args, **kwargs: P.kwargs) -> Query[R]: ...

def use_query(*args: Any, **kwargs: Any) -> Query[Any]: ...

class Options: ...

[Archmonger]

Works, but wouldn't provide any useful type errors if the user accidentally provides too many args.

The type checker would just assume those unneeded args get lumped into the final definition of *args: Any. We could throw an exception during that situation, but that's not as graceful as proper type checking.