From 190052b580722e8d62a87c9f406b6e6ae8c0d98e Mon Sep 17 00:00:00 2001 From: Joel Taylor Date: Sun, 28 Mar 2021 19:04:22 -0700 Subject: [PATCH] Allow StripeClient to be configured per instance This changes allows for each instance of StripeClient to have its own configuration object instead of relying on the global config. Each instance can be configured to override any global config values previously set. --- lib/stripe.rb | 2 + lib/stripe/connection_manager.rb | 18 +-- lib/stripe/oauth.rb | 7 +- lib/stripe/resources/account.rb | 11 +- lib/stripe/resources/file.rb | 3 +- lib/stripe/stripe_client.rb | 161 +++++++++++++++-------- lib/stripe/stripe_configuration.rb | 18 +++ lib/stripe/util.rb | 18 ++- test/stripe/connection_manager_test.rb | 43 ++++++ test/stripe/oauth_test.rb | 45 +++++++ test/stripe/stripe_client_test.rb | 126 ++++++++++++++---- test/stripe/stripe_configuration_test.rb | 30 ++++- test/stripe_test.rb | 5 + 13 files changed, 380 insertions(+), 107 deletions(-) diff --git a/lib/stripe.rb b/lib/stripe.rb index 759269a64..06e6f5b70 100644 --- a/lib/stripe.rb +++ b/lib/stripe.rb @@ -62,6 +62,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/connection_manager.rb b/lib/stripe/connection_manager.rb index e3ecc4bcc..0058cdc7b 100644 --- a/lib/stripe/connection_manager.rb +++ b/lib/stripe/connection_manager.rb @@ -15,8 +15,10 @@ class ConnectionManager # by `StripeClient` to determine whether a connection manager should be # garbage collected or not. attr_reader :last_used + attr_reader :config - def initialize + def initialize(config = Stripe.configuration) + @config = config @active_connections = {} @last_used = Util.monotonic_time @@ -117,17 +119,17 @@ 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 if connection.respond_to?(:write_timeout=) - connection.write_timeout = Stripe.write_timeout + connection.write_timeout = config.write_timeout end 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 @@ -141,10 +143,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..580b424c8 100644 --- a/lib/stripe/oauth.rb +++ b/lib/stripe/oauth.rb @@ -7,8 +7,8 @@ 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] ||= opts[:client] || StripeClient.active_client + opts[:api_base] ||= opts[:client].config.connect_base super(method, url, params, opts) end @@ -29,7 +29,8 @@ def self.get_client_id(params = {}) end def self.authorize_url(params = {}, opts = {}) - base = opts[:connect_base] || Stripe.connect_base + client = opts[: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/resources/account.rb b/lib/stripe/resources/account.rb index c4dbb36bc..53e96d0df 100644 --- a/lib/stripe/resources/account.rb +++ b/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 = {}) - id = if id.equal?(ARGUMENT_NOT_PROVIDED) - nil - else - Util.check_string_argument!(id) - end + def self.retrieve(id = nil, opts = {}) + Util.check_string_argument!(id) if id # Account used to be a singleton, where this method's signature was # `(opts={})`. For the sake of not breaking folks who pass in an OAuth @@ -136,11 +132,10 @@ 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 - ARGUMENT_NOT_PROVIDED = Object.new - private def serialize_additional_owners(legal_entity, additional_owners) original_value = legal_entity 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..7072508b7 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -13,15 +13,31 @@ class StripeClient @thread_contexts_with_connection_managers_mutex = Mutex.new @last_connection_manager_gc = Util.monotonic_time - # Initializes a new `StripeClient`. - # - # Takes a connection manager object for backwards compatibility only, and - # that use is DEPRECATED. - def initialize(_connection_manager = nil) + # Initializes a new StripeClient + def initialize(config_overrides = {}) @system_profiler = SystemProfiler.new @last_request_metrics = nil + + # Supports accepting a connection manager object for backwards + # compatibility only, and that use is DEPRECATED. + @config_overrides = case config_overrides + when Stripe::ConnectionManager + {} + when String + { api_key: config_overrides } + else + config_overrides + end + end + + # Always base config off the global Stripe configuration to ensure the + # client picks up any changes to the config. + def config + Stripe.configuration.reverse_duplicate_merge(@config_overrides) 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 @@ -51,8 +67,8 @@ def self.clear_all_connection_managers # its connection manager and remove our reference to it. If it ever # makes a new request we'll give it a new connection manager and # it'll go back into `@thread_contexts_with_connection_managers`. - thread_context.default_connection_manager.clear - thread_context.default_connection_manager = nil + thread_context.default_connection_managers.map { |_, cm| cm.clear } + thread_context.reset_connection_managers end @thread_contexts_with_connection_managers.clear end @@ -63,10 +79,11 @@ def self.default_client current_thread_context.default_client ||= StripeClient.new end - # A default connection manager for the current thread. - def self.default_connection_manager - current_thread_context.default_connection_manager ||= begin - connection_manager = ConnectionManager.new + # A default connection manager for the current thread scoped to the + # configuration object that may be provided. + def self.default_connection_manager(config = Stripe.configuration) + current_thread_context.default_connection_managers[config.key] ||= begin + connection_manager = ConnectionManager.new(config) @thread_contexts_with_connection_managers_mutex.synchronize do maybe_gc_connection_managers @@ -80,8 +97,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 +145,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,9 +159,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 end # Gets the connection manager in use for the current `StripeClient`. @@ -187,8 +203,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 +247,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 +264,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 # @@ -328,11 +354,6 @@ class ThreadContext # the user hasn't specified their own. attr_accessor :default_client - # A default `ConnectionManager` for the thread. Normally shared between - # all `StripeClient` objects on a particular thread, and created so as to - # minimize the number of open connections that an application needs. - attr_accessor :default_connection_manager - # A temporary map of object IDs to responses from last executed API # calls. Used to return a responses from calls to `StripeClient#request`. # @@ -345,6 +366,17 @@ class ThreadContext # because that's wrapped in an `ensure` block, they should never leave # garbage in `Thread.current`. attr_accessor :last_responses + + # A default `ConnectionManager` for the thread. Normally shared between + # all `StripeClient` objects on a particular thread, and created so as to + # minimize the number of open connections that an application needs. + def default_connection_managers + @default_connection_managers ||= {} + end + + def reset_connection_managers + @default_connection_managers = {} + end end # Access data stored for `StripeClient` within the thread's current @@ -382,12 +414,15 @@ def self.maybe_gc_connection_managers pruned_thread_contexts = [] @thread_contexts_with_connection_managers.each do |thread_context| - connection_manager = thread_context.default_connection_manager - next if connection_manager.last_used > last_used_threshold - - connection_manager.clear - thread_context.default_connection_manager = nil - pruned_thread_contexts << thread_context + thread_context + .default_connection_managers + .each do |_, connection_manager| + next if connection_manager.last_used > last_used_threshold + + connection_manager.clear + thread_context.reset_connection_managers + pruned_thread_contexts << thread_context + end end @thread_contexts_with_connection_managers -= pruned_thread_contexts @@ -397,7 +432,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 +506,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 +533,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 +660,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 +710,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 +743,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 +755,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 +776,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 +784,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 +813,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 +831,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 +845,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 +856,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 814d3e650..097854974 100644 --- a/lib/stripe/stripe_configuration.rb +++ b/lib/stripe/stripe_configuration.rb @@ -101,6 +101,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 @@ -174,5 +182,15 @@ def ca_store def enable_telemetry? enable_telemetry end + + # Generates a deterministic key to identify configuration objects with + # identical configuration values. + def key + @key ||= begin + instance_variables + .collect { |variable| instance_variable_get(variable) } + .join + end + end end end diff --git a/lib/stripe/util.rb b/lib/stripe/util.rb index dfb4146ac..343fa7756 100644 --- a/lib/stripe/util.rb +++ b/lib/stripe/util.rb @@ -76,24 +76,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/connection_manager_test.rb b/test/stripe/connection_manager_test.rb index f73a56eac..c5efcb3b0 100644 --- a/test/stripe/connection_manager_test.rb +++ b/test/stripe/connection_manager_test.rb @@ -79,6 +79,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/oauth_test.rb b/test/stripe/oauth_test.rb index c8ac13f2e..be7b4dc56 100644 --- a/test/stripe/oauth_test.rb +++ b/test/stripe/oauth_test.rb @@ -44,6 +44,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 +91,29 @@ 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 +130,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..e872a7b1b 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -4,6 +4,24 @@ module Stripe class StripeClientTest < Test::Unit::TestCase + context "initializing a StripeClient" do + should "allow a String to be passed in order to set the api key" do + assert_equal StripeClient.new("test_123").config.api_key, "test_123" + end + + should "allow for overrides via a Hash" do + config = { api_key: "test_123", open_timeout: 100 } + client = StripeClient.new(config) + + assert_equal client.config.api_key, "test_123" + assert_equal client.config.open_timeout, 100 + end + + should "support deprecated ConnectionManager objects" do + assert StripeClient.new(Stripe::ConnectionManager.new).config.is_a?(Stripe::StripeConfiguration) + end + end + context ".active_client" do should "be .default_client outside of #request" do assert_equal StripeClient.default_client, StripeClient.active_client @@ -82,8 +100,8 @@ class StripeClientTest < Test::Unit::TestCase assert_equal 1, StripeClient.maybe_gc_connection_managers # And as an additional check, the connection manager of the current - # thread context should have been set to `nil` as it was GCed. - assert_nil StripeClient.current_thread_context.default_connection_manager + # thread context should have been removed as it was GCed. + assert_equal({}, StripeClient.current_thread_context.default_connection_managers) end end @@ -160,11 +178,26 @@ class StripeClientTest < Test::Unit::TestCase thread.join refute_equal StripeClient.default_connection_manager, other_thread_manager end + + should "create a separate connection manager per configuration" do + config = Stripe::StripeConfiguration.setup { |c| c.open_timeout = 100 } + connection_manager_one = StripeClient.default_connection_manager + connection_manager_two = StripeClient.default_connection_manager(config) + + assert_equal connection_manager_one.config.open_timeout, 30 + assert_equal connection_manager_two.config.open_timeout, 100 + end + + should "create a single connection manager for identitical configurations" do + 2.times { StripeClient.default_connection_manager(Stripe::StripeConfiguration.setup) } + + assert_equal 1, StripeClient.instance_variable_get(:@thread_contexts_with_connection_managers).first.default_connection_managers.size + end end 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 +308,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 +317,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 +328,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 +342,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 client + 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 +392,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 +407,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 +423,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 +463,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 +473,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 +489,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 +511,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 +521,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 +853,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 +903,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 +935,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 @@ -1079,7 +1159,7 @@ class StripeClientTest < Test::Unit::TestCase context "#proxy" do should "run the request through the proxy" do begin - StripeClient.current_thread_context.default_connection_manager = nil + StripeClient.clear_all_connection_managers Stripe.proxy = "http://user:pass@localhost:8080" @@ -1095,7 +1175,7 @@ class StripeClientTest < Test::Unit::TestCase ensure Stripe.proxy = nil - StripeClient.current_thread_context.default_connection_manager = nil + StripeClient.clear_all_connection_managers end end end diff --git a/test/stripe/stripe_configuration_test.rb b/test/stripe/stripe_configuration_test.rb index 1630aa3d0..9ad743122 100644 --- a/test/stripe/stripe_configuration_test.rb +++ b/test/stripe/stripe_configuration_test.rb @@ -20,6 +20,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 @@ -41,7 +42,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 @@ -57,6 +58,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 @@ -127,5 +146,14 @@ class StripeConfigurationTest < Test::Unit::TestCase config.verify_ssl_certs = false end end + + context "#key" do + should "generate the same key when values are identicial" do + assert_equal StripeConfiguration.setup.key, StripeConfiguration.setup.key + + custom_config = StripeConfiguration.setup { |c| c.open_timeout = 1000 } + refute_equal StripeConfiguration.setup.key, custom_config.key + end + end end end diff --git a/test/stripe_test.rb b/test/stripe_test.rb index 53e9d009f..c0c3bd817 100644 --- a/test/stripe_test.rb +++ b/test/stripe_test.rb @@ -114,6 +114,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