Sinatra: Inconsistency in setting params from optional route captures between 1.4 and 2.0

[nickpelone]

I'm wary to call this a bug just yet, but I wanted to bring this up since I'm noticing a different behavior. This kinda intersects with third-party Rack middleware in rack-contrib but I'm hoping this is relevant here.

In Sinatra 1.4, it looks like when the requested route has optional parameters in it, the parsed value is added to params when the value is truthy (not false/nil)(excluding the array case). So if there are items already in params, like from a Rack middleware such as PostBodyContentTypeParser, a lack of a match on the optional parameter in the route would not overwrite a value in params with nil.

An example (Sinatra 1.4):

app.post '/example/?:id?' do
  # code here
end

If I were to hit this via /example, params[:id] would be nil (on account of it not being set during route-optionals-matching). If I were to hit this via /example/2, params[:id] == 2. If I were to hit this via /example and include in my POST body the JSON: {"id": 2}, PostBodyContentTypeParser would set params[:id] to 2 as well, and Sinatra 1.4's optionals-in-route handling would not overwrite with nil.

Meanwhile, in Sinatra 2.0, it looks like the resulting matches from the Mustermann pattern are directly merged to the params hash if params.any? returns something truthy. This, _I think_ has the effect of, basically (contrived irb example incoming):

irb(main):001:0> require 'mustermann/sinatra'
=> true
irb(main):002:0> require 'sinatra/indifferent_hash'
=> true
irb(main):003:0> params = Sinatra::IndifferentHash.new
=> {}
irb(main):004:0> params.merge(id: 1 ) # taking in rack's params
=> {"id"=>1}
irb(main):005:0> pattern = Mustermann::Sinatra.new('/example(/:id)?')
=> #<Mustermann::Sinatra:"/example(/:id)?">
irb(main):006:0> params.merge(pattern.params('/example')) # This is going to override rack preset params with nil
=> {"id"=>nil}

So the params set by the middleware are lost if they are the same key as the optional in the route.

Was I relying on behavior that was never meant to be stable? Or is this an underlying issue?

And of course, thanks Sinatra team for a wonderful gem. :heart:

[namusyaka]

@nickpelone Thank you for your polite report.
Looks like a bug.
I don't see any reason that breaks the compatibility between 1.4.x and 2.0.x.
I'm going to try to fix the issue soon.

[namusyaka]

@nickpelone could you take a look at #1418 ?

[nickpelone]

Hey! Apologies, wasn't sure that needed direct review by me. I've been using the code from that fix branch internally in development over the past week, and my API is back to responding as expected. I'm not at my desk to do a final 100% confirmed OK, but I'll be sure to chime in again before the end of tomorrow. Thanks!!

[nickpelone]

I left a 👍 over there, it looked good to me. (And sorry if you already know - I forget how GitHub notifications work sometimes)

Thanks for taking care of this!

[jkowens]

@namusyaka can this issue be closed based on the outcome of #1418?