Skip to content

Commit

Permalink
stripe-ruby V5 (#815)
Browse files Browse the repository at this point in the history
* Convert library to use built-in `Net::HTTP`

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.

* Drop support for old versions of Ruby

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.

* Make `CardError`'s `code` parameter named instead of positional (#816)

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.

* Bump Rubocop to latest version (#818)

* Ruby minimum version increase followup (#819)

* Remove old deprecated methods (#820)

* Remove all alias for list methods (#823)

* Remove UsageRecord.create method (#826)

* Remove IssuerFraudRecord (#827)

* Add ErrorObject to StripeError exceptions (#811)

* Tweak retry logic to be a little more like stripe-node (#828)

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.

* Fix inverted sign for 500 retries (#830)

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.

* Remove a few more very old deprecated methods (#831)

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.

* Remove extraneous slash at the end of the line

* Reset connections when connection-changing configuration changes (#829)

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.

* Minor cleanup in `StripeClient` (#832)

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.

* Do better bookkeeping when tracking state in `Thread.current` (#833)

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.

* Add Invoice.list_upcoming_line_items method (#834)
  • Loading branch information
brandur authored and ob-stripe committed Aug 20, 2019
1 parent 8b45b1d commit 4476651
Show file tree
Hide file tree
Showing 123 changed files with 1,544 additions and 999 deletions.
21 changes: 17 additions & 4 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,31 @@ inherit_from: .rubocop_todo.yml

AllCops:
DisplayCopNames: true
TargetRubyVersion: 2.1
TargetRubyVersion: 2.3

Layout/CaseIndentation:
EnforcedStyle: end

Layout/IndentArray:
Layout/IndentFirstArrayElement:
EnforcedStyle: consistent

Layout/IndentHash:
Layout/IndentFirstHashElement:
EnforcedStyle: consistent

Metrics/LineLength:
# This can be re-enabled once we're 2.3+ only and can use the squiggly heredoc
# operator. Prior to that, Rubocop recommended bringing in a library like
# ActiveSupport to get heredoc indentation, which is just terrible.
Layout/IndentHeredoc:
Enabled: false

Metrics/ClassLength:
Exclude:
- "test/**/*.rb"

Metrics/LineLength:
Exclude:
- "lib/stripe/resources/**/*.rb"
- "test/**/*.rb"

Metrics/MethodLength:
# There's ~2 long methods in `StripeClient`. If we want to truncate those a
Expand All @@ -33,6 +43,9 @@ Style/AccessModifierDeclarations:
Style/FrozenStringLiteralComment:
EnforcedStyle: always

Style/NumericPredicate:
Enabled: false

Style/StringLiterals:
EnforcedStyle: double_quotes

Expand Down
19 changes: 10 additions & 9 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -1,24 +1,25 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2019-05-24 10:18:48 -0700 using RuboCop version 0.57.2.
# on 2019-07-30 09:56:31 +0800 using RuboCop version 0.73.0.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
# versions of RuboCop, may require this file to be generated again.

# Offense count: 20
# Offense count: 23
Metrics/AbcSize:
Max: 53
Max: 51

# Offense count: 31
# Offense count: 33
# Configuration parameters: CountComments, ExcludedMethods.
# ExcludedMethods: refine
Metrics/BlockLength:
Max: 498
Max: 509

# Offense count: 11
# Offense count: 12
# Configuration parameters: CountComments.
Metrics/ClassLength:
Max: 673
Max: 694

# Offense count: 12
Metrics/CyclomaticComplexity:
Expand All @@ -29,10 +30,10 @@ Metrics/CyclomaticComplexity:
Metrics/ParameterLists:
Max: 7

# Offense count: 7
# Offense count: 8
Metrics/PerceivedComplexity:
Max: 17

# Offense count: 84
# Offense count: 86
Style/Documentation:
Enabled: false
6 changes: 1 addition & 5 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
language: ruby

rvm:
- 2.1
- 2.2
- 2.3
- 2.4
- 2.5
- 2.6
- jruby-9.0.5.0
- jruby-9.2.7.0

notifications:
email:
Expand All @@ -25,8 +23,6 @@ cache:
- stripe-mock

before_install:
# Install bundler 1.x, because we need to support Ruby 2.1 for now
- gem install bundler -v "~> 1.0"
# Unpack and start stripe-mock so that the test suite can talk to it
- |
if [ ! -d "stripe-mock/stripe-mock_${STRIPE_MOCK_VERSION}" ]; then
Expand Down
14 changes: 2 additions & 12 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ gemspec
group :development do
gem "coveralls", require: false
gem "mocha", "~> 0.13.2"
gem "rack", ">= 2.0.6"
gem "rake"
gem "shoulda-context"
gem "test-unit"
Expand All @@ -18,18 +19,7 @@ group :development do
# `Gemfile.lock` checked in, so to prevent good builds from suddenly going
# bad, pin to a specific version number here. Try to keep this relatively
# up-to-date, but it's not the end of the world if it's not.
# Note that 0.57.2 is the most recent version we can use until we drop
# support for Ruby 2.1.
gem "rubocop", "0.57.2"

# Rack 2.0+ requires Ruby >= 2.2.2 which is problematic for the test suite on
# older Ruby versions. Check Ruby the version here and put a maximum
# constraint on Rack if necessary.
if RUBY_VERSION >= "2.2.2"
gem "rack", ">= 2.0.6"
else
gem "rack", ">= 1.6.11", "< 2.0" # rubocop:disable Bundler/DuplicatedGem
end
gem "rubocop", "0.73"

platforms :mri do
gem "byebug"
Expand Down
13 changes: 5 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ gem build stripe.gemspec

### Requirements

- Ruby 2.1+.
- Ruby 2.3+.

### Bundler

Expand Down Expand Up @@ -112,15 +112,13 @@ Stripe::Charge.retrieve(
)
```

### Configuring a Client
### Accessing a response object

While a default HTTP client is used by default, it's also possible to have the
library use any client supported by [Faraday][faraday] by initializing a
`Stripe::StripeClient` object and giving it a connection:
Get access to response objects by initializing a client and using its `request`
method:

```ruby
conn = Faraday.new
client = Stripe::StripeClient.new(conn)
client = Stripe::StripeClient.new
charge, resp = client.request do
Stripe::Charge.retrieve(
"ch_18atAXCdGbJFKhCuBAa4532Z",
Expand Down Expand Up @@ -275,7 +273,6 @@ Update the bundled [stripe-mock] by editing the version number found in
[api-keys]: https://dashboard.stripe.com/account/apikeys
[connect]: https://stripe.com/connect
[curl]: http://curl.haxx.se/docs/caextract.html
[faraday]: https://github.com/lostisland/faraday
[idempotency-keys]: https://stripe.com/docs/api/ruby#idempotent_requests
[stripe-mock]: https://github.com/stripe/stripe-mock
[versioning]: https://stripe.com/docs/api/ruby#versioning
Expand Down
15 changes: 8 additions & 7 deletions Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ RuboCop::RakeTask.new

desc "Update bundled certs"
task :update_certs do
require "faraday"
require "net/http"
require "uri"

fetch_file "https://curl.haxx.se/ca/cacert.pem",
::File.expand_path("../lib/data/ca-certificates.crt", __FILE__)
Expand All @@ -23,14 +24,14 @@ end
# helpers
#

def fetch_file(url, dest)
def fetch_file(uri, dest)
::File.open(dest, "w") do |file|
resp = Faraday.get(url)
unless resp.status == 200
abort("bad response when fetching: #{url}\n" \
"Status #{resp.status}: #{resp.body}")
resp = Net::HTTP.get_response(URI.parse(uri))
unless resp.code.to_i == 200
abort("bad response when fetching: #{uri}\n" \
"Status #{resp.code}: #{resp.body}")
end
file.write(resp.body)
puts "Successfully fetched: #{url}"
puts "Successfully fetched: #{uri}"
end
end
71 changes: 56 additions & 15 deletions lib/stripe.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
# Stripe Ruby bindings
# API spec at https://stripe.com/docs/api
require "cgi"
require "faraday"
require "json"
require "logger"
require "net/http"
require "openssl"
require "rbconfig"
require "securerandom"
Expand All @@ -28,10 +28,13 @@
require "stripe/errors"
require "stripe/object_types"
require "stripe/util"
require "stripe/connection_manager"
require "stripe/multipart_encoder"
require "stripe/stripe_client"
require "stripe/stripe_object"
require "stripe/stripe_response"
require "stripe/list_object"
require "stripe/error_object"
require "stripe/api_resource"
require "stripe/singleton_api_resource"
require "stripe/webhook"
Expand Down Expand Up @@ -70,9 +73,20 @@ module Stripe
@enable_telemetry = true

class << self
attr_accessor :stripe_account, :api_key, :api_base, :verify_ssl_certs,
:api_version, :client_id, :connect_base, :uploads_base,
:open_timeout, :read_timeout, :proxy
attr_accessor :api_key
attr_accessor :api_version
attr_accessor :client_id
attr_accessor :stripe_account

# These all get manual attribute writers so that we can reset connections
# if they change.
attr_reader :api_base
attr_reader :connect_base
attr_reader :open_timeout
attr_reader :proxy
attr_reader :read_timeout
attr_reader :uploads_base
attr_reader :verify_ssl_certs

attr_reader :max_network_retry_delay, :initial_network_retry_delay
end
Expand All @@ -87,6 +101,11 @@ def self.app_info=(info)
@app_info = info
end

def self.api_base=(api_base)
@api_base = api_base
StripeClient.clear_all_connection_managers
end

# The location of a file containing a bundle of CA certificates. By default
# the library will use an included bundle that can successfully validate
# Stripe certificates.
Expand All @@ -99,6 +118,8 @@ def self.ca_bundle_path=(path)

# empty this field so a new store is initialized
@ca_store = nil

StripeClient.clear_all_connection_managers
end

# A certificate store initialized from the the bundle in #ca_bundle_path and
Expand All @@ -118,6 +139,19 @@ def self.ca_store
end
end

def self.connection_base=(connection_base)
@connection_base = connection_base
StripeClient.clear_all_connection_managers
end

def self.enable_telemetry?
@enable_telemetry
end

def self.enable_telemetry=(val)
@enable_telemetry = val
end

# map to the same values as the standard library's logger
LEVEL_DEBUG = Logger::DEBUG
LEVEL_ERROR = Logger::ERROR
Expand Down Expand Up @@ -172,12 +206,19 @@ def self.max_network_retries=(val)
@max_network_retries = val.to_i
end

def self.enable_telemetry?
@enable_telemetry
def self.open_timeout=(open_timeout)
@open_timeout = open_timeout
StripeClient.clear_all_connection_managers
end

def self.enable_telemetry=(val)
@enable_telemetry = val
def self.proxy=(proxy)
@proxy = proxy
StripeClient.clear_all_connection_managers
end

def self.read_timeout=(read_timeout)
@read_timeout = read_timeout
StripeClient.clear_all_connection_managers
end

# Sets some basic information about the running application that's sent along
Expand All @@ -194,14 +235,14 @@ def self.set_app_info(name, partner_id: nil, url: nil, version: nil)
}
end

# DEPRECATED. Use `Util#encode_parameters` instead.
def self.uri_encode(params)
Util.encode_parameters(params)
def self.uploads_base=(uploads_base)
@uploads_base = uploads_base
StripeClient.clear_all_connection_managers
end
private_class_method :uri_encode
class << self
extend Gem::Deprecate
deprecate :uri_encode, "Stripe::Util#encode_parameters", 2016, 1

def self.verify_ssl_certs=(verify_ssl_certs)
@verify_ssl_certs = verify_ssl_certs
StripeClient.clear_all_connection_managers
end
end

Expand Down
6 changes: 0 additions & 6 deletions lib/stripe/api_operations/list.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,6 @@ def list(filters = {}, opts = {})

obj
end

# The original version of #list was given the somewhat unfortunate name of
# #all, and this alias allows us to maintain backward compatibility (the
# choice was somewhat misleading in the way that it only returned a single
# page rather than all objects).
alias all list
end
end
end
Loading

0 comments on commit 4476651

Please sign in to comment.