oliyh / pedestal-api

Easily build APIs in Pedestal using Schema and Swagger
MIT License
110 stars 13 forks source link

Can't seem to make a query parameter optional #8

Closed bobby closed 8 years ago

bobby commented 8 years ago

When decorating a handler with

(defhandler foo
 {
  ...
  :parameters {:query-params {(s/optional-key :bar) s/Bool}}
  ...
 }
[request]
{:status 200
 :headers {}
 :body (-> request :params :bar str)
}
)

I would expect to be able to omit the query parameter bar. However, currently I get the following HTTP 400 when omitting bar:

{
  "error": {
    "query-params": "missing-required-key"
  }
}

Also of note (both of which seem wrong, though less important):

I would expect to be able to pass the key bar with no value, and see {:bar true} in query params, since bar is declared as a s/Bool.

>> curl -X GET --header 'Accept: application/json' 'http://localhost:3000/handler?bar'
{"error":{"query-params":{"schema.utils.ValidationError@4488c6a2":"invalid-key"}}}

Also

>> curl -X GET --header 'Accept: application/json' 'http://localhost:3000/handler?bar='

Returns 200 with {:bar false}, as one might expect.

This next one was more surprising:

>> curl -X GET --header 'Accept: application/json' 'http://localhost:3000/handler?bar=[any string other than "true"]'

Returns 200 with {:bar false}. This seems at odds with Clojure's own truthiness semantics, where everything other than nil and false evaluate logical true. So common conventions like ?bar=1, ?bar=present, etc. get coerced to {:bar false}, which seems wrong. I'd expect the opposite, where ?bar=false and perhaps ?bar= evaluate to {:bar false}, and anything else evaluate to {:bar true}. Thoughts?

oliyh commented 8 years ago

Hi @bobby,

Thanks for reporting this. The first behaviour is due to pedestal not including a :query-params key when there are no query parameters. I think in this case nil has the same semantics as an empty map, so I have made a change in pedestal-api to merge empty maps of parameters in the common-body interceptor so that your use case works. This is now available in 0.2.0-SNAPSHOT.

The second behaviour is due to pedestal's query string parsing, you might want to raise an issue with them about that.

io.pedestal.http.route> (parse-query-string "abc=123&def")
;; => {:abc "123", nil "def"}

The third behaviour is due to schema's coercion of booleans. You can pass in your own coercions to the coerce-request interceptor to change this behaviour.

Hope this helps!

bobby commented 8 years ago

Thanks for the thorough response! I'll have a look at 0.2.0-SNAPSHOT