broken parameter assignment in routing (11.2) #1366

Closed
ghost opened this Issue Jul 29, 2013 · 9 comments

Comments

Projects
None yet
5 participants
@ghost

ghost commented Jul 29, 2013

I have a route defined as follows:

get :listeners, '/foo/:foo_id/bar/:bar_id/doo/:doo_id/boing' do
   puts params
end

when requesting it via http://localhost/foo/1/bar/2/doo/3/boing
It prints:
{:foo_id=>"1", :bar_id=>"bar/2/doo/3/boing", :doo_id=>"2"}

The routing seems to work, but the variable assignments seem to be broken

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jul 29, 2013

Contributor

Hey @type-exit, it seems to be working just fine with your code both on 0.11.2 and on edge. I'm getting {:foo_id=>"1", :bar_id=>"2", :doo_id=>"3"}. Have you tried that on a clean app? Which ruby are you using? Application server (the test I did was with a default webrick)?

Contributor

dariocravero commented Jul 29, 2013

Hey @type-exit, it seems to be working just fine with your code both on 0.11.2 and on edge. I'm getting {:foo_id=>"1", :bar_id=>"2", :doo_id=>"3"}. Have you tried that on a clean app? Which ruby are you using? Application server (the test I did was with a default webrick)?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 30, 2013

Hey dario,

upon further testing it seems that the behavior is dependent on other routes with similar parameters being present. An isolated single route it works fine. But if there's multiple similar routes they seem to be stepping on each others toes. Changing the order of the routes as well as get/post changes the param behaviour as well.

I was testing on jruby 1.7.3 / puma

I'll try to isolate that issue later today.

ghost commented Jul 30, 2013

Hey dario,

upon further testing it seems that the behavior is dependent on other routes with similar parameters being present. An isolated single route it works fine. But if there's multiple similar routes they seem to be stepping on each others toes. Changing the order of the routes as well as get/post changes the param behaviour as well.

I was testing on jruby 1.7.3 / puma

I'll try to isolate that issue later today.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 30, 2013

Ok, got it isolated in a new app, and see consistent behavior across ruby versions and servers including ruby 1.9.3 and webrick.

Generate a new app, then generate a controller mystuff:

Testapp::App.controllers :mystuff do

  post :index, :with => [:foo, :bar], :bar => /.+/ do
    params.to_s
  end

  get :index, :map => '/mystuff/:a_id/boing/:boing_id' do
    params.to_s
  end

end

open http://localhost:3000/mystuff/1/boing/2
it shows: {:a_id=>"1", :boing_id=>"boing/2"}

reverse the order of the routes in the controller (so get comes first post comes last)
it shows: {:a_id=>"1", :boing_id=>"2"}

Thanks for looking into it!

ghost commented Jul 30, 2013

Ok, got it isolated in a new app, and see consistent behavior across ruby versions and servers including ruby 1.9.3 and webrick.

Generate a new app, then generate a controller mystuff:

Testapp::App.controllers :mystuff do

  post :index, :with => [:foo, :bar], :bar => /.+/ do
    params.to_s
  end

  get :index, :map => '/mystuff/:a_id/boing/:boing_id' do
    params.to_s
  end

end

open http://localhost:3000/mystuff/1/boing/2
it shows: {:a_id=>"1", :boing_id=>"boing/2"}

reverse the order of the routes in the controller (so get comes first post comes last)
it shows: {:a_id=>"1", :boing_id=>"2"}

Thanks for looking into it!

@ujifgc ujifgc closed this Jul 30, 2013

@ujifgc ujifgc reopened this Jul 30, 2013

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jul 30, 2013

Member

Very interesting and strange, a few quick tests. Tried to write unit tests around this:

mock_app do
      post :index, :with => [:foo, :bar], :foo => /.+/ do
        params.to_s
      end

      get :foo, :map => '/mystuff/:a_id/boing/:boing_id' do
        params.to_s
      end
end

get "/mystuff/5/boing/2" returns {:a_id=>"mystuff/5/boing/2", :boing_id=>"5"}

Now with:

mock_app do
      post :index, :with => [:foo, :bar], :bar => /.+/ do
        params.to_s
      end

      get :foo, :map => '/mystuff/:a_id/boing/:boing_id' do
        params.to_s
      end
end

get "/mystuff/5/boing/2" returns nil

and in the final case:

mock_app do
      get :foo, :map => '/mystuff/:a_id/boing/:boing_id' do
        params.to_s
      end
end

it returns: {:a_id=>"5", :boing_id=>"2"}

Member

nesquena commented Jul 30, 2013

Very interesting and strange, a few quick tests. Tried to write unit tests around this:

mock_app do
      post :index, :with => [:foo, :bar], :foo => /.+/ do
        params.to_s
      end

      get :foo, :map => '/mystuff/:a_id/boing/:boing_id' do
        params.to_s
      end
end

get "/mystuff/5/boing/2" returns {:a_id=>"mystuff/5/boing/2", :boing_id=>"5"}

Now with:

mock_app do
      post :index, :with => [:foo, :bar], :bar => /.+/ do
        params.to_s
      end

      get :foo, :map => '/mystuff/:a_id/boing/:boing_id' do
        params.to_s
      end
end

get "/mystuff/5/boing/2" returns nil

and in the final case:

mock_app do
      get :foo, :map => '/mystuff/:a_id/boing/:boing_id' do
        params.to_s
      end
end

it returns: {:a_id=>"5", :boing_id=>"2"}

@gballester

This comment has been minimized.

Show comment
Hide comment
@gballester

gballester Jul 31, 2013

It's only in "development". If you comment out this code in https://github.com/padrino/padrino-framework/blob/master/padrino-core/lib/padrino-core/application.rb it fixes the bug:

  def default_routes!
    #configure :development do
    #  get '*__sinatra__/:image.png' do
     #   content_type :png
     #   filename = File.dirname(__FILE__) + "/images/#{params[:image]}.png"
    #    send_file filename
    #  end
   # end
  end

or you could add:

class << self 
  def default_routes!
    # fix development environment routing params bug
  end
end

to your app.rb as a work around.

The bug does not occur in any environment besides "development." My unit tests were still passing, so it was hard to figure out.

It's only in "development". If you comment out this code in https://github.com/padrino/padrino-framework/blob/master/padrino-core/lib/padrino-core/application.rb it fixes the bug:

  def default_routes!
    #configure :development do
    #  get '*__sinatra__/:image.png' do
     #   content_type :png
     #   filename = File.dirname(__FILE__) + "/images/#{params[:image]}.png"
    #    send_file filename
    #  end
   # end
  end

or you could add:

class << self 
  def default_routes!
    # fix development environment routing params bug
  end
end

to your app.rb as a work around.

The bug does not occur in any environment besides "development." My unit tests were still passing, so it was hard to figure out.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 31, 2013

Hey @gballester, I tried the suggested fix of switching environments and overwriting default_routes!
both did not work for me, and the problem persists. Have tried on padrino-edge and 11.2.

Cheers
Slawo

ghost commented Jul 31, 2013

Hey @gballester, I tried the suggested fix of switching environments and overwriting default_routes!
both did not work for me, and the problem persists. Have tried on padrino-edge and 11.2.

Cheers
Slawo

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Aug 2, 2013

Contributor

It's a very strange behaviour and it's probably down beneath the layer of curried http_router we have in there... This should be gone once we do #1339. Can't promise anything but will try to look into debugging @nesquena's tests. @type-exit I know it sounds patchy but do you have another way to order/write your routes at least temporarily?

Contributor

dariocravero commented Aug 2, 2013

It's a very strange behaviour and it's probably down beneath the layer of curried http_router we have in there... This should be gone once we do #1339. Can't promise anything but will try to look into debugging @nesquena's tests. @type-exit I know it sounds patchy but do you have another way to order/write your routes at least temporarily?

@sshaw

This comment has been minimized.

Show comment
Hide comment
@sshaw

sshaw Aug 3, 2013

Contributor

Without actually looking at the router code, it looks likes param regexes are not scoped the the route and/or param for which they're defined. Also, maybe the \G regex option is being used so the regex position is not being reset between each attempt to match an action?

post :index, :with => [:foo, :bar], :bar => /.+/ do
  params.to_s << "\n"
end

get :foo, :map => '/mystuff/:a_id/boing/:boing_id' do
  params.to_s << "\n"
end
~ >curl -v --get http://localhost:3000/mystuff/1/boing/2 2>&1 | grep -m3 '<'
< HTTP/1.1 405 Method Not Allowed 
< Content-Type: text/html;charset=utf-8
< Allow: POST

Here :bar => /.+/ will eat the request path causing the request to be interpreted as a GET for :index
After adding a param spec for :foo

post :index, :with => [:foo, :bar], :foo => /.+/, :bar => /.+/ do
  params.to_s << "\n"
end

# get :foo is still here
~ >curl -s --get http://localhost:3000/mystuff/1/boing/2
{:a_id=>"mystuff/1/boing/2", :boing_id=>"1"}

With a restricted param spec

post :index, :with => [:foo, :bar], :foo => /\w+/, :bar => /\w+/ do
  params.to_s << "\n"
end

# get :foo here
~ >curl -s --get http://localhost:3000/mystuff/1/boing/2
{:a_id=>"mystuff", :boing_id=>"1"}

And, with a more restrictive spec

post :index, :with => [:foo, :bar], :foo => /\d+/, :bar => /\d+/ do
  params.to_s << "\n"
end

# get :foo still here
~ >curl -s --get http://localhost:3000/mystuff/1/boing/2
{:a_id=>"1", :boing_id=>"2"}

Hope that helps.

Contributor

sshaw commented Aug 3, 2013

Without actually looking at the router code, it looks likes param regexes are not scoped the the route and/or param for which they're defined. Also, maybe the \G regex option is being used so the regex position is not being reset between each attempt to match an action?

post :index, :with => [:foo, :bar], :bar => /.+/ do
  params.to_s << "\n"
end

get :foo, :map => '/mystuff/:a_id/boing/:boing_id' do
  params.to_s << "\n"
end
~ >curl -v --get http://localhost:3000/mystuff/1/boing/2 2>&1 | grep -m3 '<'
< HTTP/1.1 405 Method Not Allowed 
< Content-Type: text/html;charset=utf-8
< Allow: POST

Here :bar => /.+/ will eat the request path causing the request to be interpreted as a GET for :index
After adding a param spec for :foo

post :index, :with => [:foo, :bar], :foo => /.+/, :bar => /.+/ do
  params.to_s << "\n"
end

# get :foo is still here
~ >curl -s --get http://localhost:3000/mystuff/1/boing/2
{:a_id=>"mystuff/1/boing/2", :boing_id=>"1"}

With a restricted param spec

post :index, :with => [:foo, :bar], :foo => /\w+/, :bar => /\w+/ do
  params.to_s << "\n"
end

# get :foo here
~ >curl -s --get http://localhost:3000/mystuff/1/boing/2
{:a_id=>"mystuff", :boing_id=>"1"}

And, with a more restrictive spec

post :index, :with => [:foo, :bar], :foo => /\d+/, :bar => /\d+/ do
  params.to_s << "\n"
end

# get :foo still here
~ >curl -s --get http://localhost:3000/mystuff/1/boing/2
{:a_id=>"1", :boing_id=>"2"}

Hope that helps.

namusyaka added a commit to namusyaka/padrino-framework that referenced this issue Aug 6, 2013

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 9, 2013

yes, that change fixes the issue for me thanks!

ghost commented Aug 9, 2013

yes, that change fixes the issue for me thanks!

@ujifgc ujifgc closed this in a125bca Aug 9, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment