Safer argument parsing (attempt 2)

lib/stripe/account.rb

@@ -13,7 +13,8 @@ module Stripe
     end
 
     # @override To make id optional
-    def self.retrieve(id=nil, opts={})
+    def self.retrieve(id=ARGUMENT_NOT_PROVIDED, opts={})
+      id = id.equal?(ARGUMENT_NOT_PROVIDED) ? nil : Util.check_string_argument!(id)
       # Account used to be a singleton, where this method's signature was `(opts={})`.
       # For the sake of not breaking folks who pass in an OAuth key in opts, let's lurkily
       # string match for it.
@@ -31,5 +32,7 @@ module Stripe
       opts.delete(:api_base) # the api_base here is a one-off, don't persist it
       Util.convert_to_stripe_object(response, opts)
     end
+
+    ARGUMENT_NOT_PROVIDED = Object.new
   end
 end

lib/stripe/api_resource.rb

@@ -25,7 +25,7 @@ module Stripe
       refresh_from(response, opts)
     end
 
-    def self.retrieve(id, opts=nil)
+    def self.retrieve(id, opts={})
       opts = Util.normalize_opts(opts)
       instance = self.new(id, opts)
       instance.refresh

lib/stripe/singleton_api_resource.rb

@@ -12,7 +12,7 @@ module Stripe
     end
 
     def self.retrieve(opts={})
-      instance = self.new(nil, opts)
+      instance = self.new(nil, Util.normalize_opts(opts))
       instance.refresh
       instance
     end

lib/stripe/util.rb

@@ -123,15 +123,24 @@ module Stripe
     # Turn this value into an api_key and a set of headers
     def self.normalize_opts(opts)
       case opts
-      when NilClass
-        {}
       when String
         {:api_key => opts}
       when Hash
+        check_api_key!(opts.fetch(:api_key)) if opts.has_key?(:api_key)
         opts.clone
       else
         raise TypeError.new('normalize_opts expects a string or a hash')
       end
     end
+
+    def self.check_string_argument!(key)
+      raise TypeError.new("argument must be a string") unless key.is_a?(String)
+      key
+    end
+
+    def self.check_api_key!(key)
+      raise TypeError.new("api_key must be a string") unless key.is_a?(String)
+      key
+    end
   end
 end

test/stripe/account_test.rb

@@ -62,5 +62,14 @@ module Stripe
       end.returns(test_response({ 'stripe_user_id' => a.id }))
       a.deauthorize('ca_1234', 'sk_test_1234')
     end
+
+    should "reject nil api keys" do
+      assert_raise TypeError do
+        Stripe::Account.retrieve(nil)
+      end
+      assert_raise TypeError do
+        Stripe::Account.retrieve(:api_key => nil)
+      end
+    end
   end
 end

test/stripe/api_resource_test.rb

@@ -37,6 +37,15 @@ module Stripe
       end
     end
 
+    should "using a nil api key should raise an exception" do
+      assert_raises TypeError do
+        Stripe::Customer.all({}, nil)
+      end
+      assert_raises TypeError do
+        Stripe::Customer.all({}, { :api_key => nil })
+      end
+    end
+
     should "specifying api credentials containing whitespace should raise an exception" do
       Stripe.api_key = "key "
       assert_raises Stripe::AuthenticationError do
@@ -454,7 +463,14 @@ module Stripe
           }
         })
 
-        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, 'legal_entity[first_name]=Bob&legal_entity[last_name]=').returns(test_response({"id" => "myid"}))
+        @mock.expects(:post).once.with(
+          "#{Stripe.api_base}/v1/accounts/myid",
+          nil,
+          any_of(
+            'legal_entity[first_name]=Bob&legal_entity[last_name]=',
+            'legal_entity[last_name]=&legal_entity[first_name]=Bob'
+          )
+        ).returns(test_response({"id" => "myid"}))
 
         acct.legal_entity = {:first_name => 'Bob'}
         acct.save

test/stripe/util_test.rb

@@ -25,5 +25,10 @@ module Stripe
       symbolized = Stripe::Util.symbolize_names(start)
       assert_equal(finish, symbolized)
     end
+
+    should "normalize_opts should reject nil keys" do
+      assert_raise { Stripe::Util.normalize_opts(nil) }
+      assert_raise { Stripe::Util.normalize_opts(:api_key => nil) }
+    end
   end
 end