Allow custom Ajv instance (closes #115) #116

pstadler · 2022-08-12T13:29:57Z

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

simonplend · 2022-08-15T13:18:30Z

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

pstadler · 2022-08-15T13:31:50Z

done 👍🏻

pstadler · 2022-10-03T14:10:41Z

Ping 👋🏻

simonplend · 2022-10-04T15:19:21Z

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

pstadler · 2022-10-04T16:13:03Z

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

simonplend · 2022-10-26T18:47:39Z

Manual testing

Test against a JavaScript application
- With default Ajv instance
- With custom Ajv instance
Test against a TypeScript application
- With default Ajv instance
- With custom Ajv instance

simonplend · 2022-10-26T19:35:35Z

@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 · 2022-10-26T19:57:38Z

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

pstadler · 2022-10-26T19:59:31Z

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 · 2022-10-26T20:04:15Z

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 · 2022-10-26T21:19:02Z

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.

simonplend · 2022-10-27T01:35:47Z

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

pstadler · 2022-10-27T11:03:46Z

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

simonplend · 2022-10-28T04:11:06Z

package.json

@@ -23,7 +23,7 @@
 	"scripts": {
 		"test": "npm run test:unit && npm run test:types",
 		"test:unit": "tap",
-		"test:types": "tsc --noEmit --strict src/index.d.ts",
+		"test:types": "tsc --noEmit --strict --target ES2018 --moduleResolution node --resolveJsonModule src/index.d.ts",


Should the --target value here be updated to match the ecmaVersion in the ESLint config (2020)?

I'm not clear why --moduleResolution node and --resolveJsonModule are needed here. Could you please explain?

simonplend · 2022-10-28T04:34:11Z

src/index.d.ts

-import Ajv, { ErrorObject, Options as AjvOptions } from "ajv";
+import { ErrorObject, Options as AjvOptions } from "ajv";
+import AjvCore from "ajv/lib/core";


I've been testing this PR with a TypeScript application and I've run into a type error, which I think is caused by this using the AjvCore type instead of the Ajv type. Official Ajv plugins use the Ajv type, for example here in ajv-formats.

With this TypeScript code:

import addFormats from "ajv-formats"; import { Validator, ValidationError } from "express-json-validator-middleware"; const validator = new Validator({ coerceTypes: true, }); addFormats(validator.ajv, ["date-time"]) .addKeyword("kind") .addKeyword("modifier");

I receive this type error when running tsc:

src/lib/middleware/validation/index.ts:9:12 - error TS2345: Argument of type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/express-json-validator-middleware/node_modules/ajv/lib/core").default' is not assignable to parameter of type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/ajv-formats/node_modules/ajv/dist/core").default'. Types of property 'opts' are incompatible. Type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/express-json-validator-middleware/node_modules/ajv/lib/core").InstanceOptions' is not assignable to type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/ajv-formats/node_modules/ajv/dist/core").InstanceOptions'. Type 'InstanceOptions' is not assignable to type 'CurrentOptions'. Types of property 'keywords' are incompatible. Type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/express-json-validator-middleware/node_modules/ajv/lib/types/index").Vocabulary | undefined' is not assignable to type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/ajv-formats/node_modules/ajv/dist/types/index").Vocabulary | undefined'. Type 'Vocabulary' is not assignable to type '(string | KeywordDefinition)[]'. Type 'string | import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/express-json-validator-middleware/node_modules/ajv/lib/types/index").KeywordDefinition' is not assignable to type 'string | import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/ajv-formats/node_modules/ajv/dist/types/index").KeywordDefinition'. Type 'CodeKeywordDefinition' is not assignable to type 'string | KeywordDefinition'. Type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/express-json-validator-middleware/node_modules/ajv/lib/types/index").CodeKeywordDefinition' is not assignable to type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/ajv-formats/node_modules/ajv/dist/types/index").CodeKeywordDefinition'. Types of property 'code' are incompatible. Type '(cxt: import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/express-json-validator-middleware/node_modules/ajv/lib/compile/validate/index").KeywordCxt, ruleType?: string | undefined) => void' is not assignable to type '(cxt: import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/ajv-formats/node_modules/ajv/dist/compile/validate/index").KeywordCxt, ruleType?: string | undefined) => void'. Types of parameters 'cxt' and 'cxt' are incompatible. Type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/ajv-formats/node_modules/ajv/dist/compile/validate/index").KeywordCxt' is not assignable to type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/express-json-validator-middleware/node_modules/ajv/lib/compile/validate/index").KeywordCxt'. The types of 'gen._scope' are incompatible between these types. Type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/ajv-formats/node_modules/ajv/dist/compile/codegen/scope").Scope' is not assignable to type 'import("/home/simonplend/dev/github/simonteaching/api-project/node_modules/express-json-validator-middleware/node_modules/ajv/lib/compile/codegen/scope").Scope'. Property '_names' is protected but type 'Scope' is not a class derived from 'Scope'. 9 addFormats(validator.ajv, ["date-time"]) ~~~~~~~~~~~~~

Can this be changed back to importing the Ajv type, and then that type be used in the constructor instead of AjvCore?

simonplend

This looks great overall! There are two things to mention:

I've tested these changes with a JavaScript application and it works as expected, but I'm getting a type error when testing it with a TypeScript application (see my comment on the types).
Please add an explanation on how to set a custom Ajv instance to the beginning of the Ajv instance section in the README. A short description and the code example from the PR description should suffice.

simonplend · 2022-12-23T22:53:23Z

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 · 2024-04-06T21:42:16Z

@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 changed the title ~~allow custom Ajv instance (closes #115)~~ Allow custom Ajv instance (closes #115) Aug 18, 2022

simonplend mentioned this pull request Aug 30, 2022

Make options object optional for Validator constructor #114

Open

allow custom Ajv instance (closes simonplend#115)

bd6c04d

pstadler force-pushed the allow-custom-ajv-instance branch from 65853b0 to bd6c04d Compare October 27, 2022 11:03

simonplend reviewed Oct 28, 2022

View reviewed changes

simonplend requested changes Oct 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow custom Ajv instance (closes #115) #116

Allow custom Ajv instance (closes #115) #116

pstadler commented Aug 12, 2022 •

edited

simonplend commented Aug 15, 2022

pstadler commented Aug 15, 2022

pstadler commented Oct 3, 2022

simonplend commented Oct 4, 2022

pstadler commented Oct 4, 2022

simonplend commented Oct 26, 2022 •

edited

simonplend commented Oct 26, 2022 •

edited

simonplend commented Oct 26, 2022

pstadler commented Oct 26, 2022 •

edited

pstadler commented Oct 26, 2022 •

edited

pstadler commented Oct 26, 2022

simonplend commented Oct 27, 2022 •

edited

pstadler commented Oct 27, 2022

simonplend Oct 28, 2022

simonplend Oct 28, 2022

simonplend Oct 28, 2022

simonplend left a comment

simonplend commented Dec 23, 2022

simonplend commented Apr 6, 2024

Allow custom Ajv instance (closes #115) #116

Are you sure you want to change the base?

Allow custom Ajv instance (closes #115) #116

Conversation

pstadler commented Aug 12, 2022 • edited

simonplend commented Aug 15, 2022

pstadler commented Aug 15, 2022

pstadler commented Oct 3, 2022

simonplend commented Oct 4, 2022

pstadler commented Oct 4, 2022

simonplend commented Oct 26, 2022 • edited

simonplend commented Oct 26, 2022 • edited

simonplend commented Oct 26, 2022

pstadler commented Oct 26, 2022 • edited

pstadler commented Oct 26, 2022 • edited

pstadler commented Oct 26, 2022

simonplend commented Oct 27, 2022 • edited

pstadler commented Oct 27, 2022

simonplend Oct 28, 2022

Choose a reason for hiding this comment

simonplend Oct 28, 2022

Choose a reason for hiding this comment

simonplend Oct 28, 2022

Choose a reason for hiding this comment

simonplend left a comment

Choose a reason for hiding this comment

simonplend commented Dec 23, 2022

simonplend commented Apr 6, 2024

pstadler commented Aug 12, 2022 •

edited

simonplend commented Oct 26, 2022 •

edited

simonplend commented Oct 26, 2022 •

edited

pstadler commented Oct 26, 2022 •

edited

pstadler commented Oct 26, 2022 •

edited

simonplend commented Oct 27, 2022 •

edited