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

Prevent Server Error on API call with zero/negative page #463

Closed
stefanw opened this issue Feb 23, 2013 · 4 comments
Closed

Prevent Server Error on API call with zero/negative page #463

stefanw opened this issue Feb 23, 2013 · 4 comments
Labels

Comments

@stefanw
Copy link
Contributor

stefanw commented Feb 23, 2013

An API call like /api/action/current_package_list_with_resources?limit=100&page=0 results in an uncaught Exception because the page parameter is not properly checked.

@domoritz
Copy link
Contributor

As far as I can see only current_package_list_with_resources has a page argument. Most other action functions have an offset parameter. These action functions have similar problems with their offset parameter (for example /api/action/resource_search?query=name:foo&offset=-10).

Also, as I went through the code, I noticed that the conversion to int is sometimes validated and sometimes it is not validated. The same validation problem applies to required and optional parameters and int ranges.

I'm not sure whether there is generic way to implement pagination and what would be a sensible way for input validation (without doing it over and over again in each function). My question is whether we want to fix validation issues as we find them, just ignore them since a 500 is enough of an error message or think about a refactoring of the action API functions with a generic way of validating parameters.

I consider this a UX problem because the API is such an important part of the CKAN interface.

@stefanw
Copy link
Contributor Author

stefanw commented Feb 24, 2013

500 is a Server Error (which results in an admin email) which is not the case here.
The CKAN API is far from REST, but should at least use proper status codes, in this case 400. -1 for ignoring.

@tobes
Copy link
Contributor

tobes commented Feb 25, 2013

@domoritz
yeah common code paths - the problem is the way some of the api has evolved over time and without consistent oversight. We don't want to break the existing api but it would be good to get it consistent and look at a clean upgrade path. I think this would be good to do but not sure about the resource availability.

As far as bad params ideally the api would give some useful errors if we can do that nicely.

@domoritz
Copy link
Contributor

This issue will be part of #473

amercader added a commit that referenced this issue Feb 24, 2015
The following check was failing on the first load, so autozoom didn't
work:

https://github.com/okfn/recline/blob/3f4a30e054652d06b92cfe372ce66c8b5c79d4ca/src/view.map.js#L232

We are patching the local recline file but the same patch has already been
submitted to the main recline repo:

datopian/datahub#464
amercader added a commit that referenced this issue Feb 26, 2015
The following check was failing on the first load, so autozoom didn't
work:

https://github.com/okfn/recline/blob/3f4a30e054652d06b92cfe372ce66c8b5c79d4ca/src/view.map.js#L232

We are patching the local recline file but the same patch has already been
submitted to the main recline repo:

datopian/datahub#464
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants