Make request ID and response information available on all API calls #492

brandur · 2017-01-13T22:44:39Z

It's currently possible to access a Stripe request ID value from a StripeError object, but it's also quite useful to be able to access one from any API call for logging and other purposes. This patch adds a response access to APIResource, ListObject, and StripeError which includes response information including the request ID.

So for any API call it's now possible to do this:

charge = Charge.create(...)
puts "request ID = #{charge.response.request_id}"

charges = Charge.list
puts "request ID = #{charges.response.request_id}"

Or on error, the recommended approach is to now access #request_id in the same way:

begin
  charge = Charge.create(...)
rescue Stripe::StripeError => e
  puts "request ID = #{e.response.request_id}"
end

Only top level resources are given a response value, so we get a nil back for this operation:

charge = Charge.create(...)
charge.customer.response.request_id # nil

Fixes #484.

r? @dpetrovics-stripe This one is pretty big, but would you mind reviewing? We could also kick it to someone else on the team if you're pretty swamped right now. Thanks!

brandur · 2017-01-13T22:48:37Z

lib/stripe.rb

-  def self.card_error(error, resp, error_obj)
-    CardError.new(error[:message], error[:param], error[:code],
-                  resp.code, resp.body, error_obj, resp.headers)
-  end


These helper methods to create errors are a layer of obfuscation and don't add much, so I removed most of them (general_api_error survived because it's called more than once).

brandur · 2017-01-13T22:49:56Z

lib/stripe/api_operations/request.rb

@@ -12,7 +12,7 @@ def request(method, url, params={}, opts={})
          api_base = headers.delete(:api_base)
          # Assume all remaining opts must be headers

-          response, opts[:api_key] = Stripe.request(method, url, api_key, params, headers, api_base)
+          resp, opts[:api_key] = Stripe.request(method, url, api_key, params, headers, api_base)


These are nitpicky changes, but I tried to call all local variables containing a StripeResponse "resp" for global consistency.

brandur · 2017-01-13T22:50:58Z

lib/stripe/errors.rb

    attr_reader :http_body
    attr_reader :http_headers
+    attr_reader :http_status
+    attr_reader :json_body # equivalent to #data


I'm thinking it might be a good idea to deprecate the use of these fields and make them pass throughs to response in the meantime, but I deferred that for the time being.

brandur · 2017-01-13T22:51:54Z

lib/stripe/errors.rb

+    #
+    # Note: We should try to move away from the very heavy constructors ordered
+    # parameters to each just setting accessor values directly or optional
+    # arguments.


I'd like to change this constructor to just take two arguments: (message, resp) for cleanliness, but I figure that people might be instantiating errors in their test suites (even though they're not technically part of the public API), so I figured I'd wait until the next major version increment.

Yea, that makes sense to me. I would change the signature of this constructor at the same time as removing the duplicated fields above (and just use :response)

brandur · 2017-01-13T22:52:42Z

lib/stripe/util.rb

-      case resp
+    # * +response+ - An object containing information about the API response
+    #   that produced the data which is hydrating the StripeObject.
+    def self.convert_to_stripe_object(data, opts, other_opts = {})


Ugh, this should be an optional argument (just have response instead of other_opts), but it breaks the 1.9 build. I wonder if we should just rip off that band-aid.

dpetrovics-stripe · 2017-01-13T23:36:26Z

lib/stripe/api_resource.rb

@@ -2,6 +2,13 @@ module Stripe
  class APIResource < StripeObject
    include Stripe::APIOperations::Request

+    # Response contains a structure that has some basic information about the


Instead of a structure can you say StripeResponse?

dpetrovics-stripe · 2017-01-13T23:37:10Z

lib/stripe/stripe_response.rb

+  # StripeResponse encapsulates some vitals of a response that came back from
+  # the Stripe API.
+  class StripeResponse
+    # The data contained by the HTTP body of the response deserialized from


Thanks for the great comments on all these!

dpetrovics-stripe · 2017-01-13T23:41:36Z

lib/stripe/customer.rb

      subscription
    end

    def delete_discount
-      _, opts = request(:delete, discount_url)
+      self.response, opts = request(:delete, discount_url)


Nit: this looks to be unused so you could just leave as _.

Sorry, this one is a little confusing, but we're actually setting self.response which is an instance-level variable. So after you've deleted the discount object, you can still get the latest response information on the resource.

dpetrovics-stripe · 2017-01-14T00:03:23Z

lib/stripe/errors.rb

@@ -3,12 +3,24 @@ module Stripe
  # errors derive.
  class StripeError < StandardError
    attr_reader :message
-    attr_reader :http_status
+
+    # Response contains a structure that has some basic information about the


I might reference StripeResponse here again by name just for clarity

dpetrovics-stripe · 2017-01-14T00:20:04Z

Thanks for this, @brandur! The change seems pretty clean to me-- it makes sense to have this available whenever a request is made. Just some really minor comments, otherwise:

LGTM

brandur · 2017-01-17T18:03:09Z

Thanks for this, @brandur! The change seems pretty clean to me-- it makes sense to have this available whenever a request is made. Just some really minor comments, otherwise:

Thanks! And thank you for taking a look at this stuff too.

brandur · 2017-01-17T18:08:43Z

Wow, after thinking about it for the weekend, I think we can do better than this interface, and possibly also address #313 at the same time. I'm going to build one more prototype before pulling this in.

brandur · 2017-01-20T19:07:26Z

Closing in favor of #498. Most of the commits here are there as well.

brandur added 5 commits January 13, 2017 14:10

Add a StripeResponse to objects created by API calls

579d0f3

Add test suite for StripeResponse

e3edd2d

Add tests for new .convert_to_stripe_object argument

3ee4c5e

Check that responses are set on API resources

ff7a765

Change to hash parameter for now for 1.9 support

0b2672a

brandur mentioned this pull request Jan 13, 2017

Making Request Id available? #484

Closed

brandur commented Jan 13, 2017

View reviewed changes

"Fix" one more use of 2+ optional arguments

1022480

dpetrovics-stripe reviewed Jan 13, 2017

View reviewed changes

dpetrovics-stripe reviewed Jan 14, 2017

View reviewed changes

This was referenced Jan 20, 2017

Release stripe-ruby 2.0 #497

Closed

Add configurable HTTP client and make response objects available on API calls #498

Closed

brandur closed this Jan 20, 2017

brandur deleted the brandur-accessible-request-id branch January 20, 2017 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make request ID and response information available on all API calls #492

Make request ID and response information available on all API calls #492

brandur commented Jan 13, 2017 •

edited

Loading

brandur Jan 13, 2017

dpetrovics-stripe Jan 13, 2017

brandur Jan 13, 2017 •

edited

Loading

dpetrovics-stripe Jan 13, 2017

brandur Jan 13, 2017

brandur Jan 13, 2017

dpetrovics-stripe Jan 14, 2017

brandur Jan 13, 2017

dpetrovics-stripe Jan 13, 2017

brandur Jan 17, 2017

dpetrovics-stripe Jan 13, 2017

dpetrovics-stripe Jan 13, 2017

brandur Jan 17, 2017

dpetrovics-stripe Jan 17, 2017

dpetrovics-stripe Jan 14, 2017

brandur Jan 17, 2017

dpetrovics-stripe commented Jan 14, 2017

brandur commented Jan 17, 2017

brandur commented Jan 17, 2017

brandur commented Jan 20, 2017

Make request ID and response information available on all API calls #492

Make request ID and response information available on all API calls #492

Conversation

brandur commented Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandur Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpetrovics-stripe commented Jan 14, 2017

brandur commented Jan 17, 2017

brandur commented Jan 17, 2017

brandur commented Jan 20, 2017

brandur commented Jan 13, 2017 •

edited

Loading

brandur Jan 13, 2017 •

edited

Loading