Repository navigation
Conversation
YAML merge keys must insert only keys that are not already present. Re-decoding a merged alias also blew up nested merges and could resolve inner aliases against a later redefinition. Prefer the cached anchor value when merging, and skip merge keys that would overwrite. Fixes goccy#964
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #972 +/- ##
==========================================
- Coverage 80.65% 79.68% -0.97%
==========================================
Files 22 22
Lines 6845 7040 +195
==========================================
+ Hits 5521 5610 +89
- Misses 905 995 +90
- Partials 419 435 +16 🚀 New features to boost your workflow:
|
kimjooyoon
left a comment
There was a problem hiding this comment.
A native base/head review at e373c6131f19840f764850b843ad755afe5afcc4 confirms the explicit-key and definition-site alias corrections, but finds two data-loss regressions: cached typed anchors disappear in generic merges, and distinct MapSlice keys collapse when AllowDuplicateMapKey is used.
Self-contained 15-case fixture, original outputs and separate cached yaml/v3 reference. The identical fixture distinguishes 6 introduced failures, 2 corrected cases, 3 pre-existing default-parser failures and 4 passing controls. The three default-mode parser failures are not attributed to this PR; the ordered-key findings explicitly use the supported option to reach the changed code.
Execution and review were performed by an AI agent with cached Go1.27.3/macOS arm64, offline modules and bounded small inputs. No SDK/module was downloaded and no production source was modified. Deep alias/performance workloads, full-repository tests, race, vet and hosted CI are outside this review.
| case *ast.AliasNode: | ||
| name := n.Value.GetToken().Value | ||
| if v, ok := d.anchorValueMap[name]; ok && v.IsValid() { | ||
| mergeDecodedMapInto(m, v.Interface()) |
There was a problem hiding this comment.
[P2] Handle the cached anchor type before reporting merge success
With Base map[string]int and Copy interface{} fields, decoding base: &base {k: 1}\ncopy: {<<: *base}\n returns nil on both revisions, but Copy changes from {k:1} on the base to an empty map here. Pointer-to-map and struct anchors behave the same way.
The cache contains the typed value, while mergeDecodedMapInto only handles map[string]interface{} and MapSlice. It silently does nothing for these other representations, then this branch returns success instead of using the mapping nodes. Please make success depend on actually handling the cached representation, with an appropriate fallback for unsupported types.
Three introduced cases plus generic/typed-map/ordered controls and independent reference. These are tiny mappings and do not require an alias-expansion benchmark.
| return err | ||
| } | ||
| mapSlice = append(mapSlice, MapItem{Key: k, Value: v}) | ||
| if idx := orderedMapIndex(mapSlice, fmt.Sprint(k)); idx >= 0 { |
There was a problem hiding this comment.
[P2] Preserve typed MapSlice key identity when locating an entry
With yaml.UnmarshalWithOptions([]byte("1: integer\n\"1\": string\n"), &rows, yaml.AllowDuplicateMapKey()), base produces two MapSlice entries, uint64(1)->"integer" and string("1")->"string". This branch now leaves only uint64(1)->"string". Reversed order and bool(true) versus string("true") also lose an entry; no merge key is present.
fmt.Sprint(k) makes different typed keys compare equal and overwrites an unrelated value. Please preserve the actual key identity when updating ordered entries (YAML scalar equality includes tags). The explicit option is part of this reproduction: default mode already rejects these textual key collisions on both revisions, and that pre-existing behavior is recorded separately.
Option-enabled base/head originals and self-contained regression cases.
…ntity Fall back to the AST when a cached merge anchor is not a usable mapping form was wrong: typed maps were treated as success and dropped. Copy typed maps into generic merge targets via reflect, and only treat the cache hit as done when that succeeds. Also stop collapsing distinct MapSlice keys that share a string form (e.g. uint64(1) vs "1" under AllowDuplicateMapKey).
|
Addressed both P2 notes from @kimjooyoon:
Added regression tests for both cases. |
kimjooyoon
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I re-ran the unchanged15-case fixture at afde3bc8ba5853b79cd405e353d14743bc33dd31 (fixture SHA256 da0c0eb2f956b8edeb1253c610893dedd6973b9a3d218cdaa03814d475253adb). All six cases behind the two P2 findings now pass. The two original intent fixes and four controls still pass. The three pre-existing default-parser failures have exactly the same messages as base and the previous head, so the full fixture command still exits1; those are not new regressions.
Follow-up report and original test events. Both P2 findings are addressed within this fixture.
Execution and review were performed by an AI agent with cached Go1.27.3/macOS arm64 and offline modules. Full-repository tests, race, vet, hosted CI and performance remain outside this review.
Summary
A
<<merge was re-decoding the anchored mapping at the merge site and writing over keys that were already present. That broke three cases from the merge key type:Merges now only fill missing keys. When the merge target is an alias (or a sequence of them) and the anchor is already decoded, that cached value is reused.
Fixes #964
Test plan
<<keeps the local value (map, struct, YAMLToJSON)go test ./...