This project is archived and is in readonly mode.

#1939 ✓resolved
Johan Sørensen

Bad routing recognition with globbing/format/requirements

Reported by Johan Sørensen | February 11th, 2009 @ 01:48 PM | in 3.x

The changes introduced in fef6c32afe2276df ("Added optimal formatted routes to rails, deprecating the formatted_* methods[...]") breaks the use of :requirement parameter on :format (useful on "globbed" route paths where you'd only want to allow certain formats)

Here's a failing test case:


  def test_paths_should_allow_to_set_format_regexp_requirement
    rs.draw do |map|
      map.connect('files/*path.:format', :controller => 'content', 
        :action => 'show_file', :requirements => {:format => /(zip|tar.gz)/})
    end
    
    assert_equal({:controller => "content", :action => 'show_file', :path => ['foo', 'bar'], 
        :format => "zip" }, rs.recognize_path("/files/foo/bar.zip"))
    assert_equal({:controller => "content", :action => 'show_file', :path => ['foo', 'bar'], 
        :format => "tar.gz" }, rs.recognize_path("/files/foo/bar.tar.gz"))
  end

Comments and changes to this ticket

  • CancelProfileIsBroken

    CancelProfileIsBroken March 10th, 2009 @ 12:55 PM

    • Assigned user set to “Joshua Peek”
    • State changed from “new” to “verified”
    • Title changed from “"optimal formatted routes" breaks format requirements” to “Bad routing recognition with globbing/format/requirements”

    There are a couple of general issues tangled up together here. You can see both by looking at the route recognizer we build in this case:

    
    def recognize(path, env = {})
    if (match = /\A\/files(?:\/?\Z|\/(.*)(?:\/|(\.[^\/?\.]+)?)??)\Z/.match(path))
    params = parameter_shell.dup
    params[:path] = PathSegment::Result.new_escaped((match[1] || "").split('/'))
    
              if (m = match[2])
                params[:format] = URI.unescape(m.from(1))
              end
    
    params
    end
    end
    

    Two problems:

    1) Because we use (.*) to match the globbed path, there will never be a match[2] here to match against the format

    2) Even if we fixed that, this would still fail because the regex segment we generate for the format includes [^\/?.], causing it to only recognize formats without a dot in them - so .tar.gz will never been seen as a format in any route.

    Both of these issues seem fairly intractable for the 2.3 cycle. It looks like changing the format recognition regexp segment to include [^\/?] would fix the second issue, but that causes test_irregular_id_with_no_requirements_should_raise_error(ResourcesTest) to fail - personally I'm not too happy about that test, but it documents existing behavior that we probably don't want to change.

    For what it's worth, here's a patch with a bunch of tests to document the failures. In the current edge code, test_path_without_format_should_include_format_in_path passes and all the rest fail. I think in an ideal world all should pass.

  • Joshua Peek

    Joshua Peek March 11th, 2009 @ 05:32 PM

    • Assigned user cleared.
  • Pratik

    Pratik March 12th, 2009 @ 02:49 PM

    • State changed from “verified” to “open”
  • Robert Glaser

    Robert Glaser July 15th, 2009 @ 05:16 PM

    For me, globbed routes with an attached format parameter generally don't work:

    map.article ':locale/*path.:format', :controller => "articles", :action => "show"
    

    The format parameter is always being concatenated to the last element of the path array.

  • Michael Koziarski
  • Kieran P

    Kieran P August 9th, 2009 @ 03:05 AM

    +1 The patch applies cleanly to Rails 2.3 (2-3-stable) and Rails 3.0 (master).

  • Elise Huard

    Elise Huard August 15th, 2009 @ 05:50 PM

    Mike's patch applies to master, and indeed, only test_path_without_format_should_include_format_in_path passes from the 5 proposed tests.

    IMHO there's a link with #2478

  • Peter Abrahamsen

    Peter Abrahamsen December 18th, 2009 @ 07:52 PM

    Bump for great justice; this is really annoying.

  • Rizwan Reza
  • Jeremy Kemper

    Jeremy Kemper May 4th, 2010 @ 06:48 PM

    • Milestone changed from 2.x to 3.x
  • Andrew White

    Andrew White June 25th, 2010 @ 02:44 PM

    • State changed from “open” to “resolved”
    • Assigned user set to “Andrew White”

    This seems to be working now:

      match '/:locale/*path.:format', :to => 'articles#show'
    
      # GET /en/this/is/a/long/path.html
      # => {"format"=>"html", "action"=>"show", "path"=>"this/is/a/long/path", "controller"=>"articles", "locale"=>"en"}
    

    Please update this ticket if it isn't working for you.

  • Dawid Fatyga

    Dawid Fatyga November 11th, 2010 @ 12:56 PM

    • Importance changed from “” to “”

    Hello,

    what about cases where :format is omitted? The route to /en/this/is/a/long/path will not match articles#show action with :format => 'html'.

    Adding:

    :defaults => { :format => 'html' }
    

    does not solve the problem. Also changing the match to:

    match '/:locale/*path(.:format)', :to => 'articles#show'
    

    The routes that work as expected (allows the optional format) should look like this:

    match '/:locale/*path.:format', :to => 'articles#show'
    match '/:locale/*path',         :to => 'articles#show', :as => "article"
    

    I am submitting a diff with failing tests to clarify my point.

    Regards,
    Dawid

  • Andrew White

    Andrew White November 11th, 2010 @ 01:58 PM

    The problem with trying to make the format parameter optional is that glob parameter will eat up the format even if you set the parameter regexp to be non-greedy. You need a fixed item in the path to stop the glob matching - the following works:

      match '/:locale/*category/:article(.:format)' => 'articles#show', :as => :article
      # Matches the following:
      # - /en/computers/apple
      # - /en/computers/apple.html
      # - /en/computers/apple.xml
      # - /en/computers/apple/macbook
      # - /en/computers/apple/macbook.html
      # - /en/computers/apple/macbook.xml
    

    You can make the category optional as well

      match '/:locale/(*category)/:article(.:format)' => 'articles#show', :as => :article
    

    You don't need the format parameter - Rails will add that by default unless you pass :format => false.

  • xxchenjialong

    xxchenjialong January 8th, 2011 @ 02:14 AM

    In spite of there are thousands and millions shoes offered in the market, pittsburgh steelers jerseys make your shoe storage cabinet houston texans authentic jerseys plenty choices by engaging a new pair of black wedge boots today, trust my word, Nike Air Max 95 shoes definitely worth more than its value. dolphins football jerseys This style is the aboriginal shoes to affection Air Max cushioning new york giants nfl arrangement in both the heel and forefoot. The air max 95 wants to action you a nfl jerseys for women brace air max shoes as adequate as fashionable. In 1995, Nike launched this oakland raiders jerseys version which won the high reputation in that time. Everybody loves Nike Air Max 95 that Nike had to re-issue the archetypal so abounding times. Most of the people are attracted by this unique design of Nike Air Max shoes. It comes in suede and mesh construction with some reflective 3M for visibility, and whilst this new pair makes use of none of those resources, it’ll have no difficulty being seen. These new Ninety-Fives are crafted fully in black leather, but simply because some of it’s smooth and supple while contrasting tiers are accomplished in patent leather, you receive a rather wide selection of sd chargers jerseys this ‘un-color’. And as with pretty much any cincinnati bengals jerseys athletic shoe, adding red and white accents new york giants nfl to a black base helps make cleveland browns jerseys for a must-cop. With long-term and rich experience, we believe Air youth nfl football jersey Max shoes san diego chargers jerseys will create our characterized selling points that offer new recent models with new color way based on detroit lions jerseys our high philadelphia eagles jerseys quality, collegiate-inspired, casual luxury footwear.

  • happy2011

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>

Shared Ticket Bins

Pages