fix: guard against parser field drift, and close the gaps the guard found - #416
Merged
Conversation
A field on a parser type has to be handled in several independent places: the hand-built slow MarshalJSON path, the structural hasher, each type's equality function, and the joiner's schema comparison. Nothing connected them, so a field could be added, tested and shipped while silently vanishing from JSON documents carrying an extension, or while leaving two different schemas looking identical. internal/driftguard closes the loop. It reflects over each type, sets one field at a time, and requires the behavior under test to notice: the field survives both marshal paths, moves the hash, and makes both equality checks disagree. Asking whether the output changes rather than whether the field appears in some switch is what makes it honest. Deliberate exclusions are listed with a reason beside the check that honors them. Written first, it named the gaps rather than leaving them to be triaged by hand, and it found more than the audit did. Marshal: nine fields reached the struct tags and not the map builder, so they were dropped from any object that also carried an x- extension. Four are the OAS 3.2 features #397 recorded as already implemented ($self, components.mediaTypes, query, additionalOperations); five are JSON Schema 2020-12 keywords, so OAS 3.1 documents were affected too. Hash: thirteen structural fields were never read, including $id, $dynamicRef and $schema, which decide what a reference resolves to, and collectionFormat, which decides an array's wire format. Three list-valued fields also ran together unframed, so required ["ab"] hashed identically to ["a", "b"]; they are length-framed now, as xml and discriminator already were. Equivalence: joiner's deep comparison read 38 of Schema's 65 fields. Twenty-four structural ones are now compared, and $comment, externalDocs and deprecated join the documentation set so they follow EquivalenceDocsIgnore. For six fields the hash was silent too, so schemas differing only in collectionFormat, default, $schema, $id, $dynamicRef or deprecated were being merged outright. parser's own Equals needed no change; the guard confirms it already covered every field but the deliberately excluded StringForm. Structural hashes therefore change for most non-trivial schemas. The pending release note says so and explains why. Fixes #414 Fixes #410
|
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 PR adds missing JSON marshaling fields, expands schema hashing and deep equivalence checks, prevents framing collisions, and adds reflection-based drift guards for parser schema coverage. ChangesSchema consistency
Estimated code review effort: 4 (Complex) | ~60 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 #416 +/- ##
==========================================
+ Coverage 85.12% 86.62% +1.50%
==========================================
Files 200 200
Lines 28606 28778 +172
==========================================
+ Hits 24350 24930 +580
+ Misses 2864 2549 -315
+ Partials 1392 1299 -93
🚀 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: 7
🤖 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 `@internal/driftguard/equivalence_test.go`:
- Around line 67-69: Remove the unreachable f.name == "StringForm" conditional
and its t.Skip call from the fieldsOf[parser.Schema]() equivalence test, leaving
the surrounding field comparison logic unchanged.
In `@internal/driftguard/fields_test.go`:
- Around line 94-98: Update the reflect.Interface branch in the relevant test
helper to assign marker only when the destination interface is empty or
otherwise accepts the marker value; for named non-empty interfaces, return false
instead of calling fv.Set and allowing a panic. Preserve the existing true
result for fields declared as any.
In `@internal/driftguard/marshal_test.go`:
- Around line 107-110: Update the marshal assertion in the test’s value-encoding
loop to unmarshal encoded into map[string]json.RawMessage and assert that
f.jsonKey exists as a top-level key, replacing the raw JSON assert.Contains
check while preserving the existing error and diagnostic context.
In `@internal/schemautil/hash.go`:
- Around line 416-441: Rename or restructure hashContentKeywords so its name
accurately reflects that it hashes default, collectionFormat, and
content-related keywords. Prefer moving the two content keywords into a separate
helper or renaming the function to hashValueAndSerialization, and update its doc
comment and all call sites accordingly.
- Around line 394-406: Update the $vocabulary hashing loop in hashIdentity to
write each strconv.FormatBool(schema.Vocabulary[k]) value through h.writeLabeled
with an appropriate label, rather than h.writeString, so the boolean is
length-framed consistently with other hashed values. Include this hash-format
change in the existing release note for structural hash changes.
In `@joiner/equivalence.go`:
- Line 604: Update the comparison call in the additional-items handling to
provide an item-specific path, rather than allowing
compareAdditionalPropertiesSchemas to record differences under
additionalProperties. Split or parameterize compareAdditionalPropertiesSchemas
so its path label is additionalItems for this call while preserving the existing
additionalProperties behavior elsewhere.
- Around line 507-516: Add a nil-value guard at the start of compareDeep, before
any operand dereference. Treat two nil operands as equal, record the appropriate
mismatch when only one operand is nil, and return without dereferencing either
value; keep the existing deep comparison unchanged when both values are non-nil.
🪄 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: ebb27dad-b641-4fb7-8360-e8e03cffbb1d
📒 Files selected for processing (11)
internal/driftguard/doc.gointernal/driftguard/equivalence_test.gointernal/driftguard/fields_test.gointernal/driftguard/hash_test.gointernal/driftguard/marshal_test.gointernal/schemautil/hash.gojoiner/equivalence.gojoiner/joiner_dedupe_test.goparser/oas3_json.goparser/paths_json.goparser/schema_json.go
…rd nil map values CodeRabbit review on #416. Two of the seven were defects in this branch. compareAdditionalPropertiesSchemas hardcodes its own path segment, so reusing it for AdditionalItems reported every item difference under additionalProperties. The comparison was right and the diagnostic lied about which keyword differed. The keyword is a parameter now, in compareSchemaOrBool, and the old function is a one-line wrapper, so a future reuse cannot misname a field either. compareSchemaMaps handed map values straight to compareDeep, which dereferences both operands with no nil guard. A map may hold a nil value for a present key — `patternProperties: {"^a": }` parses that way — and comparing one against a present schema panicked. The pre-existing Properties and DependentSchemas loops share the hazard; this fixes the two maps the new helper added. The rest are hygiene in the same spirit as the guard itself: - the marshal guard matched its key as a substring of the encoded JSON, which a nested value could satisfy. It decodes and checks for a top-level key now: it exposed nothing, but a guard that passes falsely is worth nothing. - populate panicked on a named interface field rather than reporting it uncovered. A panic aborts the whole guard, which is how the pointer bug during development reported one failure instead of nine. - the $vocabulary boolean was the one unframed write in a function whose point is injectivity, so the next value type added there would have copied it. - hashContentKeywords also hashed default and collectionFormat, which are not content keywords; hashValueSemantics now holds those two and both names are honest. - dropped an unreachable skip for Discriminator.StringForm in a guard that only ever iterates Schema fields.
Owner
Author
|
All seven applied in c1e455c. Two were defects in this branch, and I confirmed both by reproduction before changing anything. Confirmed and fixed
The comparison was correct; the diagnostic lied about which keyword differed. Extracted Nil map value panicked. Reproduced with Guarded in Also applied
|
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 `@internal/schemautil/hash.go`:
- Around line 267-270: Update Hash and its $ref handling so reference objects
include supported sibling keywords—Default, $id, $anchor, $dynamicRef,
ContentEncoding, ContentMediaType, and ContentSchema—rather than returning after
hashing only $ref. Reuse the existing identity, value-semantics, and
content-keyword hashing helpers as appropriate, and add regression coverage for
each sibling-field class while preserving grouping for schemas with identical
reference and sibling values.
In `@joiner/equivalence.go`:
- Around line 1250-1255: Update the schema branch in compareDeep to handle
typed-nil leftSchema and rightSchema after the type assertions: record a
presence mismatch when exactly one is nil, return without descending when both
are nil, and only call compareDeep for two non-nil schemas. Add coverage for
typed-nil Schema values supplied through AdditionalItems or
AdditionalProperties.
🪄 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: f4d120f6-4a57-4a2a-8e58-c32d69300e6d
📒 Files selected for processing (5)
internal/driftguard/equivalence_test.gointernal/driftguard/fields_test.gointernal/driftguard/marshal_test.gointernal/schemautil/hash.gojoiner/equivalence.go
💤 Files with no reviewable changes (1)
- internal/driftguard/equivalence_test.go
…package coverage Review follow-ups on #416. Every skipped case in the drift guard was a deliberate exclusion, which is the wrong way to express one: a skip checks nothing, so hashing Title by accident, or emitting StringForm, would have gone unnoticed while the guard kept reporting SKIP. Each is now an assertion that the exclusion holds, and a field populate cannot build a value for fails rather than skipping, so an unhandled type is visible instead of silently uncovered. Zero skips, more assertions. Converting them surfaced two things. Deep comparison never compared $ref, so two schemas pointing at different targets were reported equivalent; it compares them now and the exclusion list is empty. And StringForm does not drop a key, it re-spells the Discriminator as the OAS 2.0 bare string, so there is no object to decode: that case is checked on the raw bytes, and its recorded reason says what it actually does. Two findings from the second CodeRabbit pass, both verified by reproduction. A typed nil in a schema-or-bool field passed the interface nil checks and asserted cleanly, then panicked in compareDeep. And hashSchema returned as soon as it had written $ref, so {$ref: X, default: 1} and {$ref: X, default: 2} hashed alike, though the comparison did split them, making it a wasted comparison rather than a bad merge. Both fixed, both with the tests the previous round should have had. Coverage: go test attributes coverage per package, so the guards in internal/driftguard exercised joiner and internal/schemautil thoroughly and registered as almost none of it, which is why the patch check failed at 31%. -coverpkg=./... credits a test to the package it exercises. codecov.yml already notes the same limitation where it ignores internal/testutil. Overall coverage rises 83.7% to 85.6% because it stops discarding real coverage; the workflow needs no change, since it uploads whatever coverage.txt make check produced.
Owner
Author
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
joiner/equivalence.go (1)
563-565: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
maps.Equalfor the$vocabularycomparison.
Vocabularyis a concretemap[string]bool.reflect.DeepEqualworks but is not the most idiomatic choice for a typed map comparison; the standard librarymaps.Equal(Go 1.21+) expresses the same check without reflection.♻️ Optional refactor
- if !reflect.DeepEqual(left.Vocabulary, right.Vocabulary) { + if !maps.Equal(left.Vocabulary, right.Vocabulary) { result.record(path, "$vocabulary", left.Vocabulary, right.Vocabulary, "$vocabulary mismatch") }As per coding guidelines, "Use Go 1.25+ syntax and features."
🤖 Prompt for 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. In `@joiner/equivalence.go` around lines 563 - 565, Update the $vocabulary comparison in the equivalence logic to use the standard-library maps.Equal function for left.Vocabulary and right.Vocabulary instead of reflect.DeepEqual, preserving the existing mismatch recording behavior.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@joiner/equivalence.go`:
- Around line 563-565: Update the $vocabulary comparison in the equivalence
logic to use the standard-library maps.Equal function for left.Vocabulary and
right.Vocabulary instead of reflect.DeepEqual, preserving the existing mismatch
recording behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2a60a73-545a-4547-bac3-fabde80b537b
📒 Files selected for processing (8)
Makefileinternal/driftguard/equivalence_test.gointernal/driftguard/hash_test.gointernal/driftguard/marshal_test.gointernal/schemautil/hash.gointernal/schemautil/hash_test.gojoiner/equivalence.gojoiner/equivalence_test.go
parser documents "Nil and empty maps are considered equal" and uses maps.Equal
throughout. The comparisons added in this branch used reflect.DeepEqual, which
splits those two, so the packages contradicted each other about the same pair of
schemas: a discriminator declaring `mapping: {}` differed from one declaring no
mapping under joiner and matched under parser.
$vocabulary and the discriminator mapping now use maps.Equal, and externalDocs is
compared field by field rather than by DeepEqual on the pointer, whose comparison
of Extra had the same split.
Extra keeps maps.EqualFunc with reflect.DeepEqual rather than maps.Equal, which
is the pairing parser uses: specification extensions hold arbitrary JSON, and
maps.Equal compares values with ==, which panics on a slice.
The duplication behind all of this is filed as #418, along with a second
divergence it exposed: parser compares `required` order-sensitively while joiner
and the hasher both treat it as a set.
erraggy
added a commit
that referenced
this pull request
Jul 31, 2026
`oastools join --semantic-dedup` panicked on a valid document. `properties:
{name:}` is legal YAML and parses to a present key with a nil value, and
`compareDeep` dereferenced both operands immediately.
`CompareSchemasWithOptions` guards its own two arguments, but nothing
re-guarded the recursion, so every one of the fourteen nested walks that can
produce a nil was exposed. #416 patched two of them individually; that
approach needed each new nested walk to remember.
Guard once at the top of `compareDeep` instead, and revert the two spot fixes
to plain calls. A nil against a present schema is recorded as a difference at
the caller's path, which already names the field or entry, so the diagnostics
land where they did before. Two nils are equivalent.
The `*Schema` pointer fields (`not`, `if`, `then`, `else`, `contains`,
`propertyNames`, `contentSchema`) each carry their own presence check with a
field-specific message and are left alone; the central guard is a backstop
under them, not a replacement.
A driftguard case populates each of the twenty nested-schema fields with a nil
in turn, so the class cannot come back by way of a field added later. Thirteen
of those twenty fail without this fix.
Fixes #417
erraggy
added a commit
that referenced
this pull request
Jul 31, 2026
`oastools join --semantic-dedup` panicked on a valid document. `properties:
{name:}` is legal YAML and parses to a present key with a nil value, and
`compareDeep` dereferenced both operands immediately.
`CompareSchemasWithOptions` guards its own two arguments, but nothing
re-guarded the recursion, so every one of the fourteen nested walks that can
produce a nil was exposed. #416 patched two of them individually; that
approach needed each new nested walk to remember.
Guard once at the top of `compareDeep` instead, and revert the two spot fixes
to plain calls. A nil against a present schema is recorded as a difference at
the caller's path, which already names the field or entry, so the diagnostics
land where they did before. Two nils are equivalent.
The `*Schema` pointer fields (`not`, `if`, `then`, `else`, `contains`,
`propertyNames`, `contentSchema`) each carry their own presence check with a
field-specific message and are left alone; the central guard is a backstop
under them, not a replacement.
A driftguard case populates each of the twenty nested-schema fields with a nil
in turn, so the class cannot come back by way of a field added later. Thirteen
of those twenty fail without this fix.
Fixes #417
This was referenced Jul 31, 2026
erraggy
added a commit
that referenced
this pull request
Jul 31, 2026
…pes (#424) * fix(converter): report a Link Object's server name, and cover mediaTypes `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 * fix(converter): report 3.2 fields in a deterministic order 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. * fix(testdata): use a legal component name for the media type fixture `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 * test(converter): cover the response-level Link, not only the component 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.
Summary
internal/driftguard: a behavioral guard that requires every parser struct field to be handled by the marshal, hash, and equality pathsMarshalJSONpath, including four OAS 3.2 features previously recorded as implementedThe guard
A field on a parser type has to be handled in several independent places, and nothing connected them.
internal/driftguardreflects over each type, sets one field at a time, and requires the behavior under test to notice it:TestMarshalSlowPathEmitsEveryFieldx-extension present?TestMarshalFastPathEmitsEveryFieldTestHashReadsEveryStructuralSchemaFieldTestHashFramesAdjacentValuesTestDeepComparisonReadsEveryStructuralSchemaFieldTestEqualsReadsEveryStructuralSchemaFieldparser.Equals?Asking whether the output changes rather than whether the field appears in some switch statement is what makes it honest — a source-scanning check would have passed on every bug below.
It lives in one internal package with no exported symbols, so the reflection helper it shares is not a maintained API for anything else, and each deliberate exclusion sits beside the check that honors it with its reason.
Writing it first meant it named the gaps instead of my triaging 65 fields by hand — and it found more than the audit in #414 did.
What it found
Marshal — 9 fields. Present in the struct tags, absent from the map builder, so dropped from any object also carrying an extension. Four are the OAS 3.2 features #397 recorded as covered (
$self,components.mediaTypes,query,additionalOperations) — implemented for the fast path only. Five are JSON Schema 2020-12 keywords (unevaluatedProperties,unevaluatedItems,contentEncoding,contentMediaType,contentSchema), so OAS 3.1 documents were affected too.Hash — 13 fields plus 3 collisions. Never read:
$schema,$id,$anchor,$dynamicRef,$dynamicAnchor,$vocabulary,default,collectionFormat, and the same five content keywords.$idand$dynamicRefdecide what a reference resolves to;collectionFormatdecides an array's wire format. Separately,required: ["ab"]hashed identically to["a", "b"], and the same forenumanddependentRequired— length-framed now, asxmlanddiscriminatoralready were.Equivalence — 27 fields. Deep comparison read 38 of
Schema's 65. Twenty-four structural ones are now compared;$comment,externalDocsanddeprecatedjoin the documentation set so they followEquivalenceDocsIgnore. For six, the hash was silent too, so these were merged outright:collectionFormatcsvandpipesconsolidated — different wire formatdefault$schema,$id,$dynamicRefdeprecatedparser's ownEqualsneeded no change — the guard confirms it already covered every field but the deliberately excludedStringForm.Breaking
Structural hashes change for most non-trivial schemas. The hasher reads thirteen more fields and length-frames every value it writes. Nothing in oastools persists a hash across runs — the type documents it as an in-process grouping key, not a stable identifier — but treat any stored hash as invalidated. Recorded in the pending release note.
joinerandbuildersemantic deduplication now keep apart schemas they used to consolidate. That was wrong output; expect a larger schema count when joining documents using the affected fields.Testing
Components/MediaTypesfail with an actionable messagemake check) — 10203 tests, up from 9378Related Issues
Fixes #414
Fixes #410