fs: fix incorrect error message in fs.open family when flags are not string or int #2873

ronkorving · 2015-09-15T08:46:12Z

This fixes #2871

ronkorving · 2015-09-15T08:48:21Z

lib/fs.js

+  }
+
+  cb(err);
+}


Note: for async functions this now passes the error to a callback.

reqshark · 2015-09-15T09:00:36Z

lib/fs.js

@@ -537,8 +561,13 @@ fs.open = function(path, flags, mode, callback_) {
  var req = new FSReqWrap();
  req.oncomplete = callback;

+  var intFlags = stringToFlags(flags);
+  if (intFlags === undefined) {


unless flag could ever be zero, i'd just go

if (!intFlags) {

To be honest, I'm not sure if it could ever be zero... I could also do a Number.isInteger test, but I figured this would execute faster.

bnoordhuis · 2015-09-15T09:25:19Z

To stop application bugs from propagating, the rule of thumb has always been "throw on logic error, callback on run-time error." Passing in bad flags or options is a logic error and should fail synchronously.

reqshark · 2015-09-15T09:27:37Z

in that case just change an int to a string here: https://github.com/nodejs/node/blob/master/src/node_file.cc#L844

ronkorving · 2015-09-15T09:39:40Z

@bnoordhuis I was not familiar with that philosophy.
@reqshark that won't do, because int is expected and accepted even in JS-land (where both int and string are fine).

reqshark · 2015-09-15T09:55:23Z

Ok don't change the text there then.

bad flags or options is a logic error and should fail synchronously

.. sounds to me like return an err before even var callback = makeCallback( gets started

fs.open = function(path, flags, mode, callback_) {
  var callback = makeCallback(arguments[arguments.length - 1]);
  mode = modeNum(mode, 0o666);

  if (!nullCheck(path, callback)) return;

  var req = new FSReqWrap();
  req.oncomplete = callback;

  var intFlags = stringToFlags(flags);
  if (intFlags === undefined) {
    return throwFlagsError(flags, callback);
  }

  binding.open(pathModule._makeLong(path),
               intFlags,
               mode,
               req);
};

ronkorving · 2015-09-15T09:57:12Z

I'll update this PR soon.

ronkorving · 2015-09-15T10:06:05Z

@bnoordhuis now it sticks to what you explained.

bnoordhuis · 2015-09-15T10:09:10Z

test/parallel/test-fs-open.js

+  if (!(err instanceof TypeError)) {
+    throw new Error('unexpected error: ' + err);
+  }
+}


You can replace the try/catch block with an assert.throws(...)

I wanted to make sure it's a TypeError and not the Error of "bad flag value" (which can also happen when it's a string, just not the right one).

You can still do that like this :)

Understood. If you insist, I'll change it, but I would rather stick to a class check here, to allow other contributions to Node to polish error messages when needed without breaking tests left and right. imho tests shouldn't check for the human readable output when avoidable, and in this case it is avoidable.

I second brendan's suggestion. What if TypeError is thrown because of some other problem? I prefer checking the actual message.

I would say, when the mesaages are updated, let the tests also be updated.

Understood. I'll change it.

Fixed (I hope :))

bnoordhuis · 2015-09-15T10:10:15Z

Basic premise LGTM.

thefourtheye · 2015-09-16T06:03:08Z

test/parallel/test-fs-open.js

@@ -32,8 +32,21 @@ fs.open(__filename, 'rs', function(err, fd) {
  openFd2 = fd;
 });

+assert.throws(function() {
+  fs.open('/path/to/file/that/does/not/exist', { bad: 'flags' }, function() {


We can simply this a little bit,

fs.open('/path/to/file/that/does/not/exist', { bad: 'flags' }, assert.fail);

thefourtheye · 2015-09-16T06:04:22Z

LGTM, with take-it or leave-it suggetions

Before, it would show "TypeError: flags must be an int", which in native Node is technically correct. But in JS they may be (and usually are) strings. Fixes nodejs#2871

ronkorving · 2015-09-16T06:44:29Z

I liked your suggestions :)

thefourtheye · 2015-09-16T10:28:29Z

@ronkorving Glad you liked them :-) Let's give this some time so that we can get more reviewers see this (hopefully ;-)).

bnoordhuis · 2015-09-16T11:15:13Z

test/parallel/test-fs-open.js

+  if (err instanceof TypeError) {
+    return err.message === 'File open flags may only be string or integer';
+  }
+});


You can condense this to assert.throws(function() { ... }, /File open flags may only be string or integer/).

Then I wouldn't be checking that it's a TypeError. That's not a problem?

bnoordhuis · 2015-09-16T11:15:35Z

LGTM

cjihrig · 2015-09-16T14:00:09Z

test/parallel/test-fs-open.js

+assert.throws(function() {
+  fs.open('/path/to/file/that/does/not/exist', { bad: 'flags' }, assert.fail);
+}, function(err) {
+  if (err instanceof TypeError) {


I don't think you want to check if the error is a TypeError. That should be unconditional, right?

Well I wrote the error as a TypeError since its thrown on bad types only, so might as well test for it, no?

Let me rephrase. You should assert that the error is a TypeError, not put it in an if statement.

Please see my comment below to @bnoordhuis. I can't seem to satisfy both your requests.

Do what @bnoordhuis is asking for :-)

cjihrig · 2015-09-16T14:00:22Z

LGTM with one comment.

sam-github · 2015-09-17T15:51:47Z

lib/fs.js

    return flag;
  }

+  // We only allow strings from here on
+  if (typeof flag !== 'string') {
+    throw new TypeError('File open flags may only be string or integer');


Anyone who sees this message and looks at the docs would immediately report think this a bug, since it conflicts with the docs. If you want to say that, I suggest documenting the behaviour.

I strongly suspect that the fact that flags can be an int is historical, kept for backwards compat from the v0.small days, and should be deprecated, but its hard to deprecate a piece of an API.

sam-github · 2015-09-17T15:53:44Z

lib/fs.js

-  // Only mess with strings
-  if (typeof flag !== 'string') {
+  // Integers are already good to go
+  if (Number.isInteger(flag)) {


This check doesn't work as you think it does: the C++ layer requires flags to fit within 32 bits, this check here only requires it to be integral. Try with 0xfffffffff, and you will see.

That's why in my PR I pass anything with typeof number to the C++ layer, it seems easier to let it do this check, because it does it correctly. Though you can put more code in here, checking the bounds futher.

ronkorving · 2015-09-18T02:08:45Z

Closing in favor of #2902

ronkorving reviewed Sep 15, 2015
View reviewed changes

lib/fs.js

}

cb(err);

}

Copy link

Contributor Author

ronkorving Sep 15, 2015

Choose a reason for hiding this comment

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

Note: for async functions this now passes the error to a callback.

brendanashworth added the fs Issues and PRs related to the fs subsystem / file system. label Sep 15, 2015

ronkorving force-pushed the fix-flags branch from 5a1bd16 to 47f4ce2 Compare September 15, 2015 08:50

reqshark reviewed Sep 15, 2015
View reviewed changes

ronkorving force-pushed the fix-flags branch from 47f4ce2 to 341619a Compare September 15, 2015 10:04

bnoordhuis reviewed Sep 15, 2015
View reviewed changes

ronkorving changed the title ~~fs: ensure async API doesn't throw on bad flags or options~~ fs: fix incorrect error message in fs.open family when flags are not string or int Sep 15, 2015

ronkorving force-pushed the fix-flags branch 2 times, most recently from d405e03 to 163b012 Compare September 16, 2015 04:02

thefourtheye reviewed Sep 16, 2015
View reviewed changes

fs: fix the error message when file open flags are a bad type

3f691b6

Before, it would show "TypeError: flags must be an int", which in native Node is technically correct. But in JS they may be (and usually are) strings. Fixes nodejs#2871

ronkorving force-pushed the fix-flags branch from 163b012 to 3f691b6 Compare September 16, 2015 06:44

bnoordhuis reviewed Sep 16, 2015
View reviewed changes

cjihrig reviewed Sep 16, 2015
View reviewed changes

sam-github mentioned this pull request Sep 16, 2015

fs: fix confusing flags TypeError msg #2902

Closed

sam-github reviewed Sep 17, 2015
View reviewed changes

ronkorving closed this Sep 18, 2015

ChALkeR mentioned this pull request Sep 29, 2015

dns.reverse throws on error instead of invoking the callback #3112

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: fix incorrect error message in fs.open family when flags are not string or int #2873

fs: fix incorrect error message in fs.open family when flags are not string or int #2873

ronkorving commented Sep 15, 2015

ronkorving Sep 15, 2015

reqshark Sep 15, 2015

ronkorving Sep 15, 2015

bnoordhuis commented Sep 15, 2015

reqshark commented Sep 15, 2015

ronkorving commented Sep 15, 2015

reqshark commented Sep 15, 2015

ronkorving commented Sep 15, 2015

ronkorving commented Sep 15, 2015

bnoordhuis Sep 15, 2015

ronkorving Sep 15, 2015

brendanashworth Sep 15, 2015

ronkorving Sep 16, 2015

thefourtheye Sep 16, 2015

ronkorving Sep 16, 2015

ronkorving Sep 16, 2015

bnoordhuis commented Sep 15, 2015

thefourtheye Sep 16, 2015

thefourtheye commented Sep 16, 2015

ronkorving commented Sep 16, 2015

thefourtheye commented Sep 16, 2015

bnoordhuis Sep 16, 2015

ronkorving Sep 17, 2015

bnoordhuis commented Sep 16, 2015

cjihrig Sep 16, 2015

ronkorving Sep 16, 2015

cjihrig Sep 16, 2015

ronkorving Sep 17, 2015

cjihrig Sep 17, 2015

cjihrig commented Sep 16, 2015

sam-github Sep 17, 2015

sam-github Sep 17, 2015

ronkorving commented Sep 18, 2015

fs: fix incorrect error message in fs.open family when flags are not string or int #2873

fs: fix incorrect error message in fs.open family when flags are not string or int #2873

Conversation

ronkorving commented Sep 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Sep 15, 2015

reqshark commented Sep 15, 2015

ronkorving commented Sep 15, 2015

reqshark commented Sep 15, 2015

ronkorving commented Sep 15, 2015

ronkorving commented Sep 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Sep 15, 2015

Choose a reason for hiding this comment

thefourtheye commented Sep 16, 2015

ronkorving commented Sep 16, 2015

thefourtheye commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Sep 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronkorving commented Sep 18, 2015