Ticket #9450: 9450.diff

a/actionpack/lib/action_controller/integration.rb

@@ -74 +74 @@
-
-        unless defined? @named_routes_configured
-          # install the named routes in this session instance.
+          # But we have to disable the optimisation code so that we can
+          # generate routes without @request being initialized
+          Routing.optimise_named_routes=false
+          Routing::Routes.reload!
-          klass = class<<self; self; end
-          Routing::Routes.install_helpers(klass)
-

a/actionpack/lib/action_controller/routing.rb

@@ -1 +1 @@
-require 'cgi'
-require 'uri'
-require 'action_controller/polymorphic_routes'
+require 'action_controller/routing_optimisation'
-
-class Object
-  def to_param
@@ -255 +256 @@
-    mattr_accessor :controller_paths
-    self.controller_paths = []
-    
+    # Indicates whether or not optimise the generated named
+    # route helper methods
+    mattr_accessor :optimise_named_routes
+    self.optimise_named_routes = true
+    
-    # A helper module to hold URL related helpers.
-    module Helpers
-      include PolymorphicRoutes
@@ -325 +331 @@
-    end
-  
-    class Route #:nodoc:
-
+, :optimise
-      
-      def initialize
-        @segments = []
-        @requirements = {}
-        @conditions = {}
-      end
+      
+      # Indicates whether the routes should be optimised with the string interpolation
+      # version of the named routes methods.
+      def optimise?
+        @optimise
+      end
+      
+      def segment_keys
+        segments.collect do |segment|
+          segment.key if segment.respond_to? :key
+        end.compact
+      end
-  
-      # Write and compile a +generate+ method for this Route.
-      def write_generation
@@ -381 +399 @@
-        end
-        requirement_conditions * ' && ' unless requirement_conditions.empty?
-      end
+      
-      def generation_structure
-        segments.last.string_structure segments[0..-2]
-      end
@@ -977 +996 @@
-        requirements = assign_route_options(segments, defaults, requirements)
-
-        route = Route.new
+
-        route.segments = segments
-        route.requirements = requirements
-        route.conditions = conditions
+        
+        # Routes cannot use the current string interpolation method
+        # if there are user-supplied :requirements as the interpolation
+        # code won't raise RoutingErrors when generating
+        route.optimise = !options.key?(:requirements) && ActionController::Routing.optimise_named_routes
-
-        if !route.significant_keys.include?(:action) && !route.requirements[:action]
-          route.requirements[:action] = "index"
@@ -1056 +1081 @@
-      # named routes.
-      class NamedRouteCollection #:nodoc:
-        include Enumerable
-
+
-        attr_reader :routes, :helpers, :deprecated_named_routes
-
-        def initialize
@@ -1140 +1165 @@
-          
-          def define_url_helper(route, name, kind, options)
-            selector = url_helper_name(name, kind)
-            
-            # The segment keys used for positional paramters
-
-
-
+
-            hash_access_method = hash_access_name(name, kind)
-            
-            @module.send :module_eval, <<-end_eval # We use module_eval to avoid leaks
-              def #{selector}(*args)
+
+                #{generate_optimisation_block(route, kind)}
+                
-                opts = if args.empty? || Hash === args.first
-                  args.first || {}
-                else
@@ -1166 +1191 @@
-                  #   foo_url(bar, baz, bang, :sort_by => 'baz')
-                  #
-                  options = args.last.is_a?(Hash) ? args.pop : {}
-
+route.
-                    h[k] = v
-                    h
-                  end
@@ -1179 +1204 @@
-            @module.send(:protected, selector)
-            helpers << selector
-          end
-          
-      end
-  
-      attr_accessor :routes, :named_routes

/dev/null

@@ +1 @@
+module ActionController
+  module Routing
+    # Much of the slow performance from routes comes from the 
+    # complexity of expiry, :requirements matching, defaults providing
+    # and figuring out which url pattern to use.  With named routes 
+    # we can avoid the expense of finding the right route.  So if 
+    # they've provided the right number of arguments, and have no
+    # :requirements, we can just build up a string and return it
+    #
+    # This means that we end up generating code that looks like this:
+    # map.person_friend '/people/:person_id/friend/:friend_id',...
+    # 
+    #     def person_friend_path(*args)
+    #       if args.size == 2
+    #         return "/people/#{URI.escape(args[0].to_param)}/friend/#{URI.escape(args[1].to_param)}"
+    #       elsif args.size == 3 && args[3].is_a?(Hash)
+    #         return "/people/#{URI.escape(args[0].to_param)}/friend/#{URI.escape(args[1].to_param)}?#{args[2].to_query_string}"
+    #       end
+    # 
+    #       SLOW_CODE
+    #     end    
+    module Optimisation
+      def generate_optimisation_block(route, kind)
+        return "" unless route.optimise?
+        OPTIMISERS.inject("") do |memo, klazz|
+          optimiser = klazz.new(route, kind)
+          memo << "return #{optimiser.generation_code} if #{optimiser.guard_condition}\n"
+        end
+      end
+
+      class Optimiser
+        attr_reader :route, :kind
+        def initialize(route, kind)
+          @route = route
+          @kind  = kind
+        end
+      end
+
+      class PositionalArguments < Optimiser
+        def guard_condition
+          number_of_arguments = route.segment_keys.size
+          # if they're using foo_url(:id=>2) it's one 
+          # argument, but we don't want to generate /foos/id2
+          if number_of_arguments == 1
+            "args.size == 1 && !args.first.is_a?(Hash)"
+          else
+            "args.size == #{number_of_arguments}"
+          end
+        end
+
+        def generation_code
+          elements = []
+          idx = 0
+
+          if kind == :url
+            elements << '#{request.protocol}'
+            elements << '#{request.host_with_port}'
+          end
+
+          # The last entry in route.segments appears to
+          # *always* be a 'divider segment' for '/'
+          # but we have assertions to ensure that we don't
+          # include the trailing slashes, so skip them
+          route.segments[0..-2].each do |segment|
+            if segment.is_a?(DynamicSegment)
+              elements << "\#{URI.escape(args[#{idx}].to_param, ActionController::Routing::Segment::UNSAFE_PCHAR)}"
+              idx += 1
+            else
+              elements << segment.to_s
+            end
+          end
+          %("#{elements * ''}")
+        end
+      end
+      
+      
+      # supports additional arguments like
+      # - person_url(@person, :q=>"some search term")
+      class PositionalArgumentsWithAdditionalParams < PositionalArguments
+        def guard_condition
+          number_of_arguments = route.segment_keys.size
+          if number_of_arguments == 1
+            "args.size == 2 && !args.first.is_a?(Hash)"
+          else
+            "args.size == #{number_of_arguments + 1}"
+          end
+        end
+        
+        # Use the same code as without the arguments, but add an
+        # args.last.to_query on the end
+        def generation_code
+          orig_code = super
+          orig_code[-1] = '?#{args.last.to_query}"'
+          orig_code
+        end
+      end
+      
+      OPTIMISERS = [PositionalArguments, PositionalArgumentsWithAdditionalParams]
+    end
+  end
+end

a/actionpack/test/controller/resources_test.rb

@@ -26 +26 @@
-end
-
-class ResourcesTest < Test::Unit::TestCase
+  
+  
+  # The assertions in these tests are incompatible with the hash method
+  # optimisation.  This could indicate user level problems
+  def setup
+    ActionController::Routing.optimise_named_routes = false
+  end
+  
+  def tear_down
+    ActionController::Routing.optimise_named_routes = true
+  end
+  
-  def test_should_arrange_actions
-    resource = ActionController::Resources::Resource.new(:messages,
-      :collection => { :rss => :get, :reorder => :post, :csv => :post },

a/actionpack/test/controller/routing_test.rb

@@ -47 +47 @@
-class LegacyRouteSetTests < Test::Unit::TestCase
-  attr_reader :rs
-  def setup
+    # These tests assume optimisation is on, so re-enable it.
+    ActionController::Routing.optimise_named_routes = true
-    @rs = ::ActionController::Routing::RouteSet.new
-    @rs.draw {|m| m.connect ':controller/:action/:id' }
-    ActionController::Routing.use_controllers! %w(content admin/user admin/news_feed)
@@ -166 +168 @@
-  
-  def test_basic_named_route
-    rs.add_named_route :home, '', :controller => 'content', :action => 'list' 
-.new
-{:controller => 'content', :action => 'list', :use_route => :home, :only_path => false}
+
+"http://named.route.test"
-                 x.send(:home_url))
-  end
-
-  def test_named_route_with_option
-    rs.add_named_route :page, 'page/:title', :controller => 'content', :action => 'show_page'
-.new
-{:controller => 'content', :action => 'show_page', :title => 'new stuff', :use_route => :page, :only_path => false}
+
+"http://named.route.test/page/new%20stuff"
-                 x.send(:page_url, :title => 'new stuff'))
-  end
-
-  def test_named_route_with_default
-    rs.add_named_route :page, 'page/:title', :controller => 'content', :action => 'show_page', :title => 'AboutPage'
-
-
-
-
+
+
-                 x.send(:page_url, :title => "AboutRails"))
-
-  end
-
-  def test_named_route_with_nested_controller
-    rs.add_named_route :users, 'admin/user', :controller => '/admin/user', :action => 'index'
-.new
-{:controller => '/admin/user', :action => 'index', :use_route => :users, :only_path => false}
+
+"http://named.route.test/admin/user"
-                 x.send(:users_url))
-  end
-
-  def setup_for_named_route
-
-
-
-
+
+
+
-  end
-
-  def test_named_route_without_hash
@@ -214 +213 @@
-        :year => /\d+/, :month => /\d+/, :day => /\d+/
-      map.connect ':controller/:action/:id'
-    end
-.new
-
-
-
-
+
+# 
+# 
+# 
+# 
-    assert_equal(
-{:controller => 'page', :action => 'show', :title => 'hi', :day => 10, :year => 2005, :month => 6, :use_route => :article, :only_path => false}
+"http://named.route.test/page/2005/6/10/hi"
-      x.send(:article_url, :title => 'hi', :day => 10, :year => 2005, :month => 6)
-    )
-  end
@@ -408 +407 @@
-    assert_equal '/test', rs.generate(:controller => 'post', :action => 'show')
-    assert_equal '/test', rs.generate(:controller => 'post', :action => 'show', :year => nil)
-    
-.new
-{:controller => 'post', :action => 'show', :use_route => :blog, :only_path => false}
+
+"http://named.route.test/test"
-                 x.send(:blog_url))
-  end
-  
@@ -455 +454 @@
-    assert_equal '/', rs.generate(:controller => 'content', :action => 'index')
-    assert_equal '/', rs.generate(:controller => 'content')
-    
-.new
-{:controller => 'content', :action => 'index', :use_route => :home, :only_path => false}
+
+"http://named.route.test"
-                 x.send(:home_url))
-  end
-  
@@ -571 +570 @@
-  ensure
-    Object.send(:remove_const, :SubpathBooksController) rescue nil
-  end
+  
+  def test_failed_requirements_raises_exception_with_violated_requirements
+    rs.draw do |r|
+      r.foo_with_requirement 'foos/:id', :controller=>'foos', :requirements=>{:id=>/\d+/}
+    end
+    
+    x = setup_for_named_route
+    assert_raises(ActionController::RoutingError) do 
+      x.send(:foo_with_requirement_url, "I am Against the requirements")
+    end
+  end
-end
-
-class SegmentTest < Test::Unit::TestCase
@@ -840 +850 @@
-end
-
-uses_mocha 'RouteTest' do
-
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
-class RouteTest < Test::Unit::TestCase
-
-  def setup
@@ -1354 +1403 @@
-  
-end
-
-class RouteSetTest < Test::Unit::TestCase
-  class MockController
-    attr_accessor :routes
-
-    def initialize(routes)
-      self.routes = routes
-    end
-
-    def url_for(options)
-      only_path = options.delete(:only_path)
-      path = routes.generate(options)
-      only_path ? path : "http://named.route.test#{path}"
-    end
-  end
-
-  class MockRequest
-    attr_accessor :path, :path_parameters, :host, :subdomains, :domain, :method
-
-
-
-
-
-
-
-
-
+
-
-  def set
-    @set ||= ROUTING::RouteSet.new
@@ -1510 +1537 @@
-      controller.send(:multi_url, 7, "hello", 5, :baz => "bar")
-  end
-  
-ordered_parameters_and_hash_ordered_parameters_override_hash
+no_positional_arguments
-    controller = setup_named_route_test
-/go/7/hello/joe/5
-multi_url, 7, "hello", 5, :foo => 666
+
+index_url
-  end
-  
-  def test_draw_default_route