Sinatra: Unexpected param parsing with optional url parameters

[lvonk]

While investigating https://github.com/sinatra/sinatra/issues/1361 I found the following which I think is a separate issue. Since this issue also occurs in Sinatra 1.4.8 I don't know if this is by design or not, but imho it leads to unexpected behaviour.

I think it boils down to the fact that Sinatra should let the most specific route match do the parameter parsing and use those parsed values for the filters (like before). Not doing so will cause possible different param values for the same param in filters (before) and the verbs (get, post etc)

Testcase:

require 'minitest/autorun'
require 'sinatra/base'
require 'rack/test'

VALUES = {}

class App < Sinatra::Base

  before '/foo/:bar/?:baz?' do
    VALUES[:before] = params[:baz]
  end

  get '/foo/:bar/?:baz?/1' do
    VALUES[:get] = params[:baz]
  end
end

class NestedNamespacesTest < Minitest::Test
  include Rack::Test::Methods

  def app
    App
  end

  def test_same_values
    get "/foo/2017/1"
    assert_equal VALUES[:before], VALUES[:get]
  end
end

This test will fail, meaning that in the before filter baz yields to 1 but in the actual route it is nil.

[namusyaka]

First of all, you should use (/:bar)? as an optional parameter instead of :bar/?.
The captured name depends on the context. Therefore, I think the behavior is not a bug.

[lvonk]

Hi @namusyaka thanks for replying. The optional parameter is baz in the example, not bar. '/foo/:bar/?:baz?' (my apologies for the confusing names in the example). Both ways of defining an optional parameter seem to work the same /?:baz? versus (/?:baz)?. Changing to (/?:baz)? does not result in a different outcome.

The captured name depends on the context

I would argue the context is the most specific route. In fact that is also what the readme states (or at least that is how I interpret it):

Before filters are evaluated before each request within the same context as the routes will be and can modify the request and response.

Bug or not, IMHO it makes most sense that filters work from within that context. I can't think of a context or situation where I do want param values to differ between filter and route?

[namusyaka]

The optional parameter is baz in the example, not bar. '/foo/:bar/?:baz?' (my apologies for the confusing names in the example). Both ways of defining an optional parameter seem to work the same /?:baz? versus (/?:baz)?. Changing to (/?:baz)? does not result in a different outcome.

Sorry for your confusing. I wanted to say that the optional parameter should be (/:foo)?, please see mustermann's readme.

Bug or not, IMHO it makes most sense that filters work from within that context. I can't think of a context or situation where I do want param values to differ between filter and route?

Actually the patterns in the route and the filter are not necessarily closely related.
This behavior is established.

[lvonk]

Actually the patterns in the route and the filter are not necessarily closely related.
This behavior is established.

Hmm okay. I did not consider that. It still feels strange though to have a param's value differ in a filter and route in a single request...

Well anyway if this is the way it should work, than indeed it is not a bug. Thanks for taking the time to look into this! Feel free to close this issue if you consider it handled.

[namusyaka]

I think this is not a bug, but I'd like to keep this issue for discussing the API-design.
@lvonk Thanks for your report.

cc @zzak