Allow access to request metadata on success

[sedouard]

Hey @ob-stripe this is my first stab at getting response headers out of the python bindings.

I'm trying to follow the same pattern by including a response resource to the returned API models.

However one tricky part is understanding where the best spot is to place an optional flag to include the response field on returned API resources. Each API request method contains a paramsparameter that directly maps to the API's URL parameters. Placing some kind of include_response flag here would stick out since it won't be going to the API.

Another thought was using a static variable somewhere and checking that variable within the api_requestor to include the response or not.

Do you have any opinions here?

[ob-stripe]

sigh Yeah. The way we handle keyword parameters in Python makes it really inconvenient to add new "special" parameters. We already have a few of those (api_key, stripe_version, stripe_account, idempotency_key) and they need to be present on every request method's signature (e.g. here).

I would really rather avoid introducing new special parameters, esp. since this one as you pointed out would just be a configuration flag not directly related to the API. How about introducing a context manager for this? E.g.:

with stripe.catch_responses() as r:
    charge = stripe.Charge.retrieve("ch_...")
    request_id = r[0].headers['Request-Id']

Basically, the context manager would collect the responses to all requests sent within the context.

[sedouard]

My concern with that approach is that to get your headers for any arbitrary response the consuming application needs to keep track of sequence number of the request which sounds like a pain. But then again I don't regularly develop asynchronous code in python - so maybe it is not a big deal.

I personally think some type of options hash on the base stripe import would be more straight forward however the downside is that it would be a global change to the application and not on a request-by-request basis. Is that something we'd be ok with?

Both of our approaches in java PR and in the the Node SDK issue intend on using some reference off the retrieved api model object. I was hoping we could do something similar here

@dalan-stripe do you have any opinions about the UX regarding how to optionally enable responses upon requests?

[ob-stripe]

What if the context manager attached the response object to the resource object?

with stripe.store_responses():
    charge = stripe.Charge.retrieve("ch_...")
    request_id = charge.response.headers['Request-Id']

(Just thinking aloud here, not sure how feasible this would be in practice.)

[sedouard]

@ob-stripe Yeah I like that much better. I think the name should be include_responses() such that it conforms more to the naming of java's optional flag and it doesn't imply that stripe will hold references to each response made through the library.

This way consumers can manage the scope of the optional change. I'm actually good with this approach

[ob-stripe]

👍

You might want to take a look at the implementation in stripe-ruby. Basically the HTTP client holds a reference to the last response (here), and the request method returns the reference in addition to the API resource (here).

In our case we would do something similar, except that we'd copy the last response object onto the resource's response attribute.

[dalan-stripe]

I'm not so sure that this would be an appropriate usage of a context manager in Python. It also adds a lot of complexity to python-stripe I believe, because wouldn't you need to build your request objects in some way and then do?:

stripe.bulk_requests = True #signals that requests will be done in bulk 
requests = [stripe.Charge.retrieve('ch_123'), stripe.Charge.retrieve('ch_124'), stripe.Charge.retrieve('ch_125')] 

with requests.include_responses() as completed_requests: 
  for response,  headers in completed_requests: 
    print(f"{response} - {headers}")

It just seems very un-pythonic and potentially introduces problems for async (not certain if we support this anyway)

My vote is to just include it by default in the spirit of simplicity on the outermost requested resource. For example .headers exists on the Charge if you only did .retrieve but exists on the list if you did a stripe.Charge.list(). It seems like given the exposure to Requests in Python you'd just expect .headers to exist on the object itself. Although I could see the confusion between having this object that represents a resource on Stripe that is also munged together with an HTTP request.

[ob-stripe]

After giving it some more thought, I agree that a context manager may not be the best solution here. Also, we don't persist clients in the Python library (which is probably something we should fix too) which makes my proposed solution somewhat difficult to implement.

I don't think we should start storing responses by default however, so as not to increase the memory footprint for users who don't need this feature. Also, we should not expose any requests-specific attributes as the library can use different HTTP clients.

How about this:

• have a global configuration flag that needs to be enabled, e.g. Stripe.include_responses = True

• when the flag is enabled, all Stripe resources returned by the API have a response attribute, which is an instance of StripeResponse

• StripeResponse stores the body, headers and status code of the response

We'll also probably have to define a custom JSON encoder to exclude response from the serialization of Stripe objects.

[deontologician-stripe]

Why do we want to optionally make the headers available? Why not just make them always available?

[sedouard]

@deontologician-stripe I think the concern is that if there's a consumer that happens to be making a ton of Stripe requests (and holding references to each response) that adding a reference to each of those response objects we deserialize and return to the consumer could cause memory bloat.

@ob-stripe can elaborate if I'm off.

[deontologician-stripe]

As long as you're not holding references to all of your objects, it shouldn't be too much bloat. I think this is probably a case of premature optimization. In any case I agree with @dalan-stripe that having a context manager just for this option is not great ergonomics. A global module flag would be a good option, but ultimately I don't think this feature warrants a flag to turn it off since it's not going to cause most people any problems

[ob-stripe]

So, it turns out that this feature has already existed in stripe-php for a little over 2 years: stripe/stripe-php#206.

In light of the arguments above and in the interest of feature parity and implementation consistency across our libraries, I recommend that:

• we store the last response object on all API resource instances (by default and with no configuration flag)
• we store the attribute in _last_response and make it accessible via get_last_response() (StripeObjects handle non-_-prefixed attributes as normal dictionary keys, which would cause a bunch of issues in this case)
• we blacklist last_response as a field name in the API

We'd also need to update the pending Java implementation accordingly.

[sedouard]

@ob-stripe sounds good. I've updated the PR to go in this direction.

Only change is a little different is that I don't think get_last_response() is very pythonic. Instead I went with using an property for StripeObject.last_response pointing to StripeObject._last_response. It doesn't appear this is being serialized out on future instance method calls.

[ob-stripe]

Only change is a little different is that I don't think get_last_response() is very pythonic. Instead I went with using an property for StripeObject.last_response pointing to StripeObject._last_response. It doesn't appear this is being serialized out on future instance method calls.

Nice! Good call. 👍

I'm not a big fan of storing the StripeResponse instance inside the resp dictionary however. I find it fairly confusing to have a dictionary that contains a _last_response key whose value is a StripeResponse, that has a body attribute whose value is the JSON string of the top-level dictionary, minus the _last_response key.

Instead, I think StripeResponse should contain both the deserialized dictionary and the JSON string, and request() should return a StripeResponse instead of a dictionary. This will require modifying a few places to look for the now nested dictionary (e.g. call convert_to_stripe_object() with resp.data instead of resp).

This would also bring the implementation closer to stripe-ruby's:

• https://github.com/stripe/stripe-ruby/blob/master/lib/stripe/stripe_response.rb
• https://github.com/stripe/stripe-ruby/blob/234b0809bfd737f7a8ddf215d23e15cab6efc741/lib/stripe/api_operations/create.rb#L5-L6

wdyt?

[sedouard]

I'm not a big fan of storing the StripeResponse instance inside the resp dictionary however. I find it fairly confusing to have a dictionary that contains a _last_response key whose value is a StripeResponse, that has a body attribute whose value is the JSON string of the top-level dictionary, minus the _last_response key.

fair I think the modifications in the unit test reflect this awkwardness.

I think the reason I did it this way was to get around having to make a bunch of changes all over the API resource classes which use the requestor interface directly :-/

[sedouard]

@ob-stripe I'm able to reduce the amount of code change needed from changing APIRequestor.request by doing this little awkward bit. Essentially unpacking the response body dict inside convert_to_stripe_object.

APIRequestor.request's only real consumer is convert_to_stripe_object we only need to change it here.

wdyt?

[ob-stripe]

👍 Yeah, that sounds reasonable and less annoying than having to change all the call sites. It also maintains the existing behavior of convert_to_stripe_object which is nice even if it's an internal function.

[sedouard]

@ob-stripe I updated the PR with a similar approach to what you mention. The exception is instead of calling convert_to_stripe_object with a resp.data arg, I'm unpacking that arg within the function to prevent having to change a bunch of calling code to convert_to_stripe_object

[ob-stripe]

Requested one small change, but I think we'll be ready to merge afterwards. Thanks for bearing with me :)

[ob-stripe]

One minor optimization is that you could instantiate the StripeResponse in the try clause, to avoid parsing the JSON twice.

[sedouard]

✅ done!

[ob-stripe]

👍 Yeah, that sounds reasonable and less annoying than having to change all the call sites. It also maintains the existing behavior of convert_to_stripe_object which is nice even if it's an internal function.

[ob-stripe]

Okay, this looks good to me! Thanks for making all the changes @sedouard.

@brandur-stripe, mind taking a quick look as well?

[brandur-stripe]

Sorry for the delay guys! And nice one @sedouard, this looks great. Huge +1 on building out a custom StripeResponse class instead of relying on some kind of loosely typed dict.

This is a bit of a nit, but do you think we could introduce a basic unit test suite for StripeResponse? It doesn't have to do that much, but maybe just make sure of things like (1) it parses JSON in body into data, (2) passes idempotency_key and request_id through to the headers dict. It may be a little redundant right now, but I think it'll be helpful in the future if anyone adds something a little more complicated to StripeResponse because the groundwork will be lain for them to start writing tests off of.

Otherwise, LGTM. Thanks again.

[jleclanche]

Paging devs, is this getting merged?

[brandur-stripe]

Yeah, we should get this in.

@sedouard Mind taking a look at the comment above? Thanks!

[stevene-stripe]

Yeah sorry guys for letting this PR hang out! I'll get this buttoned up in the next couple days. I got caught up with some other hi-pri work.

[stevene-stripe]

@brandur-stripe I added new tests for StripeResponse covering all methods and fields.

[brandur-stripe]

Looks great! Thanks @stevene-stripe!

[brandur-stripe]

Released as 1.77.0.

[jleclanche]

Thanks! :)

[jleclanche]

Hey, am I crazy or should this work?

customer = stripe_customer.api_retrieve()
customer.account_balance -= 250
customer.save(idempotency_key=ik)
credit_request_id = customer.last_response.request_id  # Breaks! last_response is None

[ob-stripe]

@jleclanche You're not crazy! Looks like the current implementation does not work well with save() calls.

Mind opening a new issue for this? I think the fix should be fairly simple.

[jleclanche]

Sure, #393