Objection.js: Async validation

[joaonice]

Hi!

One of the greatest features of AjvValidator is the async schema validation and currently objection.js doesn't implement it. Is there any plans to implement model async validation?

Ref #113

[fl0w]

I don't think this'll get implemented unless koskimas changes his mind on marrying Ajv with objection. But I would rather, personally, see this as a plugin.

[koskimas]

I'd really like to implement this, but unfortunately it's a little difficult because of the way things are currently done:

Currently models are validated synchronously when they are created. For example when you do this:

const person = Person.fromJson({firstName: 'Foo'});

If we were to make that asynchronous, alot of code would break and everything from that line forward would need to be async also. I don't think fromJson is very widely used, but internally it is used a lot. We simply cannot make it async or we would need to rewrite a large part of objection and it would have severe performance implications in the (most usual) synchronous case.

We could move the validation closer to where it is absolutely needed (just before insert and update), but that would also break a lot (?) of code. I've seen people doing things like this:

app.post('/people', (req, res) => {
  const person = Person.fromJson(req.body);

  // Do stuff assuming `fromJson` didn't throw and validation passed.
})

Another option would be to do validation in two places

1. in fromJson when the validation is synchronous
2. in insert and update implementations when async

But that would be kind of hacky and could cause confusion.

What should we do?

A plugin could be a good solution like @fl0w suggested. As a separate module the validation could be done in a different phase and it wouldn't be as confusing. People would learn it when they start using the plugin.

[fl0w]

removed because stupidity

[koskimas]

@fl0w I'm sorry, I didn't understand your point. Could you elaborate?

[fl0w]

Yea, no - I'm sorry.

Internally (and correct me if I'm off), fromJson is called for each row fetched by knex (sooner or later)? Having it being async would be real world performance disaster. But the question is if, when objection is constructing models internally, does it ever really need to do any validation?

If validation wasn't part of fromJson and instead only done in lifecycle hooks, wouldn't async validation be easier to implement?

[fl0w]

And I just realised that's pretty much what you said, but more eloquently - but that it would possibly break allot of code. You know what, I'll show myself out for tonight.

[lehni]

I agree, this would a be a great feature to have... A good use-case is in the documentation of AJV itself, look for checkIdExists() in the docs about Asynchronous validation

Since JSON schema validations that are to run asynchronously are marked with $async = true (also on the compiled function), you could cache this value per model class, use the old synchronous behaviour and preserve current behaviour if it is false, and use promises when it's true.

But since this is so deeply entwined with the code-base, it definitely sounds like a non-trivial change...

[lehni]

I think there really are two kinds of validations, one basic one that checks values to be correct, not empty, in the right range, of the right type, etc. This is how validation appears to have been used in objection.js so far. From a quick scan of the code-base, it does look like this happens in too many places, since it could probably be assumed that data coming from the database doesn't need to be validated again?

The more complex validations are the ones that need to check context as well, e.g. for a name to be unique in a collection, or a file to not exist, etc, and these will mostly be async.

Perhaps it makes sense to have two schemas? the current jsonSchema that more or less describes the database schema, and an optional asyncSchema for more complex checks that are allowed to run asynchronously? This async validation then should be run before insert and update also.

$validate() could then return a promise only if asyncSchema is defined, or yo decide to break compatibility and always returns a promise for simplicity...

And if this manual splitting of schemas sounds too crazy, here another thought: When defining additional keywords and formats in AJV, you need to mark them as async if they need to run asynchronously. It would therefore fairly easy to scan the jsonSchema and figure out if any property is async or not. The splitting into two versions of the schema could happen programmatically from one jsonSchema description, based on this information.

This would be the cleanest approach probably, and the one that wouldn't break any existing code or cause much confusion.

If $ref-type references are used between the schemas of two models (see https://github.com/Vincit/objection.js/issues/549#issuecomment-336055117), then they too could split into sync and async schema versions, simply by prefixing the stored schema's ids with _sync and _async (something that would be done on all schemas) and using that automatically in the $ref properties as an added prefix when splitting the schemas.

[koskimas]

I'm going to say no to this for now.

To create async validation keywords with ajv, you need to

1. Setup a custom AjvValidator
2. Define the async keyword using addKeyword method.

You can just as easily add the async validation stuff to $beforeInsert and $beforeUpdate hooks.

The other reason is that there really is no clean way to support async validation with the way objection validates stuff.