Sinatra: Nested namespace with optional url parts do not work in sinatra => 2.0

[lvonk]

Hi,

I have the following app (which worked in sinatra < 2.0):

class App < Sinatra::Base
  register Sinatra::Namespace

  namespace '/foo/:year/?:period?' do
    get '/status' do
      status 200
    end

    namespace '/nested' do
      get '/1' do
        this_method_does_not_exist('hello')
        status 200
      end

      helpers do
        def this_method_does_not_exist(msg)
          puts "hello #{msg}"
        end
      end
    end
  end
end

When I call /foo/2018/2/nested/1 all works as expected. When I call /foo/2018/nested/1 it results in a

NoMethodError: undefined method `this_method_does_not_exist`

Duplicating the outer namespace "resolves" the issue:

class App < Sinatra::Base
  register Sinatra::Namespace

  namespace '/foo/:year/?:period?' do
    get '/status' do
      status 200
    end
  end

  namespace '/foo/:year/?:period?/nested' do
    get '/1' do
      this_method_does_not_exist('hello')
      status 200
    end

    helpers do
      def this_method_does_not_exist(msg)
        puts "hello #{msg}"
      end
    end
  end

end

Full test case can be found here: https://github.com/lvonk/sinatra-namespaces-issue

[lvonk]

So I dug a little deeper but have a hard time pinpointing the issue. I did find another unexpected behavior that might be related:

mock_app do
  namespace '/foo/:year/?:period?' do

    before do
      puts "Period in before: '#{params[:period]}'"
    end

    namespace '/nested' do
      get '/1' do
        puts "Period in get: '#{params[:period]}'"
        200
      end
    end
  end
end

If you call this with /foo/2016/nested/1 it will ouput

Period is before: 'nested'
Period in get: ''

I don't think this is correct, I expect a parameter in a single call to remain the same.

[lvonk]

Another update: So when I remove the generating of the :before block from https://github.com/sinatra/sinatra/blob/master/sinatra-contrib/lib/sinatra/namespace.rb#L232 the test case passes.

[namusyaka]

Definitely bug, but it's not Sinatra issue, but Mustermann issue.
The essence of your issue is in Mustermann::Concat, the meaning of the following two expressions is different.

p Mustermann.new("/foo/:year/?:period?") + Mustermann.new("/nested") + Mustermann.new(%r{(?:/.*)?})
p Mustermann.new("/foo/:year/?:period?") + (Mustermann.new("/nested") + Mustermann.new(%r{(?:/.*)?}))

[namusyaka]

So I'm going to fix the issue as a mustermann's problem.

[namusyaka]

@lvonk Could you confirm my changes?

The problem can be resolved in my locally.

mock_app do
  namespace '/foo/:year/?:period?' do
    namespace '/nested' do
      helpers do
        def foo
          "foo"
        end
      end
      get('/1') { foo }
    end
  end
end

get '/foo/2018/2/nested/1'
expect(last_response.body).to eq('foo')

get '/foo/2018/nested/1'
expect(last_response.body).to eq('foo')

You can specify my branch in your Gemfile, it would be something like:

gem 'mustermann', github: 'namusyaka/mustermann', branch: 'native-concat-look-ahead-parts'

[lvonk]

Thanks @namusyaka ! I can confirm the testcase now works in my example.

[namusyaka]

I have just released mustermann-1.0.2.rc1.
@lvonk Could you check it out?
I'm going to close the issue after releasing mustermann-1.0.2.

[namusyaka]

Sorry, the mustermann-1.0.2.rc1 is not correct. I re-released mustermann-1.0.2.rc2.

[namusyaka]

Closing. mustermann-1.0.2 is out.
Let me know if you still have any issues.

[lvonk]

I can confirm this works in 1.0.2

[namusyaka]

@lvonk Thank you ❤️