Skip to content

Commit

Permalink
Fix marking of custom routes for Journey
Browse files Browse the repository at this point in the history
The Mapper build_path method marks routes where path parameters are part
of a path segment as custom routes by altering the regular expression, e.g:

    get '/foo-:bar', to: 'foo#bar'

There were some edge cases where certain constructs weren't being picked
up and this commit fixes those.

Fixes #23069.
  • Loading branch information
pixeltrix committed Jan 20, 2016
1 parent 4b507df commit 1eace94
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 14 deletions.
34 changes: 20 additions & 14 deletions actionpack/lib/action_dispatch/routing/mapper.rb
Expand Up @@ -184,26 +184,32 @@ def request_method
def build_path(ast, requirements, anchor)
pattern = Journey::Path::Pattern.new(ast, requirements, JOINED_SEPARATORS, anchor)

# Get all the symbol nodes followed by literals that are not the
# dummy node.
symbols = ast.find_all { |n|
n.cat? && n.left.symbol? && n.right.cat? && n.right.left.literal?
}.map(&:left)

# Get all the symbol nodes preceded by literals.
symbols.concat ast.find_all { |n|
n.cat? && n.left.literal? && n.right.cat? && n.right.left.symbol?
}.map { |n| n.right.left }

symbols.each { |x|
x.regexp = /(?:#{Regexp.union(x.regexp, '-')})+/
# Find all the symbol nodes that are adjacent to literal nodes and alter
# the regexp so that Journey will partition them into custom routes.
ast.find_all { |node|
next unless node.cat?

if node.left.literal? && node.right.symbol?
symbol = node.right
elsif node.left.literal? && node.right.cat? && node.right.left.symbol?
symbol = node.right.left
elsif node.left.symbol? && node.right.literal?
symbol = node.left
elsif node.left.symbol? && node.right.cat? && node.right.left.literal?
symbol = node.left
else
next
end

if symbol
symbol.regexp = /(?:#{Regexp.union(symbol.regexp, '-')})+/
end
}

pattern
end
private :build_path


private
def add_wildcard_options(options, formatted, path_ast)
# Add a constraint for wildcard route to make it non-greedy and match the
Expand Down
63 changes: 63 additions & 0 deletions actionpack/test/dispatch/routing_test.rb
Expand Up @@ -4654,3 +4654,66 @@ def test_legit_routing_not_found_responses
assert_equal 404, response.status
end
end

class TestPartialDynamicPathSegments < ActionDispatch::IntegrationTest
Routes = ActionDispatch::Routing::RouteSet.new
Routes.draw do
ok = lambda { |env| [200, { 'Content-Type' => 'text/plain' }, []] }

get '/songs/song-:song', to: ok
get '/songs/:song-song', to: ok
get '/:artist/song-:song', to: ok
get '/:artist/:song-song', to: ok

get '/optional/songs(/song-:song)', to: ok
get '/optional/songs(/:song-song)', to: ok
get '/optional/:artist(/song-:song)', to: ok
get '/optional/:artist(/:song-song)', to: ok
end

APP = build_app Routes

def app
APP
end

def test_paths_with_partial_dynamic_segments_are_recognised
get '/david-bowie/changes-song'
assert_equal 200, response.status
assert_params artist: 'david-bowie', song: 'changes'

get '/david-bowie/song-changes'
assert_equal 200, response.status
assert_params artist: 'david-bowie', song: 'changes'

get '/songs/song-changes'
assert_equal 200, response.status
assert_params song: 'changes'

get '/songs/changes-song'
assert_equal 200, response.status
assert_params song: 'changes'

get '/optional/songs/song-changes'
assert_equal 200, response.status
assert_params song: 'changes'

get '/optional/songs/changes-song'
assert_equal 200, response.status
assert_params song: 'changes'

get '/optional/david-bowie/changes-song'
assert_equal 200, response.status
assert_params artist: 'david-bowie', song: 'changes'

get '/optional/david-bowie/song-changes'
assert_equal 200, response.status
assert_params artist: 'david-bowie', song: 'changes'
end

private

def assert_params(params)
assert_equal(params, request.path_parameters)
end
end

0 comments on commit 1eace94

Please sign in to comment.