Why not return errors immediately after validation? (and remove ajv.errors)

[nielskrijger]

Thanks for the great library! I've recently started migrating from tv4 to ajv, primarily because of the promised speed improvements. Thus far AJV appears to be very feature complete and I like pre-registering all schemas in a single instance beforehand; keeps my schemas managed centrally in the codebase which is nice.

I noticed one thing that might be a cause of tricky and hard to reproduce bugs when using the library improperly.

When accessing ajv.errors (or ajv.errorsText I presume as well) in a callback, promise or any tick after the one in which you've called ajv.validate there is no guarantee that ajv.errors contains the set of errors you might expect.

Example:

var async = require('async');
var Ajv = require('ajv');

var ajv = new Ajv();
ajv.addSchema({'type': 'string'}, 'testSchema');

function test(name, timeout, data, done) {
  var valid = ajv.validate('testSchema', data);
  console.log(name, 'valid =', valid + ', ajv.errors =', ajv.errors);
  setTimeout(function() {
    console.log(name + ' timeout', ' valid =', valid + ', ajv.errors =', ajv.errors);
    done();
  } , timeout);
}

async.parallel([
  function(cb) {
    test('Test 1', 100, false, cb);
  },
  function(cb) {
    test('Test 2', 50, 'test', cb);
  },
]);

This outputs:

Test 1 valid = false, ajv.errors = [ { keyword: 'type', dataPath: '', message: 'should be string' } ]
Test 2 valid = true, ajv.errors = null
Test 2 timeout  valid = true, ajv.errors = null
Test 1 timeout  valid = false, ajv.errors = null

Notice in the last line Test 1 is not valid but contains no errors anymore.

The cause is fairly obvious but it is an easy mistake made given how the library currently works. Why not have var valid = ajv.validate('testSchema', data); return a result object containing errors, errorText etc. (a sort of ValidationResult object)?

Another option to avoid misuse of the function is to rename ajv.errors to ajv.lastErrors. Not that I like that name, but it makes its intended use more clear.

I understand these changes would be backwards incompatible, but it might be worth a thought if planning a big new release.

But overall; thanks for the great library! 👍

[epoberezkin]

Thank you very much for the feedback!

When accessing ajv.errors (or ajv.errorsText I presume as well) in a callback, promise or any tick after the one in which you've called ajv.validate there is no guarantee that ajv.errors contains the set of errors you might expect.

That is correct, the errors are overwritten every time ajv.validate is called. If you plan using these errors you need to copy the reference to some var in the same tick before the next validation is called.

Why not have var valid = ajv.validate('testSchema', data); return a result object containing errors, errorText etc. (a sort of ValidationResult object)?

For performance reasons - it would be slower. In most validation calls the data is valid so you don't need to access errors. Creating a wrapper object every time you need to return the result makes it substantially slower.

Also, you may have seen in readme that although there is ajv.validate method for convenience, it is faster using precompiled validating function (ajv.validate calls it anyway but it has the cost of the lookup plus an extra function call, so if the schema is large the difference can be negligible but on the small schemas it can be more than twice faster). If you use one ajv instance per app with all schemas in one place it could be more convenient to use ajv.getSchema method (that accepts the schema ref) than ajv.compile(that needs the schema itself) - they both return the same compiled schema function (it only gets compiled once in all cases).

Another option to avoid misuse of the function is to rename ajv.errors to ajv.lastErrors. Not that I like that name, but it makes its intended use more clear.

I understand these changes would be backwards incompatible, but it might be worth a thought if planning a big new release.

You are right here. Maybe on the next major version change... I will make it clearer in readme that ajv.errors (and validate.errors) are overwritten and have to be used in the same tick.

Thanks again

[epoberezkin]

I added note in readme. errors is the same property name that is used in other validators returning boolean rather than the object (e.g., is-my-json-valid and in jsen), so I will keep it like this for consistency sake.

[nielskrijger]

Fair enough. Thanks for the explanation.
Good work!

[remyrylan]

This library and all the work by @epoberezkin is amazing, but this is a strange deviation from what most users expect. I'd love to submit a pull request if you're open to any of these suggestions:

1. Allow .validate(data) to accept a second parameter which is a callback in the typical node.js style of (err, data) => void... so if err = null then you know your validation was successful.

2. Allow .validate(data) to either return true or an instance of Error, then users would simply be able to do something like this following:

const result = schema.validate(data)
if (result === true) {
  console.log('valid schema')
} else if (result instanceof Error) {
  console.log(result)
}

[epoberezkin]

Please see https://github.com/epoberezkin/ajv/blob/master/FAQ.md#ajv-api-for-returning-validation-errors

[remyrylan]

@epoberezkin Apologies for the noise of the notification, my fault for missing the FAQ -- you've definitely had to address the issue plenty of times.

Looks like the async validation is exactly what I need. Thank you so much!

[epoberezkin]

Just be aware that async requires a non standard keyword in the schema at the moment.