This project is archived and is in readonly mode.

Bad routing recognition with globbing/format/requirements

#1939

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

Reported by Johan Sørensen · February 11th, 2009 @ 01:48 PM

Activity

CancelProfileIsBroken
- Assigned user set to josh
- 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.
- failing_routing_tests.diff 3.46 KB
March 10th, 2009 @ 12:55 PM
josh
- Assigned user cleared.
March 11th, 2009 @ 05:32 PM
Pratik
- State changed from verified to open
March 12th, 2009 @ 02:49 PM
Robert Glaser
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.
July 15th, 2009 @ 05:16 PM
Michael Koziarski
- Tag set to bugmash
August 3rd, 2009 @ 06:14 AM
Kieran P
+1 The patch applies cleanly to Rails 2.3 (2-3-stable) and Rails 3.0 (master).

August 9th, 2009 @ 03:05 AM
Elise Huard

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 :defaults not in path cause "No matching segment exists" error

August 15th, 2009 @ 05:50 PM
Peter Abrahamsen
Bump for great justice; this is really annoying.

December 18th, 2009 @ 07:52 PM
Rizwan Reza
- Tag cleared.
[bulk edit]

February 12th, 2010 @ 12:46 PM
Jeremy Kemper
- Milestone changed from 2.x to 3.x
[bulk edit]

May 4th, 2010 @ 06:48 PM

Andrew White

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.

June 25th, 2010 @ 02:44 PM

Dawid Fatyga
- 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
- globbing_routes_failing_test.diff 1.98 KB
November 11th, 2010 @ 12:56 PM
Andrew White
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.
November 11th, 2010 @ 01:58 PM