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.

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

https-agent: fix issue where creating connection modifies arguments #31151

Closed
wants to merge 1 commit into from

Conversation

@vighnesh153
Copy link
Contributor

vighnesh153 commented Jan 2, 2020

Previously, when passing options object to the agent.createConnection
method, the same options object got modified within the method. Now,
any modification will happen on only a copy of the object.

Fixes: #31119

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@antsmartian

This comment has been minimized.

Copy link
Contributor

antsmartian commented Jan 2, 2020

@vighnesh153 Welcome and thanks for your first PR! There are few slight issues in the PR:

Lint has been failed (run make lint locally to verify). These are the errors:

/home/travis/build/nodejs/node/test/parallel/test-https-agent-create-connection.js
  152:7  error  'assert.equal' is restricted from being used. Use `assert.strictEqual()` rather than `assert.equal()`  no-restricted-properties
  153:7  error  'assert.equal' is restricted from being used. Use `assert.strictEqual()` rather than `assert.equal()`  no-restricted-properties
✖ 2 problems (2 errors, 0 warnings)

Also, we need to follow commit guidelines mentioned over here: https://goo.gl/p2fr5Q. I have copied the failure for you (from CI:https://travis-ci.com/nodejs/node/jobs/271698472):

  ✖  85fcbdc557f73ea3f16882d1f8ee849c8ea1c247
     ✔  5:7      Valid fixes URL.                          fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✖  0:0      Invalid subsystem: "https-agent"          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length
@vighnesh153

This comment has been minimized.

Copy link
Contributor Author

vighnesh153 commented Jan 2, 2020

@vighnesh153 Welcome and thanks for your first PR! There are few slight issues in the PR:

Lint has been failed (run make lint locally to verify). These are the errors:

/home/travis/build/nodejs/node/test/parallel/test-https-agent-create-connection.js
  152:7  error  'assert.equal' is restricted from being used. Use `assert.strictEqual()` rather than `assert.equal()`  no-restricted-properties
  153:7  error  'assert.equal' is restricted from being used. Use `assert.strictEqual()` rather than `assert.equal()`  no-restricted-properties
✖ 2 problems (2 errors, 0 warnings)

Also, we need to follow commit guidelines mentioned over here: https://goo.gl/p2fr5Q. I have copied the failure for you (from CI:https://travis-ci.com/nodejs/node/jobs/271698472):

  ✖  85fcbdc557f73ea3f16882d1f8ee849c8ea1c247
     ✔  5:7      Valid fixes URL.                          fixes-url
     ✔  0:0      blank line after title                    line-after-title
     ✔  0:0      line-lengths are valid                    line-length
     ✖  0:0      Invalid subsystem: "https-agent"          subsystem
     ✔  0:0      Title is formatted correctly.             title-format
     ⚠  0:50     Title should be <= 50 columns.            title-length

Got it. I will work on it right away. So, should I open a new PR as there are multiple commits for a single change or should I continue with this?

@antsmartian

This comment has been minimized.

Copy link
Contributor

antsmartian commented Jan 2, 2020

No need to open a new PR. It would be great if you can squash all your commits.

@vighnesh153 vighnesh153 force-pushed the open-source-101:issue-fix/31119 branch from 0c2db72 to 2093b3b Jan 2, 2020
@vighnesh153

This comment has been minimized.

Copy link
Contributor Author

vighnesh153 commented Jan 2, 2020

No need to open a new PR. It would be great if you can squash all your commits.

Alright. Thanks man. Fixing it now.

@vighnesh153 vighnesh153 force-pushed the open-source-101:issue-fix/31119 branch from 2093b3b to 42d88e9 Jan 2, 2020
@vighnesh153

This comment has been minimized.

Copy link
Contributor Author

vighnesh153 commented Jan 2, 2020

@antsmartian Please have a look and please give me feedback about anything that needs to be changed.

lib/https.js Outdated Show resolved Hide resolved
@vighnesh153 vighnesh153 force-pushed the open-source-101:issue-fix/31119 branch from 42d88e9 to 37ffc83 Jan 2, 2020
@vighnesh153 vighnesh153 requested a review from mscdex Jan 2, 2020
@ronag
ronag approved these changes Jan 2, 2020
test/parallel/test-https-agent-create-connection.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@jasnell
jasnell approved these changes Jan 2, 2020
@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Jan 2, 2020

One other nit: in general, commit messages should be structured such that they start with a verb to make it clear what the commit is doing.

So instead of: https: Agent.createConnection mutates options object
Consider something like: https: prevent options object from being mutated

lib/https.js Outdated Show resolved Hide resolved
Previously, when passing options object to the agent.createConnection
method, the same options object got modified within the method. Now,
any modification will happen on only a copy of the object.

Fixes: #31119
@vighnesh153 vighnesh153 force-pushed the open-source-101:issue-fix/31119 branch from 37ffc83 to 9296cdd Jan 3, 2020
@vighnesh153 vighnesh153 requested a review from lpinca Jan 3, 2020
@lpinca
lpinca approved these changes Jan 3, 2020
@Trott
Trott approved these changes Jan 4, 2020
@nodejs-github-bot

This comment has been minimized.

Trott added a commit that referenced this pull request Jan 4, 2020
Previously, when passing options object to the agent.createConnection
method, the same options object got modified within the method. Now,
any modification will happen on only a copy of the object.

Fixes: #31119

PR-URL: #31151
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 4, 2020

Landed in fa94698.

Thanks for the contribution! 🎉

@Trott Trott closed this Jan 4, 2020
@vighnesh153 vighnesh153 deleted the open-source-101:issue-fix/31119 branch Jan 4, 2020
targos added a commit that referenced this pull request Jan 6, 2020
Previously, when passing options object to the agent.createConnection
method, the same options object got modified within the method. Now,
any modification will happen on only a copy of the object.

Fixes: #31119

PR-URL: #31151
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
Previously, when passing options object to the agent.createConnection
method, the same options object got modified within the method. Now,
any modification will happen on only a copy of the object.

Fixes: #31119

PR-URL: #31151
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.