Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async Pagination support #1030

Merged
merged 9 commits into from
Apr 30, 2024
Merged

Async Pagination support #1030

merged 9 commits into from
Apr 30, 2024

Conversation

jamesrkiger
Copy link
Contributor

Fixes #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.

@shmulvad
Copy link

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.

@bufke
Copy link

bufke commented Mar 2, 2024

@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.

@shmulvad
Copy link

@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.

@vitalik vitalik merged commit a937981 into vitalik:master Apr 30, 2024
27 checks passed
@vitalik
Copy link
Owner

vitalik commented Apr 30, 2024

Thank you

@stumpylog
Copy link

Testing this out in the 1.2.0 release, I'm also seeing a SynchronousOnlyOperation tracing back to

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

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

@jamesrkiger
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] TypeError: 'coroutine' object is not subscriptable [async pagination]
5 participants