From 21643f0716f34127c6fdbfbe4ed4badc9b7d6a54 Mon Sep 17 00:00:00 2001 From: Joel Taylor Date: Thu, 1 Apr 2021 14:19:38 -0700 Subject: [PATCH] Allow StripeClient to be configured per instance (#968) 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 | 164 +++++++++++++------- lib/stripe/stripe_configuration.rb | 16 ++ lib/stripe/util.rb | 18 ++- test/stripe/connection_manager_test.rb | 43 ++++++ test/stripe/oauth_test.rb | 45 ++++++ test/stripe/stripe_client_test.rb | 182 ++++++++++++++++++++--- test/stripe/stripe_configuration_test.rb | 30 +++- test/stripe_test.rb | 5 + 13 files changed, 438 insertions(+), 106 deletions(-) diff --git a/lib/stripe.rb b/lib/stripe.rb index 759269a6..06e6f5b7 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 e3ecc4bc..0058cdc7 100644 --- a/lib/stripe/connection_manager.rb +++ b/lib/stripe/connection_manager.rb @@ -15,8 +15,10 @@ module Stripe # 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 @@ module Stripe # 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 @@ module Stripe # 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 eab8fd12..580b424c 100644 --- a/lib/stripe/oauth.rb +++ b/lib/stripe/oauth.rb @@ -7,8 +7,8 @@ module Stripe 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 @@ module Stripe 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 c4dbb36b..53e96d0d 100644 --- a/lib/stripe/resources/account.rb +++ b/lib/stripe/resources/account.rb @@ -45,12 +45,8 @@ module Stripe 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 @@ module Stripe 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 d5d816a8..2f2ab339 100644 --- a/lib/stripe/resources/file.rb +++ b/lib/stripe/resources/file.rb @@ -25,8 +25,9 @@ module Stripe 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 48ee67fc..83b64dba 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -9,19 +9,35 @@ module Stripe class StripeClient # A set of all known thread contexts across all threads and a mutex to # synchronize global access to them. - @thread_contexts_with_connection_managers = [] + @thread_contexts_with_connection_managers = Set.new @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 @@ module Stripe # 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 @@ module Stripe 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 @@ module Stripe # 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 @@ module Stripe 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 @@ module Stripe 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 @@ module Stripe 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 @@ module Stripe 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 @@ module Stripe # 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 @@ module Stripe # 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 @@ module Stripe # because that's wrapped in an `ensure` block, they should never leave # garbage in `Thread.current`. attr_accessor :last_responses + + # A map of connection mangers 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,11 +414,19 @@ module Stripe 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 + thread_context + .default_connection_managers + .each do |config_key, connection_manager| + next if connection_manager.last_used > last_used_threshold + + connection_manager.clear + thread_context.default_connection_managers.delete(config_key) + end + end + + @thread_contexts_with_connection_managers.each do |thread_context| + next unless thread_context.default_connection_managers.empty? - connection_manager.clear - thread_context.default_connection_manager = nil pruned_thread_contexts << thread_context end @@ -397,7 +437,7 @@ module Stripe 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 +511,7 @@ module Stripe 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 +538,12 @@ module Stripe 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 +665,8 @@ module Stripe 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 +715,8 @@ module Stripe 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 +748,8 @@ module Stripe 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 +760,7 @@ module Stripe "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 +781,7 @@ module Stripe "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 +789,12 @@ module Stripe # 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 +818,13 @@ module Stripe 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 +836,13 @@ module Stripe 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 +850,8 @@ module Stripe 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 +861,8 @@ module Stripe 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 814d3e65..cb12b4e2 100644 --- a/lib/stripe/stripe_configuration.rb +++ b/lib/stripe/stripe_configuration.rb @@ -101,6 +101,14 @@ module Stripe @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,13 @@ module Stripe def enable_telemetry? enable_telemetry end + + # Generates a deterministic key to identify configuration objects with + # identical configuration values. + def key + instance_variables + .collect { |variable| instance_variable_get(variable) } + .join + end end end diff --git a/lib/stripe/util.rb b/lib/stripe/util.rb index dfb4146a..343fa775 100644 --- a/lib/stripe/util.rb +++ b/lib/stripe/util.rb @@ -76,24 +76,30 @@ module Stripe 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 f73a56ea..c5efcb3b 100644 --- a/test/stripe/connection_manager_test.rb +++ b/test/stripe/connection_manager_test.rb @@ -79,6 +79,49 @@ module Stripe 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 c8ac13f2..be7b4dc5 100644 --- a/test/stripe/oauth_test.rb +++ b/test/stripe/oauth_test.rb @@ -44,6 +44,14 @@ module Stripe 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 @@ module Stripe 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 @@ module Stripe 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 c00ed510..9ea451dd 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -4,6 +4,24 @@ require ::File.expand_path("../test_helper", __dir__) 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,64 @@ module Stripe 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 + + should "only garbage collect when all connection managers for a thread are expired" do + stub_request(:post, "#{Stripe.api_base}/v1/path") + .to_return(body: JSON.generate(object: "account")) + + # Make sure we start with a blank slate (state may have been left in + # place by other tests). + StripeClient.clear_all_connection_managers + + # Establish a base time. + t = 0.0 + + # And pretend that `StripeClient` was just initialized for the first + # time. (Don't access instance variables like this, but it's tricky to + # test properly otherwise.) + StripeClient.instance_variable_set(:@last_connection_manager_gc, t) + + # + # t + # + Util.stubs(:monotonic_time).returns(t) + + # Execute an initial request to ensure that a connection manager was + # created. + client = StripeClient.new + client.execute_request(:post, "/v1/path") + + # Create a new client with a unique config to make sure the thread has two + # connection managers + active_client = StripeClient.new(max_network_retries: 10) + active_client.execute_request(:post, "/v1/path") + + assert_equal 2, StripeClient.current_thread_context.default_connection_managers.keys.count + assert_equal nil, StripeClient.maybe_gc_connection_managers + + # t + StripeClient::CONNECTION_MANAGER_GC_LAST_USED_EXPIRY + 1 + # + # Move us far enough into the future that we're passed the horizons for + # both a GC run as well as well as the expiry age of a connection + # manager. That means the GC will run and collect the connection + # manager that we created above. + # + Util.stubs(:monotonic_time).returns(t + StripeClient::CONNECTION_MANAGER_GC_LAST_USED_EXPIRY + 1) + + # Manually set the active_client's last_used time into the future to prevent GC. + StripeClient.default_connection_manager(active_client.config) + .instance_variable_set(:@last_used, Util.monotonic_time + 1) + + assert_equal 0, StripeClient.maybe_gc_connection_managers + + # Move time into the future past the last GC round + current_time = Util.monotonic_time + Util.stubs(:monotonic_time).returns(current_time * 2) + + assert_equal 1, StripeClient.maybe_gc_connection_managers end end @@ -160,11 +234,26 @@ module Stripe 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 +364,7 @@ module Stripe 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 +373,8 @@ module Stripe 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 +384,8 @@ module Stripe 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 +398,23 @@ module Stripe 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 +448,10 @@ module Stripe # 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 +463,13 @@ module Stripe 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 +479,18 @@ module Stripe 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 +519,8 @@ module Stripe 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 +529,8 @@ module Stripe method: :post, path: "/v1/account", request_id: nil, - status: 500) + status: 500, + config: Stripe.configuration) error = { code: "code", @@ -428,7 +545,8 @@ module Stripe 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 +567,8 @@ module Stripe 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 +577,16 @@ module Stripe 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 +909,7 @@ module Stripe 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 +959,7 @@ module Stripe 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 +991,21 @@ module Stripe 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 +1215,7 @@ module Stripe 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 +1231,7 @@ module Stripe 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 1630aa3d..9ad74312 100644 --- a/test/stripe/stripe_configuration_test.rb +++ b/test/stripe/stripe_configuration_test.rb @@ -20,6 +20,7 @@ module Stripe 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 @@ module Stripe 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 @@ module Stripe 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 @@ module Stripe 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 53e9d009..c0c3bd81 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