Add API resource instance methods to StripeClient #954

joeltaylor · 2020-11-16T01:10:15Z

This change introduces convenience methods to access API resources
through StripeClient for per-client configuration. The instance client
can be configured with the same global Stripe configurations. As a
result, an instance of StripeClient is able to override or fallback
to the global configuration that was present at the time of
initialization.

Here's an example:

Stripe::Customer.list() == StripeClient.new.customers.list()

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.
Each API resource is pluralized to align with the conventions of other
Stripe libraries (e.g. Node and PHP). The pluralization itself is
quite naive but can easily be switched out for something more advanced
once the need arises.
Each API resource spec was converted to use instance based
methods and was done to ensure adequate test coverage. Since this
entire feature is built on proxying methods, testing via the client
implicitly tests the original implementation for "free".

This is a clean-up of #921 as I felt that it didn't have reliable enough test coverage.

joeltaylor · 2020-11-16T01:15:06Z

lib/stripe/client_api_operations.rb

+      def update_args_with_client!(method, args)
+        opts_pos = @resource.method(method).parameters.index(%i[opt opts])
+
+        return unless opts_pos
+
+        opts = opts_pos >= args.length ? {} : args[opts_pos]
+
+        normalized_opts = Stripe::Util.normalize_opts(opts)
+        args[opts_pos] = { client: @client }.merge(normalized_opts)
+      end


The primary regressions that I found during the first PR all had to do with nuances when it came time to merge the options. I was able to identify the scenarios and lock them in under test: https://github.com/stripe/stripe-ruby/pull/954/files#diff-50b82fc5b424d8d4c21c2ddd6c67485e40fbc76fdf5cc1e4320949a4ad20fb62R56

joeltaylor · 2020-11-16T01:19:01Z

lib/stripe/resources/account.rb

@@ -45,12 +45,8 @@ def resource_url
    end

    # @override To make id optional
-    def self.retrieve(id = ARGUMENT_NOT_PROVIDED, opts = {})


ARGUMENT_NOT_PROVIDED doesn't play very well with how we merge options. The args object in method_missing is an array and merging options when an ID is omitted results in [nil, {}] being sent to the resource, which isn't desirable.

However, I wasn't able to re-create any of the scenarios that warranted the addition of it in c269e9b and believe it's safe for removal.

lib/stripe/stripe_client.rb

joeltaylor · 2020-11-16T01:26:53Z

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 didn't like the complexity of execute_request so I extracted these out.

lib/stripe/stripe_configuration.rb

joeltaylor · 2020-11-16T01:30:17Z

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 are worth exposing since everything else is.

joeltaylor · 2020-11-16T01:45:24Z

@brandur-stripe this is the alternative approach I mentioned. The most noteworthy change is the testing strategy. Considering that testing the client implicitly tests the original implementation, I thought it was a viable path but I'm certainly open to feedback. Either way, I was able to find some edge cases and refactor some of the original logic with a high level of confidence.

There are a few outstanding questions:

I didn't remove capability from the dynamic method generation so someone can technically do StripeClient.new.capabilitys.list(). It'll throw the same error as Stripe::Capability.list(), but figured it's worth noting even though they're accessed via Stripe::Account. I'm happy to remove it and any others that fall into the same category.
There are two funny methods that may be worth modifying:
- StripeClient.new.three_d_secures
- StripeClient.new.invoiceitems
CI is failing due to Ruby 2.3, which has reached EOL. I'm happy to open a PR to drop 2.3 (and 2.4) support unless you all prefer to support EOL versions.
This feature is likely worth a README update but I'm punting until the end on that in case any significant changes come along.

joeltaylor · 2021-03-15T14:26:39Z

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-stripe Moving the conversation over from #921. I'm a fan of extracting out the StripeClient configuration portion from this PR. Considering how large this PR is, that seems like a good seam for more manageable chunks. I'll follow-up with a PR this week.

Regarding testing, I'm curious what your thoughts are one #954 (comment)?

brandur-stripe · 2021-03-15T17:43:03Z

@brandur-stripe Moving the conversation over from #921. I'm a fan of extracting out the StripeClient configuration portion from this PR. Considering how large this PR is, that seems like a good seam for more manageable chunks. I'll follow-up with a PR this week.

Awesome! And thanks for the quick reply on this. I also took a shot at this on Friday and although I didn't get it 100% working before the day ended, it was relatively straightforward. It was all your code though, and it'd be better if it all came in under your name, so will wait for your PR.

Testing answers below.

@brandur-stripe this is the alternative approach I mentioned. The most noteworthy change is the testing strategy. Considering that testing the client implicitly tests the original implementation, I thought it was a viable path but I'm certainly open to feedback. Either way, I was able to find some edge cases and refactor some of the original logic with a high level of confidence.

@joeltaylor Nice. As long as the two usage types are relatively bound together, I think this approach is okay. The only part that strikes me as a bit a problem is that it's now hard to find usage examples of the original "style" in the test suite. IMO though, we should try to bias towards getting this merged and figure that out later.

There are a few outstanding questions:

I didn't remove capability from the dynamic method generation so someone can technically do StripeClient.new.capabilitys.list(). It'll throw the same error as Stripe::Capability.list(), but figured it's worth noting even though they're accessed via Stripe::Account. I'm happy to remove it and any others that fall into the same category.

Hmm, would the code to have it not be at the top level be a little gnarly? (Like, did you not add it because no special casing is needed right now?) I think it's fine as long as we're erroring, but hmm, I guess it would be slightly better if the invalid methods were not available at all.

Separately: what do you think about potentially removing the pluralization of resources? It's not as "Ruby-ish", but the upsides are that we don't have to either (1) deal with pluralization oddities like "capabilitys", or (2) introduce a pluralization engine that can map the special exceptions. It might be simpler overall.

There are two funny methods that may be worth modifying:

Ah, good to call these out.

StripeClient.new.three_d_secures

Let's leave this one as is.

StripeClient.new.invoiceitems

Hm, I know this is based off our weird endpoint naming, but IMO we should remap this to invoice_items as long as it's relatively easy to do.

CI is failing due to Ruby 2.3, which has reached EOL. I'm happy to open a PR to drop 2.3 (and 2.4) support unless you all prefer to support EOL versions.

We don't formally support EOLed Ruby, but we do sort of have a policy of trying to keep them on life support as long as possible. It kind of looks like from the failing build that it might be relatively easy to put in a patch that keeps 2.3 working (probably just an extra check on nil somewhere). What do you think?

This feature is likely worth a README update but I'm punting until the end on that in case any significant changes come along.

Yeah, good call. That's the type of thing that I'm more than willing to do as well.

ob-stripe

Left some comments. Thanks for working on this @joeltaylor!

ob-stripe · 2021-03-16T16:50:14Z

lib/stripe/oauth.rb

+        opts[:client] ||= params[:client] || StripeClient.active_client
+        opts[:api_base] ||= opts[:client].config.connect_base

+        params.delete(:client)


OAuth methods are a bit weird, but I think the client should always be passed in opts, not in params.

Great call out. I'll see if I can get that behavior under test and will make the change.

ob-stripe · 2021-03-16T16:50:24Z

lib/stripe/oauth.rb

+      client = params[:client] || StripeClient.active_client
+      base = opts[:connect_base] || client.config.connect_base


ob-stripe · 2021-03-16T16:51:14Z

lib/stripe/object_types.rb

+      }.merge(api_object_names_to_classes)
+    end

-        # business objects
+    def self.api_object_names_to_classes
+      {


OOC, why is this change necessary? list is an API object name 🤔

@ob-stripe it was done so I could get a list of API resources to dynamically define the methods here: https://github.com/stripe/stripe-ruby/pull/954/files#diff-992f231b0add61aeb46ab11fb68529936809a99a74ed9f1c1755a4b01f57fb58R63

Certainly open to other suggestions!

lib/stripe/stripe_client.rb

ob-stripe · 2021-03-16T17:04:56Z

lib/stripe/stripe_client.rb

      @system_profiler = SystemProfiler.new
      @last_request_metrics = nil
+      @config_overides = config_overides


In e.g. stripe-php we support two different syntaxes for initializing clients:

passing a configuration hash (what you're doing already)

passing just the API key as a string

Would you mind adding support for the latter? I think it should be as simple as:

@config_overides = config_overrides.is_a?(String) ? {api_key: config_overrides} : config_overrides

That's a great suggestion and certainly improves usability. I'll get it added!

joeltaylor · 2021-04-02T21:14:35Z

@brandur-stripe I'll get this PR up to date with the main branch shortly. Wanted to give a 👍 to your answers and suggestions in #954 (comment)

To recap:

I'll remove the pluralization so we can get rid of the oddities
Will bias towards merging over fiddling with the testing strategy
Will be sure to support 2.3 (if I recall correctly it is just another nil check)

I think we're in the home stretch 😄

joeltaylor · 2021-04-06T03:00:38Z

lib/stripe/api_operations/list.rb

@@ -11,7 +11,7 @@ def list(filters = {}, opts = {})

        # set filters so that we can fetch the same limit, expansions, and
        # predicates when accessing the next and previous pages
-        obj.filters = filters.dup
+        obj.filters = filters.dup unless filters.nil?


It's possible for filters to be nil due to how we delegate methods to the API resources.

This change introduces convenience methods to access API resources through `StripeClient` for per-client configuration. The instance client can be configured with the same global Stripe configurations. As a result, an instance of `StripeClient` is able to override or fallback to the global configuration that was present at the time of initialization. Here's an example: ```ruby Stripe::Customer.list() == StripeClient.new.customer.list() ``` 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. - Each API resource spec was converted to use instance based methods and was done to ensure adequate test coverage. Since this entire feature is built on proxying methods, testing via the client implicitly tests the original implementation for "free".

It's possible for `filters` to be `nil`, which causes Ruby 2.3 to raise an error: `TypeError: can't dup NilClass`

joeltaylor · 2021-04-06T03:07:41Z

lib/stripe/client_api_operations.rb

+        # Update `invoiceitems` to match snake case convention
+        invoice_item_class = api_resources.delete("invoiceitem")
+        api_resources["invoice_item"] = invoice_item_class


@brandur-stripe Here's the update to turn invoiceitem into invoice_item that you previously mentioned.

ob-stripe · 2021-05-03T23:17:04Z

@richardm-stripe Reassigned this to you since Brandur is no longer around.

joeltaylor · 2022-06-21T20:04:16Z

@richardm-stripe I'm going to close this PR out given it's been awhile and there are a few merge conflicts. Happy to revisit in the future if this is still a desired direction.

joeltaylor commented Nov 16, 2020

View reviewed changes

joeltaylor force-pushed the add-client-instance-api branch from 47e8dc1 to b989aed Compare January 10, 2021 22:44

remi-stripe mentioned this pull request Mar 5, 2021

Add Stripe.with_api_key helper to make working with multiple keys easier #966

Closed

brandur mentioned this pull request Mar 10, 2021

Add API resource instance methods to StripeClient #921

Closed

stripe deleted a comment from travisbarronsanabria Mar 15, 2021

ob-stripe reviewed Mar 16, 2021

View reviewed changes

stripe deleted a comment from travisbarronsanabria Mar 25, 2021

joeltaylor mentioned this pull request Mar 27, 2021

Allow StripeClient to be configured per instance #968

Merged

joeltaylor force-pushed the add-client-instance-api branch 2 times, most recently from 55c4b2c to e3ed0a2 Compare April 6, 2021 02:14

joeltaylor commented Apr 6, 2021

View reviewed changes

joeltaylor force-pushed the add-client-instance-api branch from 0c9a8c1 to 36d27d9 Compare April 6, 2021 03:00

joeltaylor and others added 3 commits April 5, 2021 20:05

Revert API resources back to singular

e5ed86c

Add nil check for Ruby 2.3

de9c3de

It's possible for `filters` to be `nil`, which causes Ruby 2.3 to raise an error: `TypeError: can't dup NilClass`

joeltaylor force-pushed the add-client-instance-api branch from 36d27d9 to de9c3de Compare April 6, 2021 03:06

joeltaylor commented Apr 6, 2021

View reviewed changes

joeltaylor requested review from brandur-stripe and ob-stripe April 6, 2021 03:10

ob-stripe removed request for ob-stripe and brandur-stripe May 3, 2021 23:16

ob-stripe assigned richardm-stripe May 3, 2021

joeltaylor closed this Jun 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API resource instance methods to StripeClient #954

Add API resource instance methods to StripeClient #954

joeltaylor commented Nov 16, 2020

joeltaylor Nov 16, 2020

joeltaylor Nov 16, 2020

joeltaylor Nov 16, 2020

joeltaylor Nov 16, 2020

joeltaylor commented Nov 16, 2020 •

edited

Loading

joeltaylor commented Mar 15, 2021

brandur-stripe commented Mar 15, 2021 •

edited

Loading

ob-stripe left a comment

ob-stripe Mar 16, 2021

joeltaylor Mar 23, 2021

ob-stripe Mar 16, 2021

ob-stripe Mar 16, 2021

joeltaylor Mar 23, 2021

ob-stripe Mar 16, 2021

joeltaylor Mar 23, 2021

joeltaylor commented Apr 2, 2021

joeltaylor Apr 6, 2021

joeltaylor Apr 6, 2021

ob-stripe commented May 3, 2021

joeltaylor commented Jun 21, 2022

		client = params[:client] \|\| StripeClient.active_client
		base = opts[:connect_base] \|\| client.config.connect_base

Add API resource instance methods to StripeClient #954

Add API resource instance methods to StripeClient #954

Conversation

joeltaylor commented Nov 16, 2020

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 Nov 16, 2020 • edited Loading

joeltaylor commented Mar 15, 2021

brandur-stripe commented Mar 15, 2021 • edited Loading

ob-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

joeltaylor commented Apr 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ob-stripe commented May 3, 2021

joeltaylor commented Jun 21, 2022

joeltaylor commented Nov 16, 2020 •

edited

Loading

brandur-stripe commented Mar 15, 2021 •

edited

Loading