Skip to content

Commit

Permalink
Report all missing path parameters, not just the first
Browse files Browse the repository at this point in the history
This makes it possible to take remedial action such as copying the
parameters from the parent.

Even without this change, it is no longer necessary to symbolize the
keys or duplicate the attributes in request_path.
  • Loading branch information
chewi committed Nov 27, 2015
1 parent 8fcb92e commit 282aecd
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 18 deletions.
6 changes: 3 additions & 3 deletions lib/her/errors.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
module Her
module Errors
class PathError < StandardError
attr_reader :missing_parameter
attr_reader :missing_parameters

def initialize(message, missing_parameter=nil)
def initialize(message, missing_parameters=nil)
super(message)
@missing_parameter = missing_parameter
@missing_parameters = missing_parameters
end
end

Expand Down
3 changes: 2 additions & 1 deletion lib/her/model/introspection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ def inspect
resource_path = begin
request_path
rescue Her::Errors::PathError => e
"<unknown path, missing `#{e.missing_parameter}`>"
joined = e.missing_parameters.map { |m| "`#{m}`" }.join(", ")
"<unknown path, missing #{joined}>"
end

"#<#{self.class}(#{resource_path}) #{attributes.keys.map { |k| "#{k}=#{attribute_for_inspect(send(k))}" }.join(" ")}>"
Expand Down
24 changes: 14 additions & 10 deletions lib/her/model/paths.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module Paths
# @param [Hash] params An optional set of additional parameters for
# path construction. These will not override attributes of the resource.
def request_path(params = {})
self.class.build_request_path(params.merge(attributes.dup))
self.class.build_request_path(params.merge(attributes))
end

module ClassMethods
Expand Down Expand Up @@ -104,15 +104,19 @@ def build_request_path(path=nil, parameters={})
path.gsub!(/(\A|\/):id(\Z|\/)/, "\\1:#{primary_key}\\2")
end

path.gsub(/:([\w_]+)/) do
# Look for :key or :_key, otherwise raise an exception
key = $1.to_sym
value = parameters.delete(key) || parameters.delete(:"_#{key}")
if value
Faraday::Utils.escape value
else
raise(Her::Errors::PathError.new("Missing :_#{$1} parameter to build the request path. Path is `#{path}`. Parameters are `#{parameters.symbolize_keys.inspect}`.", $1))
end
missing = []

result = path.gsub(/:([\w_]+)/) do
# Look for "key" or "_key", otherwise add to the missing list and raise below
value = parameters[$1] || parameters["_#{$1}"] || (missing.push($1) && $1)
Faraday::Utils.escape value
end

if missing.empty?
result
else
joined = missing.map { |m| ":_#{m}" }.join(", ")
raise Her::Errors::PathError.new("Missing #{joined} parameters to build the request path. Path is `#{path}`. Parameters are `#{parameters.symbolize_keys.inspect}`.", missing)
end
end

Expand Down
8 changes: 4 additions & 4 deletions spec/model/paths_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@
end

it "raises exceptions when building a path without required custom variables" do
Foo::User.collection_path "/organizations/:organization_id/utilisateurs"
expect { Foo::User.new(:id => "foo").request_path }.to raise_error(Her::Errors::PathError, "Missing :_organization_id parameter to build the request path. Path is `/organizations/:organization_id/utilisateurs/:id`. Parameters are `{:id=>\"foo\"}`.")
Foo::User.collection_path "/organizations/:organization_id/departments/:department_id/utilisateurs"
expect { Foo::User.new(:id => "foo").request_path }.to raise_error(Her::Errors::PathError, "Missing :_organization_id, :_department_id parameters to build the request path. Path is `/organizations/:organization_id/departments/:department_id/utilisateurs/:id`. Parameters are `{:id=>\"foo\"}`.")
end

it "escapes the variable values" do
Expand Down Expand Up @@ -121,12 +121,12 @@

it "raises exceptions when building a path without required custom variables" do
Foo::AdminUser.collection_path "/organizations/:organization_id/users"
expect { Foo::AdminUser.new(:id => "foo").request_path }.to raise_error(Her::Errors::PathError, "Missing :_organization_id parameter to build the request path. Path is `/organizations/:organization_id/users/:id`. Parameters are `{:id=>\"foo\"}`.")
expect { Foo::AdminUser.new(:id => "foo").request_path }.to raise_error(Her::Errors::PathError, "Missing :_organization_id parameters to build the request path. Path is `/organizations/:organization_id/users/:id`. Parameters are `{:id=>\"foo\"}`.")
end

it "raises exceptions when building a relative path without required custom variables" do
Foo::AdminUser.collection_path "organizations/:organization_id/users"
expect { Foo::AdminUser.new(:id => "foo").request_path }.to raise_error(Her::Errors::PathError, "Missing :_organization_id parameter to build the request path. Path is `organizations/:organization_id/users/:id`. Parameters are `{:id=>\"foo\"}`.")
expect { Foo::AdminUser.new(:id => "foo").request_path }.to raise_error(Her::Errors::PathError, "Missing :_organization_id parameters to build the request path. Path is `organizations/:organization_id/users/:id`. Parameters are `{:id=>\"foo\"}`.")
end
end
end
Expand Down

0 comments on commit 282aecd

Please sign in to comment.