-
Notifications
You must be signed in to change notification settings - Fork 756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Always encode arrays as integer-indexed hashes #565
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks so ✨ !
Few quick questions
{plan: 'potato'}, | ||
{plan: 'rutabaga'}, | ||
{id: 'SOME_ID', deleted: true}, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm somewhat worried about our test coverage reduction here... is there anything in our test suite to verify that these POST bodies are form-encoded with subscription_items[0][plan]
instead of subscription_items[][plan]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair! Looking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so this is difficult to test in the current state. The URL-encoding of the request parameters happens in the original _request
method, but getSpyableStripe()
returns an instance of stripe
where _request
is patched to memorize the method parameters instead of actually sending the request.
We've recently added nock
for testing the telemetry stuff, so we could probably write some new tests that use it to check the actual body of our HTTP requests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think nock sounds like the right tool for this job, should be pretty straightforward - fft DM me or paul if you need help with it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I lied, we didn't add nock at all. No idea why I thought that was the case!
Nevertheless, I tried writing a test with nock to test StripeResource._request
, but I can't seem to have nock intercept the request -- for some reason the request is always sent to the real API server. Mind giving it a try @rattrayalex-stripe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We add nock in #559 😄
Actually, yeah, would be a big +1 to using |
@rattrayalex-stripe I added a couple of encoding tests in Unfortunately, for GET requests, it seems nock doesn't let you check the raw query string -- instead, it uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For GET requests, I believe gives you the path with query, but that's fine because we test GET querystrings elsewhere.
One clarifying question that I'm fairly sure has the answer I'm looking for; assuming so,
LGTM
Released as 6.25.1. |
r? @brandur-stripe @rattrayalex-stripe
cc @stripe/api-libraries
Always encode arrays as integer-indexed hashes. This is the default behavior of
qs.stringify()
. Stripe's API can now accept integer-indexed hashes for all array parameters (includingexpand
) so there is no longer a need to special-case arrays of hashes parameters likesubscription_items
.I've also updated the encoding method to change square brackets back to their literals to make the strings easier to read. This is consistent with most of our other libraries.
Fixes #561.