Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Rails4: Routes that uses same variable more than once in url requires it supplied more than once. #12808

Closed
jarl-dk opened this Issue · 22 comments

7 participants

@jarl-dk

I am on the path to upgrade a Rails 3.2 app to Rails 4.0.1

I have a route that looks like this:

get 'download/:id/:id.tar' => 'downloader#tar_file', :as => :tar_file_downloader, :id => /\d+/

Then in my view I had links to that using

<%=  link_to 'Download TAR file', :tar_file_downloader(@order)  -%>

That worked fine with Rails 3.x, but with Rails 4, it fails with:

No route matches {:id=>nil, :controller=>"downloader", :action=>"tar_file", :format=>nil} missing required keys: [:id, :id]

Meaning that the url helper requires the :id parameter to be supplied twice, and yes if I change my view to

<%=  link_to 'Download TAR file', :tar_file_downloader(@order, @order)  -%>

Then the problem disappears, but that is very inappropriate to supply the same information twice to the url generator just because the resulting URL contains the :id twice.

@rafaelfranca
Owner

To me it make sense.

@schneems
Collaborator

Can you give me a small project that reproduces the error? I tried adding a test case to master, and this passes

In actiopack/test/dispatch/routing_test.rb:

  def test_duplicate_keys_work
    draw do
      get 'todos/:id/:id.tar' => 'todos#show', as: "todo"
    end

    get '/todos/1/1.tar'
    assert_equal 'todos#show', @response.body
    assert_equal '/todos/1/1.tar', todo_path(:id => '1')
  end
@amiel

I agree that it doesn't make sense to supply the param twice. However, given @schneems's test, it looks like supplying the param once is possible if you specify a key.

This would be consistent with the activerecord interpolation api:

where(['id = ? OR token = ?', token, token])
where(['id = :token OR token = :token', { token: token }])

If this is the case, my opinion would be to leave it as is.

@pixeltrix
Owner

@amiel what's happening is that when you supply positional args Rails will use an optimised route generation code path that substitutes the args in the path with the corresponding positional arg. Obviously if the number of positional args doesn't match then this will generate an error. However if you supply a hash then the normal code path is followed which generates the route as in 3.2.

I'll have a look at the code but I suspect it may be an intractable problem with the optimisation method used by the url generator.

@jarl-dk

Can you tell me what is the easiest way to run test for a specific file like the one you mention?

@schneems
Collaborator

Clone the rails repo the cd into it. Next cd into action pack and run

$ rake test TEST=test/dispatch/routing_test.rb

Note you need to add in my code from above it is not already present.

@jarl-dk

@schneems : The bug report goes on the following test (where the id is only supplied once) failing (and it did not in 3.2.x series):

  def test_duplicate_keys_work
    draw do
      get 'todos/:id/:id.tar' => 'todos#show', as: "todo", 
    end

    get '/todos/1/1.tar'
    assert_equal 'todos#show', @response.body
    assert_equal '/todos/1/1.tar', todo_path('1')
  end
@jarl-dk

@pixeltrix : How did this work in 3.2 (where there was no such problem)?

@pixeltrix
Owner

@jarl-dk there was no optimized url generation in Rails 3.2 - positional args were converted to a hash

@jarl-dk

The API in rails 3.2 seems much simpler. Would it be possible to achieve the same behaviour/API as in rails 3.2?

@jarl-dk

Any new status on this?

@pixeltrix pixeltrix was assigned
@pixeltrix pixeltrix referenced this issue from a commit
@pixeltrix pixeltrix Unique the segment keys array for non-optimized url helpers
In Rails 3.2 you only needed pass an argument for dynamic segment once so
unique the segment keys array to match the number of args. Since the number
of args is less than required parts the non-optimized code path is selected.
This means to benefit from optimized url generation the arg needs to be
specified as many times as it appears in the path.

Fixes #12808

(cherry picked from commit 6b54883)
5d3a8f1
@pixeltrix pixeltrix closed this issue from a commit
@pixeltrix pixeltrix Unique the segment keys array for non-optimized url helpers
In Rails 3.2 you only needed pass an argument for dynamic segment once so
unique the segment keys array to match the number of args. Since the number
of args is less than required parts the non-optimized code path is selected.
This means to benefit from optimized url generation the arg needs to be
specified as many times as it appears in the path.

Fixes #12808
6b54883
@pixeltrix pixeltrix closed this in 6b54883
@jarl-dk

So I guess that means it will go into 4.0.3 and 4.1.0 when time comes... Right?

@pixeltrix
Owner

@jarl-dk yep, barring any regressions

@schneems
Collaborator

:smile: great work @pixeltrix :heart:

@jarl-dk jarl-dk referenced this issue in adzap/validates_timeliness
Open

Support for Rails 4 #101

@jarl-dk

I can't verify that 6b54883 is part of v4.0.3. Is it intentionally only part of 4.1.0-series?

@jarl-dk

Since this is set for milestone 4.0.3 I think it should be reopened and set for milestone 4.0.4

@jarl-dk

To be honest I think something went wrong when tagging v4.0.3.

$ git log --oneline v4.0.2..v4.0.3 | wc -l
3
$ git log --oneline v4.0.2..origin/4-0-stable | wc -l
266

v4.0.3 only contains three commits not found in v4.0.2

@dmathieu
Collaborator

And it shouldn't be reopened since it has been merged. It will be in the next non-security 4.0.x release.
Also, it is already released in 4.1.rc1.

@jarl-dk

I see, sorry for the noise. Maybe you want to consider changing the milestone to communicate this...

@dmathieu
Collaborator

I just renamed the 4.0.3 milestone to 4.0.4. Thank you.

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.