1 files changed, 76 insertions, 56 deletions

diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 3ae9f92c0b..e699419f23 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -1,11 +1,13 @@
 require 'action_dispatch/journey'
 require 'forwardable'
 require 'thread_safe'
+require 'active_support/concern'
 require 'active_support/core_ext/object/to_query'
 require 'active_support/core_ext/hash/slice'
 require 'active_support/core_ext/module/remove_method'
 require 'active_support/core_ext/array/extract_options'
 require 'action_controller/metal/exceptions'
+require 'action_dispatch/http/request'
 
 module ActionDispatch
   module Routing
@@ -28,7 +30,7 @@ module ActionDispatch
         def call(env)
           params = env[PARAMETERS_KEY]
 
-          # If any of the path parameters has a invalid encoding then
+          # If any of the path parameters has an invalid encoding then
           # raise since it's likely to trigger errors further on.
           params.each do |key, value|
             next unless value.respond_to?(:valid_encoding?)
@@ -155,7 +157,7 @@ module ActionDispatch
           end
 
           def self.optimize_helper?(route)
-            route.requirements.except(:controller, :action).empty?
+            !route.glob? && route.path.requirements.empty?
           end
 
           class OptimizedUrlHelper < UrlHelper # :nodoc:
@@ -163,15 +165,15 @@ module ActionDispatch
 
             def initialize(route, options)
               super
-              @path_parts   = @route.required_parts
-              @arg_size     = @path_parts.size
-              @string_route = @route.optimized_path
+              @klass          = Journey::Router::Utils
+              @required_parts = @route.required_parts
+              @arg_size       = @required_parts.size
+              @optimized_path = @route.optimized_path
             end
 
             def call(t, args)
               if args.size == arg_size && !args.last.is_a?(Hash) && optimize_routes_generation?(t)
-                options = @options.dup
-                options.merge!(t.url_options) if t.respond_to?(:url_options)
+                options = t.url_options.merge @options
                 options[:path] = optimized_helper(args)
                 ActionDispatch::Http::URL.url_for(options)
               else
@@ -182,41 +184,60 @@ module ActionDispatch
             private
 
             def optimized_helper(args)
-              path = @string_route.dup
-              klass = Journey::Router::Utils
-
-              @path_parts.zip(args) do |part, arg|
-                # Replace each route parameter
-                # e.g. :id for regular parameter or *path for globbing
-                # with ruby string interpolation code
-                path.gsub!(/(\*|:)#{part}/, klass.escape_fragment(arg.to_param))
+              params = Hash[parameterize_args(args)]
+              missing_keys = missing_keys(params)
+
+              unless missing_keys.empty?
+                raise_generation_error(params, missing_keys)
               end
-              path
+
+              @optimized_path.map{ |segment| replace_segment(params, segment) }.join
+            end
+
+            def replace_segment(params, segment)
+              Symbol === segment ? @klass.escape_segment(params[segment]) : segment
             end
 
             def optimize_routes_generation?(t)
               t.send(:optimize_routes_generation?)
             end
+
+            def parameterize_args(args)
+              @required_parts.zip(args.map(&:to_param))
+            end
+
+            def missing_keys(args)
+              args.select{ |part, arg| arg.nil? || arg.empty? }.keys
+            end
+
+            def raise_generation_error(args, missing_keys)
+              constraints = Hash[@route.requirements.merge(args).sort]
+              message = "No route matches #{constraints.inspect}"
+              message << " missing required keys: #{missing_keys.sort.inspect}"
+
+              raise ActionController::UrlGenerationError, message
+            end
           end
 
           def initialize(route, options)
             @options      = options
-            @segment_keys = route.segment_keys
+            @segment_keys = route.segment_keys.uniq
             @route        = route
           end
 
           def call(t, args)
-            t.url_for(handle_positional_args(t, args, @options, @segment_keys))
+            options = t.url_options.merge @options
+            hash = handle_positional_args(t, args, options, @segment_keys)
+            t._routes.url_for(hash)
           end
 
-          def handle_positional_args(t, args, options, keys)
+          def handle_positional_args(t, args, result, keys)
             inner_options = args.extract_options!
-            result = options.dup
 
             if args.size > 0
               if args.size < keys.size - 1 # take format into account
-                keys -= t.url_options.keys if t.respond_to?(:url_options)
-                keys -= options.keys
+                keys -= t.url_options.keys
+                keys -= result.keys
               end
               keys -= inner_options.keys
               result.merge!(Hash[keys.zip(args)])
@@ -336,7 +357,7 @@ module ActionDispatch
         include UrlFor
       end
 
-      # Contains all the mounted helpers accross different
+      # Contains all the mounted helpers across different
       # engines and the `main_app` helper for the application.
       # You can include this in your classes if you want to
       # access routes for other engines.
@@ -374,6 +395,8 @@ module ActionDispatch
             @_routes = routes
             class << self
               delegate :url_for, :optimize_routes_generation?, :to => '@_routes'
+              attr_reader :_routes
+              def url_options; {}; end
             end
 
             # Make named_routes available in the module singleton
@@ -489,11 +512,12 @@ module ActionDispatch
           @recall      = recall.dup
           @set         = set
 
+          normalize_recall!
           normalize_options!
           normalize_controller_action_id!
           use_relative_controller!
           normalize_controller!
-          handle_nil_action!
+          normalize_action!
         end
 
         def controller
@@ -512,6 +536,11 @@ module ActionDispatch
           end
         end
 
+        # Set 'index' as default action for recall
+        def normalize_recall!
+          @recall[:action] ||= 'index'
+        end
+
         def normalize_options!
           # If an explicit :controller was given, always make :action explicit
           # too, so that action expiry works as expected for things like
@@ -527,8 +556,8 @@ module ActionDispatch
             options[:controller]   = options[:controller].to_s
           end
 
-          if options[:action]
-            options[:action] = options[:action].to_s
+          if options.key?(:action)
+            options[:action] = (options[:action] || 'index').to_s
           end
         end
 
@@ -538,8 +567,6 @@ module ActionDispatch
         # :controller, :action or :id is not found, don't pull any
         # more keys from the recall.
         def normalize_controller_action_id!
-          @recall[:action] ||= 'index' if current_controller
-
           use_recall_for(:controller) or return
           use_recall_for(:action) or return
           use_recall_for(:id)
@@ -561,13 +588,11 @@ module ActionDispatch
           @options[:controller] = controller.sub(%r{^/}, '') if controller
         end
 
-        # This handles the case of action: nil being explicitly passed.
-        # It is identical to action: "index"
-        def handle_nil_action!
-          if options.has_key?(:action) && options[:action].nil?
-            options[:action] = 'index'
+        # Move 'index' action from options to recall
+        def normalize_action!
+          if @options[:action] == 'index'
+            @recall[:action] = @options.delete(:action)
           end
-          recall[:action] = options.delete(:action) if options[:action] == 'index'
         end
 
         # Generates a path from routes, returns [path, params].
@@ -618,28 +643,34 @@ module ActionDispatch
         !mounted? && default_url_options.empty?
       end
 
-      def _generate_prefix(options = {})
-        nil
+      def find_script_name(options)
+        options.delete :script_name
       end
 
-      # The +options+ argument must be +nil+ or a hash whose keys are *symbols*.
+      # The +options+ argument must be a hash whose keys are *symbols*.
       def url_for(options)
-        options = default_url_options.merge(options || {})
+        options = default_url_options.merge options
+
+        user = password = nil
+
+        if options[:user] && options[:password]
+          user     = options.delete :user
+          password = options.delete :password
+        end
 
-        user, password = extract_authentication(options)
-        recall  = options.delete(:_recall)
+        recall  = options.delete(:_recall) { {} }
 
-        original_script_name = options.delete(:original_script_name).presence
-        script_name = options.delete(:script_name).presence || _generate_prefix(options)
+        original_script_name = options.delete(:original_script_name)
+        script_name = find_script_name options
 
         if script_name && original_script_name
           script_name = original_script_name + script_name
         end
 
-        path_options = options.except(*RESERVED_OPTIONS)
-        path_options = yield(path_options) if block_given?
+        path_options = options.dup
+        RESERVED_OPTIONS.each { |ro| path_options.delete ro }
 
-        path, params = generate(path_options, recall || {})
+        path, params = generate(path_options, recall)
         params.merge!(options[:params] || {})
 
         ActionDispatch::Http::URL.url_for(options.merge!({
@@ -694,17 +725,6 @@ module ActionDispatch
 
         raise ActionController::RoutingError, "No route matches #{path.inspect}"
       end
-
-      private
-
-        def extract_authentication(options)
-          if options[:user] && options[:password]
-            [options.delete(:user), options.delete(:password)]
-          else
-            nil
-          end
-        end
-
     end
   end
 end