Warn if idempotency_key is in params hash #545

andrewyang-stripe · 2017-05-24T18:28:08Z

Fixes #544. Also add a test case to ensure that there's a warning in stderr when idempotency_key is in the params hash, and not in the opts hash.

brandur-stripe · 2017-05-24T20:46:13Z

@andrewyang-stripe Thanks! I really like the test addition.

A couple notes:

Remember how we talked about how this could potentially extend beyond just a create to an update or a save? Lets see if we can hoist this up a level so that those could have some coverage as well.
Covering idempotency_key is good, but I think we also want to cover a few other common options like stripe_account and api_key (maybe check the code to make sure I didn't miss any others too).

ptal @andrewyang-stripe

andrewyang-stripe · 2017-05-24T22:04:31Z

@brandur-stripe

If I try to check the params hash one level deeper (https://github.com/stripe/stripe-ruby/blob/master/lib/stripe/api_operations/request.rb#L41), I don't think I would be able to test the changes because I use stub_request in the test cases. However, I did identify the update and save methods, which are both in lib/stripe/api_operations/save.rb.
I only see stripe_account being used in methods that don't have both params and opts as parameters (e.g. https://github.com/stripe/stripe-ruby/blob/master/test/stripe/api_resource_test.rb#L299). So the only other important option I could identify is api_key and maybe content_type.

Perhaps I can create a helper method called self.validate_params in lib/stripe/util.rb so I won't have to repeat the same code across create, update, and save operations.

brandur-stripe · 2017-05-24T22:15:49Z

If I try to check the params hash one level deeper (https://github.com/stripe/stripe-ruby/blob/master/lib/stripe/api_operations/request.rb#L41), I don't think I would be able to test the changes because I use stub_request in the test cases. However, I did identify the update and save methods, which are both in lib/stripe/api_operations/save.rb.

So in this case stub_request does not literally mean "stub the request method" :) It's actually stubbing out at a much lower level than our library's #request method.

I only see stripe_account being used in methods that don't have both params and opts as parameters (e.g. https://github.com/stripe/stripe-ruby/blob/master/test/stripe/api_resource_test.rb#L299). So the only other important option I could identify is api_key and maybe content_type.

As discussed IRL, although we only test it in the one place here, you're allowed to pass this in for any request, and it's as general as something like idempotency_key. Lets include it in our check list.

andrewyang-stripe · 2017-05-25T18:08:49Z

I've figured out where to write the check for params. I also wrote a helper method validate_params! in lib/stripe/util.rb and more tests covering cases where a warning should and shouldn't be triggered.
ptal @brandur-stripe

brandur-stripe · 2017-05-25T22:25:43Z

test/stripe/api_resource_test.rb

+        ensure
+          $stderr = old_stderr
+        end
+      end


Good thinking, but I think having tests for both idempotency key and API key for create/update is a bit overkill. Let's take these two out okay?

Maybe you can generalize the name of the other with something like just refer to a general "opt" instead of api_key or idempotency_key specifically.

brandur-stripe · 2017-05-25T22:26:46Z

test/stripe/api_resource_test.rb

          to_return(body: JSON.generate({ :count => 1, :data => [API_FIXTURES.fetch(:charge)] }))
        Stripe::Charge.create(:amount => 50, :currency => 'usd', :card => { :number => nil })
      end

+      should "putting idempotency_key correctly as an opt should not trigger a warning" do


So the naming of these test cases should be readable from the "should".

"should putting idempotency_key correctly ..." doesn't make sense. Maybe something like "should trigger a warning ...".

brandur-stripe · 2017-05-25T22:27:50Z

lib/stripe/api_operations/request.rb

@@ -5,6 +5,8 @@ module ClassMethods
        OPTS_KEYS_TO_PERSIST = Set[:api_key, :api_base, :client, :stripe_account, :stripe_version]

        def request(method, url, params={}, opts={})
+          Util.validate_params!(params)


Given that we're only going to be using this helper in one place, lets move it back to request.rb okay?

brandur-stripe · 2017-05-25T22:28:23Z

lib/stripe/util.rb

@@ -2,6 +2,8 @@

 module Stripe
  module Util
+    @invalid_params = Set.new [:idempotency_key, :api_key, :stripe_account]


Lets make this a constant and give it a slightly more descriptive name, okay? KNOWN_OPTS or something like that.

Oh, and can we also alphabetize the keys in the list and also move it to the private section of the class? You can use private_const :CONST to protect it.

brandur-stripe · 2017-05-25T22:29:22Z

lib/stripe/util.rb

@@ -213,6 +215,14 @@ def self.check_string_argument!(key)
      key
    end

+    def self.validate_params!(params)


Can we call this something a little more appropriate like warn_on_opts_in_params? Lets remove the bang (!) too. This is generally used to show that the method may throw an error, which isn't the case here.

brandur-stripe · 2017-05-25T22:29:42Z

Added a couple change requests, but looking good. Thanks!

andrewyang-stripe · 2017-05-25T23:29:31Z

Made the change requests.
ptal @brandur-stripe

brandur-stripe

Thanks for the changes! I left a couple more comments, but I think we're almost there.

brandur-stripe · 2017-05-26T00:17:50Z

test/stripe/api_resource_test.rb

+      ensure
+        $stderr = old_stderr
+      end
+    end


Let's get rid of this test too. I think just the two below give us enough coverage.

brandur-stripe · 2017-05-26T00:18:13Z

lib/stripe/api_operations/request.rb

@@ -3,8 +3,19 @@ module APIOperations
    module Request
      module ClassMethods
        OPTS_KEYS_TO_PERSIST = Set[:api_key, :api_base, :client, :stripe_account, :stripe_version]


I think this line gives us another hint for an opt that we can warn on! What do you think about adding stripe_version to the set?

Should OPTS_KEYS_TO_PERSIST and KNOWN_OPTS be merged?

brandur-stripe · 2017-05-26T00:19:05Z

lib/stripe/api_operations/request.rb

@@ -3,8 +3,19 @@ module APIOperations
    module Request
      module ClassMethods
        OPTS_KEYS_TO_PERSIST = Set[:api_key, :api_base, :client, :stripe_account, :stripe_version]
+        KNOWN_OPTS = Set[:api_key, :idempotency_key, :stripe_account]
+
+        def warn_on_opts_in_params(params)


Can you move both this method and the constant above down to a new private section in the file? Lets also put a private_constant :KNOWN_OPTS under the KNOWN_OPTS declaration.

brandur-stripe · 2017-05-26T19:02:02Z

lib/stripe/api_operations/request.rb

+
+        private
+
+        KNOWN_OPTS = Set[:api_key, :idempotency_key, :stripe_account, :stripe_version]


I don't think the private block will affect visibility for constants, so can put in the private_constant tag like we talked about?

brandur-stripe · 2017-05-26T19:02:21Z

Very close!

ptal @andrew-stripe

andrewyang-stripe · 2017-05-26T19:58:30Z

Fixed. ptal @brandur-stripe

brandur-stripe · 2017-05-26T20:18:23Z

Great! After you rebase, then I think we're good to go.

andrewyang-stripe requested a review from brandur-stripe May 24, 2017 18:28

andrewyang-stripe force-pushed the andrewyang-idempotency-key-param branch from 721b156 to 5a421b7 Compare May 24, 2017 18:32

andrewyang-stripe self-assigned this May 24, 2017

andrewyang-stripe force-pushed the andrewyang-idempotency-key-param branch from 5a421b7 to c321924 Compare May 24, 2017 18:56

brandur-stripe mentioned this pull request May 24, 2017

Not obvious when not using idempotency_key incorrectly #544

Closed

brandur-stripe reviewed May 25, 2017

View reviewed changes

brandur-stripe reviewed May 26, 2017

View reviewed changes

andrewyang-stripe force-pushed the andrewyang-idempotency-key-param branch from fa6a046 to 10b7828 Compare May 26, 2017 17:08

brandur-stripe reviewed May 26, 2017

View reviewed changes

andrewyang-stripe force-pushed the andrewyang-idempotency-key-param branch from 10b7828 to 7a9ae05 Compare May 26, 2017 19:52

Warn user if a known opt (such as idempotency_key) is in params

e66eac4

andrewyang-stripe force-pushed the andrewyang-idempotency-key-param branch from 7a9ae05 to e66eac4 Compare May 26, 2017 20:30

andrewyang-stripe merged commit 855ce0c into master May 26, 2017

andrewyang-stripe deleted the andrewyang-idempotency-key-param branch May 26, 2017 20:50

brandur-stripe mentioned this pull request Aug 22, 2017

Bad options keys; should we throw? stripe/stripe-node#375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if idempotency_key is in params hash #545

Warn if idempotency_key is in params hash #545

andrewyang-stripe commented May 24, 2017

brandur-stripe commented May 24, 2017

andrewyang-stripe commented May 24, 2017

brandur-stripe commented May 24, 2017

andrewyang-stripe commented May 25, 2017

brandur-stripe May 25, 2017

brandur-stripe May 25, 2017

brandur-stripe May 25, 2017

brandur-stripe May 25, 2017

brandur-stripe May 25, 2017

brandur-stripe May 25, 2017

brandur-stripe commented May 25, 2017

andrewyang-stripe commented May 25, 2017

brandur-stripe left a comment

brandur-stripe May 26, 2017

brandur-stripe May 26, 2017

andrewyang-stripe May 26, 2017

brandur-stripe May 26, 2017

brandur-stripe May 26, 2017

brandur-stripe commented May 26, 2017

andrewyang-stripe commented May 26, 2017

brandur-stripe commented May 26, 2017


		private

		KNOWN_OPTS = Set[:api_key, :idempotency_key, :stripe_account, :stripe_version]

Warn if idempotency_key is in params hash #545

Warn if idempotency_key is in params hash #545

Conversation

andrewyang-stripe commented May 24, 2017

brandur-stripe commented May 24, 2017

andrewyang-stripe commented May 24, 2017

brandur-stripe commented May 24, 2017

andrewyang-stripe commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented May 25, 2017

andrewyang-stripe commented May 25, 2017

brandur-stripe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur-stripe commented May 26, 2017

andrewyang-stripe commented May 26, 2017

brandur-stripe commented May 26, 2017