check paginate args page, per_page type

[akhilharihar]

Documentation for flask_sqlalchemy paginate class

When error_out is True (default), the following rules will cause a 404 response:

• No items are found and page is not 1.
• page is less than 1, or per_page is negative.
• page or per_page are not ints.

But the paginate class does not have an implementation to check if the given page and per_page arguments are int.

[rsyring]

@akhilharihar Thank you for the contribution. Explicit errors are always better than explicit ones. After an initial review of the code, I'm a +1 on getting this merged. However, will you please add tests for this PR. If you need suggestions on what to test, let me know.

Also, could you detail how this PR affects backwards compatibility? Are we throwing errors where no errors would have show up previously or are we just throwing better errors? This could affect whether this PR should target master or the 2.x branch.

[akhilharihar]

Thank for the consideration.

If the page and per_page arguments in paginate method are not validated or if they are passed directly from a request object, we would end up with internal TypeError: '<' not supported between instances of 'str' and 'int'.

This commit removes the need for a developer to validate the page, per_page param's. If page, per_page are not of integer data types, their values would change to 1, 20 or would return 404 error if error_out is true making it backwards compatible.

In this case, we are throwing 404 error response instead of 500 to respect the value of error_out argument.

Tests results from tests/test_pagination.py - test_pagination.txt

Since the docs already mention that a 404 response is returned when page or per_page are not and error_out is true, it would be optimal to target master branch.

[takwas]

@akhilharihar Thank you for the contribution. Explicit errors are always better than explicit ones. After an initial review of the code, I'm a +1 on getting this merged. However, will you please add tests for this PR. If you need suggestions on what to test, let me know.

Also, could you detail how this PR affects backwards compatibility? Are we throwing errors where no errors would have show up previously or are we just throwing better errors? This could affect whether this PR should target master or the 2.x branch.

@rsyring: I think you meant "Explicit errors are always better than implicit ones".

[akhilharihar]

Looks like the CI build failed for this PR. I did check the details, unfortunately, the error is being triggered at another location and not at the lines the changes are made. Can you run the build on this PR again?

[davidism]

fixed in #1087