Promises and rejections #10

Closed
sebastianseilund opened this Issue Oct 28, 2014 · 18 comments

Comments

Projects
None yet
5 participants
@sebastianseilund
Contributor

sebastianseilund commented Oct 28, 2014

Thanks for a cool validation module.

One thing that bugs me is how validate.async() rejects with the errors. This is not analogous to how validate() works. It returns no matter the validation result, either with undefined for success or a hash of errors if not successful. It does not throw the errors. So validate.async should also do the same: Resolve with either undefined if successful or a hash of errors if not successful.

Rejecting the promise corresponds to throwing in validate(). It indicates an application error, not a validation error.

This goes for both validate.async and the promise custom async validators return.

First of all nothing should reject with random objects or strings. You should reject with a proper Error instance.

Secondly, it becomes a problem when the validation rejection gets mixed up with application errors. Worst case scenario is that an application error containing sensitive information is leaked to a user. Here's an example of that:

validate.validators.userExists = function(userId) {
  return db.query("SELECT id FROM users WHERE ?", [userId])
    .then(function(rows) {
      if (rows.length === 0) {
        throw 'User does not exist';
      }
    })
}

If db.query rejects, the db error object will be included in the final validation errors hash.

I would gladly submit a PR to fix this, if you want to take the library this way (I definitely think you should).

@sebastianseilund

This comment has been minimized.

Show comment
Hide comment
@sebastianseilund

sebastianseilund Oct 28, 2014

Contributor

Another problem is that there is no way to actually handle the error from db.query in the above example.

It would need to bubble up to the caller of validate.async():

var constraints = {
  userId: {
    userExists: trur
  }
}
validate.async({userId: '123'}, constraints)
  .then(function(result) {
    //`result` should either be `undefined` or a hash of errors
  }, function(e) {
    //This is where I need to handle my db error
  }
Contributor

sebastianseilund commented Oct 28, 2014

Another problem is that there is no way to actually handle the error from db.query in the above example.

It would need to bubble up to the caller of validate.async():

var constraints = {
  userId: {
    userExists: trur
  }
}
validate.async({userId: '123'}, constraints)
  .then(function(result) {
    //`result` should either be `undefined` or a hash of errors
  }, function(e) {
    //This is where I need to handle my db error
  }
@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Dec 9, 2014

Owner

Well, I don't agree about the semantics but I do see your point.

Let me think about this for a while and I'll get back to you.

Owner

ansman commented Dec 9, 2014

Well, I don't agree about the semantics but I do see your point.

Let me think about this for a while and I'll get back to you.

@Jokero

This comment has been minimized.

Show comment
Hide comment
@Jokero

Jokero Dec 29, 2014

Contributor

Or will be better so:

validate.async({userId: '123'}, constraints)
  .then(function() {
    // ok
  }, function(err) {
    if (err instanceof Error) {
      // application error
    } else {
      // validation error is plain object
    }
  }
Contributor

Jokero commented Dec 29, 2014

Or will be better so:

validate.async({userId: '123'}, constraints)
  .then(function() {
    // ok
  }, function(err) {
    if (err instanceof Error) {
      // application error
    } else {
      // validation error is plain object
    }
  }
@sebastianseilund

This comment has been minimized.

Show comment
Hide comment
@sebastianseilund

sebastianseilund Dec 29, 2014

Contributor

@Jokero That would be a good solution. But only if you could do it the other way around: Check if the error was a validate.js errors object. Something like:

validate.async({userId: '123'}, constraints)
  .then(function() {
    // ok
  }, function(err) {
    if (err instanceof validate.ValidationError) {
      // validation error is plain object
    } else {
      // application error
    }
  }

@ansman what do you think about that? The only change needed would be that the object that we reject with will be an instance of validate.ValidationError.

Contributor

sebastianseilund commented Dec 29, 2014

@Jokero That would be a good solution. But only if you could do it the other way around: Check if the error was a validate.js errors object. Something like:

validate.async({userId: '123'}, constraints)
  .then(function() {
    // ok
  }, function(err) {
    if (err instanceof validate.ValidationError) {
      // validation error is plain object
    } else {
      // application error
    }
  }

@ansman what do you think about that? The only change needed would be that the object that we reject with will be an instance of validate.ValidationError.

@dkushner

This comment has been minimized.

Show comment
Hide comment
@dkushner

dkushner Jan 11, 2015

@sebastianseilund, I agree with your earlier proposal but your specific example is really a promise anti-pattern. The same behaviour can be accomplished this way:

validate.async({ userId: '123' }, constraints).then(function() {
    // ok
}).catch(validate.ValidationError, function(err) {
    // validation error
}).catch(function(err) {
    // "application" error
});

Like @ansman said, I see no semantic issues with the way its currently implemented. throwing inside of a promise is completely analogous to rejecting the promise and this would be a perfectly appropriate application of that mechanism. My only issue with this behavior is that, for some reason, async is not properly handling all of the rejections in a set of validations leading to some being unhandled. Not yet sure why this is happening, but I'm tracking it down.

@sebastianseilund, I agree with your earlier proposal but your specific example is really a promise anti-pattern. The same behaviour can be accomplished this way:

validate.async({ userId: '123' }, constraints).then(function() {
    // ok
}).catch(validate.ValidationError, function(err) {
    // validation error
}).catch(function(err) {
    // "application" error
});

Like @ansman said, I see no semantic issues with the way its currently implemented. throwing inside of a promise is completely analogous to rejecting the promise and this would be a perfectly appropriate application of that mechanism. My only issue with this behavior is that, for some reason, async is not properly handling all of the rejections in a set of validations leading to some being unhandled. Not yet sure why this is happening, but I'm tracking it down.

@sebastianseilund

This comment has been minimized.

Show comment
Hide comment
@sebastianseilund

sebastianseilund Jan 19, 2015

Contributor

@dkushner The .catch(ErrorType, function() {...}) API does not work with all promise libraries nor ES6 Promises (i.e. catching only errors of specific types).

@ansman What did you think about rejecting with an instance of validate.ValidationError? :)

Contributor

sebastianseilund commented Jan 19, 2015

@dkushner The .catch(ErrorType, function() {...}) API does not work with all promise libraries nor ES6 Promises (i.e. catching only errors of specific types).

@ansman What did you think about rejecting with an instance of validate.ValidationError? :)

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Jan 20, 2015

Owner

If that helps you then that sounds like a good compromise.

Owner

ansman commented Jan 20, 2015

If that helps you then that sounds like a good compromise.

@ansman ansman closed this in d255d75 Jan 20, 2015

@Jokero

This comment has been minimized.

Show comment
Hide comment
@Jokero

Jokero Jan 21, 2015

Contributor

@ansman So how it helps us if there is application error?

Example "exists" validator:

var validate = require('validate.js');
var q        = require('q');

validate.validators.exists = function() {
    var deferred = q.defer();

    // Simulation of async work (get count)
    (function(err, count) {
        if (err) {
            /**
             * It's not validation error
             * How I must reject promise to distinguish err from validate.ValidationErrors?
             */
            deferred.reject(err);
        }

        if (!count) {
            deferred.reject('notExists');
        }
    })(new Error('Something went wrong with DB request...'));

    return deferred.promise;
};

var constraints = {
    userId: {
        exists: true
    }
};

validate.async({ userId: '123' }, constraints).catch(function(err) {
    if (err instanceof validate.ValidationErrors) { // always true
        // In case of REST API it's 400 (Bad Request)
    } else {
        // Never execute
        // In case of REST API it's 500 (Server Error)
    }
});
Contributor

Jokero commented Jan 21, 2015

@ansman So how it helps us if there is application error?

Example "exists" validator:

var validate = require('validate.js');
var q        = require('q');

validate.validators.exists = function() {
    var deferred = q.defer();

    // Simulation of async work (get count)
    (function(err, count) {
        if (err) {
            /**
             * It's not validation error
             * How I must reject promise to distinguish err from validate.ValidationErrors?
             */
            deferred.reject(err);
        }

        if (!count) {
            deferred.reject('notExists');
        }
    })(new Error('Something went wrong with DB request...'));

    return deferred.promise;
};

var constraints = {
    userId: {
        exists: true
    }
};

validate.async({ userId: '123' }, constraints).catch(function(err) {
    if (err instanceof validate.ValidationErrors) { // always true
        // In case of REST API it's 400 (Bad Request)
    } else {
        // Never execute
        // In case of REST API it's 500 (Server Error)
    }
});
@Jokero

This comment has been minimized.

Show comment
Hide comment
@Jokero

Jokero Jan 26, 2015

Contributor

@ansman, @sebastianseilund What do you think about it?

Contributor

Jokero commented Jan 26, 2015

@ansman, @sebastianseilund What do you think about it?

@Jokero Jokero referenced this issue Feb 6, 2015

Closed

Default values #16

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Feb 7, 2015

Owner

Could validate.js simply check if the rejected value is an exception?

Owner

ansman commented Feb 7, 2015

Could validate.js simply check if the rejected value is an exception?

@ansman ansman reopened this Mar 8, 2015

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Mar 8, 2015

Owner

I think I understand the need better now, I'll implement something better for 0.7.0

Owner

ansman commented Mar 8, 2015

I think I understand the need better now, I'll implement something better for 0.7.0

@ansman ansman added this to the Next milestone Apr 3, 2015

@ansman ansman added the enhancement label Apr 3, 2015

@ansman ansman closed this in db913c9 Apr 7, 2015

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Apr 7, 2015

Owner

@sebastianseilund Starting with the next major version validate.js should no longer swallow exceptions like it does now.

Now you can do what you asked for like so:

validate.async(attrs, constraints).then(
  function() { /* Success */ },
  function(errors) {
    if (errors instanceof Error) {
      // An exception was thrown
    } else {
      // The errors object will contain the validation errors
    }
  }
);
Owner

ansman commented Apr 7, 2015

@sebastianseilund Starting with the next major version validate.js should no longer swallow exceptions like it does now.

Now you can do what you asked for like so:

validate.async(attrs, constraints).then(
  function() { /* Success */ },
  function(errors) {
    if (errors instanceof Error) {
      // An exception was thrown
    } else {
      // The errors object will contain the validation errors
    }
  }
);
@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Apr 8, 2015

Owner

Released in 0.7.0

Owner

ansman commented Apr 8, 2015

Released in 0.7.0

@sebastianseilund

This comment has been minimized.

Show comment
Hide comment
@sebastianseilund

sebastianseilund Apr 9, 2015

Contributor

Thanks for looking into this, @ansman .

That doesn't solve the problem completely though. With your solution here we can check if the err is an instance of Error (blacklist), and if not treat it as validation errors. What I want to be able to do is to check if the err is an instance of validate.ValidationError (whitelist), and if not treat it as an application error. So, the other way around.

The problem is that many libraries will throw or reject with non Error objects. Either strings or custom objects. I think this is a bad thing™ to do, but it happens nonetheless. Users of validate.js do it, too, when they throw strings in async validators.

Here is a backwards compatible solution proposal for you:

AFAICT the validate.js documentation tells users to throw/reject with strings when the value is invalid. Change these lines to:

//...
if (!error) {
  v.warn("Validator promise was rejected but didn't return an error");
} else if (typeof error !== 'string') {
  throw error;
}

This way only strings are considered valid reject reasons for async validators. The user should be careful when using libraries that throw/reject with strings, and the readme could note this.

Next, give the errors object its own prototype, e.g. called validate.ValidationError. This way users can check in their rejection handler like this:

validate.async(attrs, constraints).then(
  function() { /* Success */ },
  function(errors) {
    if (errors instanceof validate.ValidationError) {
      // The errors object will contain the validation errors
    } else {
      // An exception was thrown
    }
  }
);

(This is your example from above, but with the if/else flipped around.).

Sidenote: I think it's a little weird that validate.js users are asked to reject with strings. One way to solve this whole issue would be to require users to throw instances of a new error class (named something like validate.InvalidValue). Example:

validate.validators.myAsyncValidator = function(value) {
  return validate.Promise(function(resolve, reject) {
    setTimeout(function() {
      if (value === "foo") resolve();
      else reject(new validate.InvalidValue("is not foo"));
    }, 100);
  });
};

This way you would be 100% certain about what is validation errors and what's not. But this wouldn't be backwardscompatible. Maybe for 1.0?

Let me know if I can be of any help.

Contributor

sebastianseilund commented Apr 9, 2015

Thanks for looking into this, @ansman .

That doesn't solve the problem completely though. With your solution here we can check if the err is an instance of Error (blacklist), and if not treat it as validation errors. What I want to be able to do is to check if the err is an instance of validate.ValidationError (whitelist), and if not treat it as an application error. So, the other way around.

The problem is that many libraries will throw or reject with non Error objects. Either strings or custom objects. I think this is a bad thing™ to do, but it happens nonetheless. Users of validate.js do it, too, when they throw strings in async validators.

Here is a backwards compatible solution proposal for you:

AFAICT the validate.js documentation tells users to throw/reject with strings when the value is invalid. Change these lines to:

//...
if (!error) {
  v.warn("Validator promise was rejected but didn't return an error");
} else if (typeof error !== 'string') {
  throw error;
}

This way only strings are considered valid reject reasons for async validators. The user should be careful when using libraries that throw/reject with strings, and the readme could note this.

Next, give the errors object its own prototype, e.g. called validate.ValidationError. This way users can check in their rejection handler like this:

validate.async(attrs, constraints).then(
  function() { /* Success */ },
  function(errors) {
    if (errors instanceof validate.ValidationError) {
      // The errors object will contain the validation errors
    } else {
      // An exception was thrown
    }
  }
);

(This is your example from above, but with the if/else flipped around.).

Sidenote: I think it's a little weird that validate.js users are asked to reject with strings. One way to solve this whole issue would be to require users to throw instances of a new error class (named something like validate.InvalidValue). Example:

validate.validators.myAsyncValidator = function(value) {
  return validate.Promise(function(resolve, reject) {
    setTimeout(function() {
      if (value === "foo") resolve();
      else reject(new validate.InvalidValue("is not foo"));
    }, 100);
  });
};

This way you would be 100% certain about what is validation errors and what's not. But this wouldn't be backwardscompatible. Maybe for 1.0?

Let me know if I can be of any help.

@ansman

This comment has been minimized.

Show comment
Hide comment
@ansman

ansman Apr 10, 2015

Owner

I don't like the idea of exceptions being commonplace, to me they are something that should happen only when there is a programming error and not a user error.

If you are using a library that rejects promises with a string rather than an error I suggest you do something like this:

validate.validators.someAsyncValidator = function(value) {
  return myAsyncFunction()
    .then(undefined, function(message) {
      throw new Error(message)
    });
};
Owner

ansman commented Apr 10, 2015

I don't like the idea of exceptions being commonplace, to me they are something that should happen only when there is a programming error and not a user error.

If you are using a library that rejects promises with a string rather than an error I suggest you do something like this:

validate.validators.someAsyncValidator = function(value) {
  return myAsyncFunction()
    .then(undefined, function(message) {
      throw new Error(message)
    });
};

ansman added a commit that referenced this issue May 21, 2015

@cspotcode

This comment has been minimized.

Show comment
Hide comment
@cspotcode

cspotcode Nov 24, 2015

Thanks for the nifty library!

The API inconsistency becomes even more apparent when using ES7 async functions.

// Sync API
var errs = validate(attrs, constraints);
if(errs != null) {
    console.log('validation failed: ' + errs.join());
}

// Async API is more complex
class ValidationErrors {
    constructor(errs) {this.messages = errs}
}
try {
    await validate.async(attrs, constraints, {
        wrapErrors: ValidationErrors
    });
} catch(errs) {
    if(!(errs instanceof ValidationErrors)) throw errs;
    console.log('validation failed: ' + errs.messages.join());
}

I don't like the idea of exceptions being commonplace, to me they are something that should happen only when there is a programming error and not a user error.

This is exactly why the async API should resolve with validation errors instead of rejecting with them. When attributes do not pass validation, it is not a programming error.

I know this is a breaking change, but perhaps it can happen in version 1.0? With a flag to restore the old behavior, so people have an easy migration path?

EDIT: fixed a bug in sample code

Thanks for the nifty library!

The API inconsistency becomes even more apparent when using ES7 async functions.

// Sync API
var errs = validate(attrs, constraints);
if(errs != null) {
    console.log('validation failed: ' + errs.join());
}

// Async API is more complex
class ValidationErrors {
    constructor(errs) {this.messages = errs}
}
try {
    await validate.async(attrs, constraints, {
        wrapErrors: ValidationErrors
    });
} catch(errs) {
    if(!(errs instanceof ValidationErrors)) throw errs;
    console.log('validation failed: ' + errs.messages.join());
}

I don't like the idea of exceptions being commonplace, to me they are something that should happen only when there is a programming error and not a user error.

This is exactly why the async API should resolve with validation errors instead of rejecting with them. When attributes do not pass validation, it is not a programming error.

I know this is a breaking change, but perhaps it can happen in version 1.0? With a flag to restore the old behavior, so people have an easy migration path?

EDIT: fixed a bug in sample code

@Jokero

This comment has been minimized.

Show comment
Hide comment
@Jokero

Jokero Nov 26, 2015

Contributor

I agree with @cspotcode. So async validators should return resolved with string promise for validation error. Example of exists validator:

var existsValidator = function(value, options) {
    if (validate.isEmpty(value)) {
        return Promise.resolve();
    }

    var model        = options.model;
    var field        = options.field;
    var errorMessage = options.message;

    return model.count({ [field]: value })
        .then(function(count) {
            return !count
                      ? Promise.resolve(errorMessage) // validation error
                      : Promise.resolve();            // validation is ok
        })
        .catch(function(err) {
            return Promise.reject(err); // application (database, for example) error
        });
};
Contributor

Jokero commented Nov 26, 2015

I agree with @cspotcode. So async validators should return resolved with string promise for validation error. Example of exists validator:

var existsValidator = function(value, options) {
    if (validate.isEmpty(value)) {
        return Promise.resolve();
    }

    var model        = options.model;
    var field        = options.field;
    var errorMessage = options.message;

    return model.count({ [field]: value })
        .then(function(count) {
            return !count
                      ? Promise.resolve(errorMessage) // validation error
                      : Promise.resolve();            // validation is ok
        })
        .catch(function(err) {
            return Promise.reject(err); // application (database, for example) error
        });
};
@Jokero

This comment has been minimized.

Show comment
Hide comment
@Jokero

Jokero Mar 6, 2017

Contributor

I created validy module (https://github.com/Jokero/validy) which by default returns validation errors as argument of fulfilled callback, so async/await code should look like this:

async function example() {
    try {
        const errors = await validy(book, schema);
        if (errors) {
            // you have validation errors ("errors" is plain object)
        } else {
            // no errors ("errors" is undefined)
        }
    } catch(err) {
        // application error (something went wrong)
    }
}
Contributor

Jokero commented Mar 6, 2017

I created validy module (https://github.com/Jokero/validy) which by default returns validation errors as argument of fulfilled callback, so async/await code should look like this:

async function example() {
    try {
        const errors = await validy(book, schema);
        if (errors) {
            // you have validation errors ("errors" is plain object)
        } else {
            // no errors ("errors" is undefined)
        }
    } catch(err) {
        // application error (something went wrong)
    }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment