Add API resource instance methods to StripeClient #921

joeltaylor · 2020-05-28T16:31:20Z

This change introduces a proof-of-concept to add convenience methods to
access API resources through a StripeClient for per-client
configuration. This first iteration only allows for the api_key to be
configured but can be extended to allow other options such as
stripe_version, which should solve #872.

The primary workhorse for this feature is a new module called
Stripe::ClientAPIOperations that defines instance methods on
StripeClient when it is included. A ClientProxy is used to send any
method calls to an API resource with the instantiated client injected.
There are a few noteworthy aspects of this approach:

Many resources are namespaced, which introduces a unique challenge
when it comes to method chaining calls (e.g.
client.issuing.authorizations). In order to handle those cases, we
create a ClientProxy object for the root namespace (e.g., "issuing")
and define all resource methods (e.g. "authorizations") at once to
avoid re-defining the proxy object when there are multiple resources
per namespace.
Sigma deviates from other namespaced API resources and does not have
an OBJECT_NAME separated by a period. We account for that nuance
directly.
method_missing is substantially slower than direct calls. Therefore,
methods are defined where possible but method_missing is still used
at the last step when delegating resource methods to the actual
resource.

joeltaylor · 2020-05-28T16:36:29Z

lib/stripe/client_api_operations.rb

+      end
+    end
+  end
+end


100% open to other naming for all of these concepts.

joeltaylor · 2020-05-28T16:40:01Z

Wanted to go ahead and submit a proof-of-concept for feedback. Pending whether or not this approach is acceptable, I still need to verify that this doesn't cause any unforeseen issues with threaded environments or the connection_manager and can update the README.

Also, happy to include stripe_version per instance if it's desired!

brandur-stripe · 2020-05-28T17:34:22Z

Yesssss Joel, this is GREAT! I have quite a few balls up in the air over the next couple days, but will get full feedback to you on this ASAP. Thank you again!

joeltaylor · 2020-05-28T17:59:05Z

@brandur-stripe sounds good! In the meantime, I'll probably perform a few benchmarks and further testing for sanity sake.

brandur-stripe

Thanks @joeltaylor! Sorry for the grossly delayed review on this one :/ I don't have a good excuse for why things took this long, but had some other priority stuff at work.

This generally looks great! Thanks for the comments and tests, and the design looks clean to me. I found one major bug while testing — have left some details below.

brandur-stripe · 2020-07-14T19:58:26Z

lib/stripe/client_api_operations.rb

+
+      def method_missing(method, *args)
+        super unless @resource
+        @resource.send(method, *args << { client: @client }) || super


The major thing I noticed while trying this out is that this line doesn't allow for any kind of flexibility in argument positioning. As your tests show, it works for something like client.accounts.retrieve('acct_123'), but will fail if your try something like client.charges.list:

Traceback (most recent call last): 5: from client_test.rb:3:in `<main>' 4: from /Users/brandur/stripe/stripe-ruby/lib/stripe/client_api_operations.rb:25:in `method_missing' 3: from /Users/brandur/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:9:in `list' 2: from /Users/brandur/stripe/stripe-ruby/lib/stripe/api_operations/request.rb:25:in `request' 1: from /Users/brandur/stripe/stripe-ruby/lib/stripe/stripe_client.rb:203:in `execute_request' /Users/brandur/stripe/stripe-ruby/lib/stripe/stripe_client.rb:414:in `check_api_key!': No API key provided. Set your API key using "Stripe.api_key = <API-KEY>". You can generate API keys from the Stripe web interface. See https://stripe.com/api for details, or email support@stripe.com if you have any questions. (Stripe::AuthenticationError)

The reason for the failure is that the list method takes an optional filters argument in its first position:

module APIOperations module List def list(filters = {}, opts = {})

So if you call list without any arguments, { client: @client } gets put into filters instead of opts, and doesn't get picked up as the active client.

I think you may need to do some introspection to make this work for everything. Method#arity doesn't return anything useful because of the optional arguments, but you can use something like Method#parameters to look for the opts position:

> @resource.method(method).parameters.index {|p| p == [:opt, :opts]} => 1

Then munge the passed in args and send it through.

Relatedly, we should handle the case where an opts was passed as well. i.e. This should be possible to do still (even though the client's already configured with an API key):

client.accounts.list({}, {api_key: "sk_test_..."})

So we'd probably want to do something like check args at the position where we found opts and respect if if present. Something like this (warning: did not check to see if this code actually works):

args[opts_pos] = {client: @client}.merge(args[opts_pos] || {})

Ah, good catch! I continually forget that collection methods have a different signature (and I even updated the README to note that 😄).

The approach you mentioned makes sense to me. I'll update accordingly!

lib/stripe/client_api_operations.rb

brandur-stripe · 2020-07-14T20:04:08Z

lib/stripe/stripe_client.rb

@@ -17,11 +19,14 @@ class StripeClient
    #
    # Takes a connection manager object for backwards compatibility only, and
    # that use is DEPRECATED.
-    def initialize(_connection_manager = nil)
+    def initialize(_connection_manager = nil, api_key: nil)


We should probably support all the configuration options that Stripe does like stripe_account, api_base, ..., but also even things like open_timeout`.

This is going to be a more complex refactor though, so I could probably take this job.

Happy to take a stab at that unless you'd prefer to keep this PR smaller?

Totally up to you :) I don't mind doing it and offered because it's a little bit of work, but if you want to take a stab at it, that'd be great too. I haven't looked too deeply, but it might be a little non-trivial because most of them are checked in pretty random places throughout the codebase.

brandur-stripe · 2020-07-14T20:05:57Z

test/stripe/stripe_client_test.rb

+                           headers: { "Authorization" => "Bearer sk_test_local" })
+          assert list.is_a?(Stripe::Radar::ValueList)
+        end
+      end


I think it might be a good idea to introduce a basic test for each of the common types of operation like list, retrieve, delete, update just for a little extra certainty that they work. (We'd need some way of better exercising our opts position detect code anyway.)

Would you mind also checking that passing opts overrides like {api_key: 'sk_test_override'} works as well?

Yeah, that's a great idea and should help fill the gap that allowed the bug to slip in. I'll get those added 👍

Excellent! TY man.

Added some basic tests for the common operations. Just starting to crack into supporting all the other Stripe options so I imagine I'll be re-organizing these specs.

CLAassistant · 2020-07-21T17:07:59Z

All committers have signed the CLA.

Adds a `Stripe::StripeConfiguration` object to manage internal and user supplied configuation options. This is primarily motivated by stripe#921 in order to provide a way to set options on for an instance of `StripeClient`.

Adds a `Stripe::StripeConfiguration` object to manage internal and user supplied configuration options. This is primarily motivated by stripe#921 in order to provide a way to set options on for an instance of `StripeClient`.

Adds a `Stripe::StripeConfiguration` object to manage internal and user supplied configuration options. This is primarily motivated by #921 in order to provide a way to set options on for an instance of `StripeClient`.

joeltaylor · 2020-09-18T23:28:29Z

lib/stripe/resources/account.rb

@@ -136,6 +136,7 @@ def deauthorize(client_id = nil, opts = {})
        client_id: client_id,
        stripe_user_id: id,
      }
+      opts = @opts.merge(Util.normalize_opts(opts))


#deauthorize doesn't have access to @opts, which may contain the client option we need to override global configuration.

joeltaylor · 2020-09-18T23:52:26Z

lib/stripe/stripe_client.rb

-    #
-    # Takes a connection manager object for backwards compatibility only, and
-    # that use is DEPRECATED.
-    def initialize(_connection_manager = nil)


If we still need to support backwards compatibility here, I can add a helper method to ensure the first param is a Hash.

joeltaylor · 2020-09-18T23:53:56Z

lib/stripe/stripe_client.rb

+    def store_last_response(object_id, resp)
+      return unless last_response_has_key?(object_id)
+
+      self.class.current_thread_context.last_responses[object_id] = resp
+    end
+
+    def last_response_has_key?(object_id)
+      self.class.current_thread_context.last_responses&.key?(object_id)
+    end


Rubocop ABC metric was mad so I broke these out.

joeltaylor · 2020-09-18T23:57:16Z

lib/stripe/stripe_configuration.rb

@@ -27,7 +27,6 @@ module Stripe
  class StripeConfiguration
    attr_accessor :api_key
    attr_accessor :api_version
-    attr_accessor :client_id


This one snuck in on accident since client_id is still set on the Stripe object:

stripe-ruby/lib/stripe.rb

Line 87 in 0620436

attr_accessor :client_id

If it's desired for this to be figured per client, I'll also need to update the OAuth code to account for it.

joeltaylor · 2020-09-18T23:58:04Z

lib/stripe/stripe_configuration.rb

+    def max_network_retry_delay=(val)
+      @max_network_retry_delay = val.to_i
+    end
+
+    def initial_network_retry_delay=(val)
+      @initial_network_retry_delay = val.to_i
+    end


Figured these should be accessible since all the other retry configs are.

joeltaylor · 2020-09-18T23:58:55Z

lib/stripe/util.rb

+      config = data.delete(:config) || Stripe.configuration
+      logger = config.logger || Stripe.logger
+      if !logger.nil? ||
+         !config.log_level.nil? && config.log_level <= Stripe::LEVEL_DEBUG


Happy to DRY this up if anyone prefers.

joeltaylor · 2020-09-25T15:55:58Z

lib/stripe/stripe_client.rb

@@ -127,21 +136,21 @@ def self.should_retry?(error, method:, num_retries:)
      end
    end

-    def self.sleep_time(num_retries)
+    def self.sleep_time(num_retries, config: Stripe.configuration)


I saw in the git history that these methods were intended to be functional. Didn't want to stray from that so opted for injecting the config.

joeltaylor · 2020-09-25T16:58:29Z

test/stripe/stripe_client_test.rb

@@ -1145,6 +1192,193 @@ class StripeClientTest < Test::Unit::TestCase
      end
    end

+    context "API resource instance methods" do


I'm not 100% sold on this testing approach. It feels too easy for regressions to find there way in, but maybe I've stared at it for too long.

joeltaylor · 2020-09-25T17:06:58Z

@brandur-stripe Looks like it's my turn to apologize for the delay! The config changes got a bit more involved than I originally anticipated.

I've added a number of additional tests and then a few more after catching errors when running a separate production app test suite with this branch. Overall, I'm curious if this added complexity is pushing the gem in the desired direction?

This change introduces a proof-of-concept to add convenience methods to access API resources through a StripeClient for per-client configuration. This first iteration only allows for the `api_key` to be configured but can be extended to allow other options such as `stripe_version`, which should solve stripe#872. The primary workhorse for this feature is a new module called `Stripe::ClientAPIOperations` that defines instance methods on `StripeClient` when it is included. A `ClientProxy` is used to send any method calls to an API resource with the instantiated client injected. There are a few noteworthy aspects of this approach: - Many resources are namespaced, which introduces a unique challenge when it comes to method chaining calls (e.g. client.issuing.authorizations). In order to handle those cases, we create a `ClientProxy` object for the root namespace (e.g., "issuing") and define all resource methods (e.g. "authorizations") at once to avoid re-defining the proxy object when there are multiple resources per namespace. - Sigma deviates from other namespaced API resources and does not have an `OBJECT_NAME` separated by a period. We account for that nuance directly. - `method_missing` is substantially slower than direct calls. Therefore, methods are defined where possible but `method_missing` is still used at the last step when delegating resource methods to the actual resource.

joeltaylor · 2020-10-20T13:40:19Z

Came across an unexpected regression, which has confirmed my suspicion that there's a gap in the tests with this feature. Going to close this PR and try an alternative route that I think should instill more confidence.

brandur-stripe · 2020-11-05T18:25:29Z

@joeltaylor Nice. Okay, let us know if there's anything we could help out with on that alternative route — glad you caught the regression, but this code still looks pretty good to me, and it'd be a shame to throw out all that work!

joeltaylor · 2020-11-05T18:33:11Z

@brandur-stripe Thank you! I wanted to see if I could consume the OpenAPI spec to automate the testing, but hit a few roadblocks. I'm not seeing a super clear route outside of duplicating the API resource specs for both usages. I may also be able to do something dynamic and/or create a test helper. Curious what your thoughts are?

brandur · 2021-03-10T22:27:06Z

@joeltaylor

I wanted to see if I could consume the OpenAPI spec to automate the testing, but hit a few roadblocks. I'm not seeing a super clear route outside of duplicating the API resource specs for both usages. I may also be able to do something dynamic and/or create a test helper. Curious what your thoughts are?

Not super opinionated here, although it'd be nice to land with something that exercises both cases, and hopefully without too much metaprogramming. I think duplicated tests would probably be fine, although that's a lot of copying and pasting. The other option is that we don't go too crazy with the per-resource per-action testing, because if you look at what most of these are doing, they're not really testing anything all that thoroughly anyway.

Incidentally, we got a pull request recently in #966 wherein users were basically looking to have something close to per-client configuration. We could bring that in, but your more comprehensive approach here would be the much better solution IMO.

Even if we take a little further on getting all the per-client resources working correctly, would you mind if we brought in a slice of this PR that makes StripeClient configurable? If you're busy, I can make that PR.

brandur · 2021-03-10T22:28:33Z

And ah, should have commented on the newer version of this in #954.

joeltaylor force-pushed the add-client-instance-api branch from 3adf829 to 0dc9723 Compare May 28, 2020 16:35

joeltaylor commented May 28, 2020

View reviewed changes

lib/stripe/client_api_operations.rb

end

end

end

end

Copy link

Contributor Author

joeltaylor May 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% open to other naming for all of these concepts.

joeltaylor force-pushed the add-client-instance-api branch from 0dc9723 to 0b42176 Compare May 28, 2020 16:51

brandur-stripe reviewed Jul 14, 2020

View reviewed changes

joeltaylor force-pushed the add-client-instance-api branch 2 times, most recently from 01cfac9 to 0e3d9d5 Compare July 31, 2020 21:33

joeltaylor force-pushed the add-client-instance-api branch from 0e3d9d5 to a736bfc Compare August 9, 2020 18:08

joeltaylor mentioned this pull request Aug 9, 2020

Extract configurations into separate object #939

Merged

joeltaylor force-pushed the add-client-instance-api branch 2 times, most recently from 7a8fa03 to 3abe003 Compare August 25, 2020 17:13

joeltaylor marked this pull request as draft September 2, 2020 16:25

joeltaylor force-pushed the add-client-instance-api branch 3 times, most recently from 43ae45f to fd1d9f9 Compare September 18, 2020 23:26

joeltaylor commented Sep 18, 2020

View reviewed changes

joeltaylor force-pushed the add-client-instance-api branch 2 times, most recently from 4a7ca18 to c87403f Compare September 18, 2020 23:50

joeltaylor commented Sep 18, 2020

View reviewed changes

joeltaylor force-pushed the add-client-instance-api branch from c87403f to 3f96ddf Compare September 19, 2020 00:01

joeltaylor commented Sep 25, 2020

View reviewed changes

joeltaylor force-pushed the add-client-instance-api branch 2 times, most recently from 0f2e73c to 4d5c265 Compare September 25, 2020 16:51

joeltaylor commented Sep 25, 2020

View reviewed changes

joeltaylor marked this pull request as ready for review September 25, 2020 17:07

joeltaylor force-pushed the add-client-instance-api branch from 4d5c265 to 9866a01 Compare October 20, 2020 04:16

joeltaylor closed this Oct 20, 2020

joeltaylor mentioned this pull request Nov 16, 2020

Add API resource instance methods to StripeClient #954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API resource instance methods to StripeClient #921

Add API resource instance methods to StripeClient #921

joeltaylor commented May 28, 2020

joeltaylor May 28, 2020

joeltaylor commented May 28, 2020

brandur-stripe commented May 28, 2020

joeltaylor commented May 28, 2020

brandur-stripe left a comment

brandur-stripe Jul 14, 2020

joeltaylor Jul 14, 2020

brandur-stripe Jul 14, 2020

joeltaylor Jul 14, 2020

brandur-stripe Jul 14, 2020

brandur-stripe Jul 14, 2020

joeltaylor Jul 14, 2020

brandur-stripe Jul 14, 2020

joeltaylor Jul 31, 2020

CLAassistant commented Jul 21, 2020 •

edited

Loading

joeltaylor Sep 18, 2020

joeltaylor Sep 18, 2020

joeltaylor Sep 18, 2020

joeltaylor Sep 18, 2020 •

edited

Loading

joeltaylor Sep 18, 2020 •

edited

Loading

joeltaylor Sep 18, 2020

joeltaylor Sep 25, 2020

joeltaylor Sep 25, 2020

joeltaylor commented Sep 25, 2020

joeltaylor commented Oct 20, 2020

brandur-stripe commented Nov 5, 2020

joeltaylor commented Nov 5, 2020

brandur commented Mar 10, 2021

brandur commented Mar 10, 2021

Add API resource instance methods to StripeClient #921

Add API resource instance methods to StripeClient #921

Conversation

joeltaylor commented May 28, 2020

Choose a reason for hiding this comment

joeltaylor commented May 28, 2020

brandur-stripe commented May 28, 2020

joeltaylor commented May 28, 2020

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

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

CLAassistant commented Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeltaylor Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

joeltaylor Sep 18, 2020 • edited Loading

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

joeltaylor commented Sep 25, 2020

joeltaylor commented Oct 20, 2020

brandur-stripe commented Nov 5, 2020

joeltaylor commented Nov 5, 2020

brandur commented Mar 10, 2021

brandur commented Mar 10, 2021

CLAassistant commented Jul 21, 2020 •

edited

Loading

joeltaylor Sep 18, 2020 •

edited

Loading

joeltaylor Sep 18, 2020 •

edited

Loading