Optimize routes generation in simple cases

actionpack/lib/action_controller/metal/url_for.rb

@@ -1,7 +1,7 @@
-# Includes +url_for+ into the host class. The class has to provide a +RouteSet+ by implementing 
+# Includes +url_for+ into the host class. The class has to provide a +RouteSet+ by implementing
 # the <tt>_routes</tt> method. Otherwise, an exception will be raised.
 #
-# In addition to <tt>AbstractController::UrlFor</tt>, this module accesses the HTTP layer to define 
+# In addition to <tt>AbstractController::UrlFor</tt>, this module accesses the HTTP layer to define
 # url options like the +host+. In order to do so, this module requires the host class
 # to implement +env+ and +request+, which need to be a Rack-compatible.
 #
@@ -18,30 +18,28 @@
 #       @url        = root_path # named route from the application.
 #     end
 #   end
-# 
+#
 module ActionController
   module UrlFor
     extend ActiveSupport::Concern
 
     include AbstractController::UrlFor
 
     def url_options
-      @_url_options ||= super.reverse_merge(
-        :host => request.host,
-        :port => request.optional_port,
-        :protocol => request.protocol,
-        :_path_segments => request.symbolized_path_parameters
-      ).freeze
+      @_url_options ||= begin
+        hash = super.reverse_merge(
+          :host => request.host,
+          :port => request.optional_port,
+          :protocol => request.protocol,
+          :_path_segments => request.symbolized_path_parameters
+        )
 
-      if _routes.equal?(env["action_dispatch.routes"])
-        @_url_options.dup.tap do |options|
-          options[:script_name] = request.script_name.dup
-          options.freeze
+        if _routes.equal?(env["action_dispatch.routes"])
+          hash[:script_name] = request.script_name.dup
         end
-      else
-        @_url_options
+
+        hash.freeze
       end
     end
-
   end
 end

actionpack/lib/action_dispatch/http/url.rb

@@ -40,7 +40,9 @@ def url_for(options = {})
             rewritten_url << ":#{options.delete(:port)}" if options[:port]
           end
 
-          path = options.delete(:path) || ''
+          path = ""
+          path << options.delete(:script_name).to_s.chomp("/")
+          path << options.delete(:path).to_s
 
           params = options[:params] || {}
           params.reject! {|k,v| v.to_param.nil? }

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -446,7 +446,11 @@ def define_generate_prefix(app, name)
             _route = @set.named_routes.routes[name.to_sym]
             _routes = @set
             app.routes.define_mounted_helper(name)
-            app.routes.class_eval do
+            app.routes.singleton_class.class_eval do
+              define_method :mounted? do
+                true
+              end
+
               define_method :_generate_prefix do |options|
                 prefix_options = options.slice(*_route.segment_keys)
                 # we must actually delete prefix segment keys to avoid passing them to next url_for

actionpack/lib/action_dispatch/routing/route_set.rb

@@ -191,14 +191,50 @@ def define_url_helper(route, name, kind, options)
             selector = url_helper_name(name, kind)
             hash_access_method = hash_access_name(name, kind)
 
-            @module.module_eval <<-END_EVAL, __FILE__, __LINE__ + 1
-              remove_possible_method :#{selector}
-              def #{selector}(*args)
-                url_for(#{hash_access_method}(*args))
-              end
-            END_EVAL
+            if optimize_helper?(kind, route)
+              @module.module_eval <<-END_EVAL, __FILE__, __LINE__ + 1
+                remove_possible_method :#{selector}
+                def #{selector}(*args)
+                  if args.size == #{route.required_parts.size} && !args.last.is_a?(Hash) && optimize_routes_generation?
+                    options = #{options.inspect}.merge!(url_options)
+                    options[:path] = "#{optimized_helper(route)}"
+                    ActionDispatch::Http::URL.url_for(options)
+                  else
+                    url_for(#{hash_access_method}(*args))
+                  end
+                end
+              END_EVAL
+            else
+              @module.module_eval <<-END_EVAL, __FILE__, __LINE__ + 1
+                remove_possible_method :#{selector}
+                def #{selector}(*args)
+                  url_for(#{hash_access_method}(*args))
+                end
+              END_EVAL
+            end
+
             helpers << selector
           end
+
+          # Clause check about when we need to generate an optimized helper.
+          def optimize_helper?(kind, route) #:nodoc:
+            route.ast.grep(Journey::Nodes::Star).empty? && route.requirements.except(:controller, :action).empty?
+          end
+
+          # Generates the interpolation to be used in the optimized helper.
+          def optimized_helper(route)
+            string_route = route.ast.to_s
+
+            while string_route.gsub!(/\([^\)]*\)/, "")
+              true
+            end
+
+            route.required_parts.each_with_index do |part, i|
+              string_route.gsub!(part.inspect, "\#{Journey::Router::Utils.escape_fragment(args[#{i}].to_param)}")
+            end
+
+            string_route
+          end
       end
 
       attr_accessor :formatter, :set, :named_routes, :default_scope, :router
@@ -323,7 +359,7 @@ def url_helpers
             # Rails.application.routes.url_helpers.url_for(args)
             @_routes = routes
             class << self
-              delegate :url_for, :to => '@_routes'
+              delegate :url_for, :optimize_routes_generation?, :to => '@_routes'
             end
 
             # Make named_routes available in the module singleton
@@ -557,6 +593,14 @@ def generate(options, recall = {}, extras = false)
       RESERVED_OPTIONS = [:host, :protocol, :port, :subdomain, :domain, :tld_length,
                           :trailing_slash, :anchor, :params, :only_path, :script_name]
 
+      def mounted?
+        false
+      end
+
+      def optimize_routes_generation?
+        !mounted? && default_url_options.empty?
+      end
+
       def _generate_prefix(options = {})
         nil
       end
@@ -568,19 +612,17 @@ def url_for(options)
 
         user, password = extract_authentication(options)
         path_segments  = options.delete(:_path_segments)
-        script_name    = options.delete(:script_name)
-
-        path = (script_name.blank? ? _generate_prefix(options) : script_name.chomp('/')).to_s
+        script_name    = options.delete(:script_name).presence || _generate_prefix(options)
 
         path_options = options.except(*RESERVED_OPTIONS)
         path_options = yield(path_options) if block_given?
 
-        path_addition, params = generate(path_options, path_segments || {})
-        path << path_addition
+        path, params = generate(path_options, path_segments || {})
         params.merge!(options[:params] || {})
 
         ActionDispatch::Http::URL.url_for(options.merge!({
           :path => path,
+          :script_name => script_name,
           :params => params,
           :user => user,
           :password => password

actionpack/lib/action_dispatch/routing/url_for.rb

@@ -102,6 +102,9 @@ def initialize(*)
         super
       end
 
+      # Hook overriden in controller to add request information
+      # with `default_url_options`. Application logic should not
+      # go into url_options.
       def url_options
         default_url_options
       end
@@ -152,6 +155,11 @@ def url_for(options = nil)
 
       protected
 
+      def optimize_routes_generation?
+        return @_optimized_routes if defined?(@_optimized_routes)
+        @_optimized_routes = _routes.optimize_routes_generation? && default_url_options.empty?
+      end
+
       def _with_routes(routes)
         old_routes, @_routes = @_routes, routes
         yield

actionpack/lib/action_view/helpers/url_helper.rb

@@ -23,20 +23,25 @@ module UrlHelper
       include ActionDispatch::Routing::UrlFor
       include TagHelper
 
-      def _routes_context
-        controller
-      end
+      # We need to override url_optoins, _routes_context
+      # and optimize_routes_generation? to consider the controller.
 
-      # Need to map default url options to controller one.
-      # def default_url_options(*args) #:nodoc:
-      #   controller.send(:default_url_options, *args)
-      # end
-      #
-      def url_options
+      def url_options #:nodoc:
         return super unless controller.respond_to?(:url_options)
         controller.url_options
       end
 
+      def _routes_context #:nodoc:
+        controller
+      end
+      protected :_routes_context
+
+      def optimize_routes_generation? #:nodoc:
+        controller.respond_to?(:optimize_routes_generation?) ?
+          controller.optimize_routes_generation? : super
+      end
+      protected :optimize_routes_generation?
+
       # Returns the URL for the set of +options+ provided. This takes the
       # same options as +url_for+ in Action Controller (see the
       # documentation for <tt>ActionController::Base#url_for</tt>). Note that by default

actionpack/test/controller/base_test.rb

@@ -56,7 +56,7 @@ def from_view
   end
 
   def url_options
-    super.merge(:host => 'www.override.com', :action => 'new', :locale => 'en')
+    super.merge(:host => 'www.override.com')
   end
 end
 
@@ -183,9 +183,9 @@ def test_url_options_override
 
       get :from_view, :route => "from_view_url"
 
-      assert_equal 'http://www.override.com/from_view?locale=en', @response.body
-      assert_equal 'http://www.override.com/from_view?locale=en', @controller.send(:from_view_url)
-      assert_equal 'http://www.override.com/default_url_options/new?locale=en', @controller.url_for(:controller => 'default_url_options')
+      assert_equal 'http://www.override.com/from_view', @response.body
+      assert_equal 'http://www.override.com/from_view', @controller.send(:from_view_url)
+      assert_equal 'http://www.override.com/default_url_options/index', @controller.url_for(:controller => 'default_url_options')
     end
   end
 

actionpack/test/controller/routing_test.rb

@@ -59,11 +59,11 @@ def test_route_generation_allows_passing_non_string_values_to_generated_helper
 class MockController
   def self.build(helpers)
     Class.new do
-      def url_for(options)
+      def url_options
+        options = super
         options[:protocol] ||= "http"
         options[:host] ||= "test.host"
-
-        super(options)
+        options
       end
 
       include helpers

actionpack/test/template/sprockets_helper_with_routes_test.rb

@@ -17,7 +17,7 @@ class SprocketsHelperWithRoutesTest < ActionView::TestCase
 
   def setup
     super
-    @controller         = BasicController.new
+    @controller = BasicController.new
 
     @assets = Sprockets::Environment.new
     @assets.append_path(FIXTURES.join("sprockets/app/javascripts"))
@@ -34,7 +34,7 @@ def setup
 
   test "namespace conflicts on a named route called asset_path" do
     # Testing this for sanity - asset_path is now a named route!
-    assert_match asset_path('test_asset'), '/assets/test_asset'
+    assert_equal asset_path('test_asset'), '/assets/test_asset'
 
     assert_match %r{/assets/logo-[0-9a-f]+.png},
       path_to_asset("logo.png")