From 37bf8b8d882816234af44687736ef4635a41a938 Mon Sep 17 00:00:00 2001 From: Helen Ye Date: Wed, 8 Jan 2025 15:37:35 -0500 Subject: [PATCH 1/5] Fix client options not importing global config --- lib/stripe.rb | 22 ++++++++++++++++++++++ lib/stripe/stripe_client.rb | 7 ++++++- lib/stripe/stripe_configuration.rb | 20 ++++++++++++++++++++ test/stripe/stripe_client_test.rb | 26 ++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 1 deletion(-) diff --git a/lib/stripe.rb b/lib/stripe.rb index d05f13d0a..f3032de0b 100644 --- a/lib/stripe.rb +++ b/lib/stripe.rb @@ -74,6 +74,28 @@ module Stripe DEFAULT_UPLOAD_BASE = "https://files.stripe.com" DEFAULT_METER_EVENTS_BASE = "https://meter-events.stripe.com" + # Options that can be configured globally by users + USER_CONFIGURABLE_GLOBAL_OPTIONS = Set.new([ + :api_key, + :api_version, + :stripe_account, + :api_base, + :uploads_base, + :connect_base, + :meter_events_base, + :open_timeout, + :read_timeout, + :write_timeout, + :proxy, + :verify_ssl_certs, + :ca_bundle_path, + :log_level, + :logger, + :max_network_retries, + :enable_telemetry, + :client_id + ]) + @app_info = nil @config = Stripe::StripeConfiguration.setup diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 782b862a4..084dde077 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -10,6 +10,10 @@ class StripeClient # attr_readers: The end of the section generated from our OpenAPI spec + # For internal use only. Does not provide a stable API and may be broken + # with future non-major changes. + CLIENT_OPTIONS = Set.new([:api_key, :stripe_account, :stripe_context, :api_version, :api_base, :uploads_base, :connect_base, :meter_events_base, :client_id]) + # Initializes a new StripeClient def initialize(api_key, # rubocop:todo Metrics/ParameterLists stripe_account: nil, @@ -40,7 +44,8 @@ def initialize(api_key, # rubocop:todo Metrics/ParameterLists client_id: client_id, }.reject { |_k, v| v.nil? } - @requestor = APIRequestor.new(config_opts) + config = StripeConfiguration.client_init(config_opts) + @requestor = APIRequestor.new(config) # top-level services: The beginning of the section generated from our OpenAPI spec @v1 = Stripe::V1Services.new(@requestor) diff --git a/lib/stripe/stripe_configuration.rb b/lib/stripe/stripe_configuration.rb index 175691662..f60d9f38a 100644 --- a/lib/stripe/stripe_configuration.rb +++ b/lib/stripe/stripe_configuration.rb @@ -37,6 +37,26 @@ def self.setup end end + # Set options to the StripeClient configured options, if valid as a client option and provided + # Otherwise, for user configurable global options, set them to the global configuration + # For all other options, set them to the StripeConfiguration default value + def self.client_init(config_opts) + global = Stripe.config + imported_options = Stripe::USER_CONFIGURABLE_GLOBAL_OPTIONS - StripeClient::CLIENT_OPTIONS + StripeConfiguration.setup do |instance| + imported_options.each do |key| + if global.respond_to?(key) + instance.public_send("#{key}=", global.public_send(key)) + end + end + StripeClient::CLIENT_OPTIONS.each do |key| + if config_opts.include?(key) + instance.public_send("#{key}=", config_opts[key]) + end + end + end + end + # Create a new config based off an existing one. This is useful when the # caller wants to override the global configuration def reverse_duplicate_merge(hash) diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index 056182c9c..95d374cfa 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -20,6 +20,7 @@ class StripeClientTest < Test::Unit::TestCase @orig_api_key = Stripe.api_key @orig_stripe_account = Stripe.stripe_account @orig_open_timeout = Stripe.open_timeout + @orig_api_version = Stripe.api_version Stripe.api_key = "DONT_USE_THIS_KEY" Stripe.stripe_account = "DONT_USE_THIS_ACCOUNT" @@ -30,6 +31,7 @@ class StripeClientTest < Test::Unit::TestCase Stripe.api_key = @orig_api_key Stripe.stripe_account = @orig_stripe_account Stripe.open_timeout = @orig_open_timeout + Stripe.api_version = @orig_api_version end should "use default config options" do @@ -67,6 +69,30 @@ class StripeClientTest < Test::Unit::TestCase assert_equal "2022-11-15", req.headers["Stripe-Version"] end + should "use global config options for options unavailable in client" do + Stripe.api_key = "NOT_THIS_KEY" + Stripe.stripe_account = "NOT_THIS_ACCOUNT" + Stripe.api_version = "2022-11-15" + client = StripeClient.new("test_123", stripe_account: "acct_123") + # Imported from global options + assert_equal 30_000, client.instance_variable_get(:@requestor).config.open_timeout + # Not set in client options, not imported from global + assert_equal client.instance_variable_get(:@requestor).config.api_base, Stripe::DEFAULT_API_BASE + assert_equal client.instance_variable_get(:@requestor).config.api_version, Stripe::ApiVersion::CURRENT + + req = nil + stub_request(:get, "#{Stripe::DEFAULT_API_BASE}/v1/customers/cus_123") + .with { |request| req = request } + .to_return(body: JSON.generate(object: "customer")) + + client.v1.customers.retrieve("cus_123") + + # Set in client options + assert_equal "Bearer test_123", req.headers["Authorization"] + assert_equal "acct_123", req.headers["Stripe-Account"] + assert_requested(:get, "#{Stripe::DEFAULT_API_BASE}/v1/customers/cus_123") + end + should "request options overrides client config options" do client = StripeClient.new("test_123", stripe_version: "2022-11-15", stripe_account: "acct_123") From 72c085642ab396b87d7c892ecb0286f764a3a39b Mon Sep 17 00:00:00 2001 From: Helen Ye Date: Thu, 9 Jan 2025 12:48:38 -0500 Subject: [PATCH 2/5] update tests and fix bugs --- lib/stripe/stripe_configuration.rb | 18 ++++------ test/stripe/stripe_client_test.rb | 1 - test/stripe/stripe_configuration_test.rb | 44 ++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 12 deletions(-) diff --git a/lib/stripe/stripe_configuration.rb b/lib/stripe/stripe_configuration.rb index f60d9f38a..0b1a63168 100644 --- a/lib/stripe/stripe_configuration.rb +++ b/lib/stripe/stripe_configuration.rb @@ -41,20 +41,16 @@ def self.setup # Otherwise, for user configurable global options, set them to the global configuration # For all other options, set them to the StripeConfiguration default value def self.client_init(config_opts) - global = Stripe.config - imported_options = Stripe::USER_CONFIGURABLE_GLOBAL_OPTIONS - StripeClient::CLIENT_OPTIONS - StripeConfiguration.setup do |instance| + global_config = Stripe.config + imported_options = USER_CONFIGURABLE_GLOBAL_OPTIONS - StripeClient::CLIENT_OPTIONS + client_config = StripeConfiguration.setup do |instance| imported_options.each do |key| - if global.respond_to?(key) - instance.public_send("#{key}=", global.public_send(key)) - end - end - StripeClient::CLIENT_OPTIONS.each do |key| - if config_opts.include?(key) - instance.public_send("#{key}=", config_opts[key]) + if global_config.respond_to?(key) + instance.public_send("#{key}=", global_config.public_send(key)) end end end + client_config.reverse_duplicate_merge(config_opts) end # Create a new config based off an existing one. This is useful when the @@ -88,7 +84,7 @@ def initialize @connect_base = DEFAULT_CONNECT_BASE @uploads_base = DEFAULT_UPLOAD_BASE @meter_events_base = DEFAULT_METER_EVENTS_BASE - @base_addresses = { api: @api_base, connect: @connect_base, files: @uploads_base, events: @meter_events_base } + @base_addresses = { api: @api_base, connect: @connect_base, files: @uploads_base, meter_events: @meter_events_base } end def log_level=(val) diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index 95d374cfa..5ae5963fc 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -47,7 +47,6 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new("test_123") assert_equal "test_123", client.instance_variable_get(:@requestor).config.api_key assert_nil client.instance_variable_get(:@requestor).config.stripe_account - assert_equal 30, client.instance_variable_get(:@requestor).config.open_timeout end should "use client config options" do diff --git a/test/stripe/stripe_configuration_test.rb b/test/stripe/stripe_configuration_test.rb index 5f1e71bb4..2eab75308 100644 --- a/test/stripe/stripe_configuration_test.rb +++ b/test/stripe/stripe_configuration_test.rb @@ -97,6 +97,50 @@ class StripeConfigurationTest < Test::Unit::TestCase end end + context "client_init" do + setup do + @client_opts = StripeClient::CLIENT_OPTIONS.to_h { |k| [k, nil] } + @old_api_key = Stripe.api_key + @old_stripe_account = Stripe.stripe_account + @old_enable_telemetry = Stripe.instance_variable_get(:@enable_telemetry) + @old_open_timeout = Stripe.open_timeout + @old_uploads_base = Stripe.uploads_base + end + + teardown do + Stripe.api_key = @old_api_key + Stripe.stripe_account = @old_stripe_account + Stripe.enable_telemetry = @old_enable_telemetry + Stripe.open_timeout = @old_open_timeout + Stripe.uploads_base = @old_uploads_base + end + + should "correctly set options for the client" do + # mix of default, global, client options + Stripe.api_key = "global_test_123" + Stripe.stripe_account = "global_acct_123" + Stripe.enable_telemetry = false + Stripe.open_timeout = 30000 + Stripe.uploads_base = "global_uploads_base.stripe.com" + + @client_opts[:api_key] = "client_test_123" + @client_opts[:stripe_account] = "client_acct_123" + @client_opts[:uploads_base] = "client_uploads_base.stripe.com" + @client_opts.reject! { |k, v| v.nil? } + + client_config = Stripe::StripeConfiguration.client_init(@client_opts) + + assert_equal("client_test_123", client_config.api_key) # client api key + assert_equal("client_acct_123", client_config.stripe_account) # client stripe account + assert_equal(false, client_config.enable_telemetry) # global telemetry + assert_equal(30000, client_config.open_timeout) # global timeout + assert_equal("client_uploads_base.stripe.com", client_config.base_addresses[:files]) # client uploads base + assert_equal(Stripe::DEFAULT_API_BASE, client_config.base_addresses[:api]) # default api base + assert_equal(ApiVersion::CURRENT, client_config.api_version) # default api version + assert_equal(Stripe::DEFAULT_CA_BUNDLE_PATH, client_config.ca_bundle_path) # default ca bundle path + end + end + context "#max_network_retries=" do should "coerce the option into an integer" do config = Stripe::StripeConfiguration.setup From 6c2f861c9a95e3620dc17e561d3aeb67d30747a7 Mon Sep 17 00:00:00 2001 From: Helen Ye Date: Thu, 9 Jan 2025 13:26:29 -0500 Subject: [PATCH 3/5] lint --- lib/stripe.rb | 38 ++++++++++++------------ lib/stripe/stripe_client.rb | 2 +- lib/stripe/stripe_configuration.rb | 7 ++--- test/stripe/stripe_configuration_test.rb | 6 ++-- 4 files changed, 26 insertions(+), 27 deletions(-) diff --git a/lib/stripe.rb b/lib/stripe.rb index f3032de0b..7fb7a0cd1 100644 --- a/lib/stripe.rb +++ b/lib/stripe.rb @@ -75,25 +75,25 @@ module Stripe DEFAULT_METER_EVENTS_BASE = "https://meter-events.stripe.com" # Options that can be configured globally by users - USER_CONFIGURABLE_GLOBAL_OPTIONS = Set.new([ - :api_key, - :api_version, - :stripe_account, - :api_base, - :uploads_base, - :connect_base, - :meter_events_base, - :open_timeout, - :read_timeout, - :write_timeout, - :proxy, - :verify_ssl_certs, - :ca_bundle_path, - :log_level, - :logger, - :max_network_retries, - :enable_telemetry, - :client_id + USER_CONFIGURABLE_GLOBAL_OPTIONS = Set.new(%i[ + api_key + api_version + stripe_account + api_base + uploads_base + connect_base + meter_events_base + open_timeout + read_timeout + write_timeout + proxy + verify_ssl_certs + ca_bundle_path + log_level + logger + max_network_retries + enable_telemetry + client_id ]) @app_info = nil diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 084dde077..d5eda6739 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -12,7 +12,7 @@ class StripeClient # For internal use only. Does not provide a stable API and may be broken # with future non-major changes. - CLIENT_OPTIONS = Set.new([:api_key, :stripe_account, :stripe_context, :api_version, :api_base, :uploads_base, :connect_base, :meter_events_base, :client_id]) + CLIENT_OPTIONS = Set.new(%i[api_key stripe_account stripe_context api_version api_base uploads_base connect_base meter_events_base client_id]) # Initializes a new StripeClient def initialize(api_key, # rubocop:todo Metrics/ParameterLists diff --git a/lib/stripe/stripe_configuration.rb b/lib/stripe/stripe_configuration.rb index 0b1a63168..c4c24d158 100644 --- a/lib/stripe/stripe_configuration.rb +++ b/lib/stripe/stripe_configuration.rb @@ -45,9 +45,7 @@ def self.client_init(config_opts) imported_options = USER_CONFIGURABLE_GLOBAL_OPTIONS - StripeClient::CLIENT_OPTIONS client_config = StripeConfiguration.setup do |instance| imported_options.each do |key| - if global_config.respond_to?(key) - instance.public_send("#{key}=", global_config.public_send(key)) - end + instance.public_send("#{key}=", global_config.public_send(key)) if global_config.respond_to?(key) end end client_config.reverse_duplicate_merge(config_opts) @@ -84,7 +82,8 @@ def initialize @connect_base = DEFAULT_CONNECT_BASE @uploads_base = DEFAULT_UPLOAD_BASE @meter_events_base = DEFAULT_METER_EVENTS_BASE - @base_addresses = { api: @api_base, connect: @connect_base, files: @uploads_base, meter_events: @meter_events_base } + @base_addresses = { api: @api_base, connect: @connect_base, files: @uploads_base, + meter_events: @meter_events_base, } end def log_level=(val) diff --git a/test/stripe/stripe_configuration_test.rb b/test/stripe/stripe_configuration_test.rb index 2eab75308..fce1e38a9 100644 --- a/test/stripe/stripe_configuration_test.rb +++ b/test/stripe/stripe_configuration_test.rb @@ -120,20 +120,20 @@ class StripeConfigurationTest < Test::Unit::TestCase Stripe.api_key = "global_test_123" Stripe.stripe_account = "global_acct_123" Stripe.enable_telemetry = false - Stripe.open_timeout = 30000 + Stripe.open_timeout = 30_000 Stripe.uploads_base = "global_uploads_base.stripe.com" @client_opts[:api_key] = "client_test_123" @client_opts[:stripe_account] = "client_acct_123" @client_opts[:uploads_base] = "client_uploads_base.stripe.com" - @client_opts.reject! { |k, v| v.nil? } + @client_opts.reject! { |_k, v| v.nil? } client_config = Stripe::StripeConfiguration.client_init(@client_opts) assert_equal("client_test_123", client_config.api_key) # client api key assert_equal("client_acct_123", client_config.stripe_account) # client stripe account assert_equal(false, client_config.enable_telemetry) # global telemetry - assert_equal(30000, client_config.open_timeout) # global timeout + assert_equal(30_000, client_config.open_timeout) # global timeout assert_equal("client_uploads_base.stripe.com", client_config.base_addresses[:files]) # client uploads base assert_equal(Stripe::DEFAULT_API_BASE, client_config.base_addresses[:api]) # default api base assert_equal(ApiVersion::CURRENT, client_config.api_version) # default api version From ca96e727215123bd8b6277e95874e6f0e48d9281 Mon Sep 17 00:00:00 2001 From: Helen Ye Date: Mon, 13 Jan 2025 16:03:53 -0500 Subject: [PATCH 4/5] catch NotImplementedError and map for older Ruby --- lib/stripe/stripe_configuration.rb | 7 ++++++- test/stripe/stripe_configuration_test.rb | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/stripe/stripe_configuration.rb b/lib/stripe/stripe_configuration.rb index c4c24d158..52c348074 100644 --- a/lib/stripe/stripe_configuration.rb +++ b/lib/stripe/stripe_configuration.rb @@ -45,7 +45,12 @@ def self.client_init(config_opts) imported_options = USER_CONFIGURABLE_GLOBAL_OPTIONS - StripeClient::CLIENT_OPTIONS client_config = StripeConfiguration.setup do |instance| imported_options.each do |key| - instance.public_send("#{key}=", global_config.public_send(key)) if global_config.respond_to?(key) + begin + instance.public_send("#{key}=", global_config.public_send(key)) if global_config.respond_to?(key) + rescue NotImplementedError => e + # In Ruby <= 2.5, we can't set write_timeout on Net::HTTP, log an error and continue + Util.log_error("Failed to set #{key} on client configuration: #{e}") + end end end client_config.reverse_duplicate_merge(config_opts) diff --git a/test/stripe/stripe_configuration_test.rb b/test/stripe/stripe_configuration_test.rb index fce1e38a9..950343997 100644 --- a/test/stripe/stripe_configuration_test.rb +++ b/test/stripe/stripe_configuration_test.rb @@ -99,7 +99,7 @@ class StripeConfigurationTest < Test::Unit::TestCase context "client_init" do setup do - @client_opts = StripeClient::CLIENT_OPTIONS.to_h { |k| [k, nil] } + @client_opts = Hash[StripeClient::CLIENT_OPTIONS.map { |k| [k, nil] }] @old_api_key = Stripe.api_key @old_stripe_account = Stripe.stripe_account @old_enable_telemetry = Stripe.instance_variable_get(:@enable_telemetry) From bdb698030c2ed718a91274a0c3cebf128ee952e6 Mon Sep 17 00:00:00 2001 From: Helen Ye Date: Mon, 13 Jan 2025 17:16:18 -0500 Subject: [PATCH 5/5] rubocop todo --- test/stripe/stripe_configuration_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/stripe/stripe_configuration_test.rb b/test/stripe/stripe_configuration_test.rb index 950343997..22d4c3bf7 100644 --- a/test/stripe/stripe_configuration_test.rb +++ b/test/stripe/stripe_configuration_test.rb @@ -99,7 +99,7 @@ class StripeConfigurationTest < Test::Unit::TestCase context "client_init" do setup do - @client_opts = Hash[StripeClient::CLIENT_OPTIONS.map { |k| [k, nil] }] + @client_opts = Hash[StripeClient::CLIENT_OPTIONS.map { |k| [k, nil] }] # rubocop:todo Style/HashConversion - necessary for Ruby <= 2.5 @old_api_key = Stripe.api_key @old_stripe_account = Stripe.stripe_account @old_enable_telemetry = Stripe.instance_variable_get(:@enable_telemetry)