Symbol captures may generate multiple path segments, so don't escape …

…/. Test splat escaping. Segregate URI path and fragment escaping utils.

lib/journey/router/utils.rb

@@ -19,18 +19,34 @@ def self.normalize_path(path)
         path
       end
 
-      RESERVED_PCHAR = ':@&=+$,;%'
-      SAFE_PCHAR = "#{URI::REGEXP::PATTERN::UNRESERVED}#{RESERVED_PCHAR}"
-      if RUBY_VERSION >= '1.9'
-        UNSAFE_PCHAR = Regexp.new("[^#{SAFE_PCHAR}]", false).freeze
-      else
-        UNSAFE_PCHAR = Regexp.new("[^#{SAFE_PCHAR}]", false, 'N').freeze
+      # URI path and fragment escaping
+      # http://tools.ietf.org/html/rfc3986
+      module UriEscape
+        # Symbol captures can generate multiple path segments, so include /.
+        reserved_segment  = '/'
+        reserved_fragment = '/?'
+        reserved_pchar    = ':@&=+$,;%'
+
+        safe_pchar    = "#{URI::REGEXP::PATTERN::UNRESERVED}#{reserved_pchar}"
+        safe_segment  = "#{safe_pchar}#{reserved_segment}"
+        safe_fragment = "#{safe_pchar}#{reserved_fragment}"
+        if RUBY_VERSION >= '1.9'
+          UNSAFE_SEGMENT  = Regexp.new("[^#{safe_segment}]", false).freeze
+          UNSAFE_FRAGMENT = Regexp.new("[^#{safe_fragment}]", false).freeze
+        else
+          UNSAFE_SEGMENT = Regexp.new("[^#{safe_segment}]", false, 'N').freeze
+          UNSAFE_FRAGMENT = Regexp.new("[^#{safe_fragment}]", false, 'N').freeze
+        end
       end
 
       Parser = URI.const_defined?(:Parser) ? URI::Parser.new : URI
 
-      def self.escape_uri(uri)
-        Parser.escape(uri.to_s, UNSAFE_PCHAR)
+      def self.escape_path(path)
+        Parser.escape(path.to_s, UriEscape::UNSAFE_SEGMENT)
+      end
+
+      def self.escape_fragment(fragment)
+        Parser.escape(fragment.to_s, UriEscape::UNSAFE_FRAGMENT)
       end
 
       def self.unescape_uri(uri)

lib/journey/visitors.rb

@@ -102,7 +102,7 @@ def visit_SYMBOL node
         if options.key? key
           value = options[key]
           consumed[key] = value
-          Router::Utils.escape_uri(value)
+          Router::Utils.escape_path(value)
         else
           "\0"
         end

test/router/test_utils.rb

@@ -3,8 +3,16 @@
 module Journey
   class Router
     class TestUtils < MiniTest::Unit::TestCase
-      def test_cgi_escape
-        assert_equal "a%2Fb", Utils.escape_uri("a/b")
+      def test_path_escape
+        assert_equal "a/b%20c+d", Utils.escape_path("a/b c+d")
+      end
+
+      def test_fragment_escape
+        assert_equal "a/b%20c+d?e", Utils.escape_fragment("a/b c+d?e")
+      end
+
+      def test_uri_unescape
+        assert_equal "a/b c+d", Utils.unescape_uri("a%2Fb%20c+d")
       end
     end
   end

test/test_router.rb

@@ -262,7 +262,7 @@ def test_generate_escapes
         nil, { :controller        => "tasks",
                :action            => "a/b c+d",
       }, {})
-      assert_equal '/tasks/a%2Fb%20c+d', path
+      assert_equal '/tasks/a/b%20c+d', path
     end
 
     def test_generate_extra_params
@@ -307,10 +307,31 @@ def test_generate_with_name
       '/content'          => { :controller => 'content' },
       '/content/list'     => { :controller => 'content', :action => 'list' },
       '/content/show/10'  => { :controller => 'content', :action => 'show', :id => "10" },
-      '/content/show/10/a%2Fb%20c+d' => { :controller => 'content', :action => 'show', :id => "10", :slug => "a/b c+d" },
     }.each do |request_path, expected|
       define_method("test_recognize_#{expected.keys.map(&:to_s).join('_')}") do
-        path  = Path::Pattern.new "/:controller(/:action(/:id(/:slug)))"
+        path  = Path::Pattern.new "/:controller(/:action(/:id))"
+        app   = Object.new
+        route = @router.routes.add_route(app, path, {}, {}, {})
+
+        env = rails_env 'PATH_INFO' => request_path
+        called   = false
+
+        @router.recognize(env) do |r, _, params|
+          assert_equal route, r
+          assert_equal(expected, params)
+          called = true
+        end
+
+        assert called
+      end
+    end
+
+    {
+      :segment => ['/a%2Fb%20c+d/splat', { :segment => 'a/b c+d', :splat => 'splat'   }],
+      :splat   => ['/segment/a/b%20c+d', { :segment => 'segment', :splat => 'a/b c+d' }]
+    }.each do |name, (request_path, expected)|
+      define_method("test_recognize_#{name}") do
+        path  = Path::Pattern.new '/:segment/*splat'
         app   = Object.new
         route = @router.routes.add_route(app, path, {}, {}, {})