New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested namespace with optional url parts do not work in sinatra => 2.0 #1361

Closed
lvonk opened this Issue Nov 7, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@lvonk
Contributor

lvonk commented Nov 7, 2017

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 lvonk changed the title from Nested namespace with optional url parts do not work in sinatra > 2.0 to Nested namespace with optional url parts do not work in sinatra => 2.0 Nov 7, 2017

@lvonk

This comment has been minimized.

Show comment
Hide comment
@lvonk

lvonk Nov 10, 2017

Contributor

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.

Contributor

lvonk commented Nov 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@lvonk

lvonk Nov 10, 2017

Contributor

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.

Contributor

lvonk commented Nov 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Nov 11, 2017

Member

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{(?:/.*)?}))
Member

namusyaka commented Nov 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Nov 12, 2017

Member

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

Member

namusyaka commented Nov 12, 2017

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

namusyaka added a commit to namusyaka/mustermann19 that referenced this issue Nov 12, 2017

look ahead same patterns as its own when concatenation
The meaning of the following two expressions is different, but it's unexpected behavior.

  Mustermann.new("/a") + Mustermann.new("/b") + Mustermann.new(/c/)
  Mustermann.new("/a") + (Mustermann.new("/b") + Mustermann.new(/c/))

The differencet leads to occur a bug on sinatra-namespace.
This commit fixes sinatra/sinatra#1361
@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Nov 12, 2017

Member

@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'
Member

namusyaka commented Nov 12, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@lvonk

lvonk Nov 12, 2017

Contributor

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

Contributor

lvonk commented Nov 12, 2017

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

namusyaka added a commit to namusyaka/mustermann19 that referenced this issue Nov 12, 2017

look ahead same patterns as its own when concatenation
The meaning of the following two expressions is different, but it's unexpected behavior.

  Mustermann.new("/a") + Mustermann.new("/b") + Mustermann.new(/c/)
  Mustermann.new("/a") + (Mustermann.new("/b") + Mustermann.new(/c/))

The differencet leads to occur a bug on sinatra-namespace.
This commit fixes sinatra/sinatra#1361
@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Feb 13, 2018

Member

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.

Member

namusyaka commented Feb 13, 2018

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

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Feb 15, 2018

Member

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

Member

namusyaka commented Feb 15, 2018

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

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Feb 16, 2018

Member

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

Member

namusyaka commented Feb 16, 2018

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

@namusyaka namusyaka closed this Feb 16, 2018

@lvonk

This comment has been minimized.

Show comment
Hide comment
@lvonk

lvonk Feb 19, 2018

Contributor

I can confirm this works in 1.0.2

Contributor

lvonk commented Feb 19, 2018

I can confirm this works in 1.0.2

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Feb 19, 2018

Member

@lvonk Thank you ❤️

Member

namusyaka commented Feb 19, 2018

@lvonk Thank you ❤️

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Mar 17, 2018

taca
www/ruby-mustermann: update to 1.0.2
* **Mustermann 1.0.2** (2017-02-17)
    * Look ahead same patterns as its own when concatenation.  Fixes [sinatra/sinatra#1361](sinatra/sinatra#1361) [@namusyaka](https://github.com/namusyaka)
    * Improve development support and documentation.  [@EdwardBetts](https://github.com/EdwardBetts), [@284km](https://github.com/284km), [@yb66](https://github.com/yb66) and [@garybernhardt](https://github.com/garybernhardt)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment