Merge branch 'mapper'

* mapper: (34 commits)
  no more is_a checks on instantiation
  Path::Pattern is instantiated internally, so make the contructor require a strexp object
  Strexp#names is only used in a test, so rm
  pass the parsed path from mapper to the Strexp
  add an alternate constructor to Strexp that takes a string
  ask the strexp for the ast
  remove dead code
  disconnect path from the instance
  reuse the ast we already made
  use a parser to extract the group parts from the path
  pass the parsed parameters through the methods so we don't reparse or require caching code
  "controllers" should be a valid path name
  controllers with slash names are also not supported, so we can reuse the message
  only validate controllers
  golf down a bit
  only error handling between controller and action is the same
  add a test for controllers without colons
  move nil check to a method that yields to a block if the value is not nil
  translate action / controller to the desired object
  only one nil check on the action variable
  ...

actionpack/lib/action_dispatch/journey/nodes/node.rb

@@ -93,6 +93,10 @@ def type; :GROUP; end
 
       class Star < Unary # :nodoc:
         def type; :STAR; end
+
+        def name
+          left.name.tr '*:', ''
+        end
       end
 
       class Binary < Node # :nodoc:

actionpack/lib/action_dispatch/journey/path/pattern.rb

@@ -1,27 +1,20 @@
+require 'action_dispatch/journey/router/strexp'
+
 module ActionDispatch
   module Journey # :nodoc:
     module Path # :nodoc:
       class Pattern # :nodoc:
         attr_reader :spec, :requirements, :anchored
 
+        def self.from_string string
+          new Journey::Router::Strexp.build(string, {}, ["/.?"], true)
+        end
+
         def initialize(strexp)
-          parser = Journey::Parser.new
-
-          @anchored = true
-
-          case strexp
-          when String
-            @spec         = parser.parse(strexp)
-            @requirements = {}
-            @separators   = "/.?"
-          when Router::Strexp
-            @spec         = parser.parse(strexp.path)
-            @requirements = strexp.requirements
-            @separators   = strexp.separators.join
-            @anchored     = strexp.anchor
-          else
-            raise ArgumentError, "Bad expression: #{strexp}"
-          end
+          @spec         = strexp.ast
+          @requirements = strexp.requirements
+          @separators   = strexp.separators.join
+          @anchored     = strexp.anchor
 
           @names          = nil
           @optional_names = nil

actionpack/lib/action_dispatch/journey/router/strexp.rb

@@ -6,18 +6,21 @@ class << self
           alias :compile :new
         end
 
-        attr_reader :path, :requirements, :separators, :anchor
+        attr_reader :path, :requirements, :separators, :anchor, :ast
 
-        def initialize(path, requirements, separators, anchor = true)
+        def self.build(path, requirements, separators, anchor = true)
+          parser = Journey::Parser.new
+          ast = parser.parse path
+          new ast, path, requirements, separators, anchor
+        end
+
+        def initialize(ast, path, requirements, separators, anchor = true)
+          @ast          = ast
           @path         = path
           @requirements = requirements
           @separators   = separators
           @anchor       = anchor
         end
-
-        def names
-          @path.scan(/:\w+/).map { |s| s.tr(':', '') }
-        end
       end
     end
   end

actionpack/lib/action_dispatch/routing/mapper.rb

@@ -62,24 +62,25 @@ def constraint_args(constraint, request)
       class Mapping #:nodoc:
         IGNORE_OPTIONS = [:to, :as, :via, :on, :constraints, :defaults, :only, :except, :anchor, :shallow, :shallow_path, :shallow_prefix, :format]
         ANCHOR_CHARACTERS_REGEX = %r{\A(\\A|\^)|(\\Z|\\z|\$)\Z}
-        WILDCARD_PATH = %r{\*([^/\)]+)\)?$}
 
-        attr_reader :scope, :path, :options, :requirements, :conditions, :defaults
+        attr_reader :scope, :options, :requirements, :conditions, :defaults
         attr_reader :to, :default_controller, :default_action
 
         def initialize(set, scope, path, options)
-          @set, @scope, @path = set, scope, path
+          @set, @scope = set, scope
           @requirements, @conditions, @defaults = {}, {}, {}
 
           options = scope[:options].merge(options) if scope[:options]
           @to                 = options[:to]
           @default_controller = options[:controller] || scope[:controller]
           @default_action     = options[:action] || scope[:action]
 
-          @options = normalize_options!(options)
-          normalize_path!
-          normalize_requirements!
-          normalize_conditions!
+          path = normalize_path! path, options[:format]
+          ast  = path_ast path
+          path_params = path_params ast
+          @options = normalize_options!(options, path_params, ast)
+          normalize_requirements!(path_params)
+          normalize_conditions!(path_params, path, ast)
           normalize_defaults!
         end
 
@@ -89,35 +90,33 @@ def to_route
 
         private
 
-          def normalize_path!
-            raise ArgumentError, "path is required" if @path.blank?
-            @path = Mapper.normalize_path(@path)
+          def normalize_path!(path, format)
+            raise ArgumentError, "path is required" if path.blank?
+            path = Mapper.normalize_path(path)
 
-            if required_format?
-              @path = "#{@path}.:format"
-            elsif optional_format?
-              @path = "#{@path}(.:format)"
+            if format == true
+              "#{path}.:format"
+            elsif optional_format?(path, format)
+              "#{path}(.:format)"
+            else
+              path
             end
           end
 
-          def required_format?
-            options[:format] == true
-          end
-
-          def optional_format?
-            options[:format] != false && !path.include?(':format') && !path.end_with?('/')
+          def optional_format?(path, format)
+            format != false && !path.include?(':format') && !path.end_with?('/')
           end
 
-          def normalize_options!(options)
-            path_without_format = path.sub(/\(\.:format\)$/, '')
-
+          def normalize_options!(options, path_params, path_ast)
             # Add a constraint for wildcard route to make it non-greedy and match the
             # optional format part of the route by default
-            if path_without_format.match(WILDCARD_PATH) && options[:format] != false
-              options[$1.to_sym] ||= /.+?/
+            if options[:format] != false
+              path_ast.grep(Journey::Nodes::Star) do |node|
+                options[node.name.to_sym] ||= /.+?/
+              end
             end
 
-            if path_without_format.match(':controller')
+            if path_params.include?(:controller)
               raise ArgumentError, ":controller segment is not allowed within a namespace block" if scope[:module]
 
               # Add a default constraint for :controller path segments that matches namespaced
@@ -127,12 +126,16 @@ def normalize_options!(options)
               options[:controller] ||= /.+?/
             end
 
-            options.merge!(default_controller_and_action)
+            if to.respond_to? :call
+              options
+            else
+              options.merge!(default_controller_and_action(path_params))
+            end
           end
 
-          def normalize_requirements!
+          def normalize_requirements!(path_params)
             constraints.each do |key, requirement|
-              next unless segment_keys.include?(key) || key == :controller
+              next unless path_params.include?(key) || key == :controller
               verify_regexp_requirement(requirement) if requirement.is_a?(Regexp)
               @requirements[key] = requirement
             end
@@ -189,18 +192,19 @@ def verify_callable_constraint(callable_constraint)
             end
           end
 
-          def normalize_conditions!
+          def normalize_conditions!(path_params, path, ast)
             @conditions[:path_info] = path
+            @conditions[:parsed_path_info] = ast
 
             constraints.each do |key, condition|
-              unless segment_keys.include?(key) || key == :controller
+              unless path_params.include?(key) || key == :controller
                 @conditions[key] = condition
               end
             end
 
             required_defaults = []
             options.each do |key, required_default|
-              unless segment_keys.include?(key) || IGNORE_OPTIONS.include?(key) || Regexp === required_default
+              unless path_params.include?(key) || IGNORE_OPTIONS.include?(key) || Regexp === required_default
                 required_defaults << key
               end
             end
@@ -236,55 +240,61 @@ def app
             end
           end
 
-          def default_controller_and_action
-            if to.respond_to?(:call)
-              { }
-            else
-              if to.is_a?(String)
-                controller, action = to.split('#')
-              elsif to.is_a?(Symbol)
-                action = to.to_s
-              end
-
-              controller ||= default_controller
-              action     ||= default_action
+          def default_controller_and_action(path_params)
+            controller, action = get_controller_and_action(default_controller,
+              default_action,
+              to,
+              @scope[:module]
+            )
 
-              if @scope[:module] && !controller.is_a?(Regexp)
-                if controller =~ %r{\A/}
-                  controller = controller[1..-1]
-                else
-                  controller = [@scope[:module], controller].compact.join("/").presence
-                end
-              end
+            hash = check_part(:controller, controller, path_params, {}) do |part|
+              translate_controller(part) {
+                message = "'#{part}' is not a supported controller name. This can lead to potential routing problems."
+                message << " See http://guides.rubyonrails.org/routing.html#specifying-a-controller-to-use"
 
-              if controller.is_a?(String) && controller =~ %r{\A/}
-                raise ArgumentError, "controller name should not start with a slash"
-              end
+                raise ArgumentError, message
+              }
+            end
 
-              controller = controller.to_s unless controller.is_a?(Regexp)
-              action     = action.to_s     unless action.is_a?(Regexp)
+            check_part(:action, action, path_params, hash) { |part|
+              part.is_a?(Regexp) ? part : part.to_s
+            }
+          end
 
-              if controller.blank? && segment_keys.exclude?(:controller)
-                message = "Missing :controller key on routes definition, please check your routes."
+          def check_part(name, part, path_params, hash)
+            if part
+              hash[name] = yield(part)
+            else
+              unless path_params.include?(name)
+                message = "Missing :#{name} key on routes definition, please check your routes."
                 raise ArgumentError, message
               end
+            end
+            hash
+          end
 
-              if action.blank? && segment_keys.exclude?(:action)
-                message = "Missing :action key on routes definition, please check your routes."
-                raise ArgumentError, message
-              end
+          def get_controller_and_action(controller, action, to, modyoule)
+            case to
+            when Symbol then action = to.to_s
+            when /#/    then controller, action = to.split('#')
+            when String then controller = to
+            end
 
-              if controller.is_a?(String) && controller !~ /\A[a-z_0-9\/]*\z/
-                message = "'#{controller}' is not a supported controller name. This can lead to potential routing problems."
-                message << " See http://guides.rubyonrails.org/routing.html#specifying-a-controller-to-use"
-                raise ArgumentError, message
+            if modyoule && !controller.is_a?(Regexp)
+              if controller =~ %r{\A/}
+                controller = controller[1..-1]
+              else
+                controller = [modyoule, controller].compact.join("/")
               end
-
-              hash = {}
-              hash[:controller] = controller unless controller.blank?
-              hash[:action]     = action unless action.blank?
-              hash
             end
+            [controller, action]
+          end
+
+          def translate_controller(controller)
+            return controller if Regexp === controller
+            return controller.to_s if controller =~ /\A[a-z_0-9][a-z_0-9\/]*\z/
+
+            yield
           end
 
           def blocks
@@ -307,16 +317,13 @@ def constraints
             end
           end
 
-          def segment_keys
-            @segment_keys ||= path_pattern.names.map{ |s| s.to_sym }
-          end
-
-          def path_pattern
-            Journey::Path::Pattern.new(strexp)
+          def path_params(ast)
+            ast.grep(Journey::Nodes::Symbol).map { |n| n.name.to_sym }
           end
 
-          def strexp
-            Journey::Router::Strexp.compile(path, requirements, SEPARATORS)
+          def path_ast(path)
+            parser = Journey::Parser.new
+            parser.parse path
           end
 
           def dispatcher

actionpack/lib/action_dispatch/routing/route_set.rb

@@ -418,16 +418,19 @@ def add_route(app, conditions = {}, requirements = {}, defaults = {}, name = nil
             "http://guides.rubyonrails.org/routing.html#restricting-the-routes-created"
         end
 
-        path = build_path(conditions.delete(:path_info), requirements, SEPARATORS, anchor)
+        path = conditions.delete :path_info
+        ast  = conditions.delete :parsed_path_info
+        path = build_path(path, ast, requirements, SEPARATORS, anchor)
         conditions = build_conditions(conditions, path.names.map { |x| x.to_sym })
 
         route = @set.add_route(app, path, conditions, defaults, name)
         named_routes[name] = route if name
         route
       end
 
-      def build_path(path, requirements, separators, anchor)
+      def build_path(path, ast, requirements, separators, anchor)
         strexp = Journey::Router::Strexp.new(
+            ast,
             path,
             requirements,
             SEPARATORS,

actionpack/test/dispatch/mapper_test.rb

@@ -72,15 +72,15 @@ def test_map_wildcard_with_other_element
         mapper = Mapper.new fakeset
         mapper.get '/*path/foo/:bar', :to => 'pages#show'
         assert_equal '/*path/foo/:bar(.:format)', fakeset.conditions.first[:path_info]
-        assert_nil fakeset.requirements.first[:path]
+        assert_equal(/.+?/, fakeset.requirements.first[:path])
       end
 
       def test_map_wildcard_with_multiple_wildcard
         fakeset = FakeSet.new
         mapper = Mapper.new fakeset
         mapper.get '/*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[:foo])
         assert_equal(/.+?/, fakeset.requirements.first[:bar])
       end