* Codegen for openapi 474461f
* Add and fix tests for the latest stripe-mock
Some of the tests had to be changed/mocked because stripe-mock has a bug
where the includable sub-lists it returns have the wrong url set.
Because of this, when you call create/list/etc. on one of those sub-lists
the calls fails due to that URL being incorrect.
Moved one test to use charge+refund (auto-expanded) and another used a
mock to have the right URL returned.
There are two kinds of API operations: collection and element specific.
The signature between the two is slightly different:
- **collection**: (params, opts)
- **element specific**: (id, params, opts)
If a user doesn't realize the difference, they may attempt to use the
collection signature when performing an element specific operation like:
```
Stripe::PaymentIntent.cancel('pi_1234', 'sk_test_key')
# Results in an error: NoMethodError: undefined method `key?' for "sk_test"
```
The resulting error message isn't very useful for debugging.
Instead,this PR adds a message letting the user know what it's expecting:
`request params should be either a Hash or nil (was a String)`
Previously, if you specified a non-nil non-string opt value, like a
symbol for `idempotency_key`, you'd get a pretty user-unfriendly error
from `Net::HTTP`:
```
/Users/brandur/.rbenv/versions/2.4.5/lib/ruby/2.4.0/net/http/header.rb:21:in `block in initialize_http_header': undefined method `strip' for :foo:Symbol (NoMethodError)
```
Here, we introduce a new argument error that makes it a little easier
for someone to read. The impetus for the change is that we had an
internal product quality report where someone ran into this and was
confused.
I'm pretty sure this change is backward compatible because `Net::HTTP`
would call `strip` on anything that was passed in as a value, and
generally just strings would support that. There may be some other less
common data type that was accidentally compatible that someone was
using, but that case should be quite unusual.
* 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)
Raises a slightly more helpful error message when passing a non-string
to a custom method (currently, it reads "no implicit conversion of Hash
into String", which is terrible).
This a partial remediation for the problem encountered in #809.
* Bump Rubocop to 0.57.2
* Style/StderrPuts: Use warn instead of .puts
* Style/ExpandPathArguments: Use expand_path('../test_helper', __dir__) instead of expand_path('../../test_helper', __FILE__)
* Style/Encoding: Unnecessary utf-8 encoding comment
* Style/StringLiterals: Prefer double-quoted strings
* Style/AccessModifierDeclarations
* Style/FormatStringToken: Prefer annotated tokens
* Naming/UncommunicativeMethodParamName
* Metrics/LineLength: set maximum line length to 100 characters
* Style/IfUnlessModifier: Favor modifier if usage when having a single-line body
* Style/ClassVars
* Metrics/LineLength: set maximum line length to 80 characters (default)
* Style/AccessModifierDeclarations: EnforcedStyle: inline
I was testing with a new version of stripe-mock and it caught a few
problems with query parameter validation on. This patch contains some
minor fixes to address them.
Adds the magic `frozen_string_literal: true` comment to every file and
enables a Rubocop rule to make sure that it's always going to be there
going forward as well.
See here for more background [1], but the basic idea is that unlike many
other languages, static strings in code are mutable by default. This has
since been acknowledged as not a particularly good idea, and the
intention is to rectify the mistake when Ruby 3 comes out, where all
string literals will be frozen. The `frozen_string_literal` magic
comment was introduced in Ruby 2.3 as a way of easing the transition,
and allows libraries and projects to freeze their literals in advance.
I don't think this is breaking in any way: it's possible that users
might've been pulling out one of are literals somehow and mutating it,
but that would probably not have been useful for anything and would
certainly not be recommended, so I'm quite comfortable pushing this
change through as a minor version.
As discussed in #641.
[1] https://stackoverflow.com/a/37799399
I wanted to see what fixing Rubocop TODOs was like, so I tried to
eliminate all the easy ones. Most of these were pretty easy, and the
changes required are relatively minimal.
Some of the stuff left is harder. Pretty much everything under
`Metrics/*` is going to be a pretty big yak shave. A few of the others
are just going to need a little more work (e.g. `Style/ClassVars` and
`Style/GuardClause`). Going to stop here for now.
Moves away from Committee and towards stripe-mock, an external
self-contained executable API stub server based on OpenAPI [1]. The
motivation here is that instead of making stripe-ruby a special
snowflake, we can use a single well-tested and feature-rich mock
implementation to drive every API's test suite.
[1] https://github.com/stripe/stripe-mock
Follows the path established in 2d75c8f by porting the rest of
stripe-ruby's tests over to OpenAPI. There are a few other changes here
where I've removed some tests that are duplicated or don't make much
sense, or reorganized how we test certain things, but this commit is
largely the same migration operation applied in bulk a few dozen test
suites.
This has been discussed, but we'll finally be doing it for the next
major version so that we can introduce a few features that depend on
gems that don't support 1.9.
Since #433, saving API resources nested under other API resources has
not been the default. Instead, any instances where this should occur
have been special cased with specific method implementations that would
set the `#save_with_parent` flag when a field is written.
This ended up causing some problems because as seen in #457, because
places that we need to do this aren't well vetted, some were forgotten.
This makes implementation of new fields that need this behavior simpler
by implementing a `.save_nested_resource` metraprogramming method on the
`APIResource` class. This can be called as necessary by any concrete API
resource implementations.
We replace existing implementatinos and also add one to `Subscription`,
which had previously been suffering from a similar problem where its
`#source` had not received a special case.
This moves away from rest-client's convention of using symbols as header
names so as to present less obfuscation as to how these are actually
named when they go over the wire.
Because headers can be injected via the bindings' API I was initially
worried that this change might break something, but upon inspection of
rest-client source, I can see now that headers take precedence as
assigned by their insertion order into the header hash, and are
"stringified" in that same loop [1]. This means that even if a user
injects a symbolized header name (`:idempotency_key`), it will still
correctly overwrite the one generated by stripe-ruby despite that using
the string format (`"Idempotency-Key"`).
[1] https://github.com/rest-client/rest-client/blob/master/lib/restclient/request.rb#L603,L625
Two changes:
1. The HTTP retry path has been refactored to make retries on errors
that are not RestClient exceptions possible by bringing the logic up a
level. This also has the effect of somewhat simplifying the exception
handling logic which can be somewhat difficult to reason about right
now.
2. Retry on `RestClient::Conflict` (a 409 from the API) as discussed in
issue #431.
Fixes#431.
I'm not sure exactly what changed here (did we change the `$VERBOSE`
setting?), but I'm not seeing a whole lot of warnings when running the
test suites locally and in CI. For example:
```
Started
........................................./home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
............../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
......../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
.../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
........./home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
...
..../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
....../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
..../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
......./home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
........./home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
........../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
................./home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
.../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
..../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
....../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
..........
........./home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
....../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
......../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
......../home/travis/build/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:6: warning: instance variable @opts not initialized
............./home/travis/build/stripe/stripe-ruby/lib/stripe/stripe_object.rb:35: warning: instance variable @values not initialized
./home/travis/build/stripe/stripe-ruby/lib/stripe/stripe_object.rb:35: warning: instance variable @values not initialized
...................../home/travis/build/stripe/stripe-ruby/lib/stripe/transfer.rb:8: warning: instance variable @api_key not initialized
..............
..
Finished in 0.785621037 seconds.
```
Most of these are due to unused or uninitialized variables. This patch
fixes all warnings by fixing offending code.
This is consistent with API library behavior in other languages, and with our
API documentation (which doesn't mention needing to handle this type of error).
Prior to my last major serialization refactor, there was a check in the
code that would remove any subobjects from serialization that were of
their own proper resource type (for example, if a charge contained a
customer, that customer would be removed).
What I didn't realize at the time is that the old serialization code had
a bug/quirk that would allow *certain types* of subobjects that were API
resources to make it through unscathed.
In short, the behavior requirement here is *directly* contradictory.
There was a test in place that would make sure that `customer` was
removed from this hash:
``` ruby
{
:id => 'ch_id',
:object => 'charge',
:customer => {
:object => 'customer',
:id => 'customer_id'
}
}
```
But, as reported in #406, we expect, and indeed need, for `source` (a
card) to make it through to the API in this hash:
``` ruby
{
:id => 'cus_id',
:object => 'customer',
:source => {
:object => 'card',
:id => 'card_id'
}
}
```
My proposal here is to just remove the check on serializing API
resources. The normal code that only sends up keys/hashes that have
changed is still in place, so in the first example, `customer` still
isn't sent unless the user has directly manipulated a field on that
subobject. I propose that in those cases we allow the API itself to
reject the request rather than try to cut it off at the client level.
Unfortunately, there is some possibility that removing those API
resources is important for some reason, but of course there's no
documentation on it beyond the after-the-fact post-justification that I
wrote during my last refactor. I can't think of any reason that it would
be too destructive, but there is some level of risk.
Hashes are converted to StripeObject when used as params of save.
They need to be converted to hash on serialize.
Signed-off-by: François de Metz <francois@stormz.me>
Signed-off-by: Cyril Duez <cyril@stormz.me>
As discussed previously in #354 and alluded to in #363, this patch
deprecates the `#refund` helpers on `Charge` and `ApplicationFee` in
favor of the resource-centric approach (i.e. `charge.refunds.create`).
We do this for a few reasons:
1. The new approach is far preferred and uses our modern endpoints. It's
also been the mechanism suggested by the documentation for ages now.
2. The old approach is somewhat risky in that a forgotten "s" can lead
to an accidental refund (i.e. `charge.refund` instead of
`charge.refunds`).
Follows up #354. Fixes#363.
