From fd1d9f9d251e8518efb65f19c51373ef614760a1 Mon Sep 17 00:00:00 2001 From: Joel Taylor Date: Sat, 23 May 2020 09:09:18 -0700 Subject: [PATCH] Add API resource instance methods to StripeClient 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. --- lib/stripe.rb | 8 +- lib/stripe/client_api_operations.rb | 89 +++++++++ lib/stripe/connection_manager.rb | 15 +- lib/stripe/oauth.rb | 8 +- lib/stripe/object_types.rb | 7 +- lib/stripe/resources/account.rb | 1 + lib/stripe/resources/file.rb | 3 +- lib/stripe/stripe_client.rb | 106 ++++++---- lib/stripe/stripe_configuration.rb | 9 +- lib/stripe/util.rb | 24 ++- test/stripe/account_test.rb | 16 ++ test/stripe/connection_manager_test.rb | 43 ++++ test/stripe/file_test.rb | 15 ++ test/stripe/oauth_test.rb | 45 ++++- test/stripe/stripe_client_test.rb | 241 +++++++++++++++++++++-- test/stripe/stripe_configuration_test.rb | 21 +- test/stripe_test.rb | 5 + 17 files changed, 577 insertions(+), 79 deletions(-) create mode 100644 lib/stripe/client_api_operations.rb diff --git a/lib/stripe.rb b/lib/stripe.rb index 13ea28835..d881bba11 100644 --- a/lib/stripe.rb +++ b/lib/stripe.rb @@ -31,7 +31,6 @@ require "stripe/util" require "stripe/connection_manager" require "stripe/multipart_encoder" -require "stripe/stripe_client" require "stripe/stripe_object" require "stripe/stripe_response" require "stripe/list_object" @@ -40,10 +39,15 @@ require "stripe/singleton_api_resource" require "stripe/webhook" require "stripe/stripe_configuration" +require "stripe/client_api_operations" # Named API resources require "stripe/resources" +# StripeClient requires API Resources to be loaded +# due to dynamic methods defined by ClientAPIOperations +require "stripe/stripe_client" + # OAuth require "stripe/oauth" @@ -62,6 +66,8 @@ module Stripe class << self extend Forwardable + attr_reader :configuration + # User configurable options def_delegators :@configuration, :api_key, :api_key= def_delegators :@configuration, :api_version, :api_version= diff --git a/lib/stripe/client_api_operations.rb b/lib/stripe/client_api_operations.rb new file mode 100644 index 000000000..d14de8e72 --- /dev/null +++ b/lib/stripe/client_api_operations.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module Stripe + # Define instance methods on the including class (i.e. StripeClient) + # to access API resources. + module ClientAPIOperations + # Proxy object to inject the client into API resources. When included, + # all resources are defined as singleton methods on the client in the + # plural form (e.g. Stripe::Account => client.accounts). + class ClientProxy + def initialize(client:, resource: nil) + @client = client + @resource = resource + end + + attr_reader :client + + def with_client(client) + @client = client + self + end + + # Used to either send a method to the API resource or the nested + # ClientProxy when a resource is namespaced. Since the method signature + # differs when operating on a collection versus a singular resource, it's + # required to perform introspection on the parameters to respect any + # passed in options or overrides. + def method_missing(method, *args) + super unless @resource + opts_pos = @resource.method(method).parameters.index(%i[opt opts]) + args[opts_pos] = { client: @client }.merge(args[opts_pos] || {}) + + @resource.public_send(method, *args) || super + end + + def respond_to_missing?(symbol, include_private = false) + super unless @resource + @resource.respond_to?(symbol) || super + end + end + + def self.included(base) + base.class_eval do + # Sigma, unlike other namespaced API objects, is not separated by a + # period so we modify the object name to follow the expected convention. + api_resources = Stripe::Util.api_object_classes + sigma_class = api_resources.delete("scheduled_query_run") + api_resources["sigma.scheduled_query_run"] = sigma_class + + # Group namespaces that have mutiple resourses + grouped_resources = api_resources.group_by do |key, _| + key.include?(".") ? key.split(".").first : key + end + + grouped_resources.each do |resource_namespace, resources| + # Namespace resource names are separated with a period by convention. + if resources[0][0].include?(".") + + # Defines the methods required for chaining calls for resources that + # are namespaced. A proxy object is created so that all resource + # methods can be defined at once. + # + # NOTE: At some point, a smarter pluralization scheme may be + # necessary for resource names with complex pluralization rules. + proxy = ClientProxy.new(client: nil) + resources.each do |resource_name, resource_class| + method_name = resource_name.split(".").last + proxy.define_singleton_method("#{method_name}s") do + ClientProxy.new(client: proxy.client, resource: resource_class) + end + end + + # Defines the first method for resources that are namespaced. By + # convention these methods are singular. A proxy object is returned + # so that the client can be injected along the method chain. + define_method(resource_namespace) do + proxy.with_client(self) + end + else + # Defines plural methods for non-namespaced resources + define_method("#{resource_namespace}s".to_sym) do + ClientProxy.new(client: self, resource: resources[0][1]) + end + end + end + end + end + end +end diff --git a/lib/stripe/connection_manager.rb b/lib/stripe/connection_manager.rb index af24fb168..0114558ed 100644 --- a/lib/stripe/connection_manager.rb +++ b/lib/stripe/connection_manager.rb @@ -16,7 +16,8 @@ class ConnectionManager # garbage collected or not. attr_reader :last_used - def initialize + def initialize(config = Stripe.configuration) + @config = config @active_connections = {} @last_used = Util.monotonic_time @@ -117,14 +118,14 @@ def execute_request(method, uri, body: nil, headers: nil, query: nil) # reused Go's default for `DefaultTransport`. connection.keep_alive_timeout = 30 - connection.open_timeout = Stripe.open_timeout - connection.read_timeout = Stripe.read_timeout + connection.open_timeout = @config.open_timeout + connection.read_timeout = @config.read_timeout connection.use_ssl = uri.scheme == "https" - if Stripe.verify_ssl_certs + if @config.verify_ssl_certs connection.verify_mode = OpenSSL::SSL::VERIFY_PEER - connection.cert_store = Stripe.ca_store + connection.cert_store = @config.ca_store else connection.verify_mode = OpenSSL::SSL::VERIFY_NONE warn_ssl_verify_none @@ -138,10 +139,10 @@ def execute_request(method, uri, body: nil, headers: nil, query: nil) # out those pieces to make passing them into a new connection a little less # ugly. private def proxy_parts - if Stripe.proxy.nil? + if @config.proxy.nil? [nil, nil, nil, nil] else - u = URI.parse(Stripe.proxy) + u = URI.parse(@config.proxy) [u.host, u.port, u.user, u.password] end end diff --git a/lib/stripe/oauth.rb b/lib/stripe/oauth.rb index eab8fd12b..fbf5979e0 100644 --- a/lib/stripe/oauth.rb +++ b/lib/stripe/oauth.rb @@ -7,9 +7,10 @@ module OAuthOperations def self.execute_resource_request(method, url, params, opts) opts = Util.normalize_opts(opts) - opts[:client] ||= StripeClient.active_client - opts[:api_base] ||= Stripe.connect_base + opts[:client] ||= params[:client] || StripeClient.active_client + opts[:api_base] ||= opts[:client].config.connect_base + params.delete(:client) super(method, url, params, opts) end end @@ -29,7 +30,8 @@ def self.get_client_id(params = {}) end def self.authorize_url(params = {}, opts = {}) - base = opts[:connect_base] || Stripe.connect_base + client = params[:client] || StripeClient.active_client + base = opts[:connect_base] || client.config.connect_base path = "/oauth/authorize" path = "/express" + path if opts[:express] diff --git a/lib/stripe/object_types.rb b/lib/stripe/object_types.rb index 2e141488e..b01d2a946 100644 --- a/lib/stripe/object_types.rb +++ b/lib/stripe/object_types.rb @@ -1,15 +1,17 @@ # frozen_string_literal: true # rubocop:disable Metrics/MethodLength - module Stripe module ObjectTypes def self.object_names_to_classes { # data structures ListObject::OBJECT_NAME => ListObject, + }.merge(api_object_names_to_classes) + end - # business objects + def self.api_object_names_to_classes + { Account::OBJECT_NAME => Account, AccountLink::OBJECT_NAME => AccountLink, AlipayAccount::OBJECT_NAME => AlipayAccount, @@ -96,5 +98,4 @@ def self.object_names_to_classes end end end - # rubocop:enable Metrics/MethodLength diff --git a/lib/stripe/resources/account.rb b/lib/stripe/resources/account.rb index c4dbb36bc..2bba33207 100644 --- a/lib/stripe/resources/account.rb +++ b/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)) OAuth.deauthorize(params, opts) end diff --git a/lib/stripe/resources/file.rb b/lib/stripe/resources/file.rb index d5d816a82..2f2ab3391 100644 --- a/lib/stripe/resources/file.rb +++ b/lib/stripe/resources/file.rb @@ -25,8 +25,9 @@ def self.create(params = {}, opts = {}) end end + config = opts[:client]&.config || Stripe.configuration opts = { - api_base: Stripe.uploads_base, + api_base: config.uploads_base, content_type: MultipartEncoder::MULTIPART_FORM_DATA, }.merge(Util.normalize_opts(opts)) super diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 48ee67fcd..a1b8f617d 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -7,6 +7,8 @@ module Stripe # recover both a resource a call returns as well as a response object that # contains information on the HTTP call. class StripeClient + include Stripe::ClientAPIOperations + # A set of all known thread contexts across all threads and a mutex to # synchronize global access to them. @thread_contexts_with_connection_managers = [] @@ -17,11 +19,20 @@ class StripeClient # # Takes a connection manager object for backwards compatibility only, and # that use is DEPRECATED. - def initialize(_connection_manager = nil) + def initialize(config_overides = {}) @system_profiler = SystemProfiler.new @last_request_metrics = nil + @config_overides = config_overides + end + + # Always base config off the global Stripe configuration to ensure the + # client picks up and changes to the config. + def config + Stripe.configuration.reverse_duplicate_merge(@config_overides) end + attr_reader :options + # Gets a currently active `StripeClient`. Set for the current thread when # `StripeClient#request` is being run so that API operations being executed # inside of that block can find the currently active client. It's reset to @@ -64,9 +75,9 @@ def self.default_client end # A default connection manager for the current thread. - def self.default_connection_manager + def self.default_connection_manager(config = Stripe.configuration) current_thread_context.default_connection_manager ||= begin - connection_manager = ConnectionManager.new + connection_manager = ConnectionManager.new(config) @thread_contexts_with_connection_managers_mutex.synchronize do maybe_gc_connection_managers @@ -80,8 +91,9 @@ def self.default_connection_manager # Checks if an error is a problem that we should retry on. This includes # both socket errors that may represent an intermittent problem and some # special HTTP statuses. - def self.should_retry?(error, method:, num_retries:) - return false if num_retries >= Stripe.max_network_retries + def self.should_retry?(error, + method:, num_retries:, config: Stripe.configuration) + return false if num_retries >= config.max_network_retries case error when Net::OpenTimeout, Net::ReadTimeout @@ -127,13 +139,13 @@ 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) # Apply exponential backoff with initial_network_retry_delay on the # number of num_retries so far as inputs. Do not allow the number to # exceed max_network_retry_delay. sleep_seconds = [ - Stripe.initial_network_retry_delay * (2**(num_retries - 1)), - Stripe.max_network_retry_delay, + config.initial_network_retry_delay * (2**(num_retries - 1)), + config.max_network_retry_delay, ].min # Apply some jitter by randomizing the value in the range of @@ -141,7 +153,7 @@ def self.sleep_time(num_retries) sleep_seconds *= (0.5 * (1 + rand)) # But never sleep less than the base sleep seconds. - sleep_seconds = [Stripe.initial_network_retry_delay, sleep_seconds].max + sleep_seconds = [config.initial_network_retry_delay, sleep_seconds].max sleep_seconds end @@ -187,8 +199,8 @@ def execute_request(method, path, raise ArgumentError, "path should be a string" \ unless path.is_a?(String) - api_base ||= Stripe.api_base - api_key ||= Stripe.api_key + api_base ||= config.api_base + api_key ||= config.api_key params = Util.objects_to_ids(params) check_api_key!(api_key) @@ -231,10 +243,12 @@ def execute_request(method, path, context.query = query http_resp = execute_request_with_rescues(method, api_base, context) do - self.class.default_connection_manager.execute_request(method, url, - body: body, - headers: headers, - query: query) + self.class + .default_connection_manager(config) + .execute_request(method, url, + body: body, + headers: headers, + query: query) end begin @@ -246,13 +260,21 @@ def execute_request(method, path, # If being called from `StripeClient#request`, put the last response in # thread-local memory so that it can be returned to the user. Don't store # anything otherwise so that we don't leak memory. - if self.class.current_thread_context.last_responses&.key?(object_id) - self.class.current_thread_context.last_responses[object_id] = resp - end + store_last_response(object_id, resp) [resp, api_key] end + 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 + # # private # @@ -397,7 +419,7 @@ def self.maybe_gc_connection_managers end private def api_url(url = "", api_base = nil) - (api_base || Stripe.api_base) + url + (api_base || config.api_base) + url end private def check_api_key!(api_key) @@ -471,7 +493,7 @@ def self.maybe_gc_connection_managers notify_request_end(context, request_duration, http_status, num_retries, user_data) - if Stripe.enable_telemetry? && context.request_id + if config.enable_telemetry? && context.request_id request_duration_ms = (request_duration * 1000).to_i @last_request_metrics = StripeRequestMetrics.new(context.request_id, request_duration_ms) @@ -498,9 +520,12 @@ def self.maybe_gc_connection_managers notify_request_end(context, request_duration, http_status, num_retries, user_data) - if self.class.should_retry?(e, method: method, num_retries: num_retries) + if self.class.should_retry?(e, + method: method, + num_retries: num_retries, + config: config) num_retries += 1 - sleep self.class.sleep_time(num_retries) + sleep self.class.sleep_time(num_retries, config: config) retry end @@ -622,7 +647,8 @@ def self.maybe_gc_connection_managers error_param: error_data[:param], error_type: error_data[:type], idempotency_key: context.idempotency_key, - request_id: context.request_id) + request_id: context.request_id, + config: config) # The standard set of arguments that can be used to initialize most of # the exceptions. @@ -671,7 +697,8 @@ def self.maybe_gc_connection_managers error_code: error_code, error_description: description, idempotency_key: context.idempotency_key, - request_id: context.request_id) + request_id: context.request_id, + config: config) args = { http_status: resp.http_status, http_body: resp.http_body, @@ -703,7 +730,8 @@ def self.maybe_gc_connection_managers Util.log_error("Stripe network error", error_message: error.message, idempotency_key: context.idempotency_key, - request_id: context.request_id) + request_id: context.request_id, + config: config) errors, message = NETWORK_ERROR_MESSAGES_MAP.detect do |(e, _)| error.is_a?(e) @@ -714,7 +742,7 @@ def self.maybe_gc_connection_managers "with Stripe. Please let us know at support@stripe.com." end - api_base ||= Stripe.api_base + api_base ||= config.api_base message = message % api_base message += " Request was retried #{num_retries} times." if num_retries > 0 @@ -735,7 +763,7 @@ def self.maybe_gc_connection_managers "Content-Type" => "application/x-www-form-urlencoded", } - if Stripe.enable_telemetry? && !@last_request_metrics.nil? + if config.enable_telemetry? && !@last_request_metrics.nil? headers["X-Stripe-Client-Telemetry"] = JSON.generate( last_request_metrics: @last_request_metrics.payload ) @@ -743,12 +771,12 @@ def self.maybe_gc_connection_managers # It is only safe to retry network failures on post and delete # requests if we add an Idempotency-Key header - if %i[post delete].include?(method) && Stripe.max_network_retries > 0 + if %i[post delete].include?(method) && config.max_network_retries > 0 headers["Idempotency-Key"] ||= SecureRandom.uuid end - headers["Stripe-Version"] = Stripe.api_version if Stripe.api_version - headers["Stripe-Account"] = Stripe.stripe_account if Stripe.stripe_account + headers["Stripe-Version"] = config.api_version if config.api_version + headers["Stripe-Account"] = config.stripe_account if config.stripe_account user_agent = @system_profiler.user_agent begin @@ -772,11 +800,13 @@ def self.maybe_gc_connection_managers idempotency_key: context.idempotency_key, method: context.method, num_retries: num_retries, - path: context.path) + path: context.path, + config: config) Util.log_debug("Request details", body: context.body, idempotency_key: context.idempotency_key, - query: context.query) + query: context.query, + config: config) end private def log_response(context, request_start, status, body) @@ -788,11 +818,13 @@ def self.maybe_gc_connection_managers method: context.method, path: context.path, request_id: context.request_id, - status: status) + status: status, + config: config) Util.log_debug("Response details", body: body, idempotency_key: context.idempotency_key, - request_id: context.request_id) + request_id: context.request_id, + config: config) return unless context.request_id @@ -800,7 +832,8 @@ def self.maybe_gc_connection_managers idempotency_key: context.idempotency_key, request_id: context.request_id, url: Util.request_id_dashboard_url(context.request_id, - context.api_key)) + context.api_key), + config: config) end private def log_response_error(context, request_start, error) @@ -810,7 +843,8 @@ def self.maybe_gc_connection_managers error_message: error.message, idempotency_key: context.idempotency_key, method: context.method, - path: context.path) + path: context.path, + config: config) end # RequestLogContext stores information about a request that's begin made so diff --git a/lib/stripe/stripe_configuration.rb b/lib/stripe/stripe_configuration.rb index 71f01f2a4..fcef741bd 100644 --- a/lib/stripe/stripe_configuration.rb +++ b/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 attr_accessor :enable_telemetry attr_accessor :logger attr_accessor :stripe_account @@ -99,6 +98,14 @@ def max_network_retries=(val) @max_network_retries = val.to_i end + 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 + def open_timeout=(open_timeout) @open_timeout = open_timeout StripeClient.clear_all_connection_managers diff --git a/lib/stripe/util.rb b/lib/stripe/util.rb index dfb4146ac..28d1c18ee 100644 --- a/lib/stripe/util.rb +++ b/lib/stripe/util.rb @@ -39,10 +39,16 @@ def self.objects_to_ids(obj) end end + # Returns a hash of all Stripe object classes. def self.object_classes @object_classes ||= Stripe::ObjectTypes.object_names_to_classes end + # Returns a hash containling only Stripe API object classes. + def self.api_object_classes + @api_object_classes ||= ::Stripe::ObjectTypes.api_object_names_to_classes + end + def self.object_name_matches_class?(object_name, klass) Util.object_classes[object_name] == klass end @@ -76,24 +82,30 @@ def self.convert_to_stripe_object(data, opts = {}) end def self.log_error(message, data = {}) - if !Stripe.logger.nil? || - !Stripe.log_level.nil? && Stripe.log_level <= Stripe::LEVEL_ERROR + config = data.delete(:config) || Stripe.configuration + logger = config.logger || Stripe.logger + if !logger.nil? || + !config.log_level.nil? && config.log_level <= Stripe::LEVEL_ERROR log_internal(message, data, color: :cyan, level: Stripe::LEVEL_ERROR, logger: Stripe.logger, out: $stderr) end end def self.log_info(message, data = {}) - if !Stripe.logger.nil? || - !Stripe.log_level.nil? && Stripe.log_level <= Stripe::LEVEL_INFO + config = data.delete(:config) || Stripe.configuration + logger = config.logger || Stripe.logger + if !logger.nil? || + !config.log_level.nil? && config.log_level <= Stripe::LEVEL_INFO log_internal(message, data, color: :cyan, level: Stripe::LEVEL_INFO, logger: Stripe.logger, out: $stdout) end end def self.log_debug(message, data = {}) - if !Stripe.logger.nil? || - !Stripe.log_level.nil? && Stripe.log_level <= Stripe::LEVEL_DEBUG + config = data.delete(:config) || Stripe.configuration + logger = config.logger || Stripe.logger + if !logger.nil? || + !config.log_level.nil? && config.log_level <= Stripe::LEVEL_DEBUG log_internal(message, data, color: :blue, level: Stripe::LEVEL_DEBUG, logger: Stripe.logger, out: $stdout) end diff --git a/test/stripe/account_test.rb b/test/stripe/account_test.rb index e4a135e31..218ada6dd 100644 --- a/test/stripe/account_test.rb +++ b/test/stripe/account_test.rb @@ -93,6 +93,22 @@ class AccountTest < Test::Unit::TestCase .to_return(body: JSON.generate("stripe_user_id" => account.id)) account.deauthorize("ca_1234", "sk_test_1234") end + + context "when the caller is a StripeClient" do + should "use the StripeClient options" do + client = Stripe::StripeClient.new(connect_base: "https://other.stripe.com") + account = client.accounts.retrieve("acct_123") + + # Unfortunately, the OpenAPI spec doesn't yet cover anything under the + # Connect endpoints, so for just stub this out with Webmock. + stub_request(:post, "https://other.stripe.com/oauth/deauthorize") + .with(body: { "client_id" => "ca_1234", "stripe_user_id" => account.id }) + .to_return(body: JSON.generate("stripe_user_id" => account.id)) + resp = account.deauthorize("ca_1234", "sk_test_1234") + + assert_equal(account.id, resp.stripe_user_id) + end + end end context "#legal_entity=" do diff --git a/test/stripe/connection_manager_test.rb b/test/stripe/connection_manager_test.rb index e12a35a24..3a41022ea 100644 --- a/test/stripe/connection_manager_test.rb +++ b/test/stripe/connection_manager_test.rb @@ -75,6 +75,49 @@ class ConnectionManagerTest < Test::Unit::TestCase end end + context "when a StripeClient has different configurations" do + should "correctly initialize a connection" do + old_proxy = Stripe.proxy + + old_open_timeout = Stripe.open_timeout + old_read_timeout = Stripe.read_timeout + + begin + client = StripeClient.new( + proxy: "http://other:pass@localhost:8080", + open_timeout: 400, + read_timeout: 500, + verify_ssl_certs: true + ) + conn = Stripe::ConnectionManager.new(client.config) + .connection_for("https://stripe.com") + + # Host/port + assert_equal "stripe.com", conn.address + assert_equal 443, conn.port + + # Proxy + assert_equal "localhost", conn.proxy_address + assert_equal 8080, conn.proxy_port + assert_equal "other", conn.proxy_user + assert_equal "pass", conn.proxy_pass + + # Timeouts + assert_equal 400, conn.open_timeout + assert_equal 500, conn.read_timeout + + assert_equal true, conn.use_ssl? + assert_equal OpenSSL::SSL::VERIFY_PEER, conn.verify_mode + assert_equal Stripe.ca_store, conn.cert_store + ensure + Stripe.proxy = old_proxy + + Stripe.open_timeout = old_open_timeout + Stripe.read_timeout = old_read_timeout + end + end + end + should "produce the same connection multiple times" do conn1 = @manager.connection_for("https://stripe.com") conn2 = @manager.connection_for("https://stripe.com") diff --git a/test/stripe/file_test.rb b/test/stripe/file_test.rb index 1b16f5bbe..d3a3b6ccd 100644 --- a/test/stripe/file_test.rb +++ b/test/stripe/file_test.rb @@ -72,6 +72,21 @@ class FileTest < Test::Unit::TestCase end assert_equal "file must respond to `#read`", e.message end + + context "when the caller is a StripeClient" do + should "permit the StripeClient to set the `api_base`" do + client = StripeClient.new(uploads_base: Stripe.uploads_base) + Stripe.configuration.stubs(:uploads_base).returns("old") + + file = client.files.create( + purpose: "dispute_evidence", + file: ::File.new(__FILE__), + file_link_data: { create: true } + ) + assert_requested :post, "#{client.config.uploads_base}/v1/files" + assert file.is_a?(Stripe::File) + end + end end should "be deserializable when `object=file`" do diff --git a/test/stripe/oauth_test.rb b/test/stripe/oauth_test.rb index c8ac13f2e..d6ebb8a24 100644 --- a/test/stripe/oauth_test.rb +++ b/test/stripe/oauth_test.rb @@ -28,7 +28,6 @@ class OAuthTest < Test::Unit::TestCase assert_equal("https", uri.scheme) assert_equal("connect.stripe.com", uri.host) assert_equal("/oauth/authorize", uri.path) - assert_equal(["ca_test"], params["client_id"]) assert_equal(["read_write"], params["scope"]) assert_equal(["test@example.com"], params["stripe_user[email]"]) @@ -44,6 +43,14 @@ class OAuthTest < Test::Unit::TestCase assert_equal("connect.stripe.com", uri.host) assert_equal("/express/oauth/authorize", uri.path) end + + should "override the api base path when a StripeClient is provided" do + client = Stripe::StripeClient.new(connect_base: "https://other.stripe.com") + uri_str = OAuth.authorize_url(client: client) + + uri = URI.parse(uri_str) + assert_equal("other.stripe.com", uri.host) + end end context ".token" do @@ -83,6 +90,28 @@ class OAuthTest < Test::Unit::TestCase code: "this_is_an_authorization_code") assert_equal("another_access_token", resp.access_token) end + + should "override the api base path when a StripeClient is provided" do + stub_request(:post, "https://other.stripe.com/oauth/token") + .with(body: { + "grant_type" => "authorization_code", + "code" => "this_is_an_authorization_code", + }) + .to_return(body: JSON.generate(access_token: "sk_access_token", + scope: "read_only", + livemode: false, + token_type: "bearer", + refresh_token: "sk_refresh_token", + stripe_user_id: "acct_test", + stripe_publishable_key: "pk_test")) + + client = Stripe::StripeClient.new(connect_base: "https://other.stripe.com") + resp = OAuth.token(grant_type: "authorization_code", + code: "this_is_an_authorization_code", + client: client) + + assert_equal("sk_access_token", resp.access_token) + end end context ".deauthorize" do @@ -99,6 +128,20 @@ class OAuthTest < Test::Unit::TestCase resp = OAuth.deauthorize(stripe_user_id: "acct_test_deauth") assert_equal("acct_test_deauth", resp.stripe_user_id) end + + should "override the api base path when a StripeClient is provided" do + stub_request(:post, "https://other.stripe.com/oauth/deauthorize") + .with(body: { + "client_id" => "ca_test", + "stripe_user_id" => "acct_test_deauth", + }) + .to_return(body: JSON.generate(stripe_user_id: "acct_test_deauth")) + + client = Stripe::StripeClient.new(connect_base: "https://other.stripe.com") + resp = OAuth.deauthorize(stripe_user_id: "acct_test_deauth", client: client) + + assert_equal("acct_test_deauth", resp.stripe_user_id) + end end end end diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index c00ed510d..a5e7a0efc 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -164,7 +164,7 @@ class StripeClientTest < Test::Unit::TestCase context ".should_retry?" do setup do - Stripe.stubs(:max_network_retries).returns(2) + Stripe::StripeConfiguration.any_instance.stubs(:max_network_retries).returns(2) end should "retry on Errno::ECONNREFUSED" do @@ -275,7 +275,7 @@ class StripeClientTest < Test::Unit::TestCase context ".sleep_time" do should "should grow exponentially" do StripeClient.stubs(:rand).returns(1) - Stripe.stubs(:max_network_retry_delay).returns(999) + Stripe.configuration.stubs(:max_network_retry_delay).returns(999) assert_equal(Stripe.initial_network_retry_delay, StripeClient.sleep_time(1)) assert_equal(Stripe.initial_network_retry_delay * 2, StripeClient.sleep_time(2)) assert_equal(Stripe.initial_network_retry_delay * 4, StripeClient.sleep_time(3)) @@ -284,8 +284,8 @@ class StripeClientTest < Test::Unit::TestCase should "enforce the max_network_retry_delay" do StripeClient.stubs(:rand).returns(1) - Stripe.stubs(:initial_network_retry_delay).returns(1) - Stripe.stubs(:max_network_retry_delay).returns(2) + Stripe.configuration.stubs(:initial_network_retry_delay).returns(1) + Stripe.configuration.stubs(:max_network_retry_delay).returns(2) assert_equal(1, StripeClient.sleep_time(1)) assert_equal(2, StripeClient.sleep_time(2)) assert_equal(2, StripeClient.sleep_time(3)) @@ -295,8 +295,8 @@ class StripeClientTest < Test::Unit::TestCase should "add some randomness" do random_value = 0.8 StripeClient.stubs(:rand).returns(random_value) - Stripe.stubs(:initial_network_retry_delay).returns(1) - Stripe.stubs(:max_network_retry_delay).returns(8) + Stripe.configuration.stubs(:initial_network_retry_delay).returns(1) + Stripe.configuration.stubs(:max_network_retry_delay).returns(8) base_value = Stripe.initial_network_retry_delay * (0.5 * (1 + random_value)) @@ -309,6 +309,23 @@ class StripeClientTest < Test::Unit::TestCase assert_equal(base_value * 4, StripeClient.sleep_time(3)) assert_equal(base_value * 8, StripeClient.sleep_time(4)) end + + should "permit passing in a configuration object" do + StripeClient.stubs(:rand).returns(1) + config = Stripe::StripeConfiguration.setup do |c| + c.initial_network_retry_delay = 1 + c.max_network_retry_delay = 2 + end + + # Set the global configuration to be different than the clinet + Stripe.configuration.stubs(:initial_network_retry_delay).returns(100) + Stripe.configuration.stubs(:max_network_retry_delay).returns(200) + + assert_equal(1, StripeClient.sleep_time(1, config: config)) + assert_equal(2, StripeClient.sleep_time(2, config: config)) + assert_equal(2, StripeClient.sleep_time(3, config: config)) + assert_equal(2, StripeClient.sleep_time(4, config: config)) + end end context "#execute_request" do @@ -342,6 +359,10 @@ class StripeClientTest < Test::Unit::TestCase # switch over to rspec-mocks at some point, we can probably remove # this. Util.stubs(:monotonic_time).returns(0.0) + + # Stub the Stripe configuration so that mocha matchers will succeed. Currently, + # mocha does not support using param matchers within hashes. + StripeClient.any_instance.stubs(:config).returns(Stripe.configuration) end should "produce appropriate logging" do @@ -353,11 +374,13 @@ class StripeClientTest < Test::Unit::TestCase idempotency_key: "abc", method: :post, num_retries: 0, - path: "/v1/account") + path: "/v1/account", + config: Stripe.configuration) Util.expects(:log_debug).with("Request details", body: "", idempotency_key: "abc", - query: nil) + query: nil, + config: Stripe.configuration) Util.expects(:log_info).with("Response from Stripe API", account: "acct_123", @@ -367,15 +390,18 @@ class StripeClientTest < Test::Unit::TestCase method: :post, path: "/v1/account", request_id: "req_123", - status: 200) + status: 200, + config: Stripe.configuration) Util.expects(:log_debug).with("Response details", body: body, idempotency_key: "abc", - request_id: "req_123") + request_id: "req_123", + config: Stripe.configuration) Util.expects(:log_debug).with("Dashboard link for request", idempotency_key: "abc", request_id: "req_123", - url: Util.request_id_dashboard_url("req_123", Stripe.api_key)) + url: Util.request_id_dashboard_url("req_123", Stripe.api_key), + config: Stripe.configuration) stub_request(:post, "#{Stripe.api_base}/v1/account") .to_return( @@ -404,7 +430,8 @@ class StripeClientTest < Test::Unit::TestCase idempotency_key: nil, method: :post, num_retries: 0, - path: "/v1/account") + path: "/v1/account", + config: Stripe.configuration) Util.expects(:log_info).with("Response from Stripe API", account: nil, api_version: nil, @@ -413,7 +440,8 @@ class StripeClientTest < Test::Unit::TestCase method: :post, path: "/v1/account", request_id: nil, - status: 500) + status: 500, + config: Stripe.configuration) error = { code: "code", @@ -428,7 +456,8 @@ class StripeClientTest < Test::Unit::TestCase error_param: error[:param], error_type: error[:type], idempotency_key: nil, - request_id: nil) + request_id: nil, + config: Stripe.configuration) stub_request(:post, "#{Stripe.api_base}/v1/account") .to_return( @@ -449,7 +478,8 @@ class StripeClientTest < Test::Unit::TestCase idempotency_key: nil, method: :post, num_retries: 0, - path: "/oauth/token") + path: "/oauth/token", + config: Stripe.configuration) Util.expects(:log_info).with("Response from Stripe API", account: nil, api_version: nil, @@ -458,14 +488,16 @@ class StripeClientTest < Test::Unit::TestCase method: :post, path: "/oauth/token", request_id: nil, - status: 400) + status: 400, + config: Stripe.configuration) Util.expects(:log_error).with("Stripe OAuth error", status: 400, error_code: "invalid_request", error_description: "No grant type specified", idempotency_key: nil, - request_id: nil) + request_id: nil, + config: Stripe.configuration) stub_request(:post, "#{Stripe.connect_base}/oauth/token") .to_return(body: JSON.generate(error: "invalid_request", @@ -788,7 +820,7 @@ class StripeClientTest < Test::Unit::TestCase context "idempotency keys" do setup do - Stripe.stubs(:max_network_retries).returns(2) + Stripe::StripeConfiguration.any_instance.stubs(:max_network_retries).returns(2) end should "not add an idempotency key to GET requests" do @@ -838,7 +870,7 @@ class StripeClientTest < Test::Unit::TestCase context "retry logic" do setup do - Stripe.stubs(:max_network_retries).returns(2) + Stripe::StripeConfiguration.any_instance.stubs(:max_network_retries).returns(2) end should "retry failed requests and raise if error persists" do @@ -870,6 +902,21 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.execute_request(:post, "/v1/charges") end + + should "pass the client configuration when retrying" do + StripeClient.expects(:sleep_time) + .with(any_of(1, 2), + has_entry(:config, kind_of(Stripe::StripeConfiguration))) + .at_least_once.returns(0) + + stub_request(:post, "#{Stripe.api_base}/v1/charges") + .to_raise(Errno::ECONNREFUSED.new) + + client = StripeClient.new + assert_raises Stripe::APIConnectionError do + client.execute_request(:post, "/v1/charges") + end + end end context "params serialization" do @@ -1145,6 +1192,162 @@ class StripeClientTest < Test::Unit::TestCase end end + context "API resource instance methods" do + should "define methods for all api resources" do + client = StripeClient.new + + # Update Sigma name to account for nuance + api_resources = Stripe::Util.api_object_classes + sigma_class = api_resources.delete("scheduled_query_run") + api_resources["sigma.scheduled_query_run"] = sigma_class + + api_resources.each do |string, _| + if string.include?(".") + resource_module, resource_name = string.split(".") + + assert client.respond_to?(resource_module), "#{resource_module} not found" + assert client.send(resource_module).respond_to?("#{resource_name}s"), "#{resource_name} not found" + else + assert client.respond_to?("#{string}s"), "#{string} not found" + end + end + end + + should "make expected request on a singular API resource" do + client = StripeClient.new + account = client.accounts.retrieve("acct_1234") + assert_requested(:get, "#{Stripe.api_base}/v1/accounts/acct_1234") + assert account.is_a?(Stripe::Account) + end + + should "make expected request on a namespaces API resource" do + client = StripeClient.new + list = client.radar.value_lists.retrieve("rsl_123") + assert_requested(:get, "#{Stripe.api_base}/v1/radar/value_lists/rsl_123") + assert list.is_a?(Stripe::Radar::ValueList) + end + + should "allow for listing a resource" do + client = StripeClient.new + accounts = client.accounts.list + assert_requested(:get, "#{Stripe.api_base}/v1/accounts") + assert accounts.data.is_a?(Array) + assert accounts.data[0].is_a?(Stripe::Account) + end + + should "allow for listing a resource with filters" do + client = StripeClient.new + accounts = client.accounts.list({ limit: 10 }) + assert_requested(:get, "#{Stripe.api_base}/v1/accounts?limit=10") + assert accounts.data.is_a?(Array) + assert accounts.data[0].is_a?(Stripe::Account) + end + + should "allow for deleting a resource" do + client = StripeClient.new + account = client.accounts.retrieve("acct_123") + account = account.delete + assert_requested :delete, "#{Stripe.api_base}/v1/accounts/#{account.id}" + assert account.is_a?(Stripe::Account) + end + + should "allow for creating a resource" do + client = StripeClient.new + charge = client.charges.create( + amount: 100, + currency: "USD", + source: "src_123" + ) + assert_requested :post, "#{Stripe.api_base}/v1/charges" + assert charge.is_a?(Stripe::Charge) + end + + should "allow for updating a resource" do + client = StripeClient.new + account = client.accounts.update("acct_123", metadata: { foo: "bar" }) + assert_requested(:post, + "#{Stripe.api_base}/v1/accounts/acct_123", + body: { metadata: { foo: "bar" } }) + assert account.is_a?(Stripe::Account) + end + + context "setting configurable options" do + should "override any global configurations" do + Stripe.api_key = "sk_test_old" + + client = StripeClient.new(api_key: "sk_test_new") + client.accounts.retrieve("acct_1234") + assert_requested(:get, + "#{Stripe.api_base}/v1/accounts/acct_1234", + headers: { + "Authorization" => "Bearer sk_test_new", + }) + end + + should "use global settings by default" do + client = StripeClient.new + client.accounts.retrieve("acct_1234") + assert_requested(:get, + "#{Stripe.api_base}/v1/accounts/acct_1234", + headers: { + "Authorization" => "Bearer #{Stripe.api_key}", + }) + end + + should "allow for overrides when retrieving a resource" do + client = StripeClient.new(api_key: "sk_test_local") + account = client.accounts.retrieve("acct_123", { api_key: "sk_test_other" }) + assert_requested(:get, "#{Stripe.api_base}/v1/accounts/acct_123", + headers: { "Authorization" => "Bearer sk_test_other" }) + assert account.is_a?(Stripe::Account) + end + + should "allow for retrieving a resource with options" do + client = Stripe::StripeClient.new(api_key: "sk_test_local") + account = client.charges.retrieve(id: "acct_123", expand: ["customer"]) + assert_requested(:get, "#{Stripe.api_base}/v1/charges/acct_123", + headers: { "Authorization" => "Bearer sk_test_local" }, + query: { "expand[]" => "customer" }) + assert account.is_a?(Stripe::Charge) + end + + should "allow for overrides when operating on a collection" do + client = StripeClient.new(api_key: "sk_test_local") + accounts = client.accounts.list({}, { api_key: "sk_test_other" }) + assert_requested(:get, + "#{Stripe.api_base}/v1/accounts", + headers: { "Authorization" => "Bearer sk_test_other" }) + assert accounts.data.is_a?(Array) + assert accounts.data[0].is_a?(Stripe::Account) + end + + should "allow for overrides when operating on a resource" do + client = StripeClient.new(api_key: "sk_test_local") + account = client.accounts.update("acct_123", + {}, + { api_key: "sk_test_other" }) + assert_requested(:post, + "#{Stripe.api_base}/v1/accounts/acct_123", + headers: { "Authorization" => "Bearer sk_test_other" }) + assert account.is_a?(Stripe::Account) + end + + should "allow for overrides when operating on an instance" do + client = StripeClient.new(api_key: "sk_test_new") + account = client.accounts.retrieve("acct_123") + account.metadata = { foo: "bar" } + account.save + assert_requested(:post, + "#{Stripe.api_base}/v1/accounts/acct_123", + body: { metadata: { foo: "bar" } }, + headers: { + "Authorization" => "Bearer sk_test_new", + }) + assert account.is_a?(Stripe::Account) + end + end + end + context "instrumentation" do teardown do Stripe::Instrumentation.unsubscribe(:request, :test) diff --git a/test/stripe/stripe_configuration_test.rb b/test/stripe/stripe_configuration_test.rb index 774d5ea3f..8bb4dad84 100644 --- a/test/stripe/stripe_configuration_test.rb +++ b/test/stripe/stripe_configuration_test.rb @@ -19,6 +19,7 @@ class StripeConfigurationTest < Test::Unit::TestCase assert_equal "https://api.stripe.com", config.api_base assert_equal "https://connect.stripe.com", config.connect_base assert_equal "https://files.stripe.com", config.uploads_base + assert_equal nil, config.api_version end should "allow for overrides when a block is passed" do @@ -38,7 +39,7 @@ class StripeConfigurationTest < Test::Unit::TestCase c.open_timeout = 100 end - duped_config = config.reverse_duplicate_merge(read_timeout: 500) + duped_config = config.reverse_duplicate_merge(read_timeout: 500, api_version: "2018-08-02") assert_equal config.open_timeout, duped_config.open_timeout assert_equal 500, duped_config.read_timeout @@ -54,6 +55,24 @@ class StripeConfigurationTest < Test::Unit::TestCase end end + context "#max_network_retry_delay=" do + should "coerce the option into an integer" do + config = Stripe::StripeConfiguration.setup + + config.max_network_retry_delay = "10" + assert_equal 10, config.max_network_retry_delay + end + end + + context "#initial_network_retry_delay=" do + should "coerce the option into an integer" do + config = Stripe::StripeConfiguration.setup + + config.initial_network_retry_delay = "10" + assert_equal 10, config.initial_network_retry_delay + end + end + context "#log_level=" do should "be backwards compatible with old values" do config = Stripe::StripeConfiguration.setup diff --git a/test/stripe_test.rb b/test/stripe_test.rb index 11f100b85..f3c11ae35 100644 --- a/test/stripe_test.rb +++ b/test/stripe_test.rb @@ -101,6 +101,11 @@ class StripeTest < Test::Unit::TestCase assert_equal "https://other.stripe.com", Stripe.api_base end + should "allow api_version to be configured" do + Stripe.api_version = "2018-02-28" + assert_equal "2018-02-28", Stripe.api_version + end + should "allow connect_base to be configured" do Stripe.connect_base = "https://other.stripe.com" assert_equal "https://other.stripe.com", Stripe.connect_base