Skip to content

Commit

Permalink
Fix decoding of merge tags.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@niemeyer
niemeyer committed May 12, 2022
1 parent 496545a commit 539c8e7
Show file tree
Hide file tree
Showing 2 changed files with 168 additions and 26 deletions.
73 changes: 60 additions & 13 deletions decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,8 @@ type decoder struct {
decodeCount int
aliasCount int
aliasDepth int

mergedFields map[interface{}]bool
}

var (
Expand Down Expand Up @@ -808,18 +810,30 @@ func (d *decoder) mapping(n *Node, out reflect.Value) (good bool) {
}
}

mergedFields := d.mergedFields
d.mergedFields = nil

var mergeNode *Node

mapIsNew := false
if out.IsNil() {
out.Set(reflect.MakeMap(outt))
mapIsNew = true
}
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()
Expand All @@ -833,6 +847,12 @@ func (d *decoder) mapping(n *Node, out reflect.Value) (good bool) {
}
}
}

d.mergedFields = mergedFields
if mergeNode != nil {
d.merge(n, mergeNode, out)
}

d.stringMapType = stringMapType
d.generalMapType = generalMapType
return true
Expand All @@ -844,7 +864,8 @@ func isStringMap(n *Node) bool {
}
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
}
}
Expand All @@ -861,7 +882,6 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
var elemType reflect.Type
if sinfo.InlineMap != -1 {
inlineMap = out.Field(sinfo.InlineMap)
inlineMap.Set(reflect.New(inlineMap.Type()).Elem())
elemType = inlineMap.Type().Elem()
}

Expand All @@ -870,6 +890,9 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
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))
Expand All @@ -879,13 +902,20 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
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()))
Expand All @@ -911,26 +941,41 @@ func (d *decoder) mappingStruct(n *Node, out reflect.Value) (good bool) {
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
}

func failWantMap() {
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()
Expand All @@ -943,6 +988,8 @@ func (d *decoder) merge(n *Node, out reflect.Value) {
default:
failWantMap()
}

d.mergedFields = mergedFields
}

func isMerge(n *Node) bool {
Expand Down
121 changes: 108 additions & 13 deletions decode_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -767,7 +767,7 @@ var unmarshalTests = []struct {
M{"a": 123456e1},
}, {
"a: 123456E1\n",
M{"a": 123456E1},
M{"a": 123456e1},
},
// yaml-test-suite 3GZX: Spec Example 7.1. Alias Nodes
{
Expand Down Expand Up @@ -802,7 +802,6 @@ var unmarshalTests = []struct {
"c": []interface{}{"d", "e"},
},
},

}

type M map[string]interface{}
Expand Down Expand Up @@ -950,14 +949,14 @@ var unmarshalErrorTests = []struct {
{"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",
},
}
Expand Down Expand Up @@ -1391,7 +1390,7 @@ inlineSequenceMap:
`

func (s *S) TestMerge(c *C) {
var want = map[interface{}]interface{}{
var want = map[string]interface{}{
"x": 1,
"y": 2,
"r": 10,
Expand Down Expand Up @@ -1436,7 +1435,103 @@ func (s *S) TestMergeStruct(c *C) {
}
}

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 },
Expand Down Expand Up @@ -1487,7 +1582,7 @@ func (s *S) TestUnmarshalNull(c *C) {
func (s *S) TestUnmarshalPreservesData(c *C) {
var v struct {
A, B int
C int `yaml:"-"`
C int `yaml:"-"`
}
v.A = 42
v.C = 88
Expand Down

0 comments on commit 539c8e7

Please sign in to comment.