tls: move parseCertString to internal by XadillaX · Pull Request #14218 · nodejs/node

XadillaX · 2017-07-13T11:11:55Z

tls.parseCertString() exposed by accident. Now move this function to
internal/tls and mark the original one as deprecated.

Refs: #14193
Refs: af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

tls

`tls.parseCertString()` exposed by accident. Now move this function to `internal/tls` and mark the original one as deprecated. Refs: nodejs#14193 Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188

XadillaX · 2017-07-13T11:14:27Z

I think we should add a document to mark this function. Refs #14196

refack · 2017-07-13T11:17:37Z

lib/tls.js

-  }
-  return out;
-};
+exports.parseCertString = internalUtil.deprecate(


~~IMHO there's no need to move the function, just replace the export with this deprecation-wrapped-export.~~

Other file depends on this function.

node/lib/_tls_common.js

Line 176 in 1a031f2

if (c.subject) c.subject = tls.parseCertString(c.subject);

refack · 2017-07-13T11:23:47Z

doc/api/deprecations.md

+
+Type: Runtime
+
+`tls.parseCertString()` was move to `internal/tls.js`.


`tls.parseCertString()` is a trivial parsing helper that was made public by mistake. Can be replaced with ```js const querystring = require('querystring'); querystring.parse(str, '\n', '=')`; ```

IMHO I don't think they are equivalent since parseCertString() does no encoding or decoding.

eg.

> querystring.parse("%E5%A5%BD=1", "\n", "="); { '好': '1' } > tls.parseCertString("%E5%A5%BD=1"); { '%E5%A5%BD': '1' }

I would like us to have a suggested alternative available, so maybe:

`tls.parseCertString()` is a trivial parsing helper that was made public by mistake. This function can usually be replaced with ```js const querystring = require('querystring'); querystring.parse(str, '\n', '=')`; ``` *Note*: One difference is that `querystring.parse` does URLDecoding, e.g.: ```js > querystring.parse("%E5%A5%BD=1", "\n", "="); { '好': '1' } > tls.parseCertString("%E5%A5%BD=1"); { '%E5%A5%BD': '1' } ```

refack · 2017-07-13T11:52:23Z

doc/api/deprecations.md

+querystring.parse(str, '\n', '=')`;
+```
+
+*Note*: This function is not 100% same as `querystring.parse()`. One difference


*Note*: This function is not completely equivalent to `querystring.parse()`, one notable difference...

bnoordhuis

Can you split the commit in two? One that moves parseCertString() and another that moves the constants?

bnoordhuis · 2017-07-13T12:27:15Z

test/parallel/test-tls-parse-cert-string.js

+  const regexp = new RegExp('^\\(node:\\d+\\) [DEP0073] DeprecationWarning: ' +
+                            'tls\\.parseCertString is deprecated$');
+
+  // test for deprecate message


s/deprecate/deprecation/ and can you capitalize + punctuate the comment?

You can/should use common.expectWarning() to check for this without having to hijack stderr

bnoordhuis · 2017-07-13T12:29:28Z

lib/tls.js

+  Object.defineProperty(exports, key, {
+    get: () => { return internalTLS[key]; },
+    set: (c) => { internalTLS[key] = c; }
+  });


Defining accessors for otherwise simple data properties is kind of wasteful, just re-export the values from internal/tls.

@bnoordhuis, I saw a test that modified the value.

Refs: https://github.com/nodejs/node/blob/v8.1.4/test/parallel/test-tls-honorcipherorder.js#L97

If we don't tie these two variables as getter and setter, the value may be not equivalent.

mscdex · 2017-07-13T15:37:23Z

Should we be deprecating something before the next major version? Or is this not going into v8.x?

refack · 2017-07-13T16:10:57Z

Should we be deprecating something before the next major version? Or is this not going into v8.x?

It's in limbo since it's undocumented in v8 so in theory could be just doc-deprecated.
/cc @nodejs/release

so @XadillaX Could you split into 3 commits: docs + function move + constant move

XadillaX · 2017-07-14T02:26:13Z

@refack Did you mean:

mark the function as deprecated with documented-only
move the function and modify the document with runtime
move the constant

refack · 2017-07-14T02:32:01Z

@refack Did you mean:

mark the function as deprecated with documented-only
move the function and modify the document with runtime
move the constant

Yes, exactly (so that we could land just (1) in v8.x)

jasnell · 2017-07-14T21:11:53Z

doc/api/deprecations.md


 *Note*: change was made while `async_hooks` was an experimental API.

+<a id="DEP0073"></a>


The deprecation code should be DEP00XX until the PR lands.

Whose job is it to replace?

@jasnell Will be replaced when landing by the lander?

My new PR for v8.x-staging made DEP0073 to DEP00XX.

jasnell

Test needs a bit of tweaking... otherwise it's looking good.

XadillaX · 2017-07-15T05:59:56Z

@refack Did you mean:

mark the function as deprecated with documented-only

move the function and modify the document with runtime

move the constant

Yes, exactly (so that we could land just (1) in v8.x)

I think the third point should be created the PR after point 2 is landed.

tls: move parseCertString to internal

1a031f2

`tls.parseCertString()` exposed by accident. Now move this function to `internal/tls` and mark the original one as deprecated. Refs: nodejs#14193 Refs: nodejs@af80e7b#diff-cc32376ce1eaf679ec2298cd483f15c7R188

nodejs-github-bot added build Issues and PRs related to build files or the CI. tls Issues and PRs related to the tls subsystem. labels Jul 13, 2017

XadillaX mentioned this pull request Jul 13, 2017

Missing documentation for tls.parseCertString #14193

Closed

XadillaX mentioned this pull request Jul 13, 2017

doc: add tls.parseCertString() #14196

Closed

2 tasks

refack reviewed Jul 13, 2017

View reviewed changes

update after reviewing

6c18793

refack reviewed Jul 13, 2017

View reviewed changes

bnoordhuis requested changes Jul 13, 2017

View reviewed changes

jasnell reviewed Jul 14, 2017

View reviewed changes

jasnell requested changes Jul 14, 2017

View reviewed changes

This was referenced Jul 15, 2017

doc, tls: mark parseCertString() as deprecated #14245

Closed

tls: deprecate parseCertString & move to internal #14249

Closed

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 18, 2017

XadillaX closed this Jul 20, 2017

refack mentioned this pull request Aug 7, 2017

meta: handling of undocumented endpoint #14679

Closed


		Type: Runtime

		`tls.parseCertString()` was move to `internal/tls.js`.


		Note: change was made while `async_hooks` was an experimental API.

		<a id="DEP0073"></a>

Uh oh!

Conversation

XadillaX commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

XadillaX commented Jul 13, 2017

Uh oh!

refack Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XadillaX Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XadillaX Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XadillaX Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 13, 2017

Uh oh!

XadillaX commented Jul 14, 2017

Uh oh!

refack commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

XadillaX Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

XadillaX commented Jul 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

XadillaX commented Jul 13, 2017 •

edited

Loading

refack Jul 13, 2017 •

edited

Loading

XadillaX Jul 13, 2017 •

edited

Loading

XadillaX Jul 13, 2017 •

edited

Loading

XadillaX Jul 13, 2017 •

edited

Loading

jasnell Jul 14, 2017 •

edited

Loading

mscdex commented Jul 13, 2017 •

edited

Loading

XadillaX Jul 15, 2017 •

edited

Loading