django-ninja icon indicating copy to clipboard operation
django-ninja copied to clipboard

Async Pagination support

Open jamesrkiger opened this issue 1 year ago • 3 comments

Fixes https://github.com/vitalik/django-ninja/issues/547

This PR adds support for async pagination and updates existing pagination classes to work with async pagination. The _inject_pagination function now checks to see if the view function is async and then, if so, adds view_with_pagination as an async function that calls an apaginate_queryset method expected in the new AsyncPaginationBase. The async view_with_pagination also forces async evaluation of the results queryset if necessary.

jamesrkiger avatar Dec 29 '23 19:12 jamesrkiger

Hi @jamesrkiger

I wanted async pagination in a project of my own and didn't want to wait for this to be merged. So I tried to incorporate what you wrote here into my codebase. I ran into the following error:

Traceback (most recent call last):
  File "my_project/.venv/lib/python3.12/site-packages/ninja/operation.py", line 281, in run
    result = await self.view_func(request, **values)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "my_project/backend/api/apagination.py", line 216, in view_with_pagination
    result[paginator.items_attribute] = [
                                        ^
  File "my_project/backend/api/apagination.py", line 206, in evaluate
    for result in results:
  File "my_project/.venv/lib/python3.12/site-packages/django/db/models/query.py", line 400, in __iter__
    self._fetch_all()
  File "my_project/.venv/lib/python3.12/site-packages/django/db/models/query.py", line 1928, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "my_project/.venv/lib/python3.12/site-packages/django/db/models/query.py", line 91, in __iter__
    results = compiler.execute_sql(
              ^^^^^^^^^^^^^^^^^^^^^
  File "my_project/.venv/lib/python3.12/site-packages/silk/sql.py", line 100, in execute_sql
    return self._execute_sql(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "my_project/.venv/lib/python3.12/site-packages/django/db/models/sql/compiler.py", line 1560, in execute_sql
    cursor = self.connection.cursor()
             ^^^^^^^^^^^^^^^^^^^^^^^^
  File "my_project/.venv/lib/python3.12/site-packages/django/utils/asyncio.py", line 24, in inner
    raise SynchronousOnlyOperation(message)
django.core.exceptions.SynchronousOnlyOperation: You cannot call this from an async context - use a thread or sync_to_async.

It seems to be a problem with the following lines:

https://github.com/vitalik/django-ninja/pull/1030/files#diff-27c0497d3e3ccd2e01267109383e86fd3bcb7685ce39d96e9ce6af4864d04aeeR199-R201

async def evaluate(results: Union[List, QuerySet]) -> AsyncGenerator:
    for result in results:
        yield result

Rewriting it to the following made it work for me:

async def evaluate(results: Union[List, QuerySet]) -> AsyncGenerator:
    if isinstance(results, QuerySet):
        async for result in results:
            yield result
    else:
        for result in results:
            yield result

I don't know if it is a general issue and you might want to incorporate this or it's a weird issue that only occured to me.

shmulvad avatar Feb 18 '24 07:02 shmulvad

@shmulvad what does your api code look like? It sounds possibly related to my comment here. Understanding the various use cases would be helpful.

@jamesrkiger and I are working together to add async + cursor pagination to our project GlitchTip. Feel free to snoop around our open source code.

Let us know if we can help further @vitalik I'd be happy to make changes, do something boring to free up your time to review, or publish pagination as a third party package. My goal is that async just works and cursor pagination is an option, built upon existing django-ninja pagination.

bufke avatar Mar 02 '24 16:03 bufke

@bufke I was just returning basic querysets from the view function. Something like return MyModel.objects.all().order_by('id'). Unfortunately I cannot share it in more details.

shmulvad avatar Mar 10 '24 09:03 shmulvad

Thank you

vitalik avatar Apr 30 '24 11:04 vitalik

Testing this out in the 1.2.0 release, I'm also seeing a SynchronousOnlyOperation tracing back to https://github.com/vitalik/django-ninja/blob/1638dd825e99dbe3f9fc43dc8770059249cfa8c6/ninja/pagination.py#L206-L208

The return from the route is again a queryset, ie return MyModel.objects.all(). Updating that to an async for works.

stumpylog avatar Jun 27 '24 15:06 stumpylog

@stumpylog @shmulvad Thank you for your notes, I will investigate this and submit another PR for a fix.

jamesrkiger avatar Jun 27 '24 17:06 jamesrkiger