express-json-validator-middleware icon indicating copy to clipboard operation
express-json-validator-middleware copied to clipboard

Allow custom Ajv instance (closes #115)

Open pstadler opened this issue 3 years ago • 15 comments

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 avatar Aug 12 '22 13:08 pstadler

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

simonplend avatar Aug 15 '22 13:08 simonplend

done 👍🏻

pstadler avatar Aug 15 '22 13:08 pstadler

Ping 👋🏻

pstadler avatar Oct 03 '22 14:10 pstadler

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

simonplend avatar Oct 04 '22 15:10 simonplend

no problem. sometimes people forget. take your time. 👍🏻

pstadler avatar Oct 04 '22 16:10 pstadler

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

simonplend avatar Oct 26 '22 18:10 simonplend

@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

(Full stack trace)

Do you have any idea what's happening?

simonplend avatar Oct 26 '22 19:10 simonplend

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

simonplend avatar Oct 26 '22 19:10 simonplend

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
{}

pstadler avatar Oct 26 '22 19:10 pstadler

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);

pstadler avatar Oct 26 '22 20:10 pstadler

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.

pstadler avatar Oct 26 '22 21:10 pstadler

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") {

simonplend avatar Oct 27 '22 01:10 simonplend

This seems to be the right approach. Change amended. Please note that eslint ecmaVersion: 2020 is required for optional chaining.

pstadler avatar Oct 27 '22 11:10 pstadler

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.

simonplend avatar Dec 23 '22 22:12 simonplend

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

simonplend avatar Apr 06 '24 21:04 simonplend