Fix decoding of merge tags.
This addresses a number of related issues on how merge tags
were handled, most notably the case of nested mappings being
merged instead of the outermost value being used as-is.
Closes: #818 #826
diff --git a/decode.go b/decode.go
index df36e3a..c59dea4 100644
--- a/decode.go
+++ b/decode.go
@@ -320,6 +320,8 @@
decodeCount int
aliasCount int
aliasDepth int
+
+ mergedFields map[interface{}]bool
}
var (
@@ -808,6 +810,11 @@
}
}
+ mergedFields := d.mergedFields
+ d.mergedFields = nil
+
+ var mergeNode *Node
+
mapIsNew := false
if out.IsNil() {
out.Set(reflect.MakeMap(outt))
@@ -815,11 +822,18 @@
}
for i := 0; i < l; i += 2 {
if isMerge(n.Content[i]) {
- d.merge(n.Content[i+1], out)
+ mergeNode = n.Content[i+1]
continue
}
k := reflect.New(kt).Elem()
if d.unmarshal(n.Content[i], k) {
+ if mergedFields != nil {
+ ki := k.Interface()
+ if mergedFields[ki] {
+ continue
+ }
+ mergedFields[ki] = true
+ }
kkind := k.Kind()
if kkind == reflect.Interface {
kkind = k.Elem().Kind()
@@ -833,6 +847,12 @@
}
}
}
+
+ d.mergedFields = mergedFields
+ if mergeNode != nil {
+ d.merge(n, mergeNode, out)
+ }
+
d.stringMapType = stringMapType
d.generalMapType = generalMapType
return true
@@ -844,7 +864,8 @@
}
l := len(n.Content)
for i := 0; i < l; i += 2 {
- if n.Content[i].ShortTag() != strTag {
+ shortTag := n.Content[i].ShortTag()
+ if shortTag != strTag && shortTag != mergeTag {
return false
}
}
@@ -861,7 +882,6 @@
var elemType reflect.Type
if sinfo.InlineMap != -1 {
inlineMap = out.Field(sinfo.InlineMap)
- inlineMap.Set(reflect.New(inlineMap.Type()).Elem())
elemType = inlineMap.Type().Elem()
}
@@ -870,6 +890,9 @@
d.prepare(n, field)
}
+ mergedFields := d.mergedFields
+ d.mergedFields = nil
+ var mergeNode *Node
var doneFields []bool
if d.uniqueKeys {
doneFields = make([]bool, len(sinfo.FieldsList))
@@ -879,13 +902,20 @@
for i := 0; i < l; i += 2 {
ni := n.Content[i]
if isMerge(ni) {
- d.merge(n.Content[i+1], out)
+ mergeNode = n.Content[i+1]
continue
}
if !d.unmarshal(ni, name) {
continue
}
- if info, ok := sinfo.FieldsMap[name.String()]; ok {
+ sname := name.String()
+ if mergedFields != nil {
+ if mergedFields[sname] {
+ continue
+ }
+ mergedFields[sname] = true
+ }
+ if info, ok := sinfo.FieldsMap[sname]; ok {
if d.uniqueKeys {
if doneFields[info.Id] {
d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s already set in type %s", ni.Line, name.String(), out.Type()))
@@ -911,6 +941,11 @@
d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s not found in type %s", ni.Line, name.String(), out.Type()))
}
}
+
+ d.mergedFields = mergedFields
+ if mergeNode != nil {
+ d.merge(n, mergeNode, out)
+ }
return true
}
@@ -918,19 +953,29 @@
failf("map merge requires map or sequence of maps as the value")
}
-func (d *decoder) merge(n *Node, out reflect.Value) {
- switch n.Kind {
+func (d *decoder) merge(parent *Node, merge *Node, out reflect.Value) {
+ mergedFields := d.mergedFields
+ if mergedFields == nil {
+ d.mergedFields = make(map[interface{}]bool)
+ for i := 0; i < len(parent.Content); i += 2 {
+ k := reflect.New(ifaceType).Elem()
+ if d.unmarshal(parent.Content[i], k) {
+ d.mergedFields[k.Interface()] = true
+ }
+ }
+ }
+
+ switch merge.Kind {
case MappingNode:
- d.unmarshal(n, out)
+ d.unmarshal(merge, out)
case AliasNode:
- if n.Alias != nil && n.Alias.Kind != MappingNode {
+ if merge.Alias != nil && merge.Alias.Kind != MappingNode {
failWantMap()
}
- d.unmarshal(n, out)
+ d.unmarshal(merge, out)
case SequenceNode:
- // Step backwards as earlier nodes take precedence.
- for i := len(n.Content) - 1; i >= 0; i-- {
- ni := n.Content[i]
+ for i := 0; i < len(merge.Content); i++ {
+ ni := merge.Content[i]
if ni.Kind == AliasNode {
if ni.Alias != nil && ni.Alias.Kind != MappingNode {
failWantMap()
@@ -943,6 +988,8 @@
default:
failWantMap()
}
+
+ d.mergedFields = mergedFields
}
func isMerge(n *Node) bool {
diff --git a/decode_test.go b/decode_test.go
index 5ade550..5f65e43 100644
--- a/decode_test.go
+++ b/decode_test.go
@@ -767,7 +767,7 @@
M{"a": 123456e1},
}, {
"a: 123456E1\n",
- M{"a": 123456E1},
+ M{"a": 123456e1},
},
// yaml-test-suite 3GZX: Spec Example 7.1. Alias Nodes
{
@@ -802,7 +802,6 @@
"c": []interface{}{"d", "e"},
},
},
-
}
type M map[string]interface{}
@@ -950,14 +949,14 @@
{"a: 1\nb: 2\nc 2\nd: 3\n", "^yaml: line 3: could not find expected ':'$"},
{
"a: &a [00,00,00,00,00,00,00,00,00]\n" +
- "b: &b [*a,*a,*a,*a,*a,*a,*a,*a,*a]\n" +
- "c: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b]\n" +
- "d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c]\n" +
- "e: &e [*d,*d,*d,*d,*d,*d,*d,*d,*d]\n" +
- "f: &f [*e,*e,*e,*e,*e,*e,*e,*e,*e]\n" +
- "g: &g [*f,*f,*f,*f,*f,*f,*f,*f,*f]\n" +
- "h: &h [*g,*g,*g,*g,*g,*g,*g,*g,*g]\n" +
- "i: &i [*h,*h,*h,*h,*h,*h,*h,*h,*h]\n",
+ "b: &b [*a,*a,*a,*a,*a,*a,*a,*a,*a]\n" +
+ "c: &c [*b,*b,*b,*b,*b,*b,*b,*b,*b]\n" +
+ "d: &d [*c,*c,*c,*c,*c,*c,*c,*c,*c]\n" +
+ "e: &e [*d,*d,*d,*d,*d,*d,*d,*d,*d]\n" +
+ "f: &f [*e,*e,*e,*e,*e,*e,*e,*e,*e]\n" +
+ "g: &g [*f,*f,*f,*f,*f,*f,*f,*f,*f]\n" +
+ "h: &h [*g,*g,*g,*g,*g,*g,*g,*g,*g]\n" +
+ "i: &i [*h,*h,*h,*h,*h,*h,*h,*h,*h]\n",
"yaml: document contains excessive aliasing",
},
}
@@ -1391,7 +1390,7 @@
`
func (s *S) TestMerge(c *C) {
- var want = map[interface{}]interface{}{
+ var want = map[string]interface{}{
"x": 1,
"y": 2,
"r": 10,
@@ -1436,7 +1435,103 @@
}
}
-var unmarshalNullTests = []struct{ input string; pristine, expected func() interface{} }{{
+var mergeTestsNested = `
+mergeouter1: &mergeouter1
+ d: 40
+ e: 50
+
+mergeouter2: &mergeouter2
+ e: 5
+ f: 6
+ g: 70
+
+mergeinner1: &mergeinner1
+ <<: *mergeouter1
+ inner:
+ a: 1
+ b: 2
+
+mergeinner2: &mergeinner2
+ <<: *mergeouter2
+ inner:
+ a: -1
+ b: -2
+
+outer:
+ <<: [*mergeinner1, *mergeinner2]
+ f: 60
+ inner:
+ a: 10
+`
+
+func (s *S) TestMergeNestedStruct(c *C) {
+ // Issue #818: Merging used to just unmarshal twice on the target
+ // value, which worked for maps as these were replaced by the new map,
+ // but not on struct values as these are preserved. This resulted in
+ // the nested data from the merged map to be mixed up with the data
+ // from the map being merged into.
+ //
+ // This test also prevents two potential bugs from showing up:
+ //
+ // 1) A simple implementation might just zero out the nested value
+ // before unmarshaling the second time, but this would clobber previous
+ // data that is usually respected ({C: 30} below).
+ //
+ // 2) A simple implementation might attempt to handle the key skipping
+ // directly by iterating over the merging map without recursion, but
+ // there are more complex cases that require recursion.
+ //
+ // Quick summary of the fields:
+ //
+ // - A must come from outer and not overriden
+ // - B must not be set as its in the ignored merge
+ // - C should still be set as it's preset in the value
+ // - D should be set from the recursive merge
+ // - E should be set from the first recursive merge, ignored on the second
+ // - F should be set in the inlined map from outer, ignored later
+ // - G should be set in the inlined map from the second recursive merge
+ //
+
+ type Inner struct {
+ A, B, C int
+ }
+ type Outer struct {
+ D, E int
+ Inner Inner
+ Inline map[string]int `yaml:",inline"`
+ }
+ type Data struct {
+ Outer Outer
+ }
+
+ test := Data{Outer{0, 0, Inner{C: 30}, nil}}
+ want := Data{Outer{40, 50, Inner{A: 10, C: 30}, map[string]int{"f": 60, "g": 70}}}
+
+ err := yaml.Unmarshal([]byte(mergeTestsNested), &test)
+ c.Assert(err, IsNil)
+ c.Assert(test, DeepEquals, want)
+
+ // Repeat test with a map.
+
+ var testm map[string]interface{}
+ var wantm = map[string]interface {} {
+ "f": 60,
+ "inner": map[string]interface{}{
+ "a": 10,
+ },
+ "d": 40,
+ "e": 50,
+ "g": 70,
+ }
+ err = yaml.Unmarshal([]byte(mergeTestsNested), &testm)
+ c.Assert(err, IsNil)
+ c.Assert(testm["outer"], DeepEquals, wantm)
+}
+
+var unmarshalNullTests = []struct {
+ input string
+ pristine, expected func() interface{}
+}{{
"null",
func() interface{} { var v interface{}; v = "v"; return &v },
func() interface{} { var v interface{}; v = nil; return &v },
@@ -1487,7 +1582,7 @@
func (s *S) TestUnmarshalPreservesData(c *C) {
var v struct {
A, B int
- C int `yaml:"-"`
+ C int `yaml:"-"`
}
v.A = 42
v.C = 88
