Merge query parameters coming from path with params argument

[brandur]

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.

r? @remi-stripe
cc @stripe/api-libraries

[remi-stripe]

The approach looks fine though I wonder if we should add a custom test for the List logic to ensure this is never lost too. It's not the first time we have a weird issue with the upcoming invoice and the fact that it has custom logic on the url and query params.

I'd also want to make sure we're not breaking some backwards compatibility logic with starting_after and ending_before which would be in the URL too if I'm not mistaken and could end up duplicated in the query params?

Also some tests are failing!

[stripe-ci]

Gerald Rule: Rubocop Core File Change

cc @pt-stripe

[brandur-stripe]

The approach looks fine though I wonder if we should add a custom test for the List logic to ensure this is never lost too. It's not the first time we have a weird issue with the upcoming invoice and the fact that it has custom logic on the url and query params.

Yeah, I see what you mean, but I kind of feel like we should just be testing this stuff where its implementation logic lives. It might have also been plausible to have put this in ListObject's implementation, but since it's in StripeClient, I kind of think that it makes sense just to put the tests there.

If .upcoming was doing anything custom, we'd probably want to test that as well, but it's really just relying on pretty standard list logic — the key here is that when a query parameter like customer= is sent to the API, the API reflects that parameter back to you in the response list object's url field. Your client is supposed to just agnostically use whatever value is in there as its next path target. That value containing query parameters is the unusual case, but it should still be handled in a general way.

I'd also want to make sure we're not breaking some backwards compatibility logic with starting_after and ending_before which would be in the URL too if I'm not mistaken and could end up duplicated in the query params?

I'm fairly confident this works, but added an extra test just in case that shows that when a parameter in params overrides one in the URL, the former is preferred. Want to take a look?

Also some tests are failing!

Rubocop :/ Fixed.

[remi-stripe]

LGTM

[brandur-stripe]

Thanks!

[brandur-stripe]

Released as 3.13.1.

[myfitment]

I was just working to figure out where this broke, which was v3.8.2. Super impressed at how quickly you guys fixed this!

[brandur-stripe]

@myfitment Thanks, and glad to help! I ran a test script to look at your problem specifically and this seemed to have fixed it pretty definitively, but let us know if you see any other trouble.