Skip to content

Commit

Permalink
Minor cleanup in StripeClient (#832)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
brandur authored and brandur-stripe committed Aug 19, 2019
1 parent a9a7af0 commit 0abfc55
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 29 deletions.
2 changes: 1 addition & 1 deletion .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
59 changes: 32 additions & 27 deletions lib/stripe/stripe_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/stripe/stripe_client_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit 0abfc55

Please sign in to comment.