Native YAML decoding on the stock parser (WIP) - #26
Draft
reuvenharrison wants to merge 32 commits into
Draft
Conversation
Same subtree as the previous spike, but against unmodified go.yaml.in/yaml/v3 instead of our patched fork, and without end positions. That is possible because a block's extent is derivable from start positions: it runs to the line before the next key or sequence item at the same or shallower indentation. Measured against recorded end positions on ~11.9M spans across kin's corpus, oasdiff's, and the GitHub and Stripe specs, the two agree on ~99.98%, and the residual is a trailing blank-or-comment boundary convention rather than a different block. So EndLine/EndColumn is a convenience, not a requirement, and dropping it removes the reason to carry a parser fork at all. Origins still match the current path on Key.Line/Column, Fields and Sequences, verified against applyOrigins on the same document. Only the end positions are absent, by design. One thing the depth test caught: setChildOriginKeys stamped the Content map itself rather than its entries, so nested media types had an Origin with no Key. Map-valued fields are decoded by the generic map decoder, which has no hook, so the parent descends into them.
Completes the decode side. Every type that implements UnmarshalJSON now
has a node-based counterpart, against unmodified go.yaml.in/yaml/v3.
They fall into four shapes, which is why this is tractable:
24 shadow-struct types, generated -- decode into a shadow, collect the
keys the struct does not declare, read the origin off the node.
The JSON versions restate the known set as ~60 lines of deletes in
Schema's case, which silently misfiles a field added to the struct
and forgotten in the list; here it comes off the struct tags.
9 $ref wrappers, one helper plus thin methods. SchemaRef differs: no
summary/description, and OAS 3.1 keyword siblings held for merging
after resolution.
3 maplike collections, one generic helper. The JSON version
re-marshals every entry back to JSON and re-parses it, once per
entry; the child node goes straight to the child decoder here.
4 special: Header defers to Parameter, and Types, BoolSchema and
ExclusiveBound are union-typed scalars.
Verified across every full document in testdata -- 18 of them -- by
decoding each both ways and comparing. Origins reach path items and
operations with the key positions oasdiff reads.
One test bug worth noting because it looked like a defect: Operations()
reports methods uppercased while the origin names the key as written, so
the assertion needed lowering, not the code.
The deepObject branch of urlValuesDecoder.DecodeObject compiled the parameter-name matcher `^<param>\[` inside the `for key := range params` loop, recompiling it once per query key. The pattern depends only on the spec-defined parameter name (constant for the loop), not on the loop variable, so it can be compiled once and reused. Because the query-key count is attacker-controlled and unbounded, the per-key recompilation turned a single request into N regexp.MustCompile calls per deepObject parameter, consuming CPU (and allocations) during request validation before any handler runs — an uncontrolled-resource -consumption DoS (CWE-400). Compile the matcher once per DecodeObject call and reuse the *regexp.Regexp for every key, mirroring the package-level deepObjectBracketRE. Matching semantics are unchanged: the pattern still derives from the parameter name via regexp.QuoteMeta (panic-safe for any name), and *regexp.Regexp is safe for reuse. Benchmarked on the deepObject decode path (Go 1.25, darwin/arm64, M4 Pro), the hoist also makes the path markedly faster across 1k–100k junk query keys: ~13–19x less CPU, ~18–26x fewer bytes, and ~44–48x fewer allocations per request. At 100k keys a single request drops from ~147 ms / ~375 MB to ~10 ms / ~14 MB. Signed-off-by: Matías Insaurralde <matias@insaurral.de>
Signed-off-by: Pierre Fenoll <pierrefenoll@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
unmarshal now decodes through UnmarshalYAML on the stock parser, falling
back to the json path for what yaml will not accept -- most importantly
duplicate keys, which json resolves last-one-wins and yaml rejects.
Status: 24 failures in openapi3, of which 13 are the deliberate
consequence of the design (tests asserting EndLine/EndColumn, which the
stock parser does not record and which the consumer now derives). The
remaining 11 are real work, in four groups:
3 the arbitrary-top-level-key $ref path, which attachOriginToResolved
handles today and the node path does not reach yet
2 nil-versus-empty on maplike collections: the helper always
allocates, so an absent responses no longer fails validation
1 TestUnmarshalError expects the field-level message the yaml
wrapper's internal json step produced; a native decode reports a
yaml-level error instead, which is more accurate for a yaml
document but is a changed contract
5 origin coverage gaps: T, schema-in-additionalProperties, origin in
properties, external-ref root
Two bugs found and fixed on the way, both of which would have been
invisible without the suite. Origins were produced even when the caller
had not asked for them, because the gate read the package-level
IncludeOrigin rather than the flag the Loader passes in -- and the
package global is seeded into NewLoader, so test pollution was making
that look correct. And a parameter inside a sequence had no Key, because
the stamping walked mappings only; a sequence item takes its own first
key, which is the choice the existing origin code makes.
Those 232 files changed because date-shaped scalars were decoding to time.Time -- the previous path disabled YAML 1.1 timestamp resolution via an option on our yaml fork, and stock go-yaml has none. Retagging such scalars before decoding reproduces that, so the fixtures should not move.
A date-shaped scalar such as an OpenAPI `example: 2020-06-11T16:32:50Z` resolves to a time.Time under YAML 1.1, then fails validation as an unhandled type. The previous path avoided this with a DisableTimestamps option on our yaml fork; stock go-yaml has none, so the node tree is parsed first and such scalars retagged as strings before decoding. Explicitly !!timestamp-tagged values are left alone, which is what the fork's option did. Found only by running against the apis-guru corpus: it moved 232 expected-output fixtures, and this accounts for about 100 of them. The rest are a wider version of the same problem, recorded here rather than papered over. The old path normalised every scalar through JSON's type system on its way through the round trip; decoding natively keeps YAML 1.1 semantics, so implicit resolution now applies. A spec with the key 18_24 loads it as the integer 1824, and examples and defaults whose type shifts stop being format-checked. Timestamps were one instance of a family, and the family needs a considered answer rather than another special case.
The comments on the new files read as a changelog -- comparing each function to the implementation it replaces, quoting line counts from the old one, and carrying the measurements that justified the approach. That belongs in the pull request; a reader of the code wants to know what it does now. Kept the rationale a reader cannot recover from the code: why Origin.Key cannot be set by the node that carries it, why timestamp resolution is suppressed, why the origin file is package-level.
The 24 shadow-struct methods differed only by type name, and the 9 $ref wrapper methods only by that and one conditional. They were produced by a throwaway script and committed as if hand-written, which is the worse half of the problem: nothing tied them to the types they mirror, and a new type would have been silently missed. Both now follow the pattern already used for refs.go: a build-ignored generator, an embedded template, a DO NOT EDIT header, and a go:generate directive. The $ref methods go into refs.tmpl, alongside the UnmarshalJSON they mirror, so the two cannot drift and a new ref type gets both. The rest come from nativeyaml.tmpl. Generics do not help here. The method must exist on each named type, and the shadow type that stops the decoder recursing cannot be expressed from a type parameter, so the declarations have to be written either way -- by hand or by a generator. Left hand-written: the maplike collections and the union-typed values, which do not share the shape. Generation is idempotent, and moving the $ref methods into the template fixed a failure, since the generated version handles extra keys as the JSON one does.
I claimed generics could not help here because the shadow type that stops the decoder recursing cannot come from a type parameter. That is true, but it does not follow that the whole body has to be written per type: the shadow can be declared locally and the receiver converted to it, which decodes in place, leaving only the two fields the decoder skips to be set. decodeMapping takes it from there. Bodies drop from twelve lines to six, and the generated file from 371 lines to 301. Keeping the generator rather than hand-writing the six-line methods, because the body will change again -- origin coverage is still incomplete -- and 24 copies that must move together are what a template is for. Size is close either way (399 against 171); lockstep is the reason to prefer this one. Behaviour is unchanged: same 23 failures before and after, and a probe comparing the two forms on the same document agreed on content, extensions and origin.
Two of the four classes of real failure.
Operation has to tell an omitted responses from an explicitly null one:
the first is allowed from OAS 3.1, the second never is. A null node
decodes to an empty Responses, indistinguishable from `{}` without the
flag, so Operation moves out of the generated set and checks the node.
A $ref to a schema under an arbitrary top-level key resolves through
plain extension data, which carries no positions. The loader now retains
the parsed node tree rather than an origin tree, and attachOriginToResolved
walks it to the fragment and decodes that subtree, which runs its
UnmarshalYAML and produces origins natively.
Real failures 11 -> 7. The remaining are five origin coverage gaps and
one changed error message; separately 13 tests assert end positions,
which are unset until the parser records them again.
Signed-off-by: Pierre Fenoll <pierrefenoll@gmail.com>
The remaining scalar divergence. Comparing the two decode paths across YAML's scalar forms, the differences were narrower than feared: integers old float64, new int -- every notation (42, 0x2A, 4_2) .inf/.nan old errored, new accepts timestamps handled already map keys old coerced 18_24 to "1824"; new keeps the text The last is the previous path being wrong, and is why a fixture asserting `map key "18_24" not found` disappeared: the lookup now succeeds. Fixed the integer one, since a consumer's type switch should not depend on how a number was written. Applied to any-typed values only: extension values, and the any fields the decoder fills directly. Not by retagging the nodes, which was the obvious approach and is wrong. Retagging !!int as !!float does give float64 in an any, but it also routes declared integer fields through float64, and 9007199254740993 comes back as ...992. The previous path preserved those, so a blanket retag would have traded one divergence for a worse one. .inf and .nan are left accepted. They cannot be represented in JSON, so the old path failed to load such a document at all; loading it is not obviously worse, and a beta is the right place to find out.
T is the document root, so nothing above it can stamp its Origin.Key. It takes its own first key, the rule a sequence item already follows. A wrapper that carries no Origin of its own is now descended into, so the schema inside an additionalProperties gets the key that heads it. Value covers the $ref wrappers, Schema covers BoolSchema. TestOrigin_OriginExistsInProperties asserted that a document with a property named __origin__ fails to load, which was true while that name was injected into the document to carry positions. Nothing is injected now, so the document loads and the property is ordinary. Rewritten to pin that, since it is a fix rather than a regression and should not look like a test quietly relaxed. Real failures 7 -> 4: two arbitrary-top-level-key ref cases, an external ref root origin, and one changed error message.
It guarded a collision that no longer exists: __origin__ was injected into the document to carry positions, so a property of that name broke the load. Positions come off the node now, and the name is ordinary.
Four tests existed only to check that the injected __origin__ key did not leak into any-typed values: AnyFieldsStripped, ExtensionValuesStripped, MaplikeNoOriginKey, NoSpuriousOriginsInComponents. Nothing is injected now, so they passed vacuously while implying the mechanism was still there. Also removed the leak assertions trailing two tests that are otherwise about positions, and the comments describing the injection in three more. The name no longer appears anywhere in the package. Deleting rather than keeping: a test that cannot fail is worse than no test, because it reads as coverage. What replaced their subject is covered by the native scalar and origin tests, which assert what the values are rather than what they are not.
Origin.Key is the key heading a mapping in its parent, stamped by that parent. A root has none: an externally $ref'd file may be a bare schema, whose whole content is the element. It takes the root node's position and an empty name. Applied only when Key is still unset, so T keeps the first-key rule it sets for itself. Real failures 4 -> 3.
265 of origin.go's 331 lines consumed a format nothing produces any more: originFromSeq parsed the injected sequence, and applyOrigins with its three helpers walked a separately-built tree to reapply what it found, with recordMapKeyLocations, isScalarValuedMapField, jsonTagName and toInt supporting them. Its last caller was the test comparing the two decode paths. That test did its job -- it caught a $ref wrapper and its value needing to share an Origin, and Key.EndLine having to come from the value node -- but it was the only thing keeping the layer compiling, and it measured against an implementation that no longer runs. Correctness is now pinned by tests asserting what origins are, rather than that they match something removed. Then grouped by concern, since origin.go was left holding only types while everything that builds origins sat in native_yaml.go next to the decoding helpers: origin construction and stamping moved to origin.go, and native_yaml.go keeps extension collection and scalar reconciliation.
Author
openapi2 is untouched, and is a possible follow-upThis migrates Dropping those dependencies entirely needs Also still on the wrapper: Deliberately out of scope here. Worth doing as its own change, where it can be reviewed against openapi2's own behaviour rather than buried in this one. |
github.com/oasdiff/yaml3 is a fork of the same upstream as go.yaml.in/yaml/v3 and exposes the same API, so these are import swaps. openapi3 and openapi3filter no longer reference it. The go.mod entries stay: openapi2 and cmd/validate still use the wrapper, and openapi2 is out of scope here. Noted on the PR as a follow-up. Two test files still use the wrapper's Unmarshal, which takes DecodeOpts and has no stock equivalent; they need rewriting rather than swapping. Verified neutral: openapi3 unchanged at 18 failures, openapi3filter at 1 before and after.
normalizeAnyFields handled fields of type any but not []any or map[string]any, so an integer example became a float64 while the enum it must match stayed an int, and a schema failed against its own allowed values. openapi3filter caught it; openapi3 did not. Fixing that surfaced a gap in the enum comparison: it converts json.Number and int64 to float64 before comparing, but falls through to an exact DeepEqual for a plain int. A Go int passed to VisitJSON therefore never matched a float64 enum, which is what a document loaded as JSON has always produced. Added the case alongside int64. TestIssue646 exercised that path only because it decoded with the raw yaml parser, which bypasses the unmarshalers and left the enum as ints on both sides. Going through UnmarshalYAML gives it JSON-shaped numbers like any other document, so it now compares an int against a float64 enum -- the case a caller loading real JSON hits. openapi3filter 1 -> 0. openapi3 unchanged at 18.
openapi2 keeps its JSON round trip and every UnmarshalJSON exactly as
they are. Only the YAML front half changes: the stock parser produces a
node tree, and the document is marshalled from it. No UnmarshalYAML
methods, no shared helpers, no behaviour change intended.
Two things must be reconciled before the tree can become JSON, which is
what the wrapper was doing:
a date-shaped scalar resolves to a timestamp, which has no JSON form
a non-string mapping key decodes to a map[any]any that json.Marshal
rejects -- and an unquoted 200: is how most specs write a status code
Both are retagged as strings, an explicit tag left alone.
The key case is worth noting because openapi2's own suite did not catch
it: the corpus quotes its status codes. A probe with an unquoted 200:
failed with "json: unsupported type: map[interface {}]interface {}"
before the retag.
cmd/validate gets openapi2.UnmarshalFromData rather than reimplementing
the round trip, mirroring the Loader the v3 side already exposes.
No non-test code references github.com/oasdiff/yaml or yaml3 now. The
go.mod entries stay until eight test files move, which is where this
stops being a minimal change.
Both forks are gone from go.mod. What the wrapper was providing is 60 lines in internal/yamlconv: a type that describes itself with json tags and UnmarshalJSON reaches YAML through JSON, and the two things YAML resolves that JSON cannot carry -- a timestamp, and a non-string mapping key -- are retagged first. openapi2/marsh.go now calls it instead of carrying its own copy. The eight test files used the wrapper for the same reason: its Marshal and Unmarshal went through JSON, so the types' methods ran. Stock go-yaml would not call them, and extensions would have been dropped from the output, so the call sites move to the helper rather than to yaml.Marshal. Unmarshal loses the wrapper's unused first return value; Marshal keeps its arity. One test stops being skipped. issue883 round-trips a document through stock go-yaml and gave up at the decode, since maplike types had no yaml Unmarshaler -- which is what the native loader added. Removing the skip leaves the test passing, so the TODO it carried is answered rather than merely relocated. openapi3 stays at its known 18 failures, all in the origin end-position expectations. Every other package is green.
Location.EndLine and EndColumn were left at zero when the native loader landed, since go-yaml reports where a node starts and nothing about where it stops. They are derivable: a block ends on the last line any part of it occupies, which is the largest line among its descendants. Reading that off the tree, rather than from indentation, is what makes a sequence item work. A parameter's key location is the item's first key, which sits at the same column as the keys following it, so no column comparison can separate the end of the item from the start of its own second field. Its subtree ends where the item ends either way. The column-based version of this got parameters wrong; the tree-based one is also shorter. Two origin behaviours that the JSON path had are restored here, both found by tests rather than by inspection: An alias reports where its content was defined. Alias1: *base decodes to the anchored schema, so its origin is Base at line 7, not the line the alias sits on. The anchor's key is recorded during the same pass that measures extents. A scalar-valued map records its keys on the enclosing struct. A map[string]string -- scopes on an OAuth flow -- decodes to a plain map with nowhere to hang an Origin, so its key locations go on the parent under the field name, sorted. This was the behaviour of the deleted recordMapKeyLocations, which the native path had not taken over. openapi3 goes from 18 failures to 3: the two arbitrary-top-level-key ref origins, and one error-message string. oasdiff against this drops from 13 to 2.
A $ref to a schema under an arbitrary top-level key resolves through T.Extensions, which decodes as plain data and carries no positions. The loader recovers the origins by decoding that subtree from the retained node tree, and two things about that decode were wrong. It stamped the wrong file. originFileVar still named the document being loaded, not the one the subtree came from, so an external ref reported openapi.yaml where the schema lives in schemas.yaml. The retained tree now carries the file and the end index it was measured against, and the decode runs with those in place. And it produced no key location. A value's key is stamped by the mapping above it, which does not run on this path -- the last fragment part is that key, so the walk keeps it and stamps it afterwards. kin-openapi is green. oasdiff against it is green too, once its copy of the error-message expectation moves with the one here.
…-stock-goyaml # Conflicts: # openapi3/loader.go # openapi3/origin.go
The comment cited IncludeOrigin as precedent for a package-level var. There are two of those, and the one that is not deprecated is per-Loader precisely because sharing it was unsafe, so the precedent argued the opposite of what it claimed. The restriction is that concurrent decodes are unsafe even with separate Loaders.
The merge dropped two doc comments that the generated API listing then caught. FieldLocations had been grafted in between Location's comment and its type, so the comment documented the wrong symbol, and upstream's note on why Sequences stays a map while Fields became a slice was lost with the conflict resolution. Both restored, .github/docs regenerated. Import grouping per goimports-reviser, and bytes.SplitSeq in the end index per modernize. One hunk here is not part of this change: modernize also flags a loop in validation_error_test.go that master has not fixed, and since CI runs it with -fix and then diffs, the PR cannot go green without carrying it.
TestIssue741 loads concurrently, each goroutine with its own Loader, and the race detector caught all three package-level origin variables being written by every decode. The path this replaces passed the file as an argument to applyOrigins, so this was a regression, and CI would not have let it through. They are package-level because UnmarshalYAML receives a node and nothing else, with no way to carry per-decode state through the call. A mutex held for the length of the decode makes that safe. The cost is real and worth naming: decodes now serialise, even between separate Loaders. Single-decode throughput is unaffected, which is what the numbers in the description measure, but a caller parsing several documents at once no longer does so in parallel. Two ways out, neither taken here. A file recorded on the node would let the callback read it, since it already has the node, but go-yaml's Node has no such field. Filling the file in by walking the document after the decode would remove that one variable, but not the other two, which carry the origins-enabled flag and the end index.
The guard asserted that yaml. appears exactly twice in openapi3 outside origin.go, which kept callers from bypassing unmarshal back when one function did all the decoding. Native decoding spreads that across the generated methods, the shared helpers and the end index, so the literal count cannot hold. The intent still can: nothing outside the decode layer should touch yaml directly. The check now names those files and requires zero references anywhere else, which is what it was really protecting.
wc -l pads its output on BSD, so the string comparison only held on the GNU wc that CI runs. -eq reads it as a number either way, and the check can be run locally before pushing.
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
3 participants
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.
Draft, not close to merge. Opening it so the work is reviewable and the open problems are visible. Status is stated honestly below; nothing here is finished.
What this is
kin decodes a YAML document by converting it to
map[string]any, re-serialising to JSON text and parsing it again, because these types implementUnmarshalJSONand notUnmarshalYAML. Positions cannot survive that, so they are smuggled through as synthetic__origin__nodes and reapplied afterwards by a reflection walk.This replaces that with
UnmarshalYAMLon every type, decoding straight from the node, against unmodifiedgo.yaml.in/yaml/v3— no fork, no patches.Why it is worth doing
Measured on real types with their real hooks, decoding 400 responses:
json.Unmarshal+ hooksUnmarshalYAML2.54x faster than today, and faster than the JSON fast path, because the hooks re-serialise:
Responses.UnmarshalJSONre-marshals every child back to JSON and re-parses it once per entry, andSchemaRef.UnmarshalJSONparses the same bytes up to four times.It also removes ~495 lines of origin transport across three repos, since a node already carries
Line/Column.Shape of the change
The 41 types fall into four groups, which is what makes this tractable:
Schema.UnmarshalJSONis 91 lines, ~60 of them — which silently misfiles a field added to the struct and forgotten in the list. Here it comes off the struct tags and cannot drift.$refwrappers, one helper plus thin methods.Headerdefers toParameter;Types,BoolSchemaandExclusiveBoundare union-typed scalars.No end positions are used. A block's extent is derivable from start positions — the next key or sequence item at the same or shallower indentation — which agrees with recorded ends on ~99.98% of ~11.9M spans. That is what removes the need for a patched parser.
Open problems
1. YAML 1.1 implicit resolution. The old path normalised every scalar through JSON's type system on the way through the round trip. Decoding natively keeps YAML 1.1 semantics, so implicit resolution now applies:
examplebecame atime.Timeand failed validation — fixed here by retagging before decode, reproducing theDisableTimestampsoption our yaml fork has and stock go-yaml does not18_24loads it as the integer1824Timestamps were one instance of a family. The family needs a considered answer, not another special case. This is the largest open item.
2. 24 failures in
openapi3. 13 are tests assertingEndLine/EndColumn, which is the deliberate design change. The other 11 are real: the arbitrary-top-level-key$refpath, nil-versus-empty on maplike collections (an absentresponsesno longer fails validation), a changed error message, and origin coverage gaps.3. Duplicate keys. JSON resolves a repeated key last-one-wins; YAML rejects it. The json path is kept as a fallback so such documents still load, without origins.
4.
originFileVaris package-level.UnmarshalYAMLreceives a node and nothing else, so the file name cannot be threaded through. This follows the precedent ofIncludeOriginand inherits its concurrency characteristics. Making both per-Loaderis worth doing and is a separate change.Validation so far
testdata(18) decodes identically to the JSON path.Key.Line/Column,FieldsandSequences, compared againstapplyOriginson the same documents.Two bugs the suite caught that would otherwise have shipped: origins were produced when the caller had not asked for them (the gate read the package global rather than the flag the
Loaderpasses, and sinceNewLoaderseeds itself from that global, test pollution made it look correct); and a parameter inside a sequence had noKey, because stamping walked mappings only.