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

errors: validate input arguments #19924

Closed
wants to merge 2 commits into from
Closed

Conversation

BridgeAR
Copy link
Member

This makes sure the input arguments get validated so implementation
errors will be caught early. It also improves a couple of error
messages by providing more detailed information and fixes errors
detected by the new functionality. Besides that a error type got
simplified and tests got refactored.

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

@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 11, 2018
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 11, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Apr 11, 2018

@AyushG3112
Copy link
Contributor

Does this remove the need for #19908? If yes, then it can be closed safely.

@BridgeAR
Copy link
Member Author

@AyushG3112 I kept one of the two checks that are changed in #19908. Your PR is very likely going to land first, so you can just keep that open.

@@ -960,7 +970,6 @@ E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error);

function invalidArgType(name, expected, actual) {
internalAssert(typeof name === 'string');
internalAssert(arguments.length === 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we keep this check? Without this, the error can be implemented without passing in actual too, so no matter what the user passes as the actual argument, the error message will say undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved the check so it will be executed for all errors. That is actually what this PR is all about.

This makes sure the input arguments get validated so implementation
errors will be caught early. It also improves a couple of error
messages by providing more detailed information and fixes errors
detected by the new functionality. Besides that a error type got
simplified and tests got refactored.
@BridgeAR
Copy link
Member Author

Rebased due to conflicts.

New CI https://ci.nodejs.org/job/node-test-pull-request/14220/

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM with some nits

@@ -920,7 +920,7 @@ Buffer.prototype.write = function write(string, offset, length, encoding) {
length = remaining;

if (string.length > 0 && (length < 0 || offset < 0))
throw new ERR_BUFFER_OUT_OF_BOUNDS('length', true);
throw new ERR_BUFFER_OUT_OF_BOUNDS();
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this and the change to fs? I didn't expect to see those here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The length was never used in this case. It was a weird API that I fixed.

Originally the implementation was like:

function a (a, b) {
  if (b === true) {
    return 'foo'
  }
  return `${a} foo`
}

Now it is:

function a(a = undefined) {
  if (a === undefined) {
    return 'foo'
  }
  return `${a} foo`
}

@@ -924,7 +934,7 @@ E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET',
Error);
E('ERR_UNESCAPED_CHARACTERS', '%s contains unescaped characters', TypeError);
E('ERR_UNHANDLED_ERROR',
(err) => {
(err = undefined) => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is truly needed. Or is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly that is necessary. Default arguments do not count towards function.length as in:

function a(a) {}
function b(a = undefined) {}
assert.strictEqual(a.length, 1);
assert.strictEqual(b.length, 0);

Otherwise it is difficult to validate a couple of functions, but I think it is worth it because it makes sure we make less errors while implementing the errors.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment before the function then? Otherwise it won't be clear to the reader.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

if (isWriting) {
return 'Attempt to write outside buffer bounds';
} else {
function bufferOutOfBounds(name = undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the default argument is truly needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 12, 2018
@BridgeAR
Copy link
Member Author

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

still LGTM

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 13, 2018
This makes sure the input arguments get validated so implementation
errors will be caught early. It also improves a couple of error
messages by providing more detailed information and fixes errors
detected by the new functionality. Besides that a error type got
simplified and tests got refactored.

PR-URL: nodejs#19924
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member Author

Landed in dca7fb2

@BridgeAR BridgeAR closed this Apr 13, 2018
jasnell pushed a commit that referenced this pull request Apr 16, 2018
This makes sure the input arguments get validated so implementation
errors will be caught early. It also improves a couple of error
messages by providing more detailed information and fixes errors
detected by the new functionality. Besides that a error type got
simplified and tests got refactored.

PR-URL: #19924
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR deleted the improve-errors branch April 1, 2019 23:40
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. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants