Bump AJV to v8 #713

JacobLey · 2022-03-19T19:07:22Z

Partially resolves #573 + #582

Unblocks path to 3.1 support

Appears to be fully backwards compatible (see notes about SerDes + Validation errors).

Happy to discuss changes made/justifications. I would also like to append a prettier commit to the end of this, but omitting for now to prevent noise. Perhaps once approved or as a follow on PR

hide it from AJV (unknown keyword)

gitguardian · 2022-03-19T19:07:58Z

⚠️ GitGuardian has uncovered 6 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	Secret	Commit	Filename
-	Generic High Entropy Secret	`95f635c`	test/resources/boba.yaml	View secret
-	Generic High Entropy Secret	`95f635c`	test/resources/boba.yaml	View secret
-	Generic High Entropy Secret	`95f635c`	test/resources/boba.yaml	View secret
-	Generic High Entropy Secret	`7f13a14`	test/resources/boba.yaml	View secret
-	Generic High Entropy Secret	`7f13a14`	test/resources/boba.yaml	View secret
-	Generic High Entropy Secret	`7f13a14`	test/resources/boba.yaml	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

JacobLey · 2022-03-19T19:08:31Z

src/framework/ajv/index.ts

    allErrors: true,
-    meta: draftSchema,


implied by ajv-draft-04

JacobLey · 2022-03-19T19:10:57Z

src/framework/ajv/index.ts

  ajv.removeKeyword('propertyNames');
  ajv.removeKeyword('contains');
  ajv.removeKeyword('const');
+  ajv.addKeyword({


Any of these keywords without explicit validation logic will auto-resolve to true.

path and components are added because it is passed to AJV as a schema in places like this: https://github.com/cdimascio/express-openapi-validator/blob/master/src/middlewares/openapi.request.validator.ts#L274

JacobLey · 2022-03-19T19:14:56Z

src/framework/ajv/index.ts

          if (sch) {
-            return function validate(data, path, obj, propName) {
+            if (sch.kind === 'res') {


See handleSerDes, this logic ensures that the deserializer (and equivelent serializer for res) will only run on proper schema.

Also considered splitting keyword into two forms x-eov-req-serdes and x-eov-res-serdes.

JacobLey · 2022-03-19T19:16:00Z

src/framework/ajv/options.ts

-      }, {}),
-      serDesMap: serDesMap,
+      validateFormats: !!validateFormats,
+      formats: <Exclude<typeof formats, unknown[]>>formats,


Guaranteed to be {}, see normalizeOptions

Perhaps would benefit from a NormalizedOptions interface that extends Options?

JacobLey · 2022-03-19T19:16:50Z

src/framework/ajv/options.ts

-        return acc;
-      }, {}),
-      serDesMap: serDesMap,
+      validateFormats: !!validateFormats,


fast + full will be truthy, then handled via addFormats (see ajvFormatsMode)

JacobLey · 2022-03-19T19:17:31Z

src/framework/openapi.schema.validator.ts

-    const v = new Ajv(options);
-    v.addMetaSchema(draftSchema);
+    const v = new AjvDraft4(options);
+    addFormats(v, ['email', 'regex', 'uri', 'uri-reference']);


These are the only formats referenced in spec. Can append all if desired but figured it wasn't necessary?

JacobLey · 2022-03-19T19:19:59Z

src/framework/types.ts

-  type?: 'number' | 'string';
-  validate: (v: any) => boolean;
-};
+export type Format =


type is necessary for AJV.

Functions typed with any are still valid (because that is how typescript works) but otherwise type must match.

Not sure if this is considered breaking... if in doubt could default the value to string and log a warning?

Actually AJV allows passing options like { foo: x => true } (see formats alwaysTrue so could use that in the case of missing type

JacobLey · 2022-03-19T19:21:03Z

src/framework/types.ts

+  /**
+   * @deprecated
+   * Use `formats` + `validateFormats` to ignore specified formats
+   */
  unknownFormats?: true | string[] | 'ignore';


unknownFormats: the same can be achieved by passing true for formats that need to be ignored via ajv.addFormat or formats option.

JacobLey · 2022-03-19T19:22:04Z

src/framework/types.ts

  unknownFormats?: true | string[] | 'ignore';
  serDes?: SerDes[];
-  formats?: Format[];
+  formats?: Format[] | Record<string, ajv.Format>;


This updated type better matches AJV's expected input.

I recommend deprecating the array version

i agreed. array version can be deprecated. can you also add this to the wiki

(note i moved the doc from README to wiki)

JacobLey · 2022-03-19T19:23:05Z

src/framework/types.ts

  fileUploader?: boolean | multer.Options;
  multerOpts?: multer.Options;
  $refParser?: {
    mode: 'bundle' | 'dereference';
  };
  operationHandlers?: false | string | OperationHandlerOptions;
-  validateFormats?: false | 'fast' | 'full';
+  validateFormats?: boolean | 'fast' | 'full';


fast and full should be deprecated...
perhaps a new option instead addFormatsOptions?

JacobLey · 2022-03-19T19:24:28Z

src/middlewares/openapi.request.validator.ts

@@ -191,7 +191,7 @@ export class RequestValidator {
      } else {
        throw new BadRequest({
          path: req.path,
-          message: `'${property}' should be equal to one of the allowed values: ${options
+          message: `'${property}' must be equal to one of the allowed values: ${options


I personally don't consider should -> must a breaking change.
Although multiple tests had to be updated to accommodate

The gist of the error is the same, and is "more correct" as MUST implies it is required, vs SHOULD implies recommendation (but not necessarily enforced)

JacobLey · 2022-03-19T19:31:49Z

src/middlewares/parsers/schema.preprocessor.ts

@@ -348,16 +352,71 @@ export class SchemaPreprocessor {

  private handleSerDes(
    parent: SchemaObject,
-    schema: SchemaObject,
+    schema: SchemaObject & { _serDesInternal?: boolean },


This function got a lot bigger... but it was the only way I was able to maintain backwards compatibility.

https://ajv.js.org/guide/modifying-data.html

The gist of the problem is that the x-eov-serdes modifies data. And AJV does not make any guarantees about the order of validations, with the exception of composite types like allOf

So the previous version of this function might accidentally skip validations like format or pattern because the string -> object deserialization happens first. So this new schema ensures that those validations happen, THEN deserialisation happens for requests (and vice versa for responses)

The second issue is that while the handleSerDes gets called for both req+res, it gets passed the same schema object. So any manipulation affects both. So x-eov-serdes handling was updated to always reject when kind does not match.

The nullable handling was added because AJV requires a type keyword alongside (which was removed from base schema). OpenAPI doesn't allow type: 'null' but AJV does just fine.

Biggest issue with this is that validation errors are much noisier. As there are messages for each oneOf failure, as well as oneOf itself.

So apparently AJV _does_ have some ability to enforce keyword ordering via `before`/`post`! Using those options, serdes schema gets a lot simpler and has more trivial error redacting

JacobLey · 2022-04-06T20:52:49Z

src/middlewares/parsers/schema.preprocessor.ts

@@ -356,8 +392,20 @@ export class SchemaPreprocessor {
      !!schema.format &&
      this.serDesMap[schema.format]
    ) {
-      (<any>schema).type = [this.serDesMap[schema.format].jsonType || 'object', 'string'];
-      schema['x-eov-serdes'] = this.serDesMap[schema.format];
+      const serDes = this.serDesMap[schema.format];


Yet another serdes refactor... pretty sure that is the last time!

So the original reasoning for my complicated schema was I thought AJV did not provide any way to enforce keyword ordering (as internal keywords do not have "guaranteed" order). But custom keywords have some capability via before and post!

So the final schema looks a lot more like the original, simpler version. Still some small handling had to be done for type + nullable but not nearly as gross as before.

This is great news, Nice find

cdimascio

this looks good. i'm considering merging shortly. is there anything else worth considering prior to doing so?

JacobLey · 2022-04-24T14:11:06Z

I don't have anything additional to add to this PR, unless someone sees something in the PR.

npm test passes for me locally so I think it is functional... Happy to see it merged!

I know you mentioned possibly doing a major version bump just out of caution. I'll continue to defer to you to that as it isn't strictly necessary, but might be a good chance to call out "lots of internal stuff changed, use some caution". Might still be good to wait until 3.1 support to actually implement the major bump (if necessary)

cdimascio · 2022-05-29T17:32:46Z

thanks @JacobLey

the PR is now merged, you can try it out using:

npm install express-openapi-validator@4.14.0-beta.1

will make version this the currentrelease version after a bit of soak time.

cdimascio · 2022-05-30T17:26:50Z

@JacobLey i created a new branch - v4.14.0-beta to host this code. i'm seeing a potential performance issue. tests are running a few seconds slower.

cdimascio · 2022-05-30T17:54:44Z

i don't see any obvious change that would cause the slowdown. clearly the internals have changed extensively with ajv8 and the external schema. that said, my expectation is that something else is at play.

the test are approximately twice as slow with these changes (v4.14.0-beta) to compared to 4.13.8 (on branch mainline)

branches:

v4.14.0-beta (new ajv8 code)
master (new ajv8 code -- currently equivalent to the v4.14.0-beta branch)
mainline (ajv6 code, 4.13.8)

cdimascio · 2022-05-30T17:55:02Z

@JacobLey any thoughts on what might be the cause?

JacobLey · 2022-05-30T18:31:07Z

I don't think any of the code I wrote should have significant performance implications... perhaps some of the custom keyword/serdes logic but that ended up relatively unchanged

So I suspect AJV v8 is the culprit, and the internal compilation/validation logic is somehow slower.

How are you benchmarking these comparisons? Do you have a particular tool? Or just checking out the branch and seeing how long tests take to execute?

Are there particularly slow tests? Most of the example schemas are pretty simple... 2x is definitely unexpectedly poor performance from AJV. It might be good to know if the performace comes from schema "compilation" (happens once at first request, performance only important to something like Lambda users) and "validation" (happens every request, important to everyone)

Unfortunately I can't find a good existing benchmark between AJV versions, only AJV and other validators.

cdimascio · 2022-05-30T18:36:21Z

I haven’t dug in too deeply, thus far the it’s effectively human observation, that is I notice the mainline tests e.g. 4.13.8 complete in about 5 seconds (on my MacBook Pro) while the v4.14.0-beta.2 branch tests run in about 9 seconds.

JacobLey · 2022-05-30T20:29:13Z

An attempt to put some hard numbers here... Adds a "listener" to test suites to track time, then reports at end (very hacky, some tests fail because they expect callbacks but those are acceptable casualities)

https://github.com/JacobLey/express-openapi-validator/tree/compile-v-validate

Hoping to:

Show time elapse differences
Highlight compile vs validate performance

Executed on master (AJV v8)

TOTAL SETUP TIME 106
{ compiles: 6904, validates: 2237, diff: 4667 }

Executed on mainline (AJV v6)

TOTAL SETUP TIME 103
{ compiles: 4378, validates: 1701, diff: 2677 }

Every run comes in a bit different... but those seem like good ballpark numbers. Assuming that added tests between two branches is negligable... It appears compiling is ~75% slower on v8, and validation is ~20%

cdimascio · 2022-06-04T15:59:49Z

Some info on ajv8 perf
ajv-validator/ajv#1386

and more info on migration to 8

cdimascio · 2022-06-04T16:21:39Z

Compiles are slower in ajv8. We could offer an option to disable optimization which should gain back 30% at compile time, express openapi validator runs the compile step once when the middleware is initialized. Hence this is not an issue for long lived apis, particularly since ajv8 provides improved safety

It may also be worth investigating stand-alone mode

cdimascio · 2022-06-04T16:24:15Z

Finally we should verify that the two validate ops are still just as performant https://github.com/cdimascio/express-openapi-validator/blob/master/src/middlewares/openapi.request.validator.ts#L174

this way we know that per req latency is just as good (or perhaps better)

JacobLey · 2022-06-05T23:36:28Z

Standalone mode is very interesting... Would require some extra build step, but should hopefully solve most of that lambda startup time. Would probably have some sort of CLI/method that takes the OpenAPI document, and writes a file with path + method mapping to a validation function, which is then loaded by the live server.

Disabling optimization sounds like a decent compromise for lambda/cloud function users that really care about the time-to-first-validation (should be opt-in).

I tried to benchmark the per-request validation-only performance above. I was not incredibly clear, just trying to get it knocked out quickly. That was the validates property which did see something like a ~20% slowdown. Turning off optimization would probably be worse.

I think long term we do want to adopt Ajv V8 (and future major versions). Not upgrading due to performance concerns is probably not a viable solution. Ajv still generally boasts the fastest validation times, so not any great alternatives (to my knowledge).

So this might justify putting this in a major version (already considered due to changes) bump, so we can warn that performance might take a hit, which to some might be seen as a breaking change.

nd-jharn · 2022-10-17T11:51:26Z

Any progress here? Will you be releasing this code as an update?

Essential Randomness and others added 17 commits August 30, 2021 18:58

try upgrading to OAPIv3.1

95f635c

Merge branch 'master' into ajvV8

5f9e3cd

Remove 3.1-support related files

7f13a14

Const typings on formats

2507329

Set _discriminator as non-enumerable

c201f65

hide it from AJV (unknown keyword)

Refactor x-eov-serdes to ensure order of validation

3f4bc1a

Update AJV options handling

2b06654

Update read/write only keywords

6bc5ce4

Add noop keywords

5bd09d7

Use AJV Draft 4 to validate OpenAPI doc

c09e11a

Use must keyword to match AJV validations

ec9cf15

Expected validation errors prefer must over should, / over .

bb092d4

Update README to reflect expected validation errors

8790c22

Explicitly pass formats to ignore

1c3a774

Serdes validation errors contain more errors

c85c40e

Update example with expected AJV errors

4117df7

Drop noisy test logs

dc27f9d

JacobLey commented Mar 19, 2022

View reviewed changes

src/framework/ajv/index.ts

allErrors: true,

meta: draftSchema,

Copy link

Collaborator Author

JacobLey Mar 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implied by ajv-draft-04

JacobLey commented Mar 19, 2022

View reviewed changes

Refactor serdes (again...) to use keyword execution order

1938497

So apparently AJV _does_ have some ability to enforce keyword ordering via `before`/`post`! Using those options, serdes schema gets a lot simpler and has more trivial error redacting

JacobLey commented Apr 6, 2022

View reviewed changes

cdimascio reviewed Apr 23, 2022

View reviewed changes

cdimascio approved these changes May 29, 2022

View reviewed changes

v4.14.0-beta.1

56075cc

cdimascio merged commit 2b27332 into cdimascio:master May 29, 2022

thernstig mentioned this pull request May 30, 2022

OpenAPI 3.1 support #573

Closed

wparad mentioned this pull request May 30, 2022

Upgrade to Ajv v8 Authress-Engineering/openapi-data-validator.js#4

Open

nd-jharn mentioned this pull request Oct 17, 2022

Support for OpenAPI 3.1.0 #755

Closed

snyk-bot mentioned this pull request Nov 20, 2022

[Snyk] Upgrade express-openapi-validator from 4.13.2 to 4.13.8 #770

Closed

rish2497 mentioned this pull request May 24, 2024

[Snyk] Upgrade express-openapi-validator from 5.0.3 to 5.1.6 i3-Market-V3-Public-Repository/SP3-SCGBSSW-CR-ConflictResolverService#20

Open

davesag mentioned this pull request Jun 23, 2024

[Snyk] Upgrade express-openapi-validator from 5.1.6 to 5.2.0 davesag/api-server-boilerplate#748

Open

This was referenced Jun 24, 2024

[Snyk] Upgrade express-openapi-validator from 5.1.6 to 5.2.0 #934

Closed

[Snyk] Upgrade express-openapi-validator from 5.1.6 to 5.2.0 #936

Merged

davesag mentioned this pull request Jul 13, 2024

[Snyk] Upgrade express-openapi-validator from 5.1.6 to 5.2.0 davesag/api-server-boilerplate#749

Open

davesag mentioned this pull request Aug 2, 2024

[Snyk] Upgrade express-openapi-validator from 5.1.6 to 5.2.0 davesag/api-server-boilerplate#750

Open

cdimascio mentioned this pull request Aug 5, 2024

[Snyk] Upgrade express-openapi-validator from 5.1.6 to 5.2.0 #944

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump AJV to v8 #713

Bump AJV to v8 #713

JacobLey commented Mar 19, 2022 •

edited

Loading

gitguardian bot commented Mar 19, 2022 •

edited

Loading

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022 •

edited

Loading

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022

cdimascio Apr 3, 2022

JacobLey Mar 19, 2022

JacobLey Mar 19, 2022

cdimascio Apr 3, 2022

JacobLey Mar 19, 2022

JacobLey Apr 6, 2022

cdimascio Apr 10, 2022

cdimascio left a comment

JacobLey commented Apr 24, 2022 •

edited

Loading

cdimascio commented May 29, 2022 •

edited

Loading

cdimascio commented May 30, 2022

cdimascio commented May 30, 2022

cdimascio commented May 30, 2022

JacobLey commented May 30, 2022

cdimascio commented May 30, 2022

JacobLey commented May 30, 2022

cdimascio commented Jun 4, 2022 •

edited

Loading

cdimascio commented Jun 4, 2022

cdimascio commented Jun 4, 2022

JacobLey commented Jun 5, 2022

nd-jharn commented Oct 17, 2022

Bump AJV to v8 #713

Bump AJV to v8 #713

Conversation

JacobLey commented Mar 19, 2022 • edited Loading

gitguardian bot commented Mar 19, 2022 • edited Loading

⚠️ GitGuardian has uncovered 6 secrets following the scan of your pull request.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobLey Mar 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdimascio left a comment

Choose a reason for hiding this comment

JacobLey commented Apr 24, 2022 • edited Loading

cdimascio commented May 29, 2022 • edited Loading

cdimascio commented May 30, 2022

cdimascio commented May 30, 2022

cdimascio commented May 30, 2022

JacobLey commented May 30, 2022

cdimascio commented May 30, 2022

JacobLey commented May 30, 2022

cdimascio commented Jun 4, 2022 • edited Loading

cdimascio commented Jun 4, 2022

cdimascio commented Jun 4, 2022

JacobLey commented Jun 5, 2022

nd-jharn commented Oct 17, 2022

JacobLey commented Mar 19, 2022 •

edited

Loading

gitguardian bot commented Mar 19, 2022 •

edited

Loading

JacobLey Mar 19, 2022 •

edited

Loading

JacobLey commented Apr 24, 2022 •

edited

Loading

cdimascio commented May 29, 2022 •

edited

Loading

cdimascio commented Jun 4, 2022 •

edited

Loading