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

Raise error/warning when using unsupported pagination method #1132

Closed
ml-evs opened this issue Apr 26, 2022 · 4 comments · Fixed by #1136
Closed

Raise error/warning when using unsupported pagination method #1132

ml-evs opened this issue Apr 26, 2022 · 4 comments · Fixed by #1136
Labels
ergonomics Features that improve the usability of the package priority/medium Issue or PR with a consensus of medium priority server Issues pertaining to the example server implementation

Comments

@ml-evs
Copy link
Member

ml-evs commented Apr 26, 2022

Currently this package only supports offset-based pagination (page_offset=10), however, number-based pagination (page_number=2) is allowed as a valid query parameter and does not return an error. A client that manually implements pagination (i.e. one that does not follow next links) that does not notice this could end up in an infinite loop of requesting the first page.

We should capture this parameter and raise an error if it used (or just implement number-based pagination).

@ml-evs ml-evs added priority/medium Issue or PR with a consensus of medium priority server Issues pertaining to the example server implementation ergonomics Features that improve the usability of the package labels Apr 26, 2022
@JPBergsma
Copy link
Contributor

JPBergsma commented Apr 26, 2022

The simplest solution would be to remove page_number from the EntryListingQueryParams class.
After #1122 the error message should be generated automatically.

Although it should not be difficult to implement it as well.

@ml-evs
Copy link
Member Author

ml-evs commented Apr 26, 2022

Trouble is that the query params listed there also (rightly) enter the openapi schema for all of OPTIMADE, so we can't just remove them. It should be easy enough to just add an error message for now.

@JPBergsma
Copy link
Contributor

In that case it would be best to implement it, it would take just a few lines of code.
Should the returned next link also use page_number, or is it ok if it uses page_offset?
How about the other pagination query params: page_cursor, page_above and page_below.
These are mentioned in the specification but do not seem to supported in the code.

@ml-evs
Copy link
Member Author

ml-evs commented May 1, 2022

In that case it would be best to implement it, it would take just a few lines of code. Should the returned next link also use page_number, or is it ok if it uses page_offset? How about the other pagination query params: page_cursor, page_above and page_below. These are mentioned in the specification but do not seem to supported in the code.

Implementations can choose to support one or the other (or both), as long as the next link points to the right place. I'd actually never noticed these other pagination systems... I would suggest we implement them and add errors for them first, then we can see which is worth supporting. Not sure we need to bother implementing anything other than offset really. I can do this today.

@ml-evs ml-evs changed the title Raise error on number-based pagination Raise error/warning when using unsupported pagination method May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ergonomics Features that improve the usability of the package priority/medium Issue or PR with a consensus of medium priority server Issues pertaining to the example server implementation
Projects
None yet
2 participants