search

kenichi / angelo

Sinatra-like DSL for Reel that supports WebSockets and SSE
Other
303 stars 23 forks source link

angelo params parser crashes on arrays #48

Closed gunnarmarten closed 9 years ago

gunnarmarten commented 9 years ago

Hi, i have a little angelo based service which implements a POST request with content_type :json. As data an array is given. So when i curl -s -v -X POST -H "Content-type:application/json" -d '["1451","1452"]' "http://0.0.0.0:3005/do_sth" it crashes with the following stacktrace:

/Users/gunnar/.rvm/gems/ruby-2.1.5@sa_media_service/gems/angelo-0.4.1/lib/angelo/params_parser.rb:64:in 'merge!' /Users/gunnar/.rvm/gems/ruby-2.1.5@sa_media_service/gems/angelo-0.4.1/lib/angelo/params_parser.rb:64:in 'initialize' /Users/gunnar/.rvm/gems/ruby-2.1.5@sa_media_service/gems/angelo-0.4.1/lib/angelo/params_parser.rb:29:in 'new' /Users/gunnar/.rvm/gems/ruby-2.1.5@sa_media_service/gems/angelo-0.4.1/lib/angelo/params_parser.rb:29:in 'parse_post_body' /Users/gunnar/.rvm/gems/ruby-2.1.5@sa_media_service/gems/angelo-0.4.1/lib/angelo/params_parser.rb:36:in 'parse_query_string_and_post_body' /Users/gunnar/.rvm/gems/ruby-2.1.5@sa_media_service/gems/angelo-0.4.1/lib/angelo/base.rb:241:in 'params'

So the params_parser always expects a hash as post_body and crashes otherwise. Though an array should be valid json too.

kenichi commented 9 years ago

at first glance, i completely agree - an array is valid JSON. what made me reconsider is how would you access that array via params? would you be expecting params to then be an array and you would access with an integer index?

kenichi commented 9 years ago

also, something to think about is posting a JSON body a route with a query string - edge case perhaps, but what would happen there?

curl -X POST -H "Content-type:application/json" -d '["1451","1452"]' http://example.com/ids?acceess_token=xxxxxxx
params[:access_token] #=> "xxxxxxx"
params[ ??? ] #=> ["1452", "1452" ]  ???
kenichi commented 9 years ago

@gunnarmarten i hope my tone did not come across as glib or flippant. i've actually thought about this very same use case in the past, and always get stuck at the above questions. i sincerely welcome any ideas how to handle it.

gunnarmarten commented 9 years ago

no worries, I was just on holiday for a few days. I would probably separate params from the body and make the body accessable and json parsed. So we would have in this case the params params[:access_token] #=> "xxxxxxx" and the body as array body #=> ["1451", "1452"]

Its convenient to merge params and body but in these cases it doesn't really make sense.

kenichi commented 9 years ago

after some discussion/tweets, i think that this is definitely a bug, since it should for sure not crash the server if someone posts an array as a body. my thoughts:

so, pretty much what you suggest :) though i do agree that objects are nicer, can have associated metadata e.g. for paging etc.

i'll mention you in the PR. thanks.

gunnarmarten commented 9 years ago

cool, thanks for digging into that. Yeah that sounds like what i was thinking of. I'll test the PR and give you feedback.