Bump AJV to v8

[JacobLey]

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

[gitguardian]

⚠️ 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

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secrets safely. Learn here the best practices.
3. Revoke and rotate these secrets.
4. 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.

---

<sup>🦉 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!</sup>

[JacobLey]

implied by ajv-draft-04

[JacobLey]

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]

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]

Guaranteed to be {}, see normalizeOptions

Perhaps would benefit from a NormalizedOptions interface that extends Options?

[JacobLey]

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

[JacobLey]

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

[JacobLey]

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?

[JacobLey]

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

[JacobLey]

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

[JacobLey]

This updated type better matches AJV's expected input.

I recommend deprecating the array version

[cdimascio]

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]

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

[JacobLey]

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)

[cdimascio]

agreed

[JacobLey]

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.

[cdimascio]

thanks @JacobLey

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

npm install [email protected]

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

[cdimascio]

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

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]

@JacobLey any thoughts on what might be the cause?

[JacobLey]

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]

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]

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:

1. Show time elapse differences
2. 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]

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

and more info on migration to 8

[cdimascio]

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]

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]

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]

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