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

tls: migrate tls.js to use internal/errors.js #13994

Closed
wants to merge 2 commits into from

Conversation

@mhdawson
Copy link
Member

@mhdawson mhdawson commented Jun 29, 2017

Migrate tls.js to use internal/errors.js as per
#11273

The internal code used to set the 'code' as the client.authorizationError, or if no code existed the 'message' as the client.authorizationError. Since with the new approach there is always a 'code' we end up setting the less descriptive code. I guess that is consistent though with what was being done for existing errors with a code anyway.

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

lib/internal/errors.js Outdated
@@ -111,6 +111,8 @@ module.exports = exports = {
// Note: Please try to keep these in alphabetical order
E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
E('ERR_ASSERTION', '%s');
E('ERR_CERT_ALTNAME_INVALID',

This comment has been minimized.

@jasnell

jasnell Jun 30, 2017
Member

ERR_TLS_CERT_ALTNAME_INVALID?

lib/internal/errors.js Outdated
@@ -111,6 +111,8 @@ module.exports = exports = {
// Note: Please try to keep these in alphabetical order
E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
E('ERR_ASSERTION', '%s');
E('ERR_CERT_ALTNAME_INVALID',
'Hostname/IP does not match certificate\'s altnames: %s');

This comment has been minimized.

@jasnell

jasnell Jun 30, 2017
Member

Perhaps use a template string form to avoid having to escape?

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jun 30, 2017

Pushed commit to address comments.

lib/internal/errors.js Outdated
E('ERR_TLS_CERT_ALTNAME_INVALID',
(reason) => {
return `Hostname/IP does not match certificate's altnames: ${reason}`;
});

This comment has been minimized.

@BridgeAR

BridgeAR Jul 5, 2017
Member

You do not have to use a function here. Instead we can rely on util.format by replacing the function with:
'Hostname/IP does not match certificate's altnames: %s'.

This is done in all other places and it would be nice to keep it consistent.

This comment has been minimized.

@mhdawson

mhdawson Jul 5, 2017
Author Member

Thanks for the suggestion, updating now

This comment has been minimized.

@mhdawson

mhdawson Jul 5, 2017
Author Member

actually that is what I had originally. The suggestion to use a function was from James to avoid having to escape the '. Having said that I think I prefer avoiding the function for consistency so I'll change back.

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 5, 2017

@BridgeAR pushed commit to change back to address your comment.

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 5, 2017

@fhinkel any chance you can review this PR ?

@jasnell
Copy link
Member

@jasnell jasnell commented Jul 10, 2017

Needs a rebase...

Copy link
Member

@refack refack left a comment

Could you add an assertion of the structure of the Error object to parallel/test-tls-friendly-error-message.js?

Migrate tls.js to use internal/errors.js as per
#11273
@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 11, 2017

@refack I'm not following, I looked at the test and I don't see the relation to this PR. I see it already validates the following:

assert.strictEqual(err.code, 'UNABLE_TO_VERIFY_LEAF_SIGNATURE');
assert.strictEqual(err.message, 'unable to verify the first certificate');
@mhdawson mhdawson force-pushed the mhdawson:napi-errors-10 branch to b90e451 Jul 11, 2017
@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 11, 2017

Rebased and squashed.

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 11, 2017

@fhinkel just need one more CTC reviewer on this one.

@refack
Copy link
Member

@refack refack commented Jul 11, 2017

@refack I'm not following, I looked at the test and I don't see the relation to this PR. I see it already validates the following:

I meant you did not add an assertion of the generated error message for the new error code you introduced ERR_TLS_CERT_ALTNAME_INVALID.
Looking at parallel/test-tls-friendly-error-message.js it seems like a good place for that (but any other place is as just as good).
IMHO we should assert the generated message somewhere.

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 12, 2017

@refack I don't think we have been requiring validating the message. I had mentioned it as a discussion point when reviewing other PRs. I do agree validation at least once makes sense, but was not sure if we should require that in every test for the code.

I'd suggest this as our approach going forward

  1. If the error message is simply a string that will be output as is, we should be able to trust that the errors mechanism will do the right thing. No validation of message output needed in this case.
  2. If the error message does have parameters, then there should be validation of the string generation in test/parallel/test-internal-errors.js
  3. Other tests using the new error should not check the string and only use the code. This means that if we change the string only the test in 2) needs to be updated.

I've gone ahead and pushed a commit to do that for this PR.

If we agree on the above we should put that guidance into the issue tracking and our internal docs. Once we have agreement I'll do that. The alternative to 3) is that we also require that the message be validated every time the code is used. This would be most thorough.

@refack
Copy link
Member

@refack refack commented Jul 12, 2017

@mhdawson Agreed, I made the same argument almost verbatim in some other thread!

@refack
refack approved these changes Jul 12, 2017
@refack
Copy link
Member

@refack refack commented Jul 12, 2017

The only caveat with (3) is if the code explicitly modifies the message after Error construction. We should mention that as a no-no in reviewing guidelines.

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 12, 2017

@refack PR to add guidance to documentation #14207

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 13, 2017

@nodejs/ctc need one more reviewer as its SemVer major.

Copy link
Member

@joyeecheung joyeecheung left a comment

Changes LGTM but I can't seem to find the documentation for the old behavior?

Copy link
Member

@mcollina mcollina left a comment

LGTM

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 18, 2017

@joyeecheung not sure what you mean by "can't seem to find the documentation for the old behavior" ?

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 18, 2017

One more time for CI run as last one seems not to exist: https://ci.nodejs.org/job/node-test-pull-request/9218/

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 18, 2017

CI run good landing.

@mhdawson
Copy link
Member Author

@mhdawson mhdawson commented Jul 18, 2017

Landed as 3ccfeb4

@mhdawson mhdawson closed this Jul 18, 2017
mhdawson added a commit that referenced this pull request Jul 18, 2017
Migrate tls.js to use internal/errors.js as per
#11273

PR-URL: #13994
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@refack refack added this to Done in Error Codes Aug 20, 2017
@mhdawson mhdawson deleted the mhdawson:napi-errors-10 branch Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.