From 9dd5bdb0e61a14e58cfc5e5b469eba4195f20a8a Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 1 Feb 2019 13:36:03 -0800 Subject: [PATCH] Use `FaradayStripeEncoder` to encode all parameter styles Makes a few tweaks to hopefully simplify clarity things: * `FaradayStripeEncoder` now becomes the way to encode all of form, multipart form, and query parameters. * Introduce a cache in it so that we don't have to encode everything twice (once for logging, and once for the request body). * Try to sanitize logging a bit by replacing `Faraday::UploadIO`s found in incoming parameters with a string representation of the file (note that all other styles of file input like `File` or `Tempfile` have been converted to `Faraday::UploadIO` by the time they reach the encoder). --- .rubocop_todo.yml | 4 +- lib/stripe/stripe_client.rb | 65 +++++++++++++++++++++++-------- test/stripe/stripe_client_test.rb | 10 +++++ 3 files changed, 60 insertions(+), 19 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 0e0e5bd5b..34519c21c 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -8,7 +8,7 @@ # Offense count: 19 Metrics/AbcSize: - Max: 53.26 + Max: 54 # Offense count: 27 # Configuration parameters: CountComments, ExcludedMethods. @@ -18,7 +18,7 @@ Metrics/BlockLength: # Offense count: 8 # Configuration parameters: CountComments. Metrics/ClassLength: - Max: 659 + Max: 670 # Offense count: 11 Metrics/CyclomaticComplexity: diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index e02ff29bc..5a4069ae2 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -123,24 +123,17 @@ def execute_request(method, path, api_base: nil, api_key: nil, headers: {}, params: {}) api_base ||= Stripe.api_base api_key ||= Stripe.api_key + params = Util.objects_to_ids(params) check_api_key!(api_key) - params = Util.objects_to_ids(params) - url = api_url(path, api_base) - body = nil query_params = nil - case method.to_s.downcase.to_sym when :get, :head, :delete query_params = params else - body = if headers[:content_type] && headers[:content_type] == "multipart/form-data" - params - else - Util.encode_parameters(params) - end + body = params end # This works around an edge case where we end up with both query @@ -162,6 +155,8 @@ def execute_request(method, path, headers = request_headers(api_key, method) .update(Util.normalize_headers(headers)) + params_encoder = FaradayStripeEncoder.new + url = api_url(path, api_base) # stores information on the request we're about to make so that we don't # have to pass as many parameters around for logging. @@ -169,19 +164,19 @@ def execute_request(method, path, context.account = headers["Stripe-Account"] context.api_key = api_key context.api_version = headers["Stripe-Version"] - context.body = body + context.body = body ? params_encoder.encode(body) : nil context.idempotency_key = headers["Idempotency-Key"] context.method = method context.path = path - context.query_params = query_params ? Util.encode_parameters(query_params) : nil + context.query_params = query_params ? params_encoder.encode(query_params) : nil + # note that both request body and query params will be passed through + # `FaradayStripeEncoder` http_resp = execute_request_with_rescues(api_base, context) do conn.run_request(method, url, body, headers) do |req| req.options.open_timeout = Stripe.open_timeout - req.options.params_encoder = FaradayStripeEncoder + req.options.params_encoder = params_encoder req.options.timeout = Stripe.read_timeout - - # note that these will be passed through `FaradayStripeEncoder` req.params = query_params unless query_params.nil? end end @@ -207,15 +202,51 @@ def execute_request(method, path, # # We work around the problem by implementing our own simplified encoder and # telling Faraday to use that. + # + # The class also performs simple caching so that we don't have to encode + # parameters twice for every request (once to build the request and once + # for logging). + # + # When initialized with `multipart: true`, the encoder just inspects the + # hash instead to get a decent representation for logging. In the case of a + # multipart request, Faraday won't use the result of this encoder. class FaradayStripeEncoder - def self.encode(hash) - Util.encode_parameters(hash) + def initialize + @cache = {} + end + + # This is quite subtle, but for a `multipart/form-data` request Faraday + # will throw away the result of this encoder and build its body. + def encode(hash) + @cache.fetch(hash) do |k| + @cache[k] = Util.encode_parameters(replace_faraday_io(hash)) + end end # We should never need to do this so it's not implemented. - def self.decode(_str) + def decode(_str) raise NotImplementedError, "#{self.class.name} does not implement #decode" end + + # Replaces instances of `Faraday::UploadIO` with a simple string + # representation so that they'll log a little better. Faraday won't use + # these parameters, so it's okay that we did this. + # + # Unfortunately the string representation still doesn't look very nice + # because we still URL-encode special symbols in the final value. It's + # possible we could stop doing this and just leave it to Faraday to do + # the right thing. + private def replace_faraday_io(value) + if value.is_a?(Array) + value.each_with_index { |v, i| value[i] = replace_faraday_io(v) } + elsif value.is_a?(Hash) + value.each { |k, v| value[k] = replace_faraday_io(v) } + elsif value.is_a?(Faraday::UploadIO) + "FILE:#{value.original_filename}" + else + value + end + end end def api_url(url = "", api_base = nil) diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index 8f1a7dcaa..fe098c6e4 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -794,6 +794,16 @@ class StripeClientTest < Test::Unit::TestCase assert(!trace_payload["last_request_metrics"]["request_duration_ms"].nil?) end end + + context "FaradayStripeEncoder" do + should "replace Faraday::UploadIO instances in parameters" do + encoder = StripeClient::FaradayStripeEncoder.new + encoded = encoder.encode(foo: [ + { file: Faraday::UploadIO.new(::File.new(__FILE__), nil) }, + ]) + assert_equal "foo[0][file]=FILE%3Astripe_client_test.rb", encoded + end + end end class SystemProfilerTest < Test::Unit::TestCase