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

Optimizer window pagination #613

Open
Kitefiko opened this issue Aug 14, 2024 · 5 comments · Fixed by #622
Open

Optimizer window pagination #613

Kitefiko opened this issue Aug 14, 2024 · 5 comments · Fixed by #622
Labels
bug Something isn't working question Further information is requested

Comments

@Kitefiko
Copy link
Contributor

Kitefiko commented Aug 14, 2024

Describe the Bug

I believe I have found multiple issues with window pagination implementation.

  • AttributeError is always thrown in ListConnectionWithTotalCount.resolve_connection_from_cache
  • backup solution in case of AttributeError prevents pagination
  • order and thus pagination itself is non-deterministic when explicit order is not used
  • usage of last returns everything and bypasses max_results

System Information

  • Operating system: linux
  • Strawberry version (if applicable): 0.47.1
  • Database version: 10.11.6-MariaDB

Additional Context

ListConnectionWithTotalCount.resolve_connection_from_cache

AttributeError is always thrown here:

has_next_page = result._strawberry_row_number < result if result else False

I think there should be something like this?

has_next_page = result[-1]._strawberry_row_number < result[-1]._strawberry_total_count if result else False

Backup solution in case of AttributeError prevents pagination

This might be intended behavior, but:
In case of AttributeError pagination falls back to resolve_connection but (probably due to unavailable overfetch?) pageInfo's hasNextPage is wrongly reporting as false

Order and thus pagination itself is non-deterministic when explicit order is not used

This is probably the biggest issue. Maybe fixable via pk ordering in case of empty order_by in apply_window_pagination ?

Usage of last returns everything and bypasses max_results

When last is used on it's own and execution ends up here:

slice_metadata = SliceMetadata.from_arguments(
Info(_raw_info=info, _field=field),
first=field_kwargs.get("first"),
last=field_kwargs.get("last"),
before=field_kwargs.get("before"),
after=field_kwargs.get("after"),
)
qs = apply_window_pagination(
qs,
related_field_id=related_field_id,
offset=slice_metadata.start,
limit=slice_metadata.end - slice_metadata.start,
)

slice_metadata.start is evaluated to 0
slice_metadata.end becomes sys.maxsize

and queryset ends up filtering out nothing

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@Kitefiko Kitefiko added the bug Something isn't working label Aug 14, 2024
@bellini666
Copy link
Member

Hey @Kitefiko ,

Hrm, I'm not really I understood the issue. When and why does AttributeError gets raised for you? Could you try to provide an MRE for it?

@bellini666 bellini666 added the question Further information is requested label Aug 19, 2024
@SupImDos
Copy link
Contributor

SupImDos commented Sep 2, 2024

Hi @bellini666

We've run into these issue as well, particularly with ListConnectionWithTotalCount.resolve_connection_from_cache here:

has_next_page = result._strawberry_row_number < result if result else False

Just so its clear, the result object here is actually the QuerySet._result_cache list, so it will never have the annotated _strawberry_row_number / _strawberry_total_count attributes. It's items will though, so as suggested in parent issue it should instead be something like:

has_next_page = (
    result[-1]._strawberry_row_number < result[-1]._strawberry_total_count
    if result
    else False
)

@SupImDos
Copy link
Contributor

SupImDos commented Sep 4, 2024

@bellini666 FYI #622 only fixes 1 of the 4 problems raised in this issue, so it may be worth keeping this open?

@bellini666
Copy link
Member

@SupImDos good point, going to reopen this

@bellini666 bellini666 reopened this Sep 4, 2024
@bellini666
Copy link
Member

@SupImDos @Kitefiko could any of you help me with some MREs for the remaining issues here so I can try to tackle?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
3 participants