Bug when parameters are empty string

[synth]

When a parameter is included in a request but is a blank empty string, there is unexpected and buggy behavior.

• When params[:page] == "", #to_i is called on it, which causes the page to default to 0. Normally, the first page is 1. This may be equivalent now, but in the future may cause unintended consequences.
• When params[:per_page] == "", this causes the error: "comparison of Fixnum with String failed".

I propose that when a parameter is included but is empty string that more defined behavior occurs such as falling back to a default.

• params[:page] should default to 1
• params[:per_page] should default to route_setting(:per_page).

[davidcelis]

Solution sounds good! I'm on vacation but would you like to take a stab at a PR?

[synth]

So, after some digging, there seems to be some parameter coercion fixes in grape 0.14.0 that coerce empty string parameters to nil which basically addresses the issue. I'm locked on 0.13.0 due to another gem(wine_bouncer) so I'm not too sure on how to proceed. I filed a ticket on wine_bouncer to bump the version...

[davidcelis]

So does any action need to be taken here on my part, @synth?

[synth]

Hmm. Not sure. Seems like it would be a good practice to better define behavior and not rely on grapes implementation so much. Who knows if coercion behavior will change in the future. At the very least I think it would be good to write tests to cover the behavior. I started some during my digging process(tdd ftw). I'll create a pr later this week and you can let me know what you think.

[davidcelis]

Sounds good, thanks!

[JiriChara]

What about throwing some custom error if the page/per_page params are not numbers? This way programmers can catch that in the controller and either render 422 or parse the value on their own. What do you think?