Promises and rejections

[sebastianseilund]

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]

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]

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]

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]

@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]

@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]

@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]

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

[Jokero]

@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]

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

[ansman]

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

[ansman]

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