padrino / padrino-framework

Padrino is a full-stack ruby framework built upon Sinatra.
http://www.padrinorb.com
MIT License
3.37k stars 509 forks source link

FIXED: url(...) tries to match routes not compatible with missing par… #2246

Closed takeshi-yashiro closed 2 years ago

takeshi-yashiro commented 2 years ago

This PR fixes #2219.

Expected to solve the issue discussed here as well: https://gitter.im/padrino/padrino-framework?at=5eba64c920eaac185304305b

nesquena commented 2 years ago

Thanks!

nesquena commented 2 years ago

@takeshi-yashiro Do you feel test coverage is already sufficient or do you think it makes sense to add a test to verify this is working correctly?

takeshi-yashiro commented 2 years ago

@takeshi-yashiro Do you feel test coverage is already sufficient or do you think it makes sense to add a test to verify this is working correctly?

I assume adding the following test is a good idea. (Test content is the same as the existing test, 'should recognize route even if paths are duplicated' in test_routing.rb, but in reversed route definition order)

  it 'should recognize route even if paths are duplicated, in reverse order' do
    mock_app do
      get(:index, :with => :id, :provides => :json) {}
      get(:index, :with => :id) {}
      get(:index) {}
    end
    assert_equal "/", @app.url_for(:index)
    assert_equal "/1234", @app.url_for(:index, :id => "1234")
    assert_equal "/1234.json?baz=baz", @app.url_for(:index, :id => "1234", :format => "json", :baz => "baz")
  end

I confirmed that master branch before this PR fails the above test fails with the following error, while the current master does not.

  1) Error:
Routing#test_0006_should recognize route even if paths are duplicated, in reverse order:
Mustermann::ExpandError: cannot expand with keys [], possible expansions: [:format, :id] or [:id]
    /opt/homebrew/lib/ruby/gems/3.0.0/gems/mustermann-1.1.1/lib/mustermann/ast/expander.rb:121:in `error_for'
    /opt/homebrew/lib/ruby/gems/3.0.0/gems/mustermann-1.1.1/lib/mustermann/ast/expander.rb:98:in `block in expand'
    /opt/homebrew/lib/ruby/gems/3.0.0/gems/mustermann-1.1.1/lib/mustermann/ast/expander.rb:98:in `fetch'
    /opt/homebrew/lib/ruby/gems/3.0.0/gems/mustermann-1.1.1/lib/mustermann/ast/expander.rb:98:in `expand'
    /opt/homebrew/lib/ruby/gems/3.0.0/gems/mustermann-1.1.1/lib/mustermann/expander.rb:150:in `expand'
    /opt/homebrew/lib/ruby/gems/3.0.0/gems/mustermann-1.1.1/lib/mustermann/expander.rb:182:in `with_rest'
    /opt/homebrew/lib/ruby/gems/3.0.0/gems/mustermann-1.1.1/lib/mustermann/expander.rb:152:in `expand'
    /opt/homebrew/lib/ruby/gems/3.0.0/gems/mustermann-1.1.1/lib/mustermann/ast/pattern.rb:108:in `expand'
    /Users/takeshi/iniad-edu-system/padrino-framework/padrino-core/lib/padrino-core/path_router/matcher.rb:44:in `expand'
    /Users/takeshi/iniad-edu-system/padrino-framework/padrino-core/lib/padrino-core/path_router.rb:70:in `path'
    /Users/takeshi/iniad-edu-system/padrino-framework/padrino-core/lib/padrino-core/application/routing.rb:432:in `make_path_with_params'
    /Users/takeshi/iniad-edu-system/padrino-framework/padrino-core/lib/padrino-core/application/routing.rb:343:in `url'
    /Users/takeshi/iniad-edu-system/padrino-framework/padrino-core/test/test_routing.rb:105:in `block (2 levels) in <top (required)>'

I have added this test as https://github.com/takeshi-yashiro/padrino-framework/commit/e9953ad67802f40c63ea5d77d46a663d9ba79277. Shall I submit a PR for this test as well?

nesquena commented 2 years ago

A PR for that test would be great thank you!