[bug] functions outputJSON and pathExists execute callbacks even if there are errors/missing args (not matching fs behaviour)

[emarteca]

Hi all! I'm working on an automated test generator and have been testing it by applying regression testing to a set of popular npm libraries, including yours! In this commit I found a difference in the behaviour of outputJSON when called with incorrect arguments.

The issue

Functions that have been converted with universalify.fromPromise execute the callback argument even when the other arguments are missing or invalid. It looks like this results in a bug, because the comparable fs functions do error checking of these arguments.

To replicate the issue:

let fse = require('fs-extra');
function callback() { 
  console.log("Callback executing!");
  console.log(arguments);
}
fse.outputJSON("newfile.json", "[ ]", callback); // correct call
fse.outputJSON( callback); // incorrect call

Before the change in this commit, the output of running the above code is:

// correct call output: 
> Callback executing!
> [Arguments] { '0': null, '1': undefined }
// incorrect call output:
> Uncaught:
TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string. Received function callback
    at validateString (internal/validators.js:124:11)
    at Object.dirname (path.js:1128:5)
...

Then, after the update, the output of running the above code is:

// correct call output:
> Callback executing!
> [Arguments] { '0': null, '1': undefined }
// incorrect call output:
> Callback executing!
> [Arguments] {
  '0': TypeError: Cannot read property 'replace' of undefined
      at stringify ...

So, the callback is now executed even when the arguments are incorrect, and have the function error passed in as an argument to the callback.

What is causing this behaviour

This change from using the universalify library’s fromPromise function instead of fromCallback is causing the difference.

Looking at the implementation of fromPromise: this function takes a function fn as an argument, and returns a new function that

• calls fn as it would normally be called, if the last argument is not a callback
• if the last argument is a callback, then fn is called with all but its last argument (this is fn.apply(this, args.slice(0, -1))). As stated in the documentation, the fromPromise function assumes that fn returns a promise. Then, it chains a .then to the return of the call, passing the function r => cb(null, r) as the resolve of this .then promise, and cb (the callback argument itself) as the reject function.

In our case, we see that the incorrect call to outputJSON does result in an error TypeError: Cannot read property 'replace' of undefined at stringify... (corresponds to this call being made with undefined). And so, the promise resulting from the call to outputJSON is rejected. As such, the reject branch of the .then is triggered, which results in our callback being executed.

We see that in our callback, the TypeError is printed as it is the argument to the callback.

pathExists: another function with this behaviour

The pathExists function is also universalified with fromPromise. As such, calling pathExists with only a callback has the same behaviour as outputJSON (i.e. the callback is executed).

fse.pathExists("newfile.json", callback); // newfile.json exists
> Callback executing!
> [Arguments] { '0': null, '1': true}

// incorrect call still executes the callback
fse.pathExists( callback); 
> Callback executing!
>  [Arguments] { '0': null, '1': false}

Comparison to behaviour of corresponding 'fs' functions

outputJSON is a special case of writeFile, so I assume it should have similar error checking to fs.writeFile.
pathExists is checking for the existence of a file and is just a wrapper for fs.access, so it should have the same error checking as fs.access.
Both of these functions have a type error if you try and call them with just a callback.

let fs = require('fs');
fs.writeFile(callback); // this function expects 3 arguments, and the last must be a callback
> Uncaught:                                                                                                                                                                            
TypeError [ERR_INVALID_CALLBACK]: Callback must be a function. Received undefined                                                                                                    
    at maybeCallback (fs.js:160:9)
    at Object.writeFile (fs.js:1431:14)

fs.access(callback);
Uncaught:                                                                                                                                                                            
TypeError [ERR_INVALID_CALLBACK]: Callback must be a function. Received undefined                                                                                                    
    at maybeCallback (fs.js:160:9)
    at Object.writeFile (fs.js:1431:14)

Fix

It is not enough to just add argument type checking code to the implementations of outputJSON and pathExists: because of the design of fromPromise, the callback will be executed regardless of the contents of these functions.
I have a proposed fix that simply involves replicating the implementation of fromPromise in the body of these functions, after some type checking.
I'll make a PR soon and link it here.

[emarteca]

Relevant PR: #866

[RyanZim]

Closing as per #826.