Fix multipart bodies vanishing #98
Conversation
When re-authing during a multipart, the bodies of the multipart appear to be empty. This keeps a copy of the body and reconstitutes the body on the second call.
|
@afirstenberg, could you lint with closure linter and please add a test? |
| bodies.forEach( function(body,i){ | ||
| opts.multipart[i].body = body; | ||
| }); | ||
|
|
||
| that.request(opts, opt_callback, true); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
I investigated this a few weeks ago and the bug is actually in the "request" library. My workaround was to refresh my access token before each request, rather than relying on the auto-refresh. |
|
@rakyll - I've linted, but coming up with a test is more difficult. I went to base it on the existing test for replaying a request after reauthing, but that test is marked as TODO. And I assume I came up with the same difficulty - having to wait for the timeout period as an actual part of the test |
|
I don't remember exactly why the test for refreshing is left as a TODO, I will try to implement it today. On the other hand, why don't we upstream the fix to requests lib instead of hacking around it? |
|
Taking a look at |
|
Makes sense. It certainly reduces the risk of what Request might do with the options in a later version. My biggest concern when I proposed the change I did was how deep a copy was going to be necessary and concerns about doing too-deep a copy and making an unnecessary copy of what might be a very large buffer. Since I didn't have a lot of experience with node, I was being conservative on what to preserve from call to call. |
|
The memory efficient solution would be replaying request's Request object rather than creating a new one. I dont know if there are public interfaces to change headers, etc. If there are, I should fix transporter to expose the Request instance and replay it. Sounds very hacky though. |
|
Any thoughts on switching from the request library, which is bloated and buggy, to something like needle (https://www.npmjs.org/package/needle)? |
|
Should be fixed in |
If a command is created withMedia() and that command fails because the authentication has expired, the retry (after the auth has been fetched) does not seem to have the bodies associated with the multiparts. This patch is a bit of a kludge, since it simply makes a copy of the multipart bodies and sets them back as part of the request after the auth succeeds.