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

fix: link header in keyset pagination #729

Merged
merged 1 commit into from
Oct 10, 2023
Merged

fix: link header in keyset pagination #729

merged 1 commit into from
Oct 10, 2023

Conversation

alnr
Copy link
Contributor

@alnr alnr commented Oct 9, 2023

fixes some issues in keyset pagination

@@ -104,7 +105,7 @@ func Parse(q url.Values, p PageTokenConstructor) ([]Option, error) {
}
opts = append(opts, WithToken(parsed))
}
if q.Has("page_size") {
if q.Get("page_size") != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this? 🤔

Copy link
Contributor Author

@alnr alnr Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've encountered GET /admin/identities?page_size= a bunch of times while working on this. I can also see an SDK generating code like that, and we should be prepared for that. Previously, this would return 400 with an internal error message referencing strconv.Atoi. Now it's just equal to default.

@zepatrik zepatrik changed the title fix: keyset pagination issues fix: link header in keyset pagination Oct 10, 2023
@zepatrik zepatrik merged commit 67f2a27 into master Oct 10, 2023
8 checks passed
@zepatrik zepatrik deleted the keyset-pagination branch October 10, 2023 08:45
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.

3 participants