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
  • happy2011

Create your profile

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

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Pages