Fix encoding of arrays that are sent in query strings #612
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ob-stripe
suggested changes
Dec 7, 2017
As discussed in #608, we currently have a problem where Faraday deconstructs a query string that we encode and strips out any of the array index numbers that we added. It's not too clear on why it does this, but it appears to be built in at a pretty low level and hard to change. I spent a little time on this and it turns out that we can avoid the bad code by depending on Faraday's `params` accessor on a request instead of trying to do the encoding ourselves. Webmock has a pretty hard time detecting the difference, but you can see some before and after encoding here. Before: ``` I, [2017-12-06T17:41:23.083942 #36737] INFO -- : get http://localhost:12111/v1/invoices/upcoming?customer=cus_123&subscription_items%5B%5D%5Bplan%5D=gold&subscription_items%5B%5D%5Bquantity%5D=2 ``` After: ``` I, [2017-12-06T17:42:12.727752 #37158] INFO -- : get http://localhost:12111/v1/invoices/upcoming?customer=cus_123&subscription_items%5B0%5D%5Bplan%5D=gold&subscription_items%5B0%5D%5Bquantity%5D=2 ``` Honestly, some of this can still use a lot of cleanup: it's weird that we manually encode parameters ourselves for bodies, but not for queries; but I think this'll fix the problem for now. Fixes #608.
brandur-stripe
force-pushed
the
brandur-fix-array-encoding-in-non-post
branch
from
c06b258 to
2bc471d
Compare
Yeah totally. I think it should be relatively easy to clean this up too, and I could potentially take a stab on a follow up pull request. |
|
r? @ob-stripe Thanks for taking a look OB! Mind doing one more review pass? Thanks! |
ob-stripe
approved these changes
Dec 7, 2017
|
lgtm 👍 |
|
FYI. this saved my life! It was really frustrated seeing that the same exact request params coming from the Stripe dashboard (while editing a subscription's items) wouldn't work went sent from my code ( using v. 3.8.0). Upgrading fixed it. 💥 Thanks guys |
|
@joebartels No worries! Glad it helped :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #608, we currently have a problem where Faraday
deconstructs a query string that we encode and strips out any of the
array index numbers that we added. It's not too clear on why it does
this, but it appears to be built in at a pretty low level and hard to
change.
I spent a little time on this and it turns out that we can avoid the bad
code by depending on Faraday's
paramsaccessor on a request instead oftrying to do the encoding ourselves. Webmock has a pretty hard time
detecting the difference, but you can see some before and after encoding
here.
Before:
After:
Honestly, some of this can still use a lot of cleanup: it's weird that
we manually encode parameters ourselves for bodies, but not for queries;
but I think this'll fix the problem for now.
Fixes #608.