search

Materials-Consortia / optimade-python-tools

Tools for implementing and consuming OPTIMADE APIs in Python
https://www.optimade.org/optimade-python-tools/
MIT License
71 stars 44 forks source link

Raise error/warning when using unsupported pagination method #1132

Closed ml-evs closed 2 years ago

ml-evs commented 2 years ago

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).

JPBergsma commented 2 years ago

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

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

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

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.