search

integrallis / stripe_event

Stripe webhook integration for Rails applications.
https://rubygems.org/gems/stripe_event
MIT License
844 stars 104 forks source link

Catch Stripe::StripeError in StripeEvent.instrument #23

Closed adamonduty closed 10 years ago

adamonduty commented 11 years ago

Hi,

I recently ran into an issue where my webhook was mistakenly raising Stripe::StripeError. My webhook implementation calls Stripe::InvoiceItem.create as part of metered billing, and it didn't occur to me at first that stripe_event was swallowing the exception.

What about catching Stripe::StripeError around event_retriever.call(params) and letting any exceptions in the user block float to the top?

Also, we can't use the Test Webhooks section in Stripe because of the evt_00000000000000 issue. Would you consider a flag that disables verification for testing purposes? Or should we just write our own event_retriever implementation?

I don't mind helping with a pull request - I just want to be sure these features are desirable.

invisiblefunnel commented 11 years ago

What about catching Stripe::StripeError around event_retriever.call(params) and letting any exceptions in the user block float to the top?

Removing the rescue completely would treat all exceptions the same. Any response other than 200 OK prompts Stripe to retry so this seems to be the most complete solution. What do you think?

Also, we can't use the Test Webhooks section in Stripe because of the evt_00000000000000 issue. Would you consider a flag that disables verification for testing purposes? Or should we just write our own event_retriever implementation?

I'm not too keen on adding a flag, but I'm happy to discuss possible implementations if you want to send a pull request. The idea was discussed in #4 also.

adamonduty commented 11 years ago

Here's what I mean for the first part. I want StripeEvent to return 401 when the event is fake, but I don't want StripeEvent to catch my own mistakes when calling back to Stripe.

I wrote an event_retriever implementation that doesn't perform verification but it didn't work. On the bright side it took 1ms instead of 1s. Let me get back in a few days and I'll update #4 then.

adamonduty commented 11 years ago

For the second issue, I sent pull request #24 instead!

invisiblefunnel commented 11 years ago

Hi @adamonduty. Can this issue be closed in favor of #24?

adamonduty commented 11 years ago

They're really two separate issues. This one is scoping an (IMO) overly broad rescue, and the other helps enable a custom event retriever...

invisiblefunnel commented 10 years ago

I agree that the rescue is too broad. Do you mind opening a new pull request with your commit so we can discuss the implementation? Thanks for working on this, it's very close already.

adamonduty commented 10 years ago

Pull request #26