fs: create consistent behavior for fs module across platforms #17927

timotew · 2017-12-31T23:08:01Z

In Windows, if a file name ends with forward slash /, fs interprets it as regular file (like there is no slash at all) and does not throw any errors. In Linux it throws error but not in Windows.

Fixes: #17801

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

devsnek · 2018-01-01T00:27:19Z

lib/fs.js

space after comment please

@devsnek Space added thanks.

I think what @devsnek meant was a space after the comment indicator (//) so:

// doing justice...

There are some lint errors you're going to want to correct (mostly around line lengths). These should have been flagged if you ran make test or vcbuild test. To just run the JS linting (since running the full tests can be time-consuming), you can use make lint-js or vcbuild lint-js.

Lastly, welcome @timotew and thanks for the pull request!

@devsnek sorry I misunderstood, @Trott Thanks a lot for having me and time for this.

starkwang · 2018-01-02T02:10:16Z

lib/fs.js

This empty line is redundant :-)

starkwang · 2018-01-02T02:22:21Z

lib/fs.js

Using errors.TypeError might be more appropriate.

The second parameter (here is path) of ERR_INVALID_FILE_URL_PATH is wrong. It should be a string contains an error description about the invalid path.
Here is some example:

node/lib/internal/url.js

Lines 1362 to 1363 in abe14ab

return new errors.TypeError('ERR_INVALID_FILE_URL_PATH',

'must not include encoded / characters');

node/lib/internal/url.js

Lines 1346 to 1347 in abe14ab

return new errors.TypeError('ERR_INVALID_FILE_URL_PATH',

'must be absolute');

I don't think errors.TypeError is appropriate here. A TypeError would be if we were expecting a string but received something else. In this case, we are expecting a string and receiving a string, but it has a value that is not valid. (By all means, please correct me if I'm wrong or misunderstanding something!)

The second point (about the second parameter) is 👍.

Now the similar ERR_INVALID_FILE_URL_PATH errors in internal/url.js are using TypeError (some examples showed above).

I do agree with your point. For semantics, using Error is better.

Thanks for the correction it's effected. I am still not clear about the error type @Trott .
From what @starkwang showed above, Please I might also be wrong but I think TypeError is appropriate for the case.
errors in internal/url.js are expecting characters that should not include some special characters, The type there might be referring to character type not data type.

starkwang · 2018-01-02T08:49:55Z

lib/fs.js

This string should be reduced into 'must not end with / character'

The first two words 'file path' is redundant because these words were already contained in template:

node/lib/internal/errors.js

Line 401 in a82b1b7

E('ERR_INVALID_FILE_URL_PATH', 'File URL path %s');

starkwang · 2018-01-02T08:50:38Z

test/parallel/test-fs-whatwg-url.js

Umm... If I'm not mistaken, the message option will not match the actual error message.

It should be 'File URL path must not end with / character'.

By the way, you can use ./configure && make -j4 test on Unix/MacOS or vcbuild test on Windows to run the tests on your computer (For more guides, you can refer to here). PR should always pass all the tests.

@starkwang Thanks a lot.

cjihrig · 2018-01-02T17:24:43Z

doc/api/fs.md

Please remove the leading -

cjihrig · 2018-01-02T17:28:35Z

lib/fs.js

I think the parens around the typeof check are unnecessary.

cjihrig · 2018-01-02T17:29:33Z

lib/fs.js

There is no need for a nested if.

cjihrig · 2018-01-02T17:30:15Z

lib/fs.js

It might be worth testing if slice() is faster than charAt().

@cjihrig charAt() is faster from my test, Thanks.

starkwang · 2018-01-03T02:48:38Z

@nodejs/fs

Trott · 2018-01-03T03:42:50Z

Might be worth a look from @nodejs/url too, especially @TimothyGu?

cjihrig · 2018-01-03T14:07:22Z

lib/fs.js

path.charAt() on this line should be aligned with isWindows on the previous line.

cjihrig · 2018-01-03T14:08:04Z

lib/fs.js

I'm not sure that "doing justice" makes a lot of sense here.

joyeecheung · 2018-01-03T15:35:29Z

If #17801 is about inconsistent behavior on different platforms, this still does not resolves that since different errors are thrown in different platforms. Also this just fixes readFile and writeFile ad-hoc, a better approach would be allowing getPathFromURL to take an option to check the trailing slash and specify that in the fs APIs that are supposed to operate on files.

joyeecheung

Making it explicit.

timotew · 2018-01-04T22:11:40Z

@joyeecheung Hi there! hope you have time to review these change.

joyeecheung · 2018-01-05T16:23:08Z

test/parallel/test-fs-whatwg-url.js

Can you add tests for other APIs with the validation changed as well? Also it would be more robust if we test paths like file:///tmp/test/ on non-windows systems because file:///c:/tmp/test/ is obviously invalid there for a different reason(drives).

You can test different type of URLs on different systems by reusing the tested URL object like this:

const urlWithSlash = common.isWindows ? new URL('file:///c:/tmp/test/') : new URL('file:///tmp/test/'); fs.readFile(urlWithSlash, common.expectsError(...));

joyeecheung · 2018-01-05T16:33:51Z

lib/internal/url.js

The typeof path.href === 'string' should be unnecessary because a valid WHATWG URL should always have a string as href

joyeecheung · 2018-01-05T16:34:54Z

lib/internal/url.js

I think it's more accurate to name this assertFilename or something similar.

joyeecheung · 2018-01-05T16:35:45Z

lib/internal/url.js

The message Illegal operation on a directory is actually more accurate here.

joyeecheung · 2018-01-05T16:50:52Z

Nice work, although I think there are still some APIs that need to be updated:

fs.appendFile / fs.appendFileSync
fs.createReadStream
fs.createWriteStream
fs.truncate/fs.truncateSync
fs.writeFile/fs.writeFileSync

Can you update those APIs as well?

timotew · 2018-01-05T19:14:55Z

ping @joyeecheung .

joyeecheung · 2018-01-09T09:51:57Z

@timotew Can you add tests for the changed APIs? Thanks!

timotew · 2018-01-09T14:16:55Z

@joyeecheung please review these test cases. Thanks!

joyeecheung · 2018-01-09T17:31:02Z

test/parallel/test-fs-whatwg-url.js

Does the linter pass on this? I think it should complain on comments without a space after //?

joyeecheung · 2018-01-09T17:33:07Z

test/parallel/test-fs-whatwg-url.js

The second argument of fs.createReadStream is an option object or an encoding, I am pretty sure this would result in an error. We can leave it as fs.createReadStream(urlWithSlash) here. The same goes to fs.createWriteStream as well

joyeecheung · 2018-01-09T17:35:59Z

test/parallel/test-fs-whatwg-url.js

You can test different type of URLs on different systems by reusing the tested URL object like this:

const urlWithSlash = common.isWindows ? new URL('file:///c:/tmp/test/') : new URL('file:///tmp/test/'); fs.readFile(urlWithSlash, common.expectsError(...));

timotew · 2018-01-09T20:01:32Z

@joyeecheung Please let me know if you notice any outstanding API.

joyeecheung · 2018-01-10T14:48:30Z

Just realized this PR only normalizes the behavior for URL objects, but it does not validate the string paths. One way to do this is to update every path validations now, or wait until #17682 lands then update the validatePath function there to reduce code duplication.

joyeecheung · 2018-01-10T14:52:34Z

lib/fs.js

We actually support fs.watchFile and fs.unwatchFile on directories. There are even tests in the async-hook test suites.

joyeecheung · 2018-01-10T14:53:29Z

lib/fs.js

fs.access is supported on directories.

Ah, did you mean to change fs.append?

joyeecheung · 2018-01-10T14:54:08Z

lib/fs.js

fs.stat is supported on directories.

joyeecheung · 2018-01-10T14:54:26Z

lib/fs.js

This supports directories as well.

starkwang

Thanks for your patience on this PR!
Just some small nits.

starkwang · 2018-01-11T07:34:41Z

test/parallel/test-fs-whatwg-url.js

This message seems to contain some syntax errors. Maybe 'File URL path cannot be a directory' ?

timotew · 2018-01-11T12:04:57Z

I try to pull #17682 and this happened! I guess my branch was out of date.
Here is the last commit 3f11d7c

jasnell · 2018-01-11T16:10:09Z

Just for future reference, we typically use git rebase rather than git pull or git merge in order to prevent merge commits. We can get this PR cleaned up no problem :-)

joyeecheung · 2018-01-11T16:52:14Z

@timotew You can rebase your branch by doing something like this:

# upstream is the remote that points to nodejs/node
git fetch upstream master
git checkout your-pr-branch
git rebase upstream/master

Looking at the commit history of this branch I think you have mixed commits from the upstream with your own commits. One way to get those back:

git checkout your-pr-branch
git checkout -b backup-branch  # back up where we are right now
git fetch upstream master
git checkout -b branch-for-cleanup  # create a new branch to clean things up
git reset --hard upstream/master  # bring the cleanup branch up-to-date with upstream
git log --reverse --pretty=format:%H --author="Your User Name" your-pr-branch | xargs -n 1 git cherry-pick
git checkout your-pr-branch
git reset --hard branch-for-cleanup # reset your pr branch to the same state as the cleanup branch

Where Your User Name is what git config --get user.name returns. This filters out all your commits and cherry-picks them onto the current master.

If you have trouble doing the above, let me know and I can help you restore the commits back (I can push to this PR branch since you allowed edits from maintainers in this PR).

timotew · 2018-01-11T22:00:54Z

@joyeecheung Thanks a lot!

joyeecheung · 2018-01-22T21:25:37Z

test/parallel/test-fs-path-err.js

You can use new Uint8Array(Buffer.from('....')) here to make it more readable.

joyeecheung · 2018-01-22T21:28:42Z

doc/api/errors.md

There is no need to differentiate between strings and buffers here, it's just about the path ending with /.
Also we should just make the error code exclusively about "it is a directory" otherwise IMO we should not invent a new error code instead of using ERR_INVALID_ARG_VALUE. Maybe change this to ERR_PATH_IS_DIRECTORY (Path %s cannot end with '/' in operations invalid on directories)?

joyeecheung · 2018-01-22T21:45:43Z

lib/fs.js

You don't need to pass a callback here or use handleError, this kind of errors should be synchronous, just like the type-checking errors (which reminds me the pattern handleError((path = getPathFromURL(path)), callback)) also look weird...why aren't they synchronous? @jasnell ).

This can just be function validatePath(path, propName = 'path', assertFilename = false), to hit the assertions you can just go with validatePath(path, 'path', true) since it does not hurt to explicitly pass in the name of the argument ('path'), then all the argument-overloading code below would be unnecessary.

joyeecheung · 2018-01-22T22:05:36Z

lib/fs.js

Note that this would be displayed as Path 102,105,108,101,58,47,47,47,116,109,112,47,116,101,115,116,47 ended with an invalid suffix character, we can either

Do not display the content in the path if it is a Uint8Array

Decode it as UTF8 and display it

Personally I prefer 1 even though we currently assume the buffer is encoded in UTF8 in the C++ layer. You can tweak the formatter in internal/errors.js and use a custom function instead of a string passed to util.format to do this.

timotew · 2018-01-23T22:14:29Z

@joyeecheung I think because fs.readFile is async it's throwing an error instead of passing it to the callback. How can can this be handled?

timotew · 2018-01-29T08:43:19Z

ping @joyeecheung

Fishrock123 · 2018-02-02T19:39:00Z

Is this not semver-major?

joyeecheung · 2018-02-03T11:03:41Z

@timotew Personally I believe these errors should be thrown synchronously like the type check errors, since they are not caused by any actual system calls and indicate that there is probably a bug in the user's code instead of the user's environment. The user either need to fix the code to trim the trailing /, or rethrow the error - that's not the error callbacks are for, IMO, they are for proper error handling like cleanup code that try to recover the state according to the syscall , not a known obvious bug in the argument, hence the errors should be thrown asap so the user can handle them asap. See #18308. But before that lands, I don't have a better opinion about this...@nodejs/tsc ?

joyeecheung · 2018-02-08T20:25:32Z

@timotew #18308 has landed so I think we are good with throwing those errors in the validation helpers. Can you rebase against master? Thanks!

joyeecheung · 2018-02-08T20:28:41Z

BTW, since there are plenty of commits in this PR, you might want to squash them on your PR branch before rebasing against master so there will be less conflicts to resolve.

BridgeAR · 2018-02-09T02:55:30Z

Ping @timotew

timotew · 2018-02-09T05:09:51Z

@joyeecheung , @BridgeAR I'am on it, Thanks.

timotew · 2018-02-10T18:38:54Z

ping @joyeecheung , @BridgeAR

BridgeAR · 2018-02-10T18:46:45Z

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

starkwang

It's quite near. Just some nits. 😉

starkwang · 2018-02-12T12:58:56Z

lib/fs.js

This line is an unnecessary change. Could you be so kind to revert it?

starkwang · 2018-02-12T13:10:23Z

lib/fs.js

There also some unnecessary blank line changes above. Could you revert it?

joyeecheung · 2018-02-14T06:02:34Z

lib/internal/errors.js

I am not sure if this is necessary, this would interfere with other formatters that might want their first arguments formatted differently. Also why are we coercing Uint8Arrays to empty strings? The message Path cannot end with / in operations invalid on directories looks rather confusing, whereas util.inspect would print the Uint8Array properly if we just use %s in the formatter.

I did this becuase you suggested it.

if (isUint8Array(path) && path[path.length - 1] === 47) { return handle(new errors.Error('ERR_INVALID_PATH_FORMAT', path));

Note that this would be displayed as Path 102,105,108,101,58,47,47,47,116,109,112,47,116,101,115,116,47 ended with an invalid suffix character, we can either
1.Do not display the content in the path if it is a Uint8Array
2. Decode it as UTF8 and display it
Personally I prefer 1 even though we currently assume the buffer is encoded in UTF8 in the C++ layer.

@timotew Sorry, totally forgot about this, I think there is a third option: display the content as util.inspect displays it...still, come to think of it, Path Uint8Array [1, 2, 3...] cannot end with / in operations invalid on directories does look confusing as well. So the current message looks fine to me, but we should only change the formatter for this error instead of the message function.

joyeecheung

I think there are some APIs missing the error after the rebase? Does the test pass locally?

joyeecheung · 2018-02-16T06:31:37Z

lib/internal/errors.js

Can you use a custom function here for not displaying Uint8Arrays?

Does the test pass locally?

Yes. it does.

joyeecheung

Eh, wrong button...did not intend to approve, this still needs some work..but very close!

BridgeAR · 2018-02-16T23:05:11Z

lib/fs.js

That should actually be the wrong spot to test for this. It should be tested in WriteStream (the same applies to ReadStream).

BridgeAR · 2018-02-16T23:08:46Z

lib/fs.js

Please write things like these as in:

const var1 = 123; const var2 = 321;

BridgeAR · 2018-02-16T23:19:49Z

lib/fs.js

Checking for an UInt8Array is not cheap. Therefore it would be good to rewrite this to:

if (typeof path === 'string') { err = nullCheck(path, propName, false); if (assertFilename && err === undefined && path[path.length - 1] === '/') err = new errors.Errors(...); } else if (isUint8Array(path)) { err = nullCheck(path, propName, false); if (assertFilename && err === undefined && path[path.length - 1] === 47) err = new errors.Error(...); } else { err = new errors.TypeError(...); }

BridgeAR · 2018-02-16T23:21:27Z

lib/internal/errors.js

Since this error will only be thrown in case it is either a string or Uint8Array it would be cheaper to test for the string case.

BridgeAR · 2018-02-16T23:23:14Z

test/parallel/test-fs-path-err.js

As above: please write this as:

const var1 = 123; const var2 = 321;

BridgeAR · 2018-02-16T23:24:40Z

test/parallel/test-fs-readfile-error.js

As question: what was the reason that this was removed?

This was to be controlled by rethrow() but because of Error.captureStackTrace(err, validatePath); in validatePath() . This assert(!/test-fs-readfile-error/.test(data)); will always be false.

BridgeAR · 2018-02-16T23:27:42Z

lib/internal/errors.js

I am not sold on showing an empty path. Instead it could just be a placeholder as [Uint8Array path].

BridgeAR · 2018-02-16T23:29:48Z

lib/internal/errors.js

I try to understand the message but after reading it multiple times, I still can not fully understand it. Can you please rephrase this?

It felt that way at first but I left it because English is not my native language and it was suggested by @joyeecheung.

One way to make this more debuggable and consistent with other error messages is:

The argument "${name}" must not end with '/'. Received ${util.inspect(value)}

That requires us to pass in the name of the argument into the formatter, similar to ERR_INVALID_ARG_TYPE, ERR_OUT_OF_RANGE and the like.

BridgeAR · 2018-03-02T04:09:47Z

Ping @timotew

timotew · 2018-03-04T08:44:17Z

ping! @BridgeAR , @joyeecheung , @starkwang .

joyeecheung · 2018-03-04T09:11:20Z

lib/internal/url.js

    throw new errors.TypeError('ERR_INVALID_URL_SCHEME', 'file');
+
+  if (assertFilename && path.href[path.href.length - 1] === '/')
+    throw new errors.Error('ERR_PATH_IS_DIRECTORY', 'path', path.pathname);


The string being checked and reported here should be the result of isWindows ? getPathFromURLWin32(path) : getPathFromURLPosix(path) for consistency, also because errors like ERR_INVALID_FILE_URL_HOST should be thrown first.

Or, maybe getPathFromURL does not even need to be changed since the path extracted from URLs will be eventually passed to validatePath anyway..from what I can tell if we update WriteStream and ReadStream to validatePath(path, true) then we don't need to a ssert the file name in getPathFromURL

joyeecheung · 2018-03-04T09:20:20Z

lib/fs.js

  if (!(this instanceof WriteStream))
    return new WriteStream(path, options);

+  path = getPathFromURL(path, true);


There is already one this.path = getPathFromURL(path); below. Also path is ignored if options.fd !== undefined so maybe we just need to delete this and change the line below to:

if (options.fd !== undefined) { this.path = getPathFromURL(path); validatePath(this.path, true); }

Same about ReadStream

validatePath only accepts Uint8Array this will work if Stream path is always Uint8Array or string.

@timotew The path in the fs stream classes can be falsy if options.fd is provdided, e.g. fs.createReadStream(null, { fd: fd, encoding: 'utf8' }) is valid. From what I can tell, validatePath accepts both string and Uint8Array.

timotew · 2018-04-09T01:01:36Z

I ran out of steam on this, if someone else wants to take it over please do, but I'm going to close it for now, kthxbai

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Dec 31, 2017

devsnek reviewed Jan 1, 2018

View reviewed changes

starkwang reviewed Jan 2, 2018

View reviewed changes

lib/fs.js Outdated

Copy link

Contributor

starkwang Jan 2, 2018

Choose a reason for hiding this comment

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

This empty line is redundant :-)

starkwang reviewed Jan 2, 2018

View reviewed changes

cjihrig reviewed Jan 2, 2018

View reviewed changes

starkwang approved these changes Jan 3, 2018

View reviewed changes

cjihrig reviewed Jan 3, 2018

View reviewed changes

joyeecheung requested changes Jan 3, 2018

View reviewed changes

joyeecheung reviewed Jan 5, 2018

View reviewed changes

joyeecheung reviewed Jan 9, 2018

View reviewed changes

joyeecheung reviewed Jan 10, 2018

View reviewed changes

starkwang suggested changes Jan 11, 2018

View reviewed changes

timotew force-pushed the master branch 2 times, most recently from 27d7d4c to 49ef465 Compare January 11, 2018 21:48

joyeecheung reviewed Jan 22, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 2, 2018

timotew force-pushed the master branch 2 times, most recently from 89f24cd to 8dc1c99 Compare February 10, 2018 18:32

starkwang reviewed Feb 12, 2018

View reviewed changes

timotew force-pushed the master branch from 8dc1c99 to f52e555 Compare February 13, 2018 14:03

joyeecheung reviewed Feb 14, 2018

View reviewed changes

joyeecheung approved these changes Feb 16, 2018

View reviewed changes

joyeecheung requested changes Feb 16, 2018

View reviewed changes

BridgeAR reviewed Feb 16, 2018

View reviewed changes

create consistent behavior for fs module across platforms

2a3eea8

timotew force-pushed the master branch from c3a336c to 2a3eea8 Compare March 4, 2018 08:40

joyeecheung reviewed Mar 4, 2018

View reviewed changes

timotew closed this Apr 9, 2018

	return new errors.TypeError('ERR_INVALID_FILE_URL_PATH',
	'must not include encoded / characters');

	return new errors.TypeError('ERR_INVALID_FILE_URL_PATH',
	'must be absolute');

Uh oh!

fs: create consistent behavior for fs module across platforms #17927

fs: create consistent behavior for fs module across platforms #17927

Uh oh!

Conversation

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

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotew Jan 1, 2018 • 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!

Trott Jan 1, 2018 • 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!

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!

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!

starkwang commented Jan 3, 2018

Uh oh!

Trott commented Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 3, 2018

Uh oh!

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

timotew commented Jan 4, 2018

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!

joyeecheung commented Jan 5, 2018

Uh oh!

timotew commented Jan 5, 2018

Uh oh!

joyeecheung commented Jan 9, 2018

Uh oh!

timotew commented Jan 9, 2018

timotew commented Dec 31, 2017 •

edited

Loading

timotew Jan 1, 2018 •

edited

Loading

Trott Jan 1, 2018 •

edited

Loading

joyeecheung Jan 9, 2018 •

edited

Loading

joyeecheung commented Jan 11, 2018 •

edited

Loading