This is just a cosmetic change that renames `Stripe.configuration` to
just `Stripe.config`. We use the shorter "config" in most other places
including `StripeClient#config`, so I feel that this is overall more
consistent.
This change is backwards compatible because the new accessor came in
with #968, and that hasn't been given a formal release yet.
I've left the class name as `StripeConfiguration` which IMO is fine. The
class uses the expanded form of the name while vars and accessors use
the shorter `config`. Also, `StripeConfiguration` has been around a
little bit longer, so renaming it is somewhat backwards incompatible
too.
Follows up #968.
As a relic from when we had global configuration, anytime any config
value is changed on any client, we still clear all connection managers
everywhere on every thread, even though this is not necessary. This
means that we lose all open connections, etc.
Here, we make changes so that if a configuration is changed, we only
clear the configuration managers pertaining to that one particular
configuration, thus conserving resources globally.
Co-authored-by: Brandur <brandur@brandur.org>
When populating `StripeObject`s, we add accessors to them so that people
can access fields like `obj.currency`.
This was probably only meant to apply to API resources, but through
what might have been an accident of history, we've also traditionally
unmarshaled any hash that comes back from the API as a `StripeObject`,
including `metadata` fields. This allows some convenience because users
can access values like `obj.metadata.my_field`, but is also obviously a
minefield for potential problems.
In issue #969, what's essentially happening is that because there's a
metadata field named `class`, we've overwritten the object's normal
`class` method with our own custom one that accesses the metadata value.
Amazingly, the object can still marshal/unmarshal mostly properly, but
fails on this line as we try to access `obj.class` and that turns out to
be a metadata value instead of a class:
``` ruby
when StripeObject
obj.class.construct_from(
...
```
Here I solve the problem by banning accessors added with the name
`class`. This has a slight risk of backward incompatibility in that
users that previously had metadata named "class" will now have to use
square bracket accessors instead like `obj.metadata[:class]`, but
honestly, I just can't see anything good in allowing "class" to be used
as an accessor.
An alternative solution might be to alias `class` in `StripeObject` and
then make sure we always use that in places like `initialize_from` and
`deep_copy`.
The best long term solution would be to stop add accessors to metadata
objects. This just seems like a bad idea given that there are still
myriads of Ruby built-ins that could potentially be overwritten. This is
definitely a considerably-sized breaking change though, so we'd have to
do it on a major.
This changes allows for each instance of StripeClient to have its own
configuration object instead of relying on the global config. Each
instance can be configured to override any global config values
previously set.
`Customer#delete_discount` has been broken for some time in that it
tries to re-initialize `self` (which is a customer) with a received
discount response. This is incorrect and leads to various problems.
Here, we redefine the return value of `delete_discount` as a discount,
and have it no longer mutate the object on which is was called. We add a
comment as well just to help flag some of the behavior which could
potentially be confusing.
Fixes#963.
* 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.
Adds a `Stripe::StripeConfiguration` object to manage internal and user
supplied configuration options.
This is primarily motivated by #921 in order to provide a way to set
options on for an instance of `StripeClient`.
The params for AccountLink changed in `v0.93.0`, which will cause the
test suite to fail unless `stripe-mock` is pinned to an earlier version.
Fixes#934
Adds a new `generate_header` method for the webhooks module, following
up #915. This method doesn't help in any way with webhook verification,
but may be useful to users in their test suites as it allows them to
easily simulate the contents of a header that Stripe might have sent.
We briefly discussed an alternative design here, but this one seems like
the best fit:
https://github.com/stripe/stripe-ruby/pull/915#issuecomment-620164654
Exposes the `.compute_signature` method, which may be useful when
testing webhook signing in test suites.
I change the API slightly so that a caller isn't forced to do as much
string mangling, and to match the one that we already have in stripe-go:
``` go
func ComputeSignature(t time.Time, payload []byte, secret string) []byte {
```
Add basic documentation and test case. I also change a few things around
so that we send `Time` objects around more often where applicable, and
don't change then to Unix integers until the last moment that we need
to.
The one other alternative API I considered is this one, which would
default the timestamp to the current time to allow the method to be
called with one fewer arg:
``` ruby
def self.compute_signature(payload, secret: timestamp: Time.now)
```
I decided against it in the end though because it does remove some
explicitness, and it's not a big deal to just pass in `Time.now`,
especially given that this is not expected to be a commonly used method.
Fixes#912.
Adds an easy accessor on `StripeError` which indicates whether the error
occurred previously, but was replayed on this request because the user
passed the same idempotency key as that original request.
Fixes#905.
Adds a new instrumentation callback called `request_begin` which, as the
name suggests, is invoked before an HTTP request is dispatched. As
outlined originally in #900, the idea is that this will enable a set of
hooks that can be used for distributed tracing.
The PR also renames the existing `request` callback to `request_end`,
although the old name is still invoked for the time being for backwards
compatibility.
A special `user_data` property is passed to `request_begin` which allows
subscribers to set custom data that will be passed through to
`request_end` for any given request. This allows, for example, a user
assigned ID to be set for the request and recognized on both ends.
I chose the naming `_begin` and `_end` (as opposed to start/finish or
any other combination) based on the naming conventions of Ruby itself.
Fixes#900.
I just noticed while running tests that we produce some accidental
output because both of `Source#source_transactions` and
`SubscriptionItem#usage_record_summaries` are considered deprecated and
have warnings attached.
Here we capture output to `$stderr` and assert on it from the test cases
that call these deprecated methods -- this pattern is already well
established elsewhere in the test suite.
* Add simple instrumentation callback
We used to insert Faraday::Request::Instrumentation into our Faraday middleware
stack to be able to instrument Stripe calls with StatsD. With Faraday being
removed in version 5, this required some rework. This commit implements a simple
callback system that can be used with any kind of instrumentation system.
* Add a topic to Stripe::Instrumentation notifications
... and a :request topic to subscribe to
* Use a RequestEvent value object instead of positional args in callback
This way the RequestLogContext object doesn't get exposed externally. Since the
same value object can be received by multiple subscribers it is frozen to
prevent accidental mutations across threads.
* Relocate tests for instrumentation and add more tests
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)`
* Support backwards pagination with list's `#auto_paging_each`
Previously, `#auto_paging_each` would always try to paginate forward,
even if it was clear based on the list's current filters that the user
had been intending to iterate backwards by specifying an `ending_before`
filter exclusively.
Here we implement backwards iteration by detecting this condition,
reversing the current list data, and making new requests for the
previous page (instead of the next one) as needed, which allows the user
to handle elements in reverse logical order.
Reversing the current page's list is intended as a minor user feature,
but may possibly be contentious. For background, when backwards
iterating in the API, results are still returned in "normal" order. So
if I specifying `ending_before=7`, the next page would look like `[4, 5,
6`] instead of `[6, 5, 4]`. In `#auto_paging_each` I reverse it to `[6,
5, 4]` so it feels to the user like they're handling elements in the
order they're iterating, which I think is okay. The reason it might be
contentious though is that it could be a tad confusing to someone who
already understands the normal `ending_before` ordering in the API.
Fixes#864.
* Allow `ending_before` and `starting_after` to remain in hydrated list object
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.
If #857 comes in, it turns out that we don't need Timecop anymore (it
doesn't freeze the monotic clock, so I had to find another way) -- here
we remove all mentions of it and drop the dependency.
I don't find it causes too much trouble so I'm not against bringing it
back in the future if we need it again, but it seems good for project
cleanliness to take it out for now.
Drops the use of `Time.now` in favor of using the system's monotonic
clock for various operations that calculate and use elapsed duration.
The latter is preferable because in some cases `Time.now` can be
unstable, like if it's set manually by a system administrator or an NTP
daemon.
I don't expect that the previous code would actually have caused trouble
in the vast majority of normal situations, so I'm not going to backport
anything, but this seems like good hygiene.
For better or worse I had to wrap the monotonic time calls in a new
`Util` function because (1) the normal invocation is long enough to have
caused a lot of overruns on our 80 character line lengths, and (2)
Timecop doesn't stub the monotonic clock, so the `Util` method gives us
a nice place that we can stub on where necessary.
As seen in stripe-node, adds support for the `Stripe-Should-Retry`
header which is sent by the API when it explicitly would like us to
either retry or _not_ retry.
I'll add `Retry-After` separately at some point, but I punted it on it
for now given that we're not using it yet.
See: https://github.com/stripe/stripe-node/pull/692
Introduces a system for garbage collecting connection managers in an
attempt to solve #850.
Previously, the number of connection managers (and by extension the
number of connections that they were holding) would stay stable if a
program used a stable number of threads. However, if threads were used
disposably, the number of active connection managers out there could
continue to grow unchecked, and each of those could be holding one or
more dead connections which are no longer open, but still holding a file
descriptor waiting to be unlinked in disposed of by Ruby's GC.
This PR introduces a connection manager garbage collector that runs
periodically whenever a new connection manager is created. Connection
managers get a timestamp to indicate when they were last used, and the
GC runs through each one and prunes any that haven't seen use within a
certain threshold (currently, 120 seconds). This should have the effect
of removing connection managers as they're not needed anymore, and thus
resolving the socket leakage seen in #850.
I had to make a couple implementation tweaks to get this working
correctly. Namely:
* The `StripeClient` class now tracks thread contexts instead of
connection managers. This is so that when we're disposing of a
connection manager, we can set `default_connection_manager` on its
parent thread context to `nil` so that it's not still tracking a
connection manager that we're trying to get rid of.
* `StripeClient` instances can still be instantiated as before, but no
longer internalize a reference to their own connection manager,
instead falling back to the one in the current thread context. The
rationale is that when trying to dispose of a connection manager, we'd
also have to dispose of its reference in any outstanding
`StripeClient` instances that might still be tracking it, and that
starts to get a little unwieldy. I've left `#connection_manager` in
place for backwards compatibility, but marked it as deprecated.
Previously, we had quite a few error tests that were written like this:
``` ruby
begin
client.execute_request(:post, "/v1/charges")
rescue Stripe::InvalidRequestError => e
assert_equal(404, e.http_status)
assert_equal(true, e.json_body.is_a?(Hash))
end
```
The trouble with that pattern is that although they'll _usually_ work,
the test will incorrectly pass if no error at all is thrown because the
`rescue` never activates and therefore the assertions never run.
We change them to use `assert_raises` like so:
``` ruby
e = assert_raises Stripe::InvalidRequestError do
client.execute_request(:post, "/v1/charges")
end
assert_equal(404, e.http_status)
assert_equal(true, e.json_body.is_a?(Hash))
```
The weird part is that many of the tests were already using
`assert_raises`, so here we're just converting them all over to use the
same convention.
I've also made a few whitespace tweaks. None of them are significant,
but they were an attempt to standardize a little on the whitespace
layout of many of these tests which were similar.
Follows up #845 to make sure that this sort of regression is much more
difficult in the future by adding a test that makes sure a request ID is
threaded all the way from an HTTP response back through to an error
object. I verified that the test failed before #845 came in.
When requesting error#request_id it returned nil although the request id was available in the request headers. With version 5 and switch to Net::HTTP this value is now a string, not a symbol which means that the request_id was not stored on the error correctly.
Tweaks the retry logic so that 429s which have the special status code
lock_timeout are retried automatically.
Similar to what was recently implemented in Go: stripe/stripe-go#935
