fs: migrate more errors to internal/errors by jasnell · Pull Request #17667 · nodejs/node · GitHub

Next batch of fs type checking migrations to use internal/errors

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

Affected core subsystem(s)

fs

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

I suspect this principle has been decided previously, but maybe not?:

Is it desirable to have such specific codes? Why not just ERR_INVALID_ARG_VALUE?

it's been debated and I'm good with it either way.

I'd be in favor of erring on the side of more general and you can always move to a more specific code later if it turns out it's really needed. But yeah, if there's fierce opinions on both sides, then ¯\(ツ)/¯

ERR_OUT_OF_RANGE is reduced into ERR_VALUE_OUT_OF_RANGE in #17648. So here would be better to use ERR_VALUE_OUT_OF_RANGE.

+1

@jasnell ^

yep, was holding off a bit to see if/when #17648 was going to land.

oh.. right, and the latest updates remove the out of range errors anyway so this is no longer relevant ;-)

It would justify this new error a bit more if the error message includes a list of valid symlink types

ping @nodejs/tsc

Chose to go with @joyeecheung's suggestion of a more specific error message. PTAL

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

Thank you @joyeecheung :-) ...

@nodejs/tsc ... need one more TSC sign off please.

Looks like there was something wrong with linux one, new linux one CI: https://ci.nodejs.org/job/node-test-commit-linuxone/11371/

Should we also check for uid >= 0? Same question for the other checks in this commit

I do believe that uid and gid both can be negative in some edge cases.

s/number/integer/

+1

typo: symlinksync

General question about Number.isInteger checks: the check passes for values > 2^31-1 in JS land but not in C++ land. Should we guard for this?

LGTM with the nits already noted addresses

the check passes for values > 2^31-1 in JS land but not in C++ land

Replacing IsInt32() and IsUint32() checks with Number.isInteger() is wrong. Their JS equivalents are x === x | 0 and x === x >>> 0 respectively.

Another issue with Number.isInteger() is that users can monkey-patch it.

Updated to address @bnoordhuis' feedback.

@mcollina and @joyeecheung ... please take another look.

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

LGTM with the nit above adressed.

@jasnell ^

IsInt32 (both lines)

FChown is not changed in this PR and also gets Uint32Value(). Are you sure negative uid and gid are possible?

hmm... they are definitely theoretically possible but you're right. I'll keep these as uint32s

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

PR-URL: #17667 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Landed in 6100e12

Belated LGTM

@jasnell it looks like the new assertions in src/node_file.cc are causing some CitGM issues. Would you mind having a look? https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1165/

@apapirovski The CITGM issues are caused by fs.readFile and fs.exists/fs.existsSync raising assertion when the path is undefined. I have a fix coming up.

PR-URL: #17852 Refs: #17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell added fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 14, 2017

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 2b87e7d to 0e284e1 Compare December 14, 2017 01:01

jasnell added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Dec 14, 2017

jasnell requested review from addaleax and joyeecheung December 14, 2017 01:02

Trott reviewed Dec 14, 2017

View reviewed changes

starkwang reviewed Dec 14, 2017

View reviewed changes

jasnell mentioned this pull request Dec 14, 2017

Extract out fs validation functions #17682

Closed

2 tasks

joyeecheung reviewed Dec 15, 2017

View reviewed changes

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 0e284e1 to c57266d Compare December 18, 2017 18:29

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 0c2cf69 to a93e157 Compare December 18, 2017 19:30

jasnell requested a review from a team December 19, 2017 15:34

joyeecheung approved these changes Dec 19, 2017

View reviewed changes

targos reviewed Dec 21, 2017

View reviewed changes

mcollina approved these changes Dec 21, 2017

View reviewed changes

mcollina approved these changes Dec 22, 2017

View reviewed changes

targos reviewed Dec 22, 2017

View reviewed changes

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 469c61a to 4e75058 Compare December 22, 2017 17:18

fs: move type checking to js

a2d9bd7

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 4e75058 to a2d9bd7 Compare December 22, 2017 20:48

jasnell added a commit that referenced this pull request Dec 22, 2017

fs: move type checking to js

6100e12

PR-URL: #17667 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell closed this Dec 22, 2017

targos reviewed Dec 22, 2017

View reviewed changes

joyeecheung mentioned this pull request Dec 24, 2017

fs: fix fs.readFile, fs.exists{Sync} regression caused by assertions #17852

Closed

4 tasks

apapirovski pushed a commit that referenced this pull request Dec 27, 2017

fs: validate path in fs.readFile

9ec700b

PR-URL: #17852 Refs: #17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

apapirovski pushed a commit that referenced this pull request Dec 27, 2017

fs: validate path in fs.exists{Sync}

71396a2

PR-URL: #17852 Refs: #17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

               //  fs.writeSync(fd, string[, position[, encoding]]);
               fs.writeSync = function(fd, buffer, offset, length, position) {
+                if (!Number.isInteger(fd))
+                  throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number');

               [`ERR_INVALID_ARG_TYPE`]: #ERR_INVALID_ARG_TYPE
               [`EventEmitter`]: events.html#events_class_eventemitter
+              [`fs.symlink()`]: fs.html#fs_symlink
+              [`fs.symlinkSync()`]: fs.html#fs_symlinksymc

+                  throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path',
+                                             ['string', 'Buffer', 'URL']);
+                }
+                if (!Number.isInteger(uid))

fs: migrate more errors to internal/errors by jasnell · Pull Request #17667 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: migrate more errors to internal/errors - #17667

fs: migrate more errors to internal/errors#17667jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:fs-migrate-internal-errors-2jasnell/node:fs-migrate-internal-errors-2Copy head branch name to clipboard

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Dec 14, 2017

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!

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!

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!

jasnell commented Dec 18, 2017

Uh oh!

jasnell commented Dec 18, 2017

Uh oh!

joyeecheung commented Dec 19, 2017

Uh oh!

jasnell commented Dec 21, 2017

Uh oh!

joyeecheung commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

targos commented Dec 21, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Dec 21, 2017

Uh oh!

jasnell commented Dec 21, 2017

Uh oh!

jasnell commented Dec 21, 2017

Uh oh!

mcollina left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 22, 2017

Uh oh!

jasnell commented Dec 22, 2017

Uh oh!

fs: migrate more errors to internal/errors#17667
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:fs-migrate-internal-errors-2

jasnell commented Dec 14, 2017 •
edited
Loading

joyeecheung commented Dec 21, 2017 •
edited
Loading