fix(validator,converter): validate every schema, not only two positions - #426
Merged
Conversation
|
Warning Review limit reached
Next review available in: 15 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change expands OAS 3 schema traversal for validation and OAS 3.2 feature detection. It covers nested schemas in media types, parameters, responses, callbacks, encodings, webhooks, and component objects. ChangesOAS 3 schema coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #426 +/- ##
==========================================
+ Coverage 86.69% 86.72% +0.03%
==========================================
Files 201 202 +1
Lines 29090 29202 +112
==========================================
+ Hits 25219 25325 +106
Misses 2560 2560
- Partials 1311 1317 +6
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@validator/schema_traversal_test.go`:
- Around line 235-253: Extend the schema traversal test table with cases
covering each untested branch: components.mediaTypes via
validateOAS3ComponentSchemas, mt.ItemEncoding and mt.PrefixEncoding via
validateMediaTypeSchemas, nested enc.Encoding/item/prefix recursion via
validateEncodingSchemas, and callback path-item item.AdditionalOperations via
validatePathItemSchemas. Assert the complete expected path for each case,
including every distinct segment produced by the traversal.
In `@validator/schema_traversal.go`:
- Around line 49-65: The callback schema traversal must detect cyclic
caller-built documents before recursive branching. Update the validation flow
reached from ValidateParsed, including validateOperationSchemasAtDepth and
validateCallbackMapSchemas, to track visited *parser.PathItem pointers and stop
or report when a path item is revisited; retain maxCallbackNestingDepth as a
fail-safe for non-cyclic deep nesting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 22b6a648-567a-4bc2-bf4e-5d491cce9174
📒 Files selected for processing (9)
converter/oas32_features.goconverter/oas32_features_test.gointernal/corpusutil/corpus.goparser/oas32_roundtrip_test.gotestdata/oas32-all-fields.jsontestdata/oas32-all-fields.yamlvalidator/oas3.govalidator/schema_traversal.govalidator/schema_traversal_test.go
validateSchema was called from exactly two places: the Request Body media type loop and `components.schemas`. Every schema in a response, a parameter or a header was therefore never validated at all — by any of the six rule families it runs, not only the OAS 3.2 one that exposed it. Adds the traversal for response content, parameter, header and components.mediaTypes schemas, at the operation, path item and components levels. `components.requestBodies` is deliberately not among them: validateOAS3Components already reaches it through validateOAS3RequestBody, and walking it here reported every error in it twice. A test caught that. The converter had the matching half. detectOAS32SchemaFeatures was called for `components.schemas` and for `mt.ItemSchema`, never for `mt.Schema` — the ordinary schema every 3.0 document already has. Google Maps stops being valid, correctly. Two of its parameters declare `"type": "string"` with `"enum": [0, 1, 2, 3, 4]`, and its own description says the values "range between 0 and 4", so the type is the mistake. That spec read as valid only because parameter schemas were never looked at. The corpus expectation records the reason, not just the number. Sections are walked in map order rather than sorted. The other passes that needed stable output sorted the errors they appended, so a clean document paid nothing; sorting keys here would allocate a slice per map on every document, and measured at double the cost for a fraction of the benefit — the validator's order is already unstable, five orderings in eight runs from the existing path walk alone. Filed as #425. Callbacks and Encoding Object headers are reached too. The callback walk tracks visited path items rather than relying on a depth bound alone: a path item whose operations each lead back to it branches, so the walk is exponential in depth and never reaches the bound — two such operations ran past forty seconds without returning. The bound stays as a fail-safe. The request body now goes through the same media type walk as every other position, so its 3.2 itemSchema is validated rather than only its schema. SmallOAS3 2211 -> 2222 MediumOAS3 18905 -> 19008 LargeOAS3 213072 -> 214264 OAS 2.0 unchanged Twenty-two positions are covered by test, each asserting the schema is reported exactly once, which is also what catches a position being reached twice. Fixes #423
erraggy
force-pushed
the
fix/schema-traversal-reaches-all-schemas
branch
from
July 31, 2026 23:01
984667a to
18a8148
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@validator/schema_traversal.go`:
- Around line 87-99: Update the fixed operation-method map in the schema
traversal logic to package scope and key it with the corresponding lowercase
constants from httputil instead of string literals. Use that shared map in the
loop around validateOperationSchemasAtDepth, while preserving the existing
operation coverage and AdditionalOperations handling.
- Around line 23-47: Make callback traversal state allocation lazy in
validateOAS3OperationSchemas and validateOperationSchemasAtDepth: avoid creating
a visited map for operations without callbacks, pass nil initially, and update
validateCallbackMapSchemas to initialize the map before its first
validatePathItemSchemas call when visited is nil. Preserve cycle tracking for
operations that do contain callbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f33661f7-5843-46b2-aed1-130753a9e3aa
📒 Files selected for processing (9)
converter/oas32_features.goconverter/oas32_features_test.gointernal/corpusutil/corpus.goparser/oas32_roundtrip_test.gotestdata/oas32-all-fields.jsontestdata/oas32-all-fields.yamlvalidator/oas3.govalidator/schema_traversal.govalidator/schema_traversal_test.go
…lazily The Path Item operation list moves to package scope as a slice keyed by the httputil method constants, per the "use constants" guideline. A slice rather than a map so it is built once instead of per path item, and iterates in a stable order. The visited set exists only for the callback traversal, and most operations carry no callbacks, so it is allocated only when one does. Nil is never written: validatePathItemSchemas is reachable only through a callback. No measurable allocation change — 2222 / 19008 / 214263 before and after, to the digit. The compiler was already handling the unconditional make. Kept for clarity, not for speed.
1 task
erraggy
added a commit
that referenced
this pull request
Aug 1, 2026
Add a new commit instead. PRs merge with squash, so the branch collapses into one commit anyway and amending buys nothing — while costing reviewers the diff between what they reviewed and what changed, and unsticking comments anchored to the old commit. It also broke a push on #426. Amending diverged local from remote, so a scheduled `git push` was rejected as a non-fast-forward; the steps after it were newline-separated rather than chained with &&, so two threaded replies posted anyway, each citing a commit that was not on the PR for four minutes. Amend freely before the first push, never after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
validateSchemawas called from exactly two places: the Request Body media type loop andcomponents.schemas. Every schema in a response, a parameter or a header was therefore never validated — by any of the six rule families it runs, not only the OAS 3.2 one that exposed it. 🔍Fixes #423
Reproduction
One 3.0.3 document, the same offending schema in four positions:
Before: two of the four reported. After: all four.
What now gets reached
Response content, parameters (schema and content form), headers,
components.mediaTypes, path-item-level parameters, Encoding Object headers, and Callback path items — at the operation, path item and components levels, underpaths,webhooksandcomponents.pathItemsalike.components.requestBodiesis deliberately not among them:validateOAS3Componentsalready reaches it throughvalidateOAS3RequestBody, and walking it here reported every error in it twice. A test caught that, which is why every case asserts its schema is reported exactly once rather than merely present.Callbacks are cycle-bounded.
Callback → PathItem → Operation → Callbackcloses a loop that a parsed document cannot build butValidateParsedaccepts from a caller — the same reasoning asmaxEncodingNestingDepthand the bound added in #419.Request bodies now go through the same media type walk as every other position, so their 3.2
itemSchemais validated rather than onlyschema.The converter had the matching half
detectOAS32SchemaFeatureswas called forcomponents.schemasand formt.ItemSchema— never formt.Schema, the ordinary schema every 3.0 document already has. Sooastools convert -t 3.0.3stayed silent on a 3.2 field in an inline media type schema.The fixture now carries an inline occurrence, so this cannot reopen silently: it previously put
nodeTypeanddefaultMappingonly undercomponents.schemas, one of the two positions that already worked.Google Maps stops being valid, correctly
Two of its parameters declare a string type with integer enum values, and the spec's own description says the values "range between 0 and 4" — so the type is the mistake, not the enum. That document read as valid only because parameter schemas were never looked at. The corpus expectation records the reason, not just the number.
All 8 corpus specs pass; no other count moved.
Order and cost
Sections are walked in map order rather than sorted. The passes that needed stable output (#411, #420) sorted the errors they appended, so a document with no findings paid nothing; sorting keys here would allocate a slice per map on every document — measured at double the cost, +1.0% versus +0.4% on MediumOAS3 — to fix a fraction of the problem. The validator's order is already unstable: the existing path walk alone produces five orderings in eight runs. Filed as #425.
Unlike the version gate in #411, this cost is real work — validating schemas that were never validated — not overhead.
Verification
22 positions covered, each asserting the schema is reported exactly once, which is also what catches a position being reached twice. Removing the traversal fails 9 subtests; removing the callback and encoding-header walks fails 3; the old request-body handling fails 2. The converter half fails 2 of its own.
make checkgreen.Release note
This changes validation outcomes, not just internals. Documents that passed at 3.0/3.1 may now report errors — five rule families beyond the 3.2 check now run in positions that were never examined. The findings are real defects that were previously invisible, but they will read as new failures in a CI pipeline, so this belongs near the top of the v1.59.0 notes rather than in a bugfix list.