Skip to content

Commit 2a3eea8

Browse files
timotewtimotew
committed
create consistent behavior for fs module across platforms
1 parent 5a55a71 commit 2a3eea8

7 files changed

Lines changed: 245 additions & 23 deletions

File tree

‎doc/api/errors.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1312,6 +1312,11 @@ A Node.js API was called in an unsupported manner, such as
13121312

13131313
A given value is out of the accepted range.
13141314

1315+
<a id="ERR_PATH_IS_DIRECTORY"></a>
1316+
### ERR_PATH_IS_DIRECTORY
1317+
1318+
Provided path ended with an unexpected character `/`.
1319+
13151320
<a id="ERR_REQUIRE_ESM"></a>
13161321
### ERR_REQUIRE_ESM
13171322

‎lib/fs.js‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -235,8 +235,8 @@ fs.readFile = function(path, options, callback) {
235235
return;
236236
}
237237

238-
path = getPathFromURL(path);
239-
validatePath(path);
238+
path = getPathFromURL(path, true);
239+
validatePath(path, 'path', true);
240240
binding.open(pathModule.toNamespacedPath(path),
241241
stringToFlags(options.flag || 'r'),
242242
0o666,
@@ -503,7 +503,7 @@ fs.open = function(path, flags, mode, callback_) {
503503
mode = modeNum(mode, 0o666);
504504

505505
path = getPathFromURL(path);
506-
validatePath(path);
506+
validatePath(path, 'path', true);
507507
validateUint32(mode, 'mode');
508508

509509
const req = new FSReqWrap();
@@ -518,7 +518,7 @@ fs.open = function(path, flags, mode, callback_) {
518518
fs.openSync = function(path, flags, mode) {
519519
mode = modeNum(mode, 0o666);
520520
path = getPathFromURL(path);
521-
validatePath(path);
521+
validatePath(path, 'path', true);
522522
validateUint32(mode, 'mode');
523523

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

1882-
src = getPathFromURL(src);
1883-
dest = getPathFromURL(dest);
1884-
validatePath(src, 'src');
1885-
validatePath(dest, 'dest');
1882+
src = getPathFromURL(src, true);
1883+
dest = getPathFromURL(dest, true);
1884+
validatePath(src, 'src', true);
1885+
validatePath(dest, 'dest', true);
18861886

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

18951895

18961896
fs.copyFileSync = function(src, dest, flags) {
1897-
src = getPathFromURL(src);
1898-
dest = getPathFromURL(dest);
1899-
validatePath(src, 'src');
1900-
validatePath(dest, 'dest');
1897+
src = getPathFromURL(src, true);
1898+
dest = getPathFromURL(dest, true);
1899+
validatePath(src, 'src', true);
1900+
validatePath(dest, 'dest', true);
19011901

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

@@ -1928,6 +1928,7 @@ function ReadStream(path, options) {
19281928
if (!(this instanceof ReadStream))
19291929
return new ReadStream(path, options);
19301930

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

2099+
path = getPathFromURL(path, true);
20982100
options = copyObject(getOptions(options, {}));
20992101

21002102
Writable.call(this, options);

‎lib/internal/errors.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -823,6 +823,11 @@ E('ERR_NO_ICU',
823823
'%s is not supported on Node.js compiled without ICU', TypeError);
824824
E('ERR_NO_LONGER_SUPPORTED', '%s is no longer supported', Error);
825825
E('ERR_OUT_OF_RANGE', outOfRange, RangeError);
826+
E('ERR_PATH_IS_DIRECTORY', (name, value) => {
827+
const util = lazyUtil();
828+
return `The argument "${name}" must not end with "/". ` +
829+
`Received ${util.inspect(value)}`;
830+
});
826831
E('ERR_REQUIRE_ESM', 'Must use import to load ES Module: %s', Error);
827832
E('ERR_SCRIPT_EXECUTION_INTERRUPTED',
828833
'Script execution was interrupted by `SIGINT`.', Error);

‎lib/internal/fs.js‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -369,18 +369,24 @@ function validateOffsetLengthWrite(offset, length, byteLength) {
369369
}
370370
}
371371

372-
function validatePath(path, propName) {
372+
function validatePath(path, propName, assertFilename) {
373373
let err;
374374

375375
if (propName === undefined) {
376376
propName = 'path';
377377
}
378378

379-
if (typeof path !== 'string' && !isUint8Array(path)) {
379+
if (typeof path === 'string') {
380+
err = nullCheck(path, propName, false);
381+
if (assertFilename && err === undefined && path[path.length - 1] === '/')
382+
err = new errors.Error('ERR_PATH_IS_DIRECTORY', propName, path);
383+
} else if (isUint8Array(path)) {
384+
err = nullCheck(path, propName, false);
385+
if (assertFilename && err === undefined && path[path.length - 1] === 47)
386+
err = new errors.Error('ERR_PATH_IS_DIRECTORY', propName, path);
387+
} else {
380388
err = new errors.TypeError('ERR_INVALID_ARG_TYPE', propName,
381389
['string', 'Buffer', 'URL']);
382-
} else {
383-
err = nullCheck(path, propName, false);
384390
}
385391

386392
if (err !== undefined) {

‎lib/internal/url.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1365,13 +1365,16 @@ function getPathFromURLPosix(url) {
13651365
return decodeURIComponent(pathname);
13661366
}
13671367

1368-
function getPathFromURL(path) {
1368+
function getPathFromURL(path, assertFilename) {
13691369
if (path == null || !path[searchParams] ||
13701370
!path[searchParams][searchParams]) {
13711371
return path;
13721372
}
13731373
if (path.protocol !== 'file:')
13741374
throw new errors.TypeError('ERR_INVALID_URL_SCHEME', 'file');
1375+
1376+
if (assertFilename && path.href[path.href.length - 1] === '/')
1377+
throw new errors.Error('ERR_PATH_IS_DIRECTORY', 'path', path.pathname);
13751378
return isWindows ? getPathFromURLWin32(path) : getPathFromURLPosix(path);
13761379
}
13771380

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

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,206 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const fs = require('fs');
5+
const URL = require('url').URL;
6+
const util = require('util');
7+
8+
function runPathTests(withSlash, withoutSlash, slashedPath, realName) {
9+
10+
if (!slashedPath) {
11+
slashedPath = withSlash;
12+
} else if (typeof slashedPath === 'boolean') {
13+
realName = slashedPath;
14+
slashedPath = withSlash;
15+
}
16+
17+
common.expectsError(
18+
() => {
19+
fs.readFile(withSlash, common.mustNotCall());
20+
},
21+
{
22+
code: 'ERR_PATH_IS_DIRECTORY',
23+
type: Error,
24+
message: 'The argument "path" must not end with "/". ' +
25+
`Received ${util.inspect(slashedPath)}`
26+
});
27+
28+
common.expectsError(
29+
() => {
30+
fs.readFileSync(withSlash, { flag: 'r' });
31+
},
32+
{
33+
code: 'ERR_PATH_IS_DIRECTORY',
34+
type: Error,
35+
message: 'The argument "path" must not end with "/". ' +
36+
`Received ${util.inspect(slashedPath)}`
37+
});
38+
39+
common.expectsError(
40+
() => {
41+
fs.open(withSlash, 'r', common.mustNotCall());
42+
},
43+
{
44+
code: 'ERR_PATH_IS_DIRECTORY',
45+
type: Error,
46+
message: 'The argument "path" must not end with "/". ' +
47+
`Received ${util.inspect(slashedPath)}`
48+
});
49+
50+
common.expectsError(
51+
() => {
52+
fs.openSync(withSlash, 'r');
53+
},
54+
{
55+
code: 'ERR_PATH_IS_DIRECTORY',
56+
type: Error,
57+
message: 'The argument "path" must not end with "/". ' +
58+
`Received ${util.inspect(slashedPath)}`
59+
});
60+
61+
common.expectsError(
62+
() => {
63+
fs.appendFile(withSlash, 'test data', common.mustNotCall());
64+
},
65+
{
66+
code: 'ERR_PATH_IS_DIRECTORY',
67+
type: Error,
68+
message: 'The argument "path" must not end with "/". ' +
69+
`Received ${util.inspect(slashedPath)}`
70+
});
71+
72+
common.expectsError(
73+
() => {
74+
fs.appendFileSync(withSlash, 'test data');
75+
},
76+
{
77+
code: 'ERR_PATH_IS_DIRECTORY',
78+
type: Error,
79+
message: 'The argument "path" must not end with "/". ' +
80+
`Received ${util.inspect(slashedPath)}`
81+
});
82+
83+
common.expectsError(
84+
() => {
85+
fs.createReadStream(withSlash);
86+
},
87+
{
88+
code: 'ERR_PATH_IS_DIRECTORY',
89+
type: Error,
90+
message: 'The argument "path" must not end with "/". ' +
91+
`Received ${util.inspect(slashedPath)}`
92+
});
93+
94+
95+
common.expectsError(
96+
() => {
97+
fs.createWriteStream(withSlash);
98+
},
99+
{
100+
code: 'ERR_PATH_IS_DIRECTORY',
101+
type: Error,
102+
message: 'The argument "path" must not end with "/". ' +
103+
`Received ${util.inspect(slashedPath)}`
104+
});
105+
106+
common.expectsError(
107+
() => {
108+
fs.truncate(withSlash, 4, common.mustNotCall());
109+
},
110+
{
111+
code: 'ERR_PATH_IS_DIRECTORY',
112+
type: Error,
113+
message: 'The argument "path" must not end with "/". ' +
114+
`Received ${util.inspect(slashedPath)}`
115+
});
116+
117+
common.expectsError(
118+
() => {
119+
fs.truncateSync(withSlash, 4);
120+
},
121+
{
122+
code: 'ERR_PATH_IS_DIRECTORY',
123+
type: Error,
124+
message: 'The argument "path" must not end with "/". ' +
125+
`Received ${util.inspect(slashedPath)}`
126+
});
127+
128+
common.expectsError(
129+
() => {
130+
fs.writeFile(withSlash, 'test data', common.mustNotCall());
131+
},
132+
{
133+
code: 'ERR_PATH_IS_DIRECTORY',
134+
type: Error,
135+
message: 'The argument "path" must not end with "/". ' +
136+
`Received ${util.inspect(slashedPath)}`
137+
});
138+
139+
common.expectsError(
140+
() => {
141+
fs.writeFileSync(withSlash, 'test data');
142+
},
143+
{
144+
code: 'ERR_PATH_IS_DIRECTORY',
145+
type: Error,
146+
message: 'The argument "path" must not end with "/". ' +
147+
`Received ${util.inspect(slashedPath)}`
148+
});
149+
150+
common.expectsError(
151+
() => {
152+
fs.copyFile(withSlash, withoutSlash, common.mustNotCall());
153+
},
154+
{
155+
code: 'ERR_PATH_IS_DIRECTORY',
156+
type: Error,
157+
message: `The argument "${realName ? 'src' : 'path'}" must not end ` +
158+
`with "/". Received ${util.inspect(slashedPath)}`
159+
});
160+
161+
common.expectsError(
162+
() => {
163+
fs.copyFile(withoutSlash, withSlash, common.mustNotCall());
164+
},
165+
{
166+
code: 'ERR_PATH_IS_DIRECTORY',
167+
type: Error,
168+
message: `The argument "${realName ? 'dest' : 'path'}" must not end ` +
169+
`with "/". Received ${util.inspect(slashedPath)}`
170+
});
171+
172+
common.expectsError(
173+
() => {
174+
fs.copyFileSync(withSlash, withoutSlash);
175+
},
176+
{
177+
code: 'ERR_PATH_IS_DIRECTORY',
178+
type: Error,
179+
message: `The argument "${realName ? 'src' : 'path'}" must not end ` +
180+
`with "/". Received ${util.inspect(slashedPath)}`
181+
});
182+
183+
common.expectsError(
184+
() => {
185+
fs.copyFileSync(withoutSlash, withSlash);
186+
},
187+
{
188+
code: 'ERR_PATH_IS_DIRECTORY',
189+
type: Error,
190+
message: `The argument "${realName ? 'dest' : 'path'}" must not end ` +
191+
`with "/". Received ${util.inspect(slashedPath)}`
192+
});
193+
}
194+
195+
const path = '/tmp/test';
196+
const stringWithSlash = common.isWindows ? `file:///c:${path}/` :
197+
`file://${path}/`;
198+
const stringWithoutSlash = common.isWindows ? `file:///c:${path}` :
199+
`file://${path}`;
200+
const urlWithSlash = new URL(stringWithSlash);
201+
const urlWithoutSlash = new URL(stringWithoutSlash);
202+
const U8ABufferWithSlash = new Uint8Array(Buffer.from(stringWithSlash));
203+
const U8ABufferWithoutSlash = new Uint8Array(Buffer.from(stringWithoutSlash));
204+
runPathTests(urlWithSlash, urlWithoutSlash, `${path}/`);
205+
runPathTests(stringWithSlash, stringWithoutSlash, true);
206+
runPathTests(U8ABufferWithSlash, U8ABufferWithoutSlash, true);

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

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,8 @@ function test(env, cb) {
4646
}));
4747
}
4848

49-
test({ NODE_DEBUG: '' }, common.mustCall((data) => {
50-
assert(/EISDIR/.test(data));
51-
assert(/test-fs-readfile-error/.test(data));
52-
}));
53-
5449
test({ NODE_DEBUG: 'fs' }, common.mustCall((data) => {
55-
assert(/EISDIR/.test(data));
50+
assert(/ERR_PATH_IS_DIRECTORY/.test(data));
5651
assert(/test-fs-readfile-error/.test(data));
5752
}));
5853

0 commit comments

Comments
 (0)