search

strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
https://strawberry.rocks/docs/django
MIT License
421 stars 122 forks source link

Optimizer window pagination #613

Open Kitefiko opened 3 months ago

Kitefiko commented 3 months ago

Describe the Bug

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

System Information

Additional Context

ListConnectionWithTotalCount.resolve_connection_from_cache

AttributeError is always thrown here: https://github.com/strawberry-graphql/strawberry-django/blob/d6051db6f1e792604d66206734df9d6ebcfbbb40/strawberry_django/relay.py#L195 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: https://github.com/strawberry-graphql/strawberry-django/blob/d6051db6f1e792604d66206734df9d6ebcfbbb40/strawberry_django/optimizer.py#L561-L573

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

and queryset ends up filtering out nothing

Upvote & Fund

Fund with Polar

bellini666 commented 3 months ago

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?

SupImDos commented 2 months ago

Hi @bellini666

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

https://github.com/strawberry-graphql/strawberry-django/blob/d6051db6f1e792604d66206734df9d6ebcfbbb40/strawberry_django/relay.py#L195

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 commented 2 months ago

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

bellini666 commented 2 months ago

@SupImDos good point, going to reopen this

bellini666 commented 1 month ago

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

SupImDos commented 4 weeks ago

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

@bellini666 Yes, we've specifically run into the _"Usage of last returns everything and bypasses maxresults" issue, so I should be able to put together an MRE for that soon.

SupImDos commented 1 week ago

@bellini666 Sorry for the delay! I've created an MRE for the _"Usage of last returns everything and bypasses maxresults" issue:

In short, doing:

query {
  milestoneConn(last: 3) {
    edges {
      node {
        id
      }
    }
  }
}

Results in:

SELECT "projects_milestone"."name",
    "projects_milestone"."id",
    "projects_milestone"."due_date",
    "projects_milestone"."project_id"
FROM "projects_milestone" LIMIT 9223372036854775807    # <- This is ``sys.maxsize``