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
Closed

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

wants to merge 1 commit into from

Conversation

vighnesh153
Copy link
Contributor

@vighnesh153 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

@nodejs-github-bot nodejs-github-bot added the https Issues or PRs related to the https subsystem. label Jan 2, 2020
@antsmartian
Copy link
Contributor

@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
Copy link
Contributor Author

@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
Copy link
Contributor

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

@vighnesh153
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 2, 2020
@nodejs-github-bot
Copy link
Collaborator

@mscdex
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
@nodejs-github-bot
Copy link
Collaborator

Trott pushed 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
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 issue-fix/31119 branch January 4, 2020 05:17
targos pushed 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 pushed 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>
BethGriggs pushed a commit that referenced this pull request Feb 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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

https: Agent.createConnection mutates options object
10 participants