This project is archived and is in readonly mode.

#2054 ✓resolved
Marc Bowes

:requirements not passed to named_route generated by :member

Reported by Marc Bowes | February 23rd, 2009 @ 10:46 PM

Example

routes.rb

map.resources :users, :requirements => { :id => %r([^/;,?]+) }, :member => { :baz => :get }

user.rb

def to_param self.name end

The error

Assume @user.name == 'foo'

user_path(@user) # => /users/foo baz_user_path(@user) # => /users/foo/baz

Both work, and would without the :requirements option on the named resource.

Now, with @user.name == 'foo.bar'

user_path(@user) # => /users/foo.bar baz_user_path(@user) # => /users/foo.bar/baz

While the generated route seems legitimate, Rails will fail to respond properly to the baz path. While the first path (/users/foo.bar) works, thanks to :requirements specifying the legal formats of params[:id], :requirements doesn't seem to be passed down to the member function.

Workarounds

One could do the follow above the users resource in routes.rb:

map.baz_user "/users/:id/baz", :requirements => { :id => %r([^/;,?]+) }

.. and then remove the member function. While the requirement RE can be moved into a constant, this is obviously an imperfect solution.

Conclusion

I hope this is a legitimate bug as I have spent some time searching both the Web and Lighthouse for similar bugs but haven't found anything.

Comments and changes to this ticket

  • Marc Bowes

    Marc Bowes February 23rd, 2009 @ 10:48 PM

    Oh dddear. The blockquotes didn't work so well for the example code, it seems to have been squished onto one line. Sorry :)

  • DHH

    DHH February 27th, 2009 @ 01:58 PM

    • Assigned user set to “josh”
    • Milestone cleared.
  • CancelProfileIsBroken

    CancelProfileIsBroken March 4th, 2009 @ 12:05 PM

    The problem is in resources.rb, in action_options_for. This method comes up with the options for a route, including which requirements to use, by looking at the action name. The baz route falls into the else:

    default_options.merge(add_conditions_for(resource.conditions, method)).merge(resource.requirements)

    That's correct for an added collection route (no ID required in the route), but for an added member route it ought to be:

    default_options.merge(add_conditions_for(resource.conditions, method)).merge(resource.requirements(require_id))

    One possible fix would be to change the else to:

    resource.member_methods.include?(method) ? default_options.merge(add_conditions_for(resource.conditions, method)).merge(resource.requirements(require_id)) : default_options.merge(add_conditions_for(resource.conditions, method)).merge(resource.requirements)

  • josh

    josh March 4th, 2009 @ 07:15 PM

    • State changed from “new” to “wontfix”
    • Milestone set to “2.x”

    @Mike that seems to cause a bunch of other problems with routes with name prefixes.

    Please attach some failing unit tests otherwise punting till 2.x

  • CancelProfileIsBroken

    CancelProfileIsBroken March 4th, 2009 @ 11:00 PM

    Here's a failing test that passes with the fix I made (though as you point out that fix has other issues). I'll spend a bit more time looking at a possible patch.

  • CancelProfileIsBroken

    CancelProfileIsBroken March 4th, 2009 @ 11:59 PM

    • State changed from “wontfix” to “open”

    And...here's a patch that passes the test, and doesn't fail any other tests. (this patch includes the tests as well)

  • Repository

    Repository March 6th, 2009 @ 12:49 AM

    • State changed from “open” to “resolved”

    (from [5b7527ca44521edf9782b3d7f449bf09a29267f2]) Failing test for routes with member & requirement [#2054 state:resolved]

    Signed-off-by: Joshua Peek josh@joshpeek.com http://github.com/rails/rails/co...

  • Luke Melia

    Luke Melia March 13th, 2009 @ 09:37 PM

    This change breaks collection routes when a resource has a collection action and member action named the same. Attached is a failing test demonstrating as much. In the context of action_options_for, there is not enough information to determine whether to add the ID resource requirement or not.

    Given we're close to 2.3, I'd recommend rolling back this commit to the previous broken behavior, rather than moving the problem, but perhaps someone can come up with a solution quickly.

  • josh

    josh March 13th, 2009 @ 10:07 PM

    • Milestone cleared.
    • State changed from “resolved” to “open”
  • Repository

    Repository March 13th, 2009 @ 11:14 PM

    • State changed from “open” to “wontfix”

    (from [5b025a1d119eaf09f5376209da212b76725747f8]) Revert 5b7527ca "Failing test for routes with member & requirement" [#2054 state:wontfix] http://github.com/rails/rails/co...

  • CancelProfileIsBroken

    CancelProfileIsBroken March 14th, 2009 @ 01:55 AM

    • State changed from “wontfix” to “open”

    Attached patch handles both cases (the original and the breakage that Luke spotted) by passing additional information into action_options_for. I'd like a more elegant solution, but I believe this one is safe.

  • Luke Melia

    Luke Melia March 14th, 2009 @ 04:04 AM

    I can confirm that Mike's new patch not only passes the test but also works properly in the real-world app that surfaced this issue.

  • Repository

    Repository March 14th, 2009 @ 03:06 PM

    • State changed from “open” to “resolved”

    (from [07710fd3e0c5c84521b7929738ba33cea99bc108]) Fix requirements for additional member/collection routes [#2054 state:resolved]

    Signed-off-by: Joshua Peek josh@joshpeek.com http://github.com/rails/rails/co...

  • David Krmpotic

    David Krmpotic April 5th, 2009 @ 04:31 PM

    Before the change this (minimal case):

    class Spot def to_param

    id

    end end

    worked. After the change, I got:

    app.spot_path(Spot.first) TypeError: can't convert Fixnum into String

    from generated code (/Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/routing/route.rb:160):6:in `generate_raw'
from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/routing/route.rb:131:in `generate'
from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/routing/route_set.rb:384:in `generate'
from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/url_rewriter.rb:205:in `rewrite_path'
from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/url_rewriter.rb:184:in `rewrite_url'
from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/url_rewriter.rb:162:in `rewrite'
from /Users/david/Projects/oc/vendor/rails/actionpack/lib/action_controller/integration.rb:243:in `url_for'
from (eval):16:in `spot_path'
from (irb):3

    I realize that I should have returned id.to_s in the first place, but I'm not sure if is/was expected to work with integers too... All in all now it doesn't anymore... Don't know enough about this subject to contribute a patch, but would love to learn more - especially if it's ok as it is or it does need to be patched.

    Thank you david

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Tags

Referenced by

Pages