Skip to content

Commit

Permalink
create consistent behavior for fs module across platforms
Browse files Browse the repository at this point in the history
  • Loading branch information
@timotew
timotew committed Mar 4, 2018
1 parent 5a55a71 commit 2a3eea8
Show file tree
Hide file tree
Showing 7 changed files with 245 additions and 23 deletions.
5 changes: 5 additions & 0 deletions doc/api/errors.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1312,6 +1312,11 @@ A Node.js API was called in an unsupported manner, such as

A given value is out of the accepted range.

<a id="ERR_PATH_IS_DIRECTORY"></a>
### ERR_PATH_IS_DIRECTORY

Provided path ended with an unexpected character `/`.

<a id="ERR_REQUIRE_ESM"></a>
### ERR_REQUIRE_ESM

Expand Down
26 changes: 14 additions & 12 deletions lib/fs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,8 +235,8 @@ fs.readFile = function(path, options, callback) {
return;
}

path = getPathFromURL(path);
validatePath(path);
path = getPathFromURL(path, true);
validatePath(path, 'path', true);
binding.open(pathModule.toNamespacedPath(path),
stringToFlags(options.flag || 'r'),
0o666,
Expand Down Expand Up @@ -503,7 +503,7 @@ fs.open = function(path, flags, mode, callback_) {
mode = modeNum(mode, 0o666);

path = getPathFromURL(path);
validatePath(path);
validatePath(path, 'path', true);
validateUint32(mode, 'mode');

const req = new FSReqWrap();
Expand All @@ -518,7 +518,7 @@ fs.open = function(path, flags, mode, callback_) {
fs.openSync = function(path, flags, mode) {
mode = modeNum(mode, 0o666);
path = getPathFromURL(path);
validatePath(path);
validatePath(path, 'path', true);
validateUint32(mode, 'mode');

const ctx = { path };
Expand Down Expand Up @@ -1879,10 +1879,10 @@ fs.copyFile = function(src, dest, flags, callback) {
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

src = getPathFromURL(src);
dest = getPathFromURL(dest);
validatePath(src, 'src');
validatePath(dest, 'dest');
src = getPathFromURL(src, true);
dest = getPathFromURL(dest, true);
validatePath(src, 'src', true);
validatePath(dest, 'dest', true);

src = pathModule._makeLong(src);
dest = pathModule._makeLong(dest);
Expand All @@ -1894,10 +1894,10 @@ fs.copyFile = function(src, dest, flags, callback) {


fs.copyFileSync = function(src, dest, flags) {
src = getPathFromURL(src);
dest = getPathFromURL(dest);
validatePath(src, 'src');
validatePath(dest, 'dest');
src = getPathFromURL(src, true);
dest = getPathFromURL(dest, true);
validatePath(src, 'src', true);
validatePath(dest, 'dest', true);

const ctx = { path: src, dest }; // non-prefixed

Expand Down Expand Up @@ -1928,6 +1928,7 @@ function ReadStream(path, options) {
if (!(this instanceof ReadStream))
return new ReadStream(path, options);

path = getPathFromURL(path, true);
// a little bit bigger buffer and water marks by default
options = copyObject(getOptions(options, {}));
if (options.highWaterMark === undefined)
Expand Down Expand Up @@ -2095,6 +2096,7 @@ function WriteStream(path, options) {
if (!(this instanceof WriteStream))
return new WriteStream(path, options);

path = getPathFromURL(path, true);
options = copyObject(getOptions(options, {}));

Writable.call(this, options);
Expand Down
5 changes: 5 additions & 0 deletions lib/internal/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -823,6 +823,11 @@ E('ERR_NO_ICU',
'%s is not supported on Node.js compiled without ICU', TypeError);
E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported', Error);
E('ERR_OUT_OF_RANGE', outOfRange, RangeError);
E('ERR_PATH_IS_DIRECTORY', (name, value) => {
const util = lazyUtil();
return `The argument "${name}" must not end with "/". ` +
`Received ${util.inspect(value)}`;
});
E('ERR_REQUIRE_ESM', 'Must use import to load ES Module: %s', Error);
E('ERR_SCRIPT_EXECUTION_INTERRUPTED',
'Script execution was interrupted by `SIGINT`.', Error);
Expand Down
14 changes: 10 additions & 4 deletions lib/internal/fs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,18 +369,24 @@ function validateOffsetLengthWrite(offset, length, byteLength) {
}
}

function validatePath(path, propName) {
function validatePath(path, propName, assertFilename) {
let err;

if (propName === undefined) {
propName = 'path';
}

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

if (err !== undefined) {
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/url.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1365,13 +1365,16 @@ function getPathFromURLPosix(url) {
return decodeURIComponent(pathname);
}

function getPathFromURL(path) {
function getPathFromURL(path, assertFilename) {
if (path == null || !path[searchParams] ||
!path[searchParams][searchParams]) {
return path;
}
if (path.protocol !== 'file:')
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);
return isWindows ? getPathFromURLWin32(path) : getPathFromURLPosix(path);
}

Expand Down
206 changes: 206 additions & 0 deletions test/parallel/test-fs-path-err.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,206 @@
'use strict';

const common = require('../common');
const fs = require('fs');
const URL = require('url').URL;
const util = require('util');

function runPathTests(withSlash, withoutSlash, slashedPath, realName) {

if (!slashedPath) {
slashedPath = withSlash;
} else if (typeof slashedPath === 'boolean') {
realName = slashedPath;
slashedPath = withSlash;
}

common.expectsError(
() => {
fs.readFile(withSlash, common.mustNotCall());
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.readFileSync(withSlash, { flag: 'r' });
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.open(withSlash, 'r', common.mustNotCall());
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.openSync(withSlash, 'r');
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.appendFile(withSlash, 'test data', common.mustNotCall());
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.appendFileSync(withSlash, 'test data');
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.createReadStream(withSlash);
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});


common.expectsError(
() => {
fs.createWriteStream(withSlash);
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.truncate(withSlash, 4, common.mustNotCall());
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.truncateSync(withSlash, 4);
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.writeFile(withSlash, 'test data', common.mustNotCall());
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.writeFileSync(withSlash, 'test data');
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: 'The argument "path" must not end with "/". ' +
`Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.copyFile(withSlash, withoutSlash, common.mustNotCall());
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: `The argument "${realName ? 'src' : 'path'}" must not end ` +
`with "/". Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.copyFile(withoutSlash, withSlash, common.mustNotCall());
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: `The argument "${realName ? 'dest' : 'path'}" must not end ` +
`with "/". Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.copyFileSync(withSlash, withoutSlash);
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: `The argument "${realName ? 'src' : 'path'}" must not end ` +
`with "/". Received ${util.inspect(slashedPath)}`
});

common.expectsError(
() => {
fs.copyFileSync(withoutSlash, withSlash);
},
{
code: 'ERR_PATH_IS_DIRECTORY',
type: Error,
message: `The argument "${realName ? 'dest' : 'path'}" must not end ` +
`with "/". Received ${util.inspect(slashedPath)}`
});
}

const path = '/tmp/test';
const stringWithSlash = common.isWindows ? `file:///c:${path}/` :
`file://${path}/`;
const stringWithoutSlash = common.isWindows ? `file:///c:${path}` :
`file://${path}`;
const urlWithSlash = new URL(stringWithSlash);
const urlWithoutSlash = new URL(stringWithoutSlash);
const U8ABufferWithSlash = new Uint8Array(Buffer.from(stringWithSlash));
const U8ABufferWithoutSlash = new Uint8Array(Buffer.from(stringWithoutSlash));
runPathTests(urlWithSlash, urlWithoutSlash, `${path}/`);
runPathTests(stringWithSlash, stringWithoutSlash, true);
runPathTests(U8ABufferWithSlash, U8ABufferWithoutSlash, true);
7 changes: 1 addition & 6 deletions test/parallel/test-fs-readfile-error.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,8 @@ function test(env, cb) {
}));
}

test({ NODE_DEBUG: '' }, common.mustCall((data) => {
assert(/EISDIR/.test(data));
assert(/test-fs-readfile-error/.test(data));
}));

test({ NODE_DEBUG: 'fs' }, common.mustCall((data) => {
assert(/EISDIR/.test(data));
assert(/ERR_PATH_IS_DIRECTORY/.test(data));
assert(/test-fs-readfile-error/.test(data));
}));

Expand Down

0 comments on commit 2a3eea8

Please sign in to comment.