fix(converter): report a Link Object's server name, and cover mediaTypes - #424
Merged
Conversation
`detectOAS32Features` walked only the document's own `servers`, so the one Server Object that is not part of a servers list went unreported: converting a 3.2 document to 3.0 said nothing about a named server inside a Link. The validator reports it as of #411, so the two disagreed — the same inconsistency #411 set out to close, pointing the other way. The section walk also reported `components.mediaTypes` as a container without walking into it, so a 3.2 field on a media type defined there was lost. `testdata/oas32-all-fields.yaml` claimed to exercise every fixed field 3.2 added, but had no `components.mediaTypes` — the field arrived with #416 and the fixture was never extended. Adding it, and a Link with a named server, gives both `parser/oas32_roundtrip_test.go` and the converter's feature test something to check; adding the media types section is what exposed the container gap above. Both fixture files are regenerated from one source, so they stay the same document, and `stripOAS32Extensions` learned the two new objects. TestDownconvertReportsOAS32Fields checks field names, which cannot separate a `name` on a Link's server from one on the document's own — it passed for the whole time the Link went unreported. A second test pins the location for every field name that more than one object carries. Converting the fixture to 3.0.3 and validating the result now report the same inventory. The only fields the converter reports and the validator does not are `defaultMapping`, `nodeType` and `in: "querystring"`, which reach the validator through their own rules. Fixes #420
detectOAS32Features walks the Components sections by ranging over their maps, and Go randomizes that order, so the same document reported its issues in four distinct orderings across eight runs of the full-field fixture. Anything diffing conversion output between runs saw changes that were not there. The file already had the concern, solved locally: the Tag fields are an ordered slice rather than a map literal, with a comment saying why. The section walks were never given the same treatment. Sorting what the pass appended keeps the walks readable and costs one sort of a short slice, where ordering every map's keys would allocate on documents that report nothing. Same approach as the validator's gate in #411, so validate and convert now agree on order as well as on inventory. Pre-existing, but the Link and mediaTypes walks in the previous commit add two more map ranges to it, so it is fixed here rather than filed.
`application/jsonl` is not a legal Components key. The spec's Components Object requires every fixed field it declares — mediaTypes included — to use keys matching `^[a-zA-Z0-9\.\-_]+$`, and a slash is not in that allowlist. The key names a reusable component, not a media type. oastools validate reported it. Nothing ran the validator on the fixture: the parser and converter tests both reach it through the parser's structure validation, which does not check the Components key charset, so an invalid document satisfied every test in both packages. A test in validator now asserts the fixture validates clean, since parser cannot import validator. Also from review: - the location assertions matched against one joined blob, where every path is a prefix of another, so a nested report could satisfy its parent's assertion; matched per issue against a path-and-field prefix instead - the round-trip test checked that the new objects exist rather than what they hold, which is the opposite of its purpose — it exists to catch values lost or mutated across the two formats. It now pins itemSchema's `$ref`, the link's operationId, and its server URL https://spec.openapis.org/oas/v3.2.0.html#components-object
|
Warning Review limit reached
Next review available in: 10 minutes 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 (4)
📝 WalkthroughWalkthroughThe converter now detects nested OAS 3.2 media type features, component links, and named link servers. It sorts issues deterministically. OAS 3.2 fixtures and parser and validator tests now cover these fields. ChangesOAS 3.2 feature detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 #424 +/- ##
==========================================
+ Coverage 86.67% 86.69% +0.02%
==========================================
Files 201 201
Lines 29072 29090 +18
==========================================
+ Hits 25197 25219 +22
+ Misses 2562 2560 -2
+ Partials 1313 1311 -2
🚀 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: 1
🤖 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 `@converter/oas32_features_test.go`:
- Around line 151-168: Add a named server to a response-level Link in the OAS
3.2 fixture used by the coverage test, ensuring it is exercised through
detectOAS32ResponseFeatures and detectOAS32LinkFeatures. Extend the want
assertions to require the corresponding response.links.<name>.server path for
both down-conversion targets, while retaining the existing
components.links.petById.server coverage.
🪄 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: 848e5afd-7437-4f5b-9412-a3fd11e5db61
📒 Files selected for processing (6)
converter/oas32_features.goconverter/oas32_features_test.goparser/oas32_roundtrip_test.gotestdata/oas32-all-fields.jsontestdata/oas32-all-fields.yamlvalidator/oas32_test.go
…t one detectOAS32ResponseFeatures and the Components walk reach Link Objects separately, but the fixture carried a link only under components, so removing the response-level walk left every test green — half the fix in this branch was unverified. The same field-name-versus-location trap this branch already called out in TestDownconvertReportsOAS32Fields, reintroduced in the code that fixed it. Adds a link with a named server to the existing 200 response, asserts its path in the converter test and its value in the round-trip test, and teaches stripOAS32Extensions the two new objects so the fast marshal path still runs clean.
erraggy
added a commit
that referenced
this pull request
Aug 1, 2026
* docs: correct instructions and field lists that this release invalidated AGENTS.md and .github/copilot-instructions.md told contributors to install golangci-lint v2.1.0. #415 pinned the version in the Makefile as GOLANGCI_VERSION and made `make lint` refuse to run against anything else, so following either file left you unable to run `make check` at all. Both now point at `make lint-install` and name no version, since the Makefile is the source of truth and a hardcoded copy drifts the moment the pin moves. AGENTS.md also claimed `make test` runs with race detection. It does not — the target uses -covermode=atomic, and #415 added -coverpkg=./... for cross-package coverage attribution. joiner/doc.go and joiner/deep_dive.md listed four documentation fields that schema equivalence keeps apart. #416 made it seven: $comment, externalDocs and deprecated joined title, description, example and examples. parser/doc.go, parser/deep_dive.md and .claude/docs/oas-concepts.md listed $self, query, additionalOperations and mediaTypes as the OAS 3.2.0 additions. All four already existed at v1.58.0, so the lists named exactly the fields #412 did not add and none of the ~19 it did. oas-concepts.md matters most of the three: it is the agent-facing reference, and the validator now errors on any 3.2 field in a pre-3.2 document, so a fixture written from the stale list is rejected. The documentation these changes need adding rather than correcting — validator/doc.go on the version gate and the widened schema traversal, converter/doc.go on 3.2 to 3.0 downgrade warnings, and the matching deep dives — is filed as a follow-up. * fix(converter): walk inside query and additionalOperations when downconverting detectOAS32PathItemFeatures took its operations from parser.GetOperations, which is version-aware and omits query and additionalOperations below 3.2. This pass runs only when the target is below 3.2, and convertOAS3ToOAS3 sets the document's version to the target before the pass runs — so the accessor dropped exactly the two operations whose contents it needed to walk. Converting 3.2 to 3.1 reported `query` itself and nothing inside it. A user told that `query` is 3.2-only removes it, never learning that a `summary` nested in its responses would also have been lost. Under-reporting is the class of defect #423 and #424 set out to close. The 3.x to 2.0 path was correct only by accident: it passes the source document, whose version is still 3.2. Operations are now listed explicitly, as validator/oas32_gate.go already does for exactly this reason, and the version parameter is gone rather than left present and ignored. additionalOperations paths also stop being fabricated: GetOperations flattens custom methods into the operations map, so the old code emitted `paths./p.PURGE`, a path the document does not contain. It now emits `paths./p.additionalOperations.PURGE`, matching the validator. Found by the pre-release quality gate. The regression test covers all three targets; the two 3.x ones fail without this fix. * docs: scope the 3.2 field list, and separate empty-schema from equivalence Both from a CodeRabbit CLI pass on the release branch. The "OAS 3.2+ Only" heading in .claude/docs/oas-concepts.md promised a 3.2 changelog while listing only the fixed fields. 3.2 also adjusted serialization behavior, so the heading now says what the list covers and notes it is not exhaustive. joiner/doc.go described empty-schema detection as ignoring four metadata fields, which read as a list. isEmptySchema tests structural constraints and reads no documentation field at all, so a schema carrying nothing but documentation is excluded before equivalence is ever consulted. The two rules are now separate paragraphs rather than one sentence that conflated them. * chore: bump plugin version to 1.59.0 * chore: add benchmark results for v1.59.0 Generated by CI benchmark workflow on chore/v1.59.0-release-prep 🤖 Generated automatically --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.
The validator reports
server.nameinside a Link Object as of #422; the converter did not. That is the same validate/convert inconsistency #411 existed to close, pointing the other way. 🔁Fixes #420
What was wrong
detectOAS32Featureswalked onlydoc.Servers, so the one Server Object that is not part of a servers list went unreported:The section walk also reported
components.mediaTypesas a container without walking into it, so a 3.2 field on a media type defined there was lost.The fixture claimed more than it covered
testdata/oas32-all-fields.yamlsays it exercises "every fixed field OAS 3.2.0 added over 3.1.1". It had nocomponents.mediaTypes— the field arrived with #416 and the fixture was never extended — so neitherparser/oas32_roundtrip_test.gonor the converter's feature test proved anything about it.Adding that section is what exposed the container gap above, and adding a Link with a named server gives the fix something to check. Both files are regenerated from one source so they stay the same document; I reproduced the existing JSON byte-for-byte before changing anything.
The test could not have caught the bug it was for
TestDownconvertReportsOAS32Fieldsasserts field names.'name'was already reported for the document's own servers, so it passed for the entire time a Link's server went unreported — I verified this by removing the fix and watching it stay green.A second test pins the location for every field name that more than one object carries:
nameon both kinds of Server Object,summaryon a Tag and on a Response, andmediaTypeswith its contents. Matched per issue rather than against one joined blob, since every path here is a prefix of another.Two things found along the way
An invalid fixture.
application/jsonlis not a legal Components key — the spec requires every Components fixed field to use^[a-zA-Z0-9\.\-_]+$, and the key names a component, not a media type.oastools validatereported it immediately; nothing had ever run the validator on the fixture, because parser and converter both reach it through the parser's structure validation, which does not check the key charset. A test invalidatornow asserts the fixture validates clean — it has to live there, sinceparsercannot importvalidator. The same string remains legal, and present, as acontentkey.Non-deterministic output (own commit, pre-existing). The section walks range over maps, so the fixture reported its issues in four distinct orderings across eight runs. The file already had the concern solved locally — the Tag fields are an ordered slice with a comment saying why — but the section walks never got the same treatment. Sorting what the pass appended matches the validator's gate in #411, so
validateandconvertnow agree on order as well as inventory. Fixed here rather than filed because the Link and mediaTypes walks add two more map ranges to it.Parity
Converting the full fixture to 3.0.3 and validating the result now report the same inventory. The only fields the converter reports and the validator does not are
defaultMapping,nodeTypeandin: "querystring", which reach the validator through their own rules. Nothing is validator-only.Follow-up filed
#423 — the schema-level 3.2 rules never reach response content or parameter schemas, in either package. Pre-existing, spans two packages, so kept out of this PR. Found while verifying a review finding here.
Checks
make checkgreen. CodeRabbit CLI across three passes: 5 findings, then 1, then 3 — each applied or skipped with the reason verified by reproduction, including two skips where the requested change was already covered (TestComponentNameCharset/mediaTypes/rejects_slashalready exists) or would have contradicted the file's design.