Fix: option KnownFields not being respected inside custom UnmarshalYAML()#332
Fix: option KnownFields not being respected inside custom UnmarshalYAML()#332stoewer wants to merge 10 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a long-standing gap where loader/decoder options (notably KnownFields) were not respected when a custom UnmarshalYAML implementation delegated to node.Decode(), because Node.Decode always used DefaultOptions. It addresses issue #321 by snapshotting the loader’s options onto the parsed node tree and having Node.Decode inherit from that snapshot; the same propagation is added to ComposeAndResolve.
Changes:
- Add an
optionssnapshot pointer onNodeand makeNode.Decodeinherit loader options when available. - Propagate loader options across the node tree after resolve (both
Loader.LoadandLoader.ComposeAndResolve). - Add regression tests (including multi-document behavior) and a benchmark to quantify overhead.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
yaml_bench_test.go |
Adds benchmark coverage for decode with/without custom UnmarshalYAML and with/without KnownFields. |
node_test.go |
Adds regression tests ensuring Node.Decode inherits KnownFields and a race-focused test. |
internal/libyaml/node.go |
Adds per-node option snapshot storage and updates Node.Decode to use it. |
internal/libyaml/loader.go |
Stamps a snapshot of loader options onto all nodes; ensures SetKnownFields updates loader options. |
internal/libyaml/loader_test.go |
Adds test ensuring ComposeAndResolve propagates options so Node.Decode respects them. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // options is set by propagateLoadOptions when a Loader produces this node. It carries | ||
| // the loader options so that Decode can inherit them in custom UnmarshalYAML functions. | ||
| // Is typically nil for user-constructed nodes. | ||
| options *Options |
There was a problem hiding this comment.
Adding the unexported options *Options field to Node changes the public yaml.Node (it’s a type alias). This can break downstream code that (a) uses unkeyed composite literals for yaml.Node{...} and (b) relies on reflect.DeepEqual/cmp equality against expected node literals (the new hidden pointer will differ). If this compatibility impact is acceptable, it should be explicitly called out (e.g., in release notes); otherwise consider an approach that doesn’t add a field to the public struct (e.g., side-table keyed by *Node, or storing options only on decoder/constructor paths).
stoewer
force-pushed
the
fix-known-fields
branch
from
15d1d7e to
fcafbe5
Compare
|
Force pushed to branch to fix commit messages |
The BenchmarkDecode compares options vs. no options and plain vs custom unmarshal
ingydotnet
force-pushed
the
fix-known-fields
branch
from
fcafbe5 to
e4db7de
Compare
|
Thanks for this. I was away but back now. |
|
@ccoVeille is this at all in conflict with #329? Look forward to your review of this one... |
|
I don't think so. And if does, the changes would be very minimal. I would merge #329 separately. Then about the current PR here, I'm unsure. I feel like it brings a complexity, and something that bounds things together. The problem is this type Config struct {
Name string `yaml:"name"`
}
func (c *Config) UnmarshalYAML(value *yaml.Node) error {
type rawConfig Config
return value.Decode((*rawConfig)(c)) // Decoder ignores options from parent decoder
}For now the workaround would be to call node.Load with option in the WithKnowFields option, instead of calling node.Decode, and expect "parent decoder options" to be inherited all down the stacks of nodes and their leaves. So something like this type Config struct {
Name string `yaml:"name"`
}
func (c *Config) UnmarshalYAML(value *yaml.Node) error {
type rawConfig Config
return value.Load((*rawConfig)(c), yaml.WithKnownFields)
}I feel like the changes in this PR should be considered carefully. Things that works because whatever reason may break if we merge. Here asking to AI might help to figure what is the best. Maybe only Decode method should inherit the parent decoder by default, maybe this behavior could become an option. But I'm out of OSS for personal reasons I shared privately with you. I won't be able to look at this. |
|
Thanks for taking the time to look at this!
Agreed,
That's actually exactly what this PR does:
By "break" do you mean code that relies on |
When a type implements
UnmarshalYAMLand callsnode.Decode()inside it, theloader options (e.g.
KnownFields) were silently dropped.Node.Decodealways constructed a new decoder from
DefaultOptions, so unknown fields wouldpass through unchecked regardless of what the caller configured.
Fixes #321
The fix walks the node tree after parsing and sets a snapshot of the loader's
options onto each node.
Node.Decodethen picks up the snapshot instead offalling back to defaults.
The same propagation is applied in
ComposeAndResolve, which previously hadthe same gap.
The cost is one addition
Optionsstruct copy per document and an O(N)pointer-write walk over the node tree. Benchmarks show little to low sec/op and allocs/op
impact (not reliably measurable); B/op is up ~4% due to an extra pointer field on the Node struct.
Benchmark results (n=10, benchstat)