From e88964699e8582efdcb14fc0c168e14df5abe024 Mon Sep 17 00:00:00 2001
From: Joel Taylor <joel@codeprivy.com>
Date: Fri, 25 Oct 2019 14:03:23 -0700
Subject: [PATCH] Raise an error when requests params are invalid

There are two kinds of API operations: collection and element specific.
The signature between the two is slightly different:
  - **collection**: (params, opts)
  - **element specific**: (id, params, opts)

If a user doesn't realize the difference, they may attempt to use the
collection signature when performing an element specific operation like:
```
Stripe::PaymentIntent.cancel('pi_1234', 'sk_test_key')
 # Results in an error: NoMethodError: undefined method `key?' for "sk_test"
```

The resulting error message isn't very useful for debugging.

Instead,this PR adds a message letting the user know what it's expecting:
`request params should be either a Hash or nil (was a String)`
---
 lib/stripe/api_operations/request.rb | 11 +++++++++++
 test/stripe/api_resource_test.rb     | 16 ++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/lib/stripe/api_operations/request.rb b/lib/stripe/api_operations/request.rb
index 579da7c95..edf4542d5 100644
--- a/lib/stripe/api_operations/request.rb
+++ b/lib/stripe/api_operations/request.rb
@@ -5,6 +5,9 @@ module APIOperations
     module Request
       module ClassMethods
         def request(method, url, params = {}, opts = {})
+          params ||= {}
+
+          error_on_invalid_params(params)
           warn_on_opts_in_params(params)
 
           opts = Util.normalize_opts(opts)
@@ -47,6 +50,14 @@ def request(method, url, params = {}, opts = {})
           end
         end
 
+        private def error_on_invalid_params(params)
+          return if params.nil? || params.is_a?(Hash)
+
+          raise ArgumentError,
+                "request params should be either a Hash or nil " \
+                  "(was a #{params.class})"
+        end
+
         private def warn_on_opts_in_params(params)
           Util::OPTS_USER_SPECIFIED.each do |opt|
             if params.key?(opt)
diff --git a/test/stripe/api_resource_test.rb b/test/stripe/api_resource_test.rb
index c0dfbc163..cc9491c4c 100644
--- a/test/stripe/api_resource_test.rb
+++ b/test/stripe/api_resource_test.rb
@@ -227,6 +227,22 @@ class NestedTestAPIResource < APIResource
         end
       end
 
+      should "error if the params is not a Hash" do
+        stub_request(:post, "#{Stripe.api_base}/v1/charges/ch_123/capture")
+          .to_return(body: JSON.generate(charge_fixture))
+
+        e = assert_raises(ArgumentError) { Stripe::Charge.capture("ch_123", "sk_test_secret") }
+
+        assert_equal "request params should be either a Hash or nil (was a String)", e.message
+      end
+
+      should "allow making a request with params set to nil" do
+        stub_request(:post, "#{Stripe.api_base}/v1/charges/ch_123/capture")
+          .to_return(body: JSON.generate(charge_fixture))
+
+        Stripe::Charge.capture("ch_123", nil, "sk_test_secret")
+      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))