express-json-validator-middleware
express-json-validator-middleware copied to clipboard
Allow custom Ajv instance (closes #115)
Previously, Validator would only take an object containing Ajv options. With this PR, we've added the possibility to pass a custom Ajv instance instead, for example one of the 2019-09 draft version[1]:
const Ajv2019 = require('ajv/dist/2019')
const ajv = new Ajv2019()
const validator = new Validator(ajv)
[1] https://ajv.js.org/json-schema.html#draft-2019-09
@pstadler Thanks for opening this PR — it looks great! I'm going to check out the branch and give it a manual test before merging.
Please can you add a description to this PR that references your original issue and explains the purpose of this feature?
done 👍🏻
Ping 👋🏻
@pstadler I've been quite overloaded recently, so haven't been able to give this the testing time it needs.
Thanks for being so patient, I'll do my best to pick it up soon!
no problem. sometimes people forget. take your time. 👍🏻
Manual testing
- [x] Test against a JavaScript application
- [x] With default Ajv instance
- [x] With custom Ajv instance
- [ ] Test against a TypeScript application
- [ ] With default Ajv instance
- [ ] With custom Ajv instance
@pstadler I've run into an issue while manually testing this PR, but I'm not quite sure what's going wrong:
I've created a minimal JavaScript application (code) and then installed ajv as an application dependency so I can create an Ajv instance:
const Ajv2019 = require("ajv/dist/2019");
const ajv = new Ajv2019();
When I run the application I receive the following error from Ajv:
Error: reference "https://json-schema.org/draft/2019-09/schema" resolves to more than one schema
Do you have any idea what's happening?
From what I can tell, this instanceof check isn't working: https://github.com/simonplend/express-json-validator-middleware/pull/116/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R11
I've confirmed that with a minimal reproduction too:
const Ajv2019 = require("ajv/dist/2019");
const AjvCore = require("ajv/dist/core").default;
const ajv = new Ajv2019();
console.log("Ajv2019 instanceof AjvCore:", Ajv2019 instanceof AjvCore);
// > Ajv2019 instanceof AjvCore: false
Just tried your sample app. It works for me:
$ curl http://localhost:3000/address -X POST -s | jq
{
"body": [
{
"instancePath": "",
"schemaPath": "#/required",
"keyword": "required",
"params": {
"missingProperty": "number"
},
"message": "must have required property 'number'"
}
]
}
$ curl http://localhost:3000/address -X POST -s \
-d '{"number":1,"street":"Infinite Loop","type":"Avenue"}' -H "content-type:application/json" | jq
{}
Your check is wrong, you need to replace:
console.log("Ajv2019 instanceof AjvCore:", Ajv2019 instanceof AjvCore);
with
console.log("Ajv2019 instanceof AjvCore:", ajv instanceof AjvCore);
Okay, I think the instaceof check could indeed fail with different ajv package versions and needs to be changed to check if the provided argument is any kind of class, not the bundled AjvCore.
Your check is wrong, you need to replace:
Good catch on the bug in my minimal reproduction. I made the replacement you suggested and it was true as expected.
Okay, I think the instaceof check could indeed fail with different ajv package versions and needs to be changed to check if the provided argument is any kind of class, not the bundled AjvCore.
How about making this check instead?
if (typeof ajvOptions?.compile === "function") {
This seems to be the right approach. Change amended. Please note that eslint ecmaVersion: 2020 is required for optional chaining.
Hey @pstadler, I think this PR is almost ready to merge - let me know if there's anything I can do to help you wrap it up.
@pstadler Any chance you'd be up for wrapping up this PR? No problem if not, I can just close it if you don't have the time or inclination.