Skip to content

Commit

Permalink
Nicer error when specifying non-nil non-string opt value
Browse files Browse the repository at this point in the history
Previously, if you specified a non-nil non-string opt value, like a
symbol for `idempotency_key`, you'd get a pretty user-unfriendly error
from `Net::HTTP`:

```
/Users/brandur/.rbenv/versions/2.4.5/lib/ruby/2.4.0/net/http/header.rb:21:in `block in initialize_http_header': undefined method `strip' for :foo:Symbol (NoMethodError)
```

Here, we introduce a new argument error that makes it a little easier
for someone to read. The impetus for the change is that we had an
internal product quality report where someone ran into this and was
confused.

I'm pretty sure this change is backward compatible because `Net::HTTP`
would call `strip` on anything that was passed in as a value, and
generally just strings would support that. There may be some other less
common data type that was accidentally compatible that someone was
using, but that case should be quite unusual.
  • Loading branch information
brandur committed Oct 4, 2019
1 parent 8fc0f70 commit f4ca68f
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 1 deletion.
18 changes: 17 additions & 1 deletion lib/stripe/api_operations/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ def request(method, url, params = {}, opts = {})
warn_on_opts_in_params(params)

opts = Util.normalize_opts(opts)
error_on_non_string_user_opts(opts)

opts[:client] ||= StripeClient.active_client

headers = opts.clone
Expand All @@ -31,10 +33,24 @@ def request(method, url, params = {}, opts = {})
[resp, opts_to_persist]
end

private def error_on_non_string_user_opts(opts)
Util::OPTS_USER_SPECIFIED.each do |opt|
next unless opts.key?(opt)

val = opts[opt]
next if val.nil?
next if val.is_a?(String)

raise ArgumentError,
"request option '#{opt}' should be a string value " \
"(was a #{val.class})"
end
end

private def warn_on_opts_in_params(params)
Util::OPTS_USER_SPECIFIED.each do |opt|
if params.key?(opt)
warn("WARNING: #{opt} should be in opts instead of params.")
warn("WARNING: '#{opt}' should be in opts instead of params.")
end
end
end
Expand Down
18 changes: 18 additions & 0 deletions test/stripe/api_resource_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,24 @@ class NestedTestAPIResource < APIResource
end
end

should "error if a user-specified opt is given a non-nil non-string value" do
stub_request(:post, "#{Stripe.api_base}/v1/charges")
.to_return(body: JSON.generate(charge_fixture))

# Works fine if not included, `nil`, or a string.
Stripe::Charge.create({ amount: 100, currency: "usd" }, {})
Stripe::Charge.create({ amount: 100, currency: "usd" }, idempotency_key: nil)
Stripe::Charge.create({ amount: 100, currency: "usd" }, idempotency_key: "12345")

# Errors on a non-string.
e = assert_raises(ArgumentError) do
Stripe::Charge.create({ amount: 100, currency: "usd" }, idempotency_key: :foo)
end
assert_equal "request option 'idempotency_key' should be a string value " \
"(was a Symbol)",
e.message
end

should "requesting with a unicode ID should result in a request" do
stub_request(:get, "#{Stripe.api_base}/v1/customers/%E2%98%83")
.to_return(body: JSON.generate(make_missing_id_error), status: 404)
Expand Down

0 comments on commit f4ca68f

Please sign in to comment.