fix: scope AJV instances per lintDocument call to fix concurrent validation#2774
fix: scope AJV instances per lintDocument call to fix concurrent validation#2774vlindhol wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 8c72d29 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
75353fe to
4572e82
Compare
a0f6003 to
18bfbff
Compare
18bfbff to
1157188
Compare
| specVersion: SpecVersion; | ||
| config?: Config; | ||
| getVisitorData: () => Record<string, unknown>; | ||
| ajvValidator: AjvValidator; |
There was a problem hiding this comment.
Please, avoid adding ajvValidator in walker, because it is a separate concept of JSON schema validation. We use this validator only for specific rule and we use Ajv validators only there, meaning, that we don't need this in general pattern of walker. Additionally, that's why all tests in pipeline failed.
There was a problem hiding this comment.
Good point, moved AjvValidator out of the walker. Each of the three example-validation rules now creates its own AjvValidator instead.
|
Thanks @vlindhol for the contribution! I left comment. Please, feel free to ask any questions. |
1157188 to
a0b6228
Compare
a0b6228 to
4a8455d
Compare
|
@vlindhol ## Check yourself
- [ ] This PR follows the [contributing guide](https://github.com/Redocly/redocly-cli/blob/main/CONTRIBUTING.md#pull-request-guidelines) <!-- TODO: make it required -->
- [ ] All new/updated code is covered by tests
- [ ] Core code changed? - Tested with other Redocly products (internal contributions only)
- [ ] New package installed? - Tested in different environments (browser/node)
- [ ] Documentation update has been considered <!-- TODO: make it required -->
## Security
- [ ] The security impact of the change has been considered <!-- required 🔒 -->
- [ ] Code follows company security practices and guidelines |
Done! Was a bit unclear which boxes I need to check but I'm happy to be corrected! |
…dation The global AJV singleton in `ajv.ts` captured a `resolve` closure from whichever document first created the instance. When consumers like `openapi-typescript` process multiple APIs concurrently via `Promise.all`, the second document's `resolve` was silently discarded, producing spurious "Example validation errored: can't resolve reference" warnings for valid same-document `$ref`s. Replace the global singleton with an `AjvValidator` class instantiated per `lintDocument` call and threaded through WalkContext/UserContext.
4a8455d to
8c72d29
Compare
|
Does this have any impact on the single entry document concept? This appears to be a fix for code generators but the main purpose of this package is to lint/join/bundle descriptions. Please verify this does not introduce any other concept outside of using the entry document as the starting point for the functionality of this package. |
Summary
The global AJV singleton in
packages/core/src/rules/ajv.tscaptured aresolveclosure from whichever document hit it first. When consumers likeopenapi-typescriptrunlintDocumentover multiple APIs concurrently viaPromise.all, the second document'sresolvewas silently discarded, producing spurious warnings:The warnings only appear when:
$refhas anexample/examplessibling$ref(which is what triggers AJV'sloadSchemaSync)redocly lintitself wasn't affected because it processes APIs sequentially.There was an earlier attempt at this fix in #2135 that went stale. This time I built a minimal repro that's checked in as a regression test.
Fix
Refactored
rules/ajv.tsfrom free functions + global singleton into anAjvValidatorclass. Each of the three example-validation rules (no-invalid-media-type-examples,no-invalid-schema-examples,no-invalid-parameter-examples) now creates its ownAjvValidatorin its factory closure. SinceinitRulesinvokes each factory once perlintDocumentcall, every call gets a fresh validator — no shared state across documents.The walker (
walk.ts) stays untouched; AJV is purely a JSON-schema-validation concern living inside the rules that need it.Check yourself
Security
Note
Medium Risk
Refactors core example-validation plumbing from a global AJV singleton to per-lint validator instances, which could subtly affect schema caching/validation behavior across runs. Adds a concurrent lint regression test, lowering risk but still touching critical linting internals.
Overview
Fixes concurrent linting producing spurious "can’t resolve reference" example-validation warnings by removing the global AJV instance and introducing a per-run
AjvValidatorwith dialect-specific cached instances.The example-validation rules now create and pass a validator instance into
validateExample, andlintDocumentno longer force-resets AJV state. Adds a regression test plus OpenAPI fixtures that lint two specs concurrently viaPromise.all, and updates AJV unit tests to targetAjvValidator.Reviewed by Cursor Bugbot for commit 8c72d29. Bugbot is set up for automated code reviews on this repo. Configure here.