From 0abfc55e26f4de55ec2fca2ba733848b553147d3 Mon Sep 17 00:00:00 2001 From: Brandur Date: Mon, 19 Aug 2019 15:57:23 -0700 Subject: [PATCH] Minor cleanup in `StripeClient` (#832) I ended up having to relax the maximum method line length in a few previous PRs, so I wanted to try one more cleanup pass in `execute_request` to see if I could get it back at all. The answer was "not by much" (without reducing clarity), but I found a few places that could be tweaked. Unfortunately, ~50 lines is probably the "right" length for this method in that you _could_ extract it further, but you'd end up passing huge amounts of state all over the place in method parameters, and it really wouldn't look that good. --- .rubocop.yml | 2 +- lib/stripe/stripe_client.rb | 59 +++++++++++++++++-------------- test/stripe/stripe_client_test.rb | 2 +- 3 files changed, 34 insertions(+), 29 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 1c7113778..6e531c80f 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -32,7 +32,7 @@ Metrics/MethodLength: # There's ~2 long methods in `StripeClient`. If we want to truncate those a # little, we could move this to be closer to ~30 (but the default of 10 is # probably too short). - Max: 55 + Max: 50 Metrics/ModuleLength: Enabled: false diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 2f2a65d1f..9714482dd 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -148,45 +148,28 @@ def execute_request(method, path, body_params = nil query_params = nil - case method.to_s.downcase.to_sym + case method when :get, :head, :delete query_params = params else body_params = params end - # This works around an edge case where we end up with both query - # parameters in `query_params` and query parameters that are appended - # onto the end of the given path. - # - # Here we decode any parameters that were added onto the end of a path - # and add them to `query_params` so that all parameters end up in one - # place and all of them are correctly included in the final request. - u = URI.parse(path) - unless u.query.nil? - query_params ||= {} - query_params = Hash[URI.decode_www_form(u.query)].merge(query_params) - - # Reset the path minus any query parameters that were specified. - path = u.path - end + query_params, path = merge_query_params(query_params, path) headers = request_headers(api_key, method) .update(Util.normalize_headers(headers)) url = api_url(path, api_base) + # Merge given query parameters with any already encoded in the path. query = query_params ? Util.encode_parameters(query_params) : nil # Encoding body parameters is a little more complex because we may have # to send a multipart-encoded body. `body_log` is produced separately as - # a variant of the encoded form that's output-friendly. e.g. Doesn't show - # as multipart. File objects are displayed as such instead of as their - # file contents. - body, body_log = if body_params - encode_body(body_params, headers) - else - [nil, nil] - end + # a log-friendly variant of the encoded form. File objects are displayed + # as such instead of as their file contents. + body, body_log = + body_params ? encode_body(body_params, headers) : [nil, nil] # stores information on the request we're about to make so that we don't # have to pass as many parameters around for logging. @@ -198,7 +181,7 @@ def execute_request(method, path, context.idempotency_key = headers["Idempotency-Key"] context.method = method context.path = path - context.query_params = query + context.query = query http_resp = execute_request_with_rescues(method, api_base, context) do connection_manager.execute_request(method, url, @@ -410,6 +393,28 @@ def execute_request(method, path, raise(error) end + # Works around an edge case where we end up with both query parameters from + # parameteers and query parameters that were appended onto the end of the + # given path. + # + # Decode any parameters that were added onto the end of a path and add them + # to a unified query parameter hash so that all parameters end up in one + # place and all of them are correctly included in the final request. + private def merge_query_params(query_params, path) + u = URI.parse(path) + + # Return original results if there was nothing to be found. + return query_params, path if u.query.nil? + + query_params ||= {} + query_params = Hash[URI.decode_www_form(u.query)].merge(query_params) + + # Reset the path minus any query parameters that were specified. + path = u.path + + [query_params, path] + end + private def specific_api_error(resp, error_data, context) Util.log_error("Stripe API error", status: resp.http_status, @@ -572,7 +577,7 @@ def execute_request(method, path, Util.log_debug("Request details", body: context.body, idempotency_key: context.idempotency_key, - query_params: context.query_params) + query: context.query) end private def log_response(context, request_start, status, body) @@ -619,7 +624,7 @@ class RequestLogContext attr_accessor :idempotency_key attr_accessor :method attr_accessor :path - attr_accessor :query_params + attr_accessor :query attr_accessor :request_id # The idea with this method is that we might want to update some of diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index 647892010..f24e85f34 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -252,7 +252,7 @@ class StripeClientTest < Test::Unit::TestCase Util.expects(:log_debug).with("Request details", body: "", idempotency_key: "abc", - query_params: nil) + query: nil) Util.expects(:log_info).with("Response from Stripe API", account: "acct_123",