Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add async schema validation support #159

Merged
merged 5 commits into from
May 2, 2017

Conversation

diastremskii
Copy link
Contributor

@diastremskii diastremskii commented Apr 22, 2017

Summary

This PR aims to add support of asynchronous schema validation. Example of async schema:

var Ajv = require('ajv');
var ajv = Ajv({allErrors: true});

var request = require("request");

ajv.addFormat('twitter', {
    async: true,
    validate: (userName) => new Promise((resolve, reject) => {
        request.get('https://twitter.com/' + userName, (err, res) => {
            if (err) reject(err);
            else resolve(res.statusCode == 200)
        });
    })
});

var schema = {
    "$async": true,
    "properties": {
        "twitter_username": {
            "type": "string",
            "format": "twitter"
        }
    }
};

Example from https://runkit.com/esp/ajv-asynchronous-validation

Other Information

I guess potentially it could break something, because tests were written in synchronous try-catch way. But Feathers.js executes hooks in async manner, so this should be fine 🤔

@eddyystop
Copy link
Collaborator

The test module changes in the PR look like they check 'test succeeds unexpectedly' only if the validation throws instead of if it succeeds.

Further, texts involving the success and fail of a genuine async schema are needed.

I don't think I should review such a sensitive code change without first knowing that it looks like it works. I can look at this further once the tests are improved.

@diastremskii
Copy link
Contributor Author

Oh, ok, sorry. Changed the schemas validation to less general, but backwards compatible way. Schema is considered async if it contains "$async: true" property, and in this case validateSchema returns promise. Otherwise schema validated in same way as before.
Also covered async validation with tests.

@eddyystop
Copy link
Collaborator

I think that's a good way to go. I review as soon as I can.

@eddyystop
Copy link
Collaborator

@beeplin Would you mind commenting on this issue and code? Thanks.

@beeplin
Copy link
Contributor

beeplin commented May 2, 2017

didn't check details about the errors handling, but according to the new test, it should be working fine. And this new feature is definitely a must-have. :)

@eddyystop
Copy link
Collaborator

This will be published with v3.2.1.

@eddyystop eddyystop merged commit 0fc410e into feathersjs-ecosystem:master May 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants