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

Require parameter for query endpoint #1975

Merged
merged 3 commits into from
Mar 16, 2015
Merged

Conversation

corylanou
Copy link
Contributor

There was a bug where if you issued a curl command like this and accidentally sent the wrong params (double encoded) and as a result, didn't send the q parameter, the endpoint would return 200 ok with an empty json result, instead of an error:

curl -G http://localhost:8086/query --data-urlencode "db=foo&q=show fred"

It now correctly returns this:

{"error":"missing required parameter \"q\""}   

@@ -5,6 +5,7 @@

### Bugfixes
- [#1971](https://github.com/influxdb/influxdb/pull/1971): Fix leader id initialization.
- [#1975](https://github.com/influxdb/influxdb/pull/1975): Require parameter for query endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a bit more precise. Which parameter after all? "Require 'q' parameter for query endpoint" perhaps?

pretty := q.Get("pretty") == "true"

qp := strings.TrimSpace(q.Get("q"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use TrimSpace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because a user can send in a parameter of q=+ which is just a space which will result in the parser still returning ok. While not exactly the same error, the result is you really didn't give me a query parameter, so it needs to return an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

@otoolep
Copy link
Contributor

otoolep commented Mar 16, 2015

Makes sense, 1 question. +1

@toddboom
Copy link
Contributor

:shipit:

toddboom added a commit that referenced this pull request Mar 16, 2015
@toddboom toddboom merged commit d8b8a8c into master Mar 16, 2015
@toddboom toddboom deleted the http-query-require-param branch March 16, 2015 22:23
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.

None yet

3 participants