Preserve query param value if named route param nil

[jkowens]

Optional named route params overwrite a query param of the same name with nil if not included. Fixes #1666.

[namusyaka]

Well, do we need to follow up on backward-compatibility between 1.4.x and 2.x? The current behavior looks reasonable to me.

[jkowens]

Good question, I don't know. I'm trying to think of a scenario where it would be necessary or easier to use query params instead of an optional named route parameter. Maybe the old behavior was accidental.

@synecy would you like to weigh in on this?

[jkowens]

@namusyaka I'm thinking it might be a nice feature if I had to dynamically build links or form data to use query params, but also to have the option of creating direct links using the (optional) named route params.

I suppose you could just create two separate routes if that was they case tho. One without the param and one with the named parameter being required.

[namusyaka]

Should it be released as a new feature?

I'm sorry, but I'm still thinking that the feature is not useful to me..
Could you write down your design with code examples?

[jkowens]

@namusyaka I honestly don't feel that strongly about this feature. I'm happy to close this PR and #1666 as a non-issue. I think if this was an issue, the workaround would be to just define an additional route without the optional named route parameters.

[synecy]

If I recall, our use case was something like this:

/test/123-123-123
/test?user_id[]=123-123-123&user_id[]=456-456-456

which no longer works in 2.x, however we've since added workarounds by either adding multiple routes or manually copying from rack.request.query_hash to restore original behavior. I think the problem with adding new routes is that if you have multiple optionals like:

/?:one?/something/?:two?/test

the number of routes you have to add in order to support every possible case gets out of hand pretty quick.

[namusyaka]

@synecy Thanks for your explanation. I got your pain point on v2.x.x.

@jkowens Your patch looks reasonable to me. Merging.

[jkowens]

Hey thanks @namusyaka. As @dentarg mentioned, I have already created a 2.2.0 tag on master.

[namusyaka]

I marked this as v3.0.0. Will cut a new release based on the v2.2.0 tag.
@jkowens Could you add this change in CHANGELOG?

[namusyaka]

Released:

• https://rubygems.org/gems/sinatra/versions/2.2.0
• https://rubygems.org/gems/sinatra-contrib/versions/2.2.0
• https://rubygems.org/gems/rack-protection/versions/2.2.0