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

Analog for more? field for Keyset pagination, but for backward direction #334

Closed
dolfinus opened this issue May 29, 2022 · 10 comments
Closed
Labels
enhancement New feature or request needs review

Comments

@dolfinus
Copy link
Contributor

Is your feature request related to a problem? Please describe.
GraphQL Relay specification has 2 fields to describe cursor position: hasNextPage and hasPreviousPage. First returns true if there is a next page, second returns true if there is a previous page.

Ash.Page.Keyset have implementation only for the first field (it is called more?), but not for the second one.

Describe the solution you'd like
Add another field to Keyset record which will behave in a similar way with more?, but for the backward paginagion direction.

Describe alternatives you've considered

%Ash.Page.Keyset{count: count, after: after_cursor, before: before_cursor} = page

has_previous_page = count > 0 and (not is_nil(after_cursor) or not is_nil(before_cursor))

But it is possible that it's working only because of some issue (like #333), or can break on the some set of conditions

Express the feature either with a change to resource syntax, or with a change to the resource interface

For example

  %Ash.Page.Keyset{more?: more?, prev?: prev?}

Additional context
Discussion of ash-project/ash_graphql#36

@zachdaniel
Copy link
Contributor

Actually, I think you are correct. Given that after and before doesn't include the record itself, your method of determining has_previous_page is correct. We can just add that into the Keyset struct.

@zachdaniel
Copy link
Contributor

count > 0 is not necessarily enough though, because count is not always selected.

@zachdaniel
Copy link
Contributor

However, it will only be "wrong" in the very rare case that the thing you are asking for has been removed i.e after: 1 when 1 has been deleted and you aren't selecting the count.

@zachdaniel
Copy link
Contributor

with #255 issue resolved actually we can detect this case, because we will look up the record before doing the rest of the request. If it isn't there then we'll know and be able to say that there isn't a previous page.

@zachdaniel
Copy link
Contributor

Alright, so my plan is to fix this after #255 and to make "fetch" style (where we look up the record by its primary key keyset and load any relevant aggregates/calculations) the only method.

@zachdaniel
Copy link
Contributor

zachdaniel commented Sep 11, 2022

Alright, I think perhaps this is all that is left to get ash-project/ash_graphql#36 going?

@dolfinus
Copy link
Contributor Author

Actually, I didn't get how prev? could be implemented with a 64b3312

@zachdaniel
Copy link
Contributor

🤔 I've just realized two things.

  1. 64b3312 falls apart when there is no record with the provided primary key. I'm not sure if that is a kryptonite for that solution or not. What we would have to do in that case is restart the pagination, which sounds like a pretty bad experience. Another flaw with that solution is that if you are sorting on something and that value changes, then you'd jump around in the page. I think that means that the "fetch" style of keyset pagination actually is unacceptable, and that we instead need to go back to the old way. I have an idea for how we can support "complex" fields the old way. I'll undo the previous commit and go back to the previous way.

  2. As for prev? I think you may be right. I think there is no way to determine if there is a previous page without running a second query.

@zachdaniel
Copy link
Contributor

i.e flip before and after and say limit: 1. We can do that only on request via a pagination option, i.e page: [populate_prev_page?: true], and then the graphql extension can do that only when that information is requested.

@zachdaniel
Copy link
Contributor

I think for now, the relay specification required for ash_graphql allows for us to just say false when showing hasNextPage combined with before and hasPrevPage when combined with after, meaning we actually have all that we need (for now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs review
Projects
Development

No branches or pull requests

2 participants