Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes a bug where args were being overridden by previous requests when using requestOptionsTransport #450

Merged
merged 1 commit into from
Jan 12, 2018
Merged

Fixes a bug where args were being overridden by previous requests when using requestOptionsTransport #450

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/clients/transports/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var HttpsProxyAgent = require('https-proxy-agent');
var has = require('lodash').has;
var partial = require('lodash').partial;
var defaults = require('lodash').defaults;
var cloneDeep = require('lodash').cloneDeep;
var request = require('request');

var handleRequestTranportRes = function handleRequestTranportRes(cb, err, response, body) {
Expand Down Expand Up @@ -64,7 +65,8 @@ var proxiedRequestTransport = function proxiedRequestTransport(proxyURL) {

var requestOptionsTransport = function requestOptionsTransport(options) {
return function _requestOptionsTransport(args, cb) {
var requestArgs = defaults(options, getRequestTransportArgs(args));
var instanceOptions = cloneDeep(options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this line be outside of the inner _requestOptionsTransport function? or in other words, what's the point in cloning the options each time a request is fired, as opposed to just cloning it once per transport "constructed"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the problem is that the way defaults works is by putting instanceOptions as the base of requestArgs and then layering the args on top of it. This means that after the args are layered on top, instanceOptions has now been overridden with the args specified by the caller.

This is problematic for the next request because then instanceOptions now also includes the args from the previous request. By ensuring that the original options object doesn't mutate and cloning it for each request, we avoid this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah! i was assuming defaults() is a pure function. silly lodash developers. 👍

var requestArgs = defaults(instanceOptions, getRequestTransportArgs(args));
request.post(requestArgs, partial(handleRequestTranportRes, cb));
};
};
Expand Down