fix(parser): accept OAS 2.0 string discriminator, reject cross-dialect forms - #398
Merged
Conversation
…t forms In OAS 2.0 a Schema Object's `discriminator` is a bare string naming the property; in OAS 3.0+ it is an object with `propertyName`. `parser.Schema` modeled only the 3.x shape, so the two versions were handled exactly backwards for 2.0 input: spec-conformant documents were rejected outright, while the illegal object form was silently accepted. Fixes #394 ## Parser All three decode paths handled the string form, not just the one in the report: - `encoding/json` — errored with a message naming the internal `parser.Alias` - YAML struct decode — errored differently (`cannot construct !!str`) - generated `decodeFromMap` (`ResolveRefs`) — **silently dropped** the field, since `m["discriminator"].(map[string]any)` merely failed its assertion `Discriminator` gains `StringForm bool` (`json:"-" yaml:"-"`) recording which dialect spelled it. This is load-bearing rather than cosmetic: the CLI writes `fix`/`join` output by marshaling the typed document, so without it a valid 2.0 spec would round-trip into the invalid 3.x object form. Both marshalers now reproduce the form they were given. `StringForm` is excluded from `equalDiscriminator` on purpose — the two forms select the same property, so counting them as different would make every cross-version diff report a phantom change. ## Validator Parsing is now deliberately permissive, so `validateDiscriminatorForm` is the only thing enforcing the dialect. It rejects the object form in 2.0 documents (the silent false-accept half of the issue) and the string form in 3.x, reusing the existing `v.oasVersion` context. Unknown versions are skipped, since they say nothing about which form is correct. ## Converter `convert -target 3.0` was emitting the string form into a 3.0 document — faithfully preserving a spelling that is illegal at the target. Both directions now re-spell the discriminator, and a dropped 3.x `mapping` is reported rather than silently lost. `walkSchemas` is extracted from `walkSchemaRefs` so the normalization reuses that traversal instead of adding a third copy of it. ## Codegen `zz_generated_decode.go` is driven by a new `stringOrObjectFields` escape hatch in the decode generator, mirroring the existing `polymorphicFields` one. The generated file is not hand-edited; `go run ./internal/codegen/decode -check` passes. ## Tests Fixtures use the canonical "Models with Polymorphism Support" example from the OAS 2.0 spec, in both JSON and YAML, plus a companion asserting the object form is rejected in a 2.0 document. Coverage spans all three decode paths, both round trips, YAML anchors, deep copy, and both conversion directions. Corpus error counts are unchanged.
Close the two branches the first commit left untested: - validateDiscriminatorForm's early return for an unrecognized OAS version, which guards a hand-assembled ParseResult that never set one - yamlKindName's full constant mapping, which error messages depend on staying in step with yaml.Kind Both new helpers now report 100% statement coverage.
|
Warning Review limit reached
Next review available in: 33 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 (2)
📝 WalkthroughWalkthroughThe change adds dual-form discriminator parsing and serialization, generated decoding support, OAS-version validation, schema-wide discriminator normalization during conversion, reference traversal reuse, conversion issue reporting, and regression fixtures and tests for JSON, YAML, nested schemas, and round trips. ChangesDiscriminator parsing and serialization
Schema traversal and conversion
Version validation
OAS 2.0 fixtures
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Document
participant Parser
participant Converter
participant Validator
Document->>Parser: parse discriminator string or object
Parser-->>Converter: decoded schema discriminator
Converter->>Converter: normalize form for target OAS version
Converter-->>Validator: converted document
Validator->>Validator: validate discriminator form
Validator-->>Document: validation result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 #398 +/- ##
==========================================
+ Coverage 85.05% 85.12% +0.07%
==========================================
Files 196 197 +1
Lines 27739 27845 +106
==========================================
+ Hits 23593 23703 +110
+ Misses 2813 2809 -4
Partials 1333 1333
🚀 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/schema_convert.go`:
- Around line 45-63: Update discriminatorToStringForm to detect non-empty
d.Extra before clearing it, and report a conversion issue through
c.addIssueWithContext analogous to the existing mapping-loss issue. Preserve the
existing d.Extra = nil and string-form conversion behavior while ensuring
discarded extensions are explicitly reported.
🪄 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: 4e9dc1f8-9626-4db4-bd19-71f814ccf0b6
📒 Files selected for processing (16)
converter/ref_rewrite.goconverter/schema_convert.goconverter/schema_discriminator_test.gointernal/codegen/decode/main.goparser/decode_helpers.goparser/schema.goparser/schema_discriminator_test.goparser/schema_equals.goparser/schema_json.goparser/schema_yaml.goparser/zz_generated_decode.gotestdata/discriminator-2.0.jsontestdata/discriminator-2.0.yamltestdata/discriminator-object-form-2.0.yamlvalidator/schema.govalidator/schema_discriminator_test.go
…nversion OAS 2.0 spells the discriminator as a bare string, so there is no object left to carry x- extensions when converting down from 3.x. discriminatorToStringForm cleared them without saying so, which was the only unreported Extra map assignment in the converter package and contradicted the project rule to track every lossy conversion (AGENTS.md). The message names the dropped keys, sorted for stable output, and points at the enclosing Schema Object, which does accept extensions in OAS 2.0. Extra is now cleared only when there was something to clear, so a lossless discriminator no longer runs the branch at all. Raised by CodeRabbit on #398.
erraggy
added a commit
that referenced
this pull request
Jul 28, 2026
* chore: prepare v1.57.0 release Pre-release review found docs that #396 and #398 made inaccurate, the version bumps those PRs did not carry, and the one gap in godoc coverage for the new public API. ## Version bumps plugin/.claude-plugin/plugin.json and .claude-plugin/marketplace.json still read 1.56.0. Neither was touched by the three commits since that tag, so both were genuinely outstanding — this is the plugin/binary version mismatch the session-start check reports. ## Docs the promotion fix made false #396 made every decode path promote Items, AdditionalProperties, AdditionalItems, UnevaluatedItems and UnevaluatedProperties to *Schema, so a parsed document no longer leaves map[string]any in any of them. Several docs described the old behavior as intended: - parser/deep_dive.md carried a `case map[string]any:` arm commented "when schema wasn't typed during parsing". That is the most actively misleading of the set: it teaches a branch that is now dead for parsed documents, and implies a $ref inside items: may still arrive unresolved — exactly the bug #396 fixed. Removed, with a note that only hand-constructed documents can produce the map form. - walker/options.go listed "Documents are parsed from raw YAML/JSON without full schema resolution" as a reason WithMapRefTracking is needed. That bullet described the bug as a feature. The option still matters for documents oastools did not parse, so the other two reasons stand. - parser/doc.go, walker/deep_dive.md, walker/example_test.go and docs/developer-guide.md carried smaller versions of the same claim. ## Docs missing the new public API Discriminator.StringForm had no documentation outside its own godoc: - CLAUDE.md gains Key Patterns entries for both the discriminator dialects and the schema-or-bool promotion, per the rule that new public API is documented there. - validator/deep_dive.md was missing the new discriminator-form check from its checked-items list. This is user-visible: `oastools validate` now reports errors it did not in v1.56.0. Notes the escape hatch too, since it is not obvious that an unset OASVersion skips the check. - converter/deep_dive.md's three fidelity tables did not mention the discriminator at all, in either direction, including the two new warnings for a dropped mapping and dropped extensions. - parser/deep_dive.md gains a Discriminator Dialects section covering the round-trip guarantee, since that file is the source for the published parser package page. - .claude/docs/oas-concepts.md and CONTRIBUTORS.md version-feature lists, plus a new pitfall: a hand-built OAS 2.0 fixture must set StringForm, or the validator rejects it as the 3.0+ object form. ## godoc examples parser/example_test.go gains ExampleDiscriminator_stringForm, covering the release's only new exported identifier including the dialect round trip, and ExampleSchema_items, which documents the type-assertion contract #396 made reliable — the any field types alone tell a reader nothing about what to assert to. ## Verified make check exit 0, 0 lint issues, 0 markdownlint issues. govulncheck clean. Benchmarked v1.56.0 against HEAD on one machine with benchstat over 6 runs: parser +4.80% allocs/op and +8.12% B/op, downstream packages +6.38% and +10.84% geomean. Both are the accepted cost of #396 — validator, converter and fixer now traverse the items: subtrees they previously skipped. Corpus findings unrelated to this release are filed as #400, #401 and #402. * chore: add benchmark results for v1.57.0 Generated by CI benchmark workflow on chore/v1.57.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.
Summary
discriminatoras a bare string; OAS 3.0+ spells it as an object.parser.Schemamodeled only the 3.x shape, so the two versions were handled exactly backwards for 2.0 input — conformant documents were rejected outright, and the illegal object form was silently accepted.Discriminator.StringForm, the first regression fixtures for a 2.0discriminatorin either form, and dialect enforcement in the validator.Changes
Parser
All three decode paths mishandled the string form, not only the one in the report:
encoding/jsonparser.Aliastypecannot construct !!strdecodeFromMap(ResolveRefs)m["discriminator"].(map[string]any)merely failed its assertionDiscriminatorgainsStringForm bool(json:"-" yaml:"-") recording which dialect spelled it.parser/schema_yaml.goaddsUnmarshalYAML/MarshalYAML, which the type previously lacked entirely. It follows anchors sodiscriminator: *anchoris classified by the node it points at, and reports an unexpected node kind by its constant name rather than yaml's default message about the unexported alias type.decodeDiscriminatorhandles both spellings on thedecodeFromMappath.Validator
Parsing is now deliberately permissive, so
validateDiscriminatorFormis the only thing enforcing the dialect. It rejects the object form in 2.0 documents — the silent false-accept half of the issue — and the bare string in 3.x, reusing the existingv.oasVersioncontext already used for the OAS 3.0type: "null"check. Unrecognized versions are skipped, since they say nothing about which form is correct.Converter
convert -target 3.0was emitting the string form into a 3.0 document, faithfully preserving a spelling illegal at the target. Both directions now re-spell the discriminator, and a dropped 3.xmappingis reported rather than silently lost.walkSchemasis extracted fromwalkSchemaRefsso this reuses the existing traversal instead of adding a third copy of it.Codegen
parser/zz_generated_decode.gois regenerated, not hand-edited. The generator gains astringOrObjectFieldsescape hatch mirroring the existingpolymorphicFieldsone, plus aDiscriminatorentry in the type-check overlay stub.go run ./internal/codegen/decode -checkpasses.Context
Why a struct field rather than just a permissive unmarshaler. The issue proposed accepting both forms on decode and left the marshaling decision open. That decision turns out to be load-bearing: the CLI writes
fix/joinoutput by marshaling the typed document (cmd/oastools/commands/common.go:182), not from the preserved source node. A permissive unmarshaler alone would have parseddiscriminator: petTypeand written back{"propertyName": "petType"}— trading a false rejection for silent corruption of a valid 2.0 spec.StringFormis additive and exported, soDeepCopy(which does*out = *in) needed no regeneration.Why
StringFormis excluded from equality.equalDiscriminatordeliberately ignores it. The flag records which dialect spelled the discriminator, not what it means — both forms select the same property, so comparing it would make every cross-version diff report a phantom change.internal/schemautil/hash.goalready hashes onlyPropertyNameandMapping, so the two stay consistent.Scope note. The issue suggested a separate
OAS2Schematype as the alternative. That models the dialects more honestly but is a large, API-breaking change; this keeps the exported shape ofDiscriminatorintact and needs no migration for library consumers.Testing
Fixtures use the canonical "Models with Polymorphism Support" example from the OAS 2.0 specification, in both JSON and YAML, plus a companion asserting the object form is rejected in a 2.0 document — the
testdata/gap the issue identified.Coverage spans all three decode paths, both round trips, YAML anchors and invalid node kinds, deep copy, equality, and both conversion directions.
Corpus error counts are unchanged (DigitalOcean 128, Asana 9, Plaid 93, GitHub 115), confirming the new validator error does not fire on real-world specs.
Every new function reports 100% statement coverage;
validateDiscriminatorForm100%,walkSchemas100%,decodeDiscriminator100%,yamlKindName100%.All tests pass (
make check) — 8901 testsCoverage reviewed (
go tool cover) — parser 80.6%, validator 91.3%, converter 88.2%Security scan clean (
govulncheck ./...— no vulnerabilities found)Local code review passed
Reviewer notes
Two things found while working on this were filed separately rather than folded in:
Schema.Itemsdecodes asmap[string]anyinstead of*Schema(the JSON path promotes it viapromoteSchemaOrBool). The validator therefore descends into nothing underitems:for YAML specs. Pre-existing, much broader, and likely to move corpus expectations. The one test that works around it carries a comment pointing at the issue.Discriminator.defaultMappingand 15 other OAS 3.2.0 fields are unmodeled, and unmodeled fields are silently dropped from JSON documents on round trip. Relevant here becauseXML.nodeTypein that issue is the same shape of problem this PR solves — a field that supersedes an existing representation rather than sitting beside it.StringFormis a reasonable precedent for carrying "which spelling did this document use" without breaking the public type.Related Issues
Fixes #394
Related to #396
Related to #397