Update the wildcard route to be non-greedy by default, therefore be able to match the (.:format) segment [#6605 state:resolved]

After some discussion with Andrew White, it seems like this is a better approach for handling a wildcard route. However, user can still bring back the old behavior by supplying `:format => false` to the route.

Signed-off-by: Andrew White <andyw@pixeltrix.co.uk>

5 files changed, 59 insertions, 3 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 6032c6b7da..3eba2281c4 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,13 @@
 *Rails 3.1.0 (unreleased)*
 
+* Wildcard route will always matching the optional format segment by default. For example if you have this route:
+
+    map '*pages' => 'pages#show'
+
+  by requesting '/foo/bar.json', your `params[:pages]` will be equals to "foo/bar" with the request format of JSON. If you want the old 3.0.x behavior back, you could supply `:format => false` like this:
+
+    map '*pages' => 'pages#show', :format => false
+
 * Added Base.http_basic_authenticate_with to do simple http basic authentication with a single class method call [DHH]
 
     class PostsController < ApplicationController
diff --git a/actionpack/actionpack.gemspec b/actionpack/actionpack.gemspec
index f6bc5e0d37..651f3b164a 100644
--- a/actionpack/actionpack.gemspec
+++ b/actionpack/actionpack.gemspec
@@ -26,7 +26,7 @@ Gem::Specification.new do |s|
   s.add_dependency('i18n',             '~> 0.5.0')
   s.add_dependency('rack',             '~> 1.2.1')
   s.add_dependency('rack-test',        '~> 0.5.7')
-  s.add_dependency('rack-mount',       '~> 0.6.13')
+  s.add_dependency('rack-mount',       '~> 0.7.1')
   s.add_dependency('tzinfo',           '~> 0.3.23')
   s.add_dependency('erubis',           '~> 2.6.6')
 end
diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 14c424f24b..35be0b3a27 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -104,10 +104,16 @@ module ActionDispatch
               @options.reverse_merge!(:controller => /.+?/)
             end
 
+            # Add a constraint for wildcard route to make it non-greedy and match the
+            # optional format part of the route by default
+            if path.match(/\*([^\/]+)$/) && @options[:format] != false
+              @options.reverse_merge!(:"#{$1}" => /.+?/)
+            end
+
             if @options[:format] == false
               @options.delete(:format)
               path
-            elsif path.include?(":format") || path.end_with?('/') || path.match(/^\/?\*/)
+            elsif path.include?(":format") || path.end_with?('/')
               path
             else
               "#{path}(.:format)"
diff --git a/actionpack/test/action_dispatch/routing/mapper_test.rb b/actionpack/test/action_dispatch/routing/mapper_test.rb
index e21b271907..f90de735b6 100644
--- a/actionpack/test/action_dispatch/routing/mapper_test.rb
+++ b/actionpack/test/action_dispatch/routing/mapper_test.rb
@@ -25,6 +25,10 @@ module ActionDispatch
         def conditions
           routes.map { |x| x[1] }
         end
+
+        def requirements
+          routes.map { |x| x[2] }
+        end
       end
 
       def test_initialize
@@ -50,8 +54,34 @@ module ActionDispatch
       def test_map_wildcard
         fakeset = FakeSet.new
         mapper = Mapper.new fakeset
-        mapper.match '/*path', :to => 'pages#show', :as => :page
+        mapper.match '/*path', :to => 'pages#show'
+        assert_equal '/*path(.:format)', fakeset.conditions.first[:path_info]
+        assert_equal /.+?/, fakeset.requirements.first[:path]
+      end
+
+      def test_map_wildcard_with_other_element
+        fakeset = FakeSet.new
+        mapper = Mapper.new fakeset
+        mapper.match '/*path/foo/:bar', :to => 'pages#show'
+        assert_equal '/*path/foo/:bar(.:format)', fakeset.conditions.first[:path_info]
+        assert_nil fakeset.requirements.first[:path]
+      end
+
+      def test_map_wildcard_with_multiple_wildcard
+        fakeset = FakeSet.new
+        mapper = Mapper.new fakeset
+        mapper.match '/*foo/*bar', :to => 'pages#show'
+        assert_equal '/*foo/*bar(.:format)', fakeset.conditions.first[:path_info]
+        assert_nil fakeset.requirements.first[:foo]
+        assert_equal /.+?/, fakeset.requirements.first[:bar]
+      end
+
+      def test_map_wildcard_with_format_false
+        fakeset = FakeSet.new
+        mapper = Mapper.new fakeset
+        mapper.match '/*path', :to => 'pages#show', :format => false
         assert_equal '/*path', fakeset.conditions.first[:path_info]
+        assert_nil fakeset.requirements.first[:path]
       end
     end
   end
diff --git a/railties/guides/source/routing.textile b/railties/guides/source/routing.textile
index c447fd911a..58b75b9a1d 100644
--- a/railties/guides/source/routing.textile
+++ b/railties/guides/source/routing.textile
@@ -557,6 +557,18 @@ match '*a/foo/*b' => 'test#index'
 
 would match +zoo/woo/foo/bar/baz+ with +params[:a]+ equals +"zoo/woo"+, and +params[:b]+ equals +"bar/baz"+.
 
+NOTE: Starting from Rails 3.1, wildcard route will always matching the optional format segment by default. For example if you have this route:
+
+<ruby>
+map '*pages' => 'pages#show'
+</ruby>
+
+NOTE: By requesting +"/foo/bar.json"+, your +params[:pages]+ will be equals to +"foo/bar"+ with the request format of JSON. If you want the old 3.0.x behavior back, you could supply +:format => false+ like this:
+
+<ruby>
+map '*pages' => 'pages#show', :format => false
+</ruby>
+
 h4. Redirection
 
 You can redirect any path to another path using the +redirect+ helper in your router: