Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Optional parameters in routes #7047

Open
woto opened this Issue · 20 comments

12 participants

Руслан Корнев Katrina Owen Carlos Antonio da Silva Andrew White Georges El Khoury Steve McKinney Andrew Vit Will Bryant Rafael Mendonça França Ruby on Rails Bot Lauro Caetano Yves Senn
Руслан Корнев

Hello, sorry for Bad English.
Today i updated from 3.1.3 to 3.2.6 and found that my routes doesn't work correct. I'm absolutely sure that it's because of update. But i'm not sure that this is documented feature.

my route looks like

  resources :searches do 
    match '(/:catalog_number(/:manufacturer(/:replacements)))' => "searches#index", :on => :collection, :as => :search, :via => :get
  end

and I call it so

<%= link_to 'Посмотреть аналоги всех найденных номеров', search_searches_path(params[:catalog_number], nil, "1"), :remote => true, :class => 'ajax-search' %>

to get url's that's i need
.../searches/catalog_number/manufacturer/1
.../searches/catalog_number/manufacturer/
.../searches/catalog_number?replacements=1

for now it's always looks like if i add nil to :manufacturer parameter
.../searches/catalog_number

I can write more cleanly examples but especially copy-paste to reproduce error.

Katrina Owen

It seems like it might have worked by accident before.

Here is a test that shows the current behavior:

    rs = ::ActionDispatch::Routing::RouteSet.new
    rs.draw do
      resources :searches do
        collection do
          match '(/:catalog_number(/:manufacturer(/:replacements)))' => "searches#index", :as => :search, :via => :get
        end
      end
    end

    x = Class.new {
      include rs.url_helpers
    }
    x.new.search_searches_path.should eq('/searches')
    x.new.search_searches_path(123).should eq('/searches/123')
    x.new.search_searches_path(123, 'fuzz').should eq('/searches/123/fuzz')
    x.new.search_searches_path(123, 'fuzz', 1).should eq('/searches/123/fuzz/1')

All of the above cases pass. The next one (from your example above) fails:

    x.new.search_searches_path(123, nil, 1).should eq('/searches/123?requirements=1') # fails

This would work if you changed the call to

    search_searches_path(catalog_number, manufacturer, :requirements => 1)
Руслан Корнев

Ok. it's hard to explain i think you will understand when see this examples

the optional parameter replacements

irb(main):036:0> x.new.search_searches_path(123, nil, :requirements => 1)
=> "/searches/123?requirements=1"

irb(main):037:0> x.new.search_searches_path(123, nil, :replacements => 1)
=> "/searches/123"
irb(main):038:0> 
Katrina Owen

Ah, I didn't see that.

All the words that I tried aside from replacements actually worked. That is to say the following pass:

    x.new.search_searches_path(123, nil, :something => 1).should eq('/searches/123?something=1')
    x.new.search_searches_path(123, nil, :whatever => 1).should eq('/searches/123?whatever=1')

However the optional parameter replacements fails whether or not manufacturer is nil:

x.new.search_searches_path(123, 'fuzz', 1, :replacements => 2).should eq('/searches/123/fuzz/1?replacements=2')

       expected: "/searches/123/fuzz/1?replacements=2"
            got: "/searches/123/fuzz/1"

x.new.search_searches_path(123, 'fuzz', :replacements => 2).should eq('/searches/123/fuzz?replacements=2')

       expected: "/searches/123/fuzz?replacements=2"
            got: "/searches/123/fuzz/2"

x.new.search_searches_path(123, 'fuzz', nil, :replacements => 2).should eq('/searches/123/fuzz?replacements=2')

       expected: "/searches/123/fuzz?replacements=2"
            got: "/searches/123/fuzz"

x.new.search_searches_path(123, nil, :replacements => 2).should eq('/searches/123?replacements=2')

       expected: "/searches/123?replacements=2"
            got: "/searches/123"

Andrew White
Owner

What's happening is that Journey (Rails 3.2) is not returning unused path parameters for building the query part of the url but Rack::Mount (Rails 3.1) does. This test demonstrates this:

require 'minitest/autorun'
require 'journey'
require 'rack/mount'

class RoutingTest < MiniTest::Unit::TestCase

  def setup
    @app = lambda { |env| [200, { 'Content-Type' => 'text/plain' }, []] }
    path = '/searches(/:catalog_number(/:manufacturer(/:replacements)))(.:format)'

    @rackmount = Rack::Mount::RouteSet.new do |set|
      set.add_route @app, { :path_info => Rack::Mount::Strexp.compile(path) }, {}, :search_searches
    end

    @journey = Journey::Formatter.new(Journey::Routes.new.tap do |set|
      set.add_route @app, Journey::Path::Pattern.new(path), {}, {}, :search_searches
    end)

    @expected = ['/searches/1', { :replacements => '3' }]
    @url_options = { :catalog_number => '1', :replacements => '3' }
  end

  def test_rackmount
    assert_equal @expected, generate(@rackmount, @url_options)
  end

  def test_journey
    assert_equal @expected, generate(@journey, @url_options)
  end

  private

  def generate(routes, params = {})
    routes.generate(:path_info, :search_searches, params)
  end
end
$ ruby -w routing_test.rb 
Run options: --seed 19535

# Running tests:

F.

Finished tests in 0.085223s, 23.4678 tests/s, 23.4678 assertions/s.

  1) Failure:
test_journey(RoutingTest) [routing_test.rb:28]:
--- expected
+++ actual
@@ -1 +1 @@
-["/searches/1", {:replacements=>"3"}]
+["/searches/1", {}]

I don't think what happens to unused keys is defined anywhere but we should probably consider this a regression (do you agree @tenderlove ?). I'll fix it in a couple of days if someone else hasn't (I've a work deadline for Tuesday, so I'm tied up with that for the moment).

As alternative you may want to consider using optional arguments in a slightly different way so that the :replacements parameter doesn't swap between the path and query segments. I use a prefix to identify a path segment so that each part can drop out without forcing the remaining parts to be dropped, e.g:

get '/searches(/c/:catalog_number)(/m/:manufacturer)(/r/:replacements)' => 'searches#index', :as => :search_searches

This gives a path of /searches/c/123/r/1 when generating a url with { :catalog_number => '123', :replacements => '1' }. I wouldn't recommend this for every url but it's useful when you need to encode state into the url when working with frameworks like Backbone.js.

Also if you need to ensure a parameter is part of the query you can put it under the :params key when building the url, e.g: search_searches(catalog_number, :params => { :replacements => 1 }).

Andrew White pixeltrix was assigned
Руслан Корнев

Hi, sorry.
Any news?

Andrew White
Owner

@woto I've identified where the problem is in Journey, I just need to discuss with @tenderlove what option he'd prefer.

@tenderlove the Journey::Visitors::Formatter class needs to keep a record of which keys haven't been used - it can do this by storing the contents of the consumed variable when visit_GROUP detects that a route contains a NUL character. The issue is getting back out to the Journey::Formatter#generate method without changing the return signature of Journey::Route#format - we can either pass in a hash that gets added to or add a instance variable to save them in the route and pull it out after calling route.format(path_options) - which would you prefer?

Руслан Корнев

Ok, may be there any right way workaround? I will rewrite my code if nobody doesn't complain this problem. And this fix will never be wiritten. Want to use 3.2 :)

Sorry for bad english.

Andrew White
Owner

@woto sorry haven't had chance to discuss the matter yet - I'll take another look next week.

Georges El Khoury

Same issue here

Georges El Khoury

One workaround with us is to give the optional parameter a nil value

Steve McKinney

Is this still under consideration? cc @tenderlove @pixeltrix

Andrew Vit

@pixeltrix is this ticket still valid? Looks like the duplicate one was closed.

Andrew White
Owner

@avit yes, it is still valid but it's going to be part of a bigger refactoring for 4.1

Will Bryant

This is affecting us to. Losing url params is a pretty major regression so waiting for 4.1 and then going from 3.1 to 4.1 in a single leap seems pretty unrealistic?

Will Bryant

*too, sorry. From my dup ticket:

Relevant part of our test case:

  match ':controller(/:action(/:id(.:format)))'
  match ':controller/:action.:format'
    url_for(:host => "test.host", :controller => "buy_power", :action => "save_txn", :format => "js", :foo => "bar")

Would ideally give "http://test.host/buy_power/save_txn.js?foo=bar". But we get "http://test.host/buy_power/save_txn?foo=bar" - note lack of extension.

Same problem if we don't have a :foo => 'bar' BTW, the :format still gets ignored.

Since the :format parts are optional, I would kind of understand if :format turned up as a query param instead, like ?format=js. This is what sometimes happened on 3.1, and it worked OK.

Will Bryant

There's something that I noticed while trying to work around that, and it turns out that if you fix that problem, our test case is also fixed. Whether this is a general solution I'm not so clear, but have a look and see what you think.

Probably easiest to explain by going through the workaround we tried, which was to make everything non-optional but have multiple routes to get the same behavior back:

  match ':controller/:action/:id.:format'
  match ':controller/:action/:id'
  match ':controller/:action.:format'
  match ':controller/:action'
  match ':controller.:format' => '#index'
  match ':controller' => '#index'

Now this turned out to work in the above test case, but it introduced another bug: on /something/foo/1234, url_for(:action => 'other') should produce /something/other, but it produced /something/other/1234. Any parts to the right of the parts you change should be dropped, but they weren't. Here's the code that is meant to implement this behavior:

        keys_to_keep = route.parts.reverse.drop_while { |part|
          !options.key?(part) || (options[part] || recall[part]).nil?
        } | route.required_parts

I reckon that, depending on your point of view, either this algorithm is wrong, or the scoring should not be allowed to use recall (nb. there is a test that checks it does - test_recall_should_be_used_when_scoring).

Personally I believe the algorithm is incorrect. What I think it needs to do is drop everything to the right of the first missing component, but what is implemented above is to drop everything to the right of the last present component. This is not the same thing - if you have a route :a/:b/:c/:d, and you try to generate with :a, :c, and :d but no :b, I would have thought you need to drop :c and :d, whereas the algorithm in the code above only drops :d because it will stop dropping when it sees there is a :c.

But I can see the case for saying the algorithm is fine - if you have tried to generate using the correct route. It'll still bail if you don't have the necessary parts in either the arguments or the recall. But in that case then it seems very wrong that recall is used in scoring, because that makes it always pick the longer match ':controller/:action/:id' route when you try to do url_for(:action => 'other') while you're currently on /something/foo/1234, and recall will therefore include :id and that will end up in your generated route.

Thoughts?

I did try patching the algorithm (using route.path.take_until instead of route.path.reverse.drop_until and inverting the condition) and it worked perfectly in our app, but it fails two recall test cases that go against the behavior I was expecting:

1) Failure:
test_recall_should_be_used_when_scoring(Journey::TestRouter) [./test/test_router.rb:291]:
Expected: "/messages/index/10"
Actual: "/messages/index"

2) Failure:
test_generate_uses_recall_if_needed(Journey::TestRouter) [./test/test_router.rb:378]:
Expected: "/tasks/index/10"
Actual: "/tasks"

I did have a look at Rack::Mount to see what it did, but it's complicated (when you see "# Nasty control flow
return :clear_remaining_segments" you know you're in trouble...).

Andrew White
Owner

Having investigated this it's going to need a bigger refactoring of the router than is possible for the 4.1 timeframe so I'm pushing it to the 4.2 release.

Руслан Корнев woto added the stale label
Rafael Mendonça França
Owner

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Lauro Caetano laurocaetano removed the stale label
Rafael Mendonça França rafaelfranca modified the milestone: 4.2.0, 5.0.0
Ruby on Rails Bot
Collaborator

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

Ruby on Rails Bot rails-bot added the stale label
Yves Senn senny added pinned and removed stale labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.