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.
* 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)
* 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
Makes a few tweaks to hopefully simplify clarity things:
* `FaradayStripeEncoder` now becomes the way to encode all of form,
multipart form, and query parameters.
* Introduce a cache in it so that we don't have to encode everything
twice (once for logging, and once for the request body).
* Try to sanitize logging a bit by replacing `Faraday::UploadIO`s found
in incoming parameters with a string representation of the file (note
that all other styles of file input like `File` or `Tempfile` have
been converted to `Faraday::UploadIO` by the time they reach the
encoder).
As reported in #608, integer-indexed maps currently work when passed as
part of the body, but they are reverted to non-indexed maps when passed
in the query.
It turns out that we actually had two problems:
1. We weren't calling our `Util.encode_parameters` on our query
parameters anywhere, and it's this method will does the integer
encoding.
2. Even when I fixed (1) by calling `Util.encode_parameters`, Faraday
would still strip the integer indexes as they were transformed in
its default `NestedParamsEncoder`.
Here we fix both issues by calling `Util.encode_parameters` and sending
Faraday a custom encoder which bypasses its normal shenanigans.
Unfortunately, this has turned out to be somewhat difficult to test
because the integer-indexed maps also seem to confuse Webmock, which
strips them down to standard maps (I even tried testing against a
string, and it still got it wrong). I did use stripe-mock though to
verify that we are now sending the right payload.
Fixes#608.
Remi pointed out in #666 that we basically just have to keep adding more
more onto the `Max` exception for both these rules every time we add a
new API resource.
Here I suggest that we modify the check on method length in two ways:
1. Permanently disable the cop on `Util.object_classes`. This is just
going to keep growing until we change are approach to it.
2. Choose a more reasonable maximum of 50 lines for elsewhere (IMO, the
default of 10 is just too short). Most of our methods already come in
below this, but there's a couple outliers like `#execute_request` in
`StripeClient`. If we knock over some of those, we could lower this
number again, but I suspect that we'd probably want somewhere closer
to 30 (instead of 10) event then.
I also disable the check on module length completely. I'm not convinced
this is a very good heuristic for code quality.
If specifying both query parameters in a path/URL down to Faraday (e.g.,
`/v1/invoices/upcoming?coupon=25OFF`) _and_ query parameters in a hash
(e.g., `{ customer: "cus_123" }`), it will silently overwrite the ones
in the path with the ones in the hash. This can cause problems where
some critical parameters are discarded and causes an error, as seen in
issue #646.
This patch modifies `#execute_request` so that before going out to
Faraday we check whether the incoming path has query parameters. If it
does, we decode them and add them to our `query_params` hash so that
all parameters from either place are preserved.
Fixes#646.
So we have a bit of a problem right now when it comes to replacing a
`StripeObject` that's embedded in an API resource.
Most of the time when someone does this, they want to _replace_ an
object embedded in another object. Take setting a source on a
subscription for example:
``` ruby
subscription.source = {
object: 'card',
number: 123,
}
subscription.save
```
In the case above, the serialized parameters should come out as:
```
source[object]=card&source[number]=123
```
That should apply even if the previous source had something else set on
it which we're not going to set this time -- say an optional parameter
like `source[address_state]`. Those should not be present at all in the
final serialized parameters.
(Another example is setting a `payout_schedule` as seen in #631 which is
PR is intended to address.)
There is an exception to this rule in the form of metadata though.
Metadata is a bit of a strange case in that the API will treat it as
additive, so if we send `metadata[foo]`, that will set the `foo` key,
but it won't overwrite any other keys that were already present.
This is a problem because when a user fully sets `metadata` to a new
object in Ruby, what they're probably trying to do is _replace_ it
rather than add to it. For example:
``` ruby
subscription.metadata
=> { old: 'bar' }
subscription.metadata = {
new: 'baz'
}
subscription.save
```
To accomplish what the user is probably trying to do, we actually need
to send `metadata[old]=&metadata[new]=baz` so that we empty the value of
`old` while simultaneously setting `new` to `baz`.
In summary, metadata behaves different from other embedded objects in a
fairly fundamental way, and because the code is currently only set up to
handle the metadata case, it's not behaving correctly when other types
of objects are being set. A lot of the time emptying values like we do
for `metadata` is benign, but as we've seen in #631, sometimes it's not.
In this patch, I modify serialization to only empty out object values
when we see that parameter is `metadata`.
I'm really not crazy about the implementation here _at all_, but I'm
having trouble thinking of a better way to do it. One possibility is to
introduce a new class annotation like `empty_embedded_object :metadata`,
but that will have to go everywhere and might be error-prone in case
someone forgets it on a new resource type. If anyone has a suggestion
for an alternative (or can let me know if I'm missing something), I'd
love to hear it.
This PR is an alternate to #631.
Excludes `idempotency_key` from opts to persist between API requests.
Obviously the same idempotency key is not something that we ever want to
use again.
Fixes#598.
We were previously using a bit of a hack to get a free deep copy
implementation through Ruby's marshaling framework. Lint call this out
as a security problem though, and rightfully so: when combined with
unsanitized user input, unmarshaling can result in very serious security
breaches involving arbitrary code execution.
This patch removes all uses of marshal/unmarshal in favor of
implementing a deep copy method for `StripeObject`. I also reworked some
of the constants around what keys are available for `opts`. I'm still
not completely happy with the results, but I think it's going to need a
slightly larger refactor in order to get somewhere truly good.
There is what could be a breaking change for people doing non-standard
stuff with the library: the opts that we copy with an object are now
whitelisted, so if they were being used to pass around extraneous data,
that might not work as expected anymore. But because this is a contract
that we never committed to, I don't think I'd bump the major version for
change.
Removes Rubocop TODO around guard clauses and fixes the outstanding
offenses.
This is starting to get into territory that feels of more dubious value
to me, but at least it did get me writing a couple more tests, so let's
see how it goes by keeping this on.
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.
