Skip to content

Commit

Permalink
Fix failing test missed for the past year :(
Browse files Browse the repository at this point in the history 
When optimized path helpers were re-introduced in d7014bc the test added
in a328f2f broke but no-one noticed because it wasn't being run by the
test suite.

Fix the test by checking for nil values or empty strings after the args
have been parameterized.
  • Loading branch information
@pixeltrix
pixeltrix committed Jul 17, 2013
1 parent 96310f6 commit 74722d6
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 3 deletions.
19 changes: 17 additions & 2 deletions actionpack/lib/action_dispatch/routing/route_set.rb
View file
Expand Up @@ -184,12 +184,27 @@ def call(t, args)
def optimized_helper(args)
path = @string_route.dup
klass = Journey::Router::Utils
parameterized_args = args.map(&:to_param)
missing_keys = []

@path_parts.zip(args) do |part, arg|
parameterized_args.each_with_index do |arg, index|
if arg.nil? || arg.empty?
missing_keys << @path_parts[index]
end
end

unless missing_keys.empty?
message = "No route matches #{Hash[@path_parts.zip(args)].inspect}"
message << " missing required keys: #{missing_keys.inspect}"

raise ActionController::UrlGenerationError, message
end

@path_parts.zip(parameterized_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))
path.gsub!(/(\*|:)#{part}/, klass.escape_fragment(arg))
end
path
end
Expand Down
2 changes: 1 addition & 1 deletion actionpack/test/routing/helper_test.rb
View file
Expand Up @@ -22,7 +22,7 @@ def test_exception
x = Class.new {
include rs.url_helpers
}
assert_raises ActionController::RoutingError do
assert_raises ActionController::UrlGenerationError do
x.new.pond_duck_path Duck.new
end
end
Expand Down

2 comments on commit 74722d6

@egilburg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest extracting the key validation to a separate method for better readability, e.g.:

def optimized_helper(args)
  path = @string_route.dup
  klass = Journey::Router::Utils

  parameterized_args = args.map(&:to_param)
  ensure_arg_keys!(parameterized_args)

  @path_parts.zip(parameterized_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))
  end
  path
end

def ensure_arg_keys!(args)
  missing_keys = []

  args.each_with_index do |arg, index|
    if arg.nil? || arg.empty?
      missing_keys << @path_parts[index]
    end
  end

  unless missing_keys.empty?
    message = "No route matches #{Hash[@path_parts.zip(args)].inspect}"
    message << " missing required keys: #{missing_keys.inspect}"

    raise ActionController::UrlGenerationError, message
  end

  args
end

@pixeltrix
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@egilburg I've refactored it so that we only build the missing_keys once we've detected a problem - see here: 1a58ac6

Hopefully, you approve 😄

Please sign in to comment.