Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor cleanup in StripeClient #832

Merged
merged 1 commit into from
Aug 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved

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
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved
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)
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved

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]
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved

# 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
ob-stripe marked this conversation as resolved.
Show resolved Hide resolved

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