stripe-ruby V5 #815
stripe-ruby V5 #815
Conversation
ob-stripe
changed the title
ob-stripe
force-pushed
the
integration-v5
branch
4 times, most recently
from
34a603a
to
6e4a65e
Compare
|
Here's the WIP wiki page for the migration guide: https://github.com/stripe/stripe-ruby/wiki/%5BWIP%5D-Migration-guide-for-v5. @brandur-stripe Could you fill the "Faraday removal" section? @brandur-stripe @remi-stripe Is there anything else we want to include in this major release? |
|
Amazing OB!! Thanks for continuing to roll this forward — when I wrote "arbitrarily target mid-August for eventual release" above that seemed like a long ways away at the time, but we're now basically there already!
Yes! Done.
I swear this is one of those things where I'll get an idea as soon as we ship V5, but I can't think of anything major right now. The only piece that occurs to me is refactoring — e.g. |
|
I've also just noticed that unfortunately our internal upgrade to V5 won't be totally trivial — in the test suite we're actually using a custom Faraday middleware to set a service name, and also using Faraday's Rack adapter to call into the Rack API. The middleware might have to be replaced by a Bapi Rack middleware, and the Rack adapter will probably need to move to webmock. Going to add a few comments about this to the changelog as well. |
ob-stripe
force-pushed
the
integration-v5
branch
3 times, most recently
from
7fa34e8
to
5695b26
Compare
Moves the library off of Faraday and over onto the standard library's built-in `Net::HTTP` module. The upside of the transition is that we break away from a few dependencies that have caused us a fair bit of trouble in the past, the downside is that we need more of our own code to do things (although surprisingly, not that much more). The biggest new pieces are: * `ConnectionManager`: A per-thread class that manages a connection to each Stripe infrastructure URL (like `api.stripe.com`, `connect.stripe.com`, etc.) so that we can reuse them between requests. It's also responsible for setting up and configuring new `Net::HTTP` connections, which is a little more heavyweight code-wise compared to other libraries. All of this could have lived in `StripeClient`, but I extracted it because that class has gotten so big. * `MultipartEncoder`: A class that does multipart form encoding for file uploads. Unfortunately, Ruby doesn't bundle anything like this. I built this by referencing the Go implementation because the original RFC is not very detailed or well-written. I also made sure that it was behaving similarly to our other custom implementations like stripe-node, and that it can really upload a file outside the test suite. There's some risk here in that it's easy to miss something across one of these big transitions. I've tried to test out various error cases through tests, but also by leaving scripts running as I terminate my network connection and bring it back. That said, we'd certainly release on a major version bump because some of the interface (like setting `Stripe.default_client`) changes.
Drops support for Ruby 2.1 (EOL March 31, 2017) and 2.2 (EOL March 31,
2018). They're removed from `.travis.yml` and the gemspec and RuboCop
configuration have also been updated to the new lower bound.
Most of the diff here are minor updates to styling as required by
RuboCop:
* String literals are frozen by default, so the `.freeze` we had
everywhere is now considered redundant.
* We can now use Ruby 1.9 style hash syntax with string keys like `{
"foo": "bar" }`.
* Converted a few heredocs over to use squiggly (leading whitespace
removed) syntax.
As discussed in Slack, I didn't drop support for Ruby 2.3 (EOL March 31,
2019) as we still have quite a few users on it. As far as I know
dropping it doesn't get us access to any major syntax improvements or
anything, so it's probably not a big deal.
Makes the `code` parameter on `CardError` named instead of positional. This makes it more consistent with the rest of the constructor's parameters and makes instantiating `CardError` from `StripeClient` cleaner. This is a minor breaking change so we're aiming to release it for the next major version of stripe-ruby.
ob-stripe
force-pushed
the
integration-v5
branch
from
d394f69
to
56ea5f4
Compare
|
@brandur-stripe @remi-stripe Unless there's anything else you want to add, I think this is ready for release! |
|
@ob-stripe I can't think of anything else. Let's go for Monday then!! |
Tweaks the retry logic to be a little more like stripe-node's. In particular, we also retry under these conditions: * If we receive a 500 on a non-`POST` request. * If we receive a 503. I made it slightly different from stripe-node which checks for a 500 with `>= 500`. I don't really like that -- if we want to retry specific status codes we should be explicit about it. We're actively re-examining ways on how to make it easier for clients to figure out when to retry right now, but I figure V5 is a good time to tweak this because the modifications change the method signature of `should_retry?` slightly, and it's technically a public method.
I messed up in #828 by (1) accidentally flipping the comparison against `:post` when checking whether to retry on 500, and (2) forgetting to write new tests for the condition, which is how (1) got through. This patch fixes both those problems.
I noticed that we had a couple of other deprecated methods on `Stripe` and `StripeObject` that have been around for a long time. May as well get rid of them too -- luckily they were using `Gem::Deprecate` so they've been producing annoying deprecated warnings for quite a while now.
Adds a few basic features around connection and connection manager management: * `clear` on connection manager, which calls `finish` on each active connection and then disposes of it. * A centralized cross-thread tracking system for connection managers in `StripeClient` and `clear_all_connection_managers` which clears all known connection managers across all threads in a thread-safe way. The addition of these allow us to modify the implementation of some of our configuration on `Stripe` so that it can reset all currently open connections when its value changes. This fixes a currently problem with the library whereby certain configuration must be set before the first request or it remains fixed on any open connections. For example, if `Stripe.proxy` is set after a request is made from the library, it has no effect because the proxy must have been set when the connection was originally being initialized. The impetus for getting this out is that I noticed that we will need this internally in a few places when we're upgrading to stripe-ruby V5. Those spots used to be able to hack around the unavailability of this feature by just accessing the Faraday connection directly and resetting state on it, but in V5 `StripeClient#conn` is gone, and that's no longer possible.
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.
This is largely just another cleanup patch, but does a couple main things: * Hoists the `last_response` value into thread state. This is a very minor nicety, but effectively makes `StripeClient` fully thread-safe, which seems like a minor nicety. Two calls to `#request` to the same `StripeObject` can now be executed on two different threads and their results won't interfere with each other. * Moves state off one-off `Thread.current` keys and into a single one for the whole client which stores a new simple type of record called `ThreadContext`. Again, this doesn't change much, but adds some minor type safety and lets us document each field we expect to have in a thread's context.
|
@ob-stripe Shall we do this today? |
Let's! |
|
Released 5.0.0! \o/ |
|
With the change to Net::HTTP, we're going to have to change how we instrument our stripe requests. With Faraday, we monkeypatched default_conn to add a faraday instrumentation middleware which emitted ActiveSupport::Notification events. I do think it'd be useful to provide a hook into the request lifecycle for non-stripe instrumentation (I do see StripeRequestMetrics, but it's not exactly the same thing we're looking for) Our plan here is to wrap the execute_request method in Stripe::Client in a The interface for execute_request appears to be public, could we make an assumption that the signature of the method will be backwards compatible for minor releases? EDIT: Sorry if this isn't a great place for questions, I figured it'd be better than opening an issue 🙏 |
Yes, we're pretty careful about backwards compatibility, and this method is considered public "enough" that we we'd only break it on a major. In general I agree though that the introduction of some kind of hooks into the library would be a good thing. I was thinking about them for use on a retried request so that users could instrument how often this is happening as opposed to it being mostly opaque, but having something on the request lifecycle would be a good idea too IMO. I'm not quite 100% sure yet what these would look like though. |
|
@Senjai I posted a similar question in #313 (comment). Perhaps we can keep the discussion going there? I'd be interested in some sort of hooks as well! |
|
Anyone has any idea what has changed in this version 5 vs 4 in terms of VCR cassette test, VCR doesn't record any cassettes anymore, Are we using any mock response in test mode? |
|
@alirezaseifi The big change from V4 to V5 was dropping Faraday in favor of Net::HTTP. Just reading the VCR README, it looks like they're both supported, but Faraday is more automatically supported out of the box. You can still use Net::HTTP, but you have to use it through WebMock. The example they have on there uses the |
|
@brandur Thank you so much, I was able to record vcr tests using one more thing is there any way of getting a source by using like
|
|
@alirezaseifi You can still do that, but you need to request that https://stripe.com/docs/api/customers/object#customer_object-sources |
This is an integration branch for the next major/breaking version of stripe-ruby, V5.
Incoming changes (feel free to add something you want to work on to this list):
(⚠️ = breaking changes)
Net::HTTP#813: Convert library to use built-inNet::HTTPCardError'scodeparameter named instead of positional #816: Makecodeargument inCardErrornamed instead of positional.UsageRecord.createmethodIssuerFraudRecordErrorObjecttoStripeErrorexceptionsStripeClient#832: Minor cleanup inStripeClientThread.current#833: Do better bookkeeping when tracking state inThread.currentInvoice.list_upcoming_line_itemsmethod #834: AddInvoice.list_upcoming_line_itemsmethodI'm going to arbitrarily target mid-August for eventual release.
cc @ob-stripe @stripe/api-libraries