From 5edc3ded41385ca1b9a80339d2a070e4d0a17cb6 Mon Sep 17 00:00:00 2001 From: Roger Peppe Date: Sun, 7 Jan 2018 10:22:59 +0000 Subject: [PATCH] Enforce key uniqueness in strict mode This is similar to PR #285 but somewhat more efficient because it does not require a new map to be created for every struct unmarshal. Fixes #284. --- .travis.yml | 3 ++ decode.go | 25 ++++++++++++-- decode_test.go | 88 +++++++++++++++++++++++++++++++++++++++++++++----- yaml.go | 16 +++++++-- 4 files changed, 118 insertions(+), 14 deletions(-) diff --git a/.travis.yml b/.travis.yml index 004172a2..9f556934 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,6 +4,9 @@ go: - 1.4 - 1.5 - 1.6 + - 1.7 + - 1.8 + - 1.9 - tip go_import_path: gopkg.in/yaml.v2 diff --git a/decode.go b/decode.go index 7993e4a8..6121143a 100644 --- a/decode.go +++ b/decode.go @@ -580,7 +580,7 @@ func (d *decoder) mapping(n *node, out reflect.Value) (good bool) { } e := reflect.New(et).Elem() if d.unmarshal(n.children[i+1], e) { - out.SetMapIndex(k, e) + d.setMapIndex(n.children[i+1], out, k, e) } } } @@ -588,6 +588,14 @@ func (d *decoder) mapping(n *node, out reflect.Value) (good bool) { return true } +func (d *decoder) setMapIndex(n *node, out, k, v reflect.Value) { + if d.strict && out.MapIndex(k) != zeroValue { + d.terrors = append(d.terrors, fmt.Sprintf("line %d: key %#v already set in map", n.line+1, k.Interface())) + return + } + out.SetMapIndex(k, v) +} + func (d *decoder) mappingSlice(n *node, out reflect.Value) (good bool) { outt := out.Type() if outt.Elem() != mapItemType { @@ -635,6 +643,10 @@ func (d *decoder) mappingStruct(n *node, out reflect.Value) (good bool) { elemType = inlineMap.Type().Elem() } + var doneFields []bool + if d.strict { + doneFields = make([]bool, len(sinfo.FieldsList)) + } for i := 0; i < l; i += 2 { ni := n.children[i] if isMerge(ni) { @@ -645,6 +657,13 @@ func (d *decoder) mappingStruct(n *node, out reflect.Value) (good bool) { continue } if info, ok := sinfo.FieldsMap[name.String()]; ok { + if d.strict { + if doneFields[info.Id] { + d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s already set in type %s", ni.line+1, name.String(), out.Type())) + continue + } + doneFields[info.Id] = true + } var field reflect.Value if info.Inline == nil { field = out.Field(info.Num) @@ -658,9 +677,9 @@ func (d *decoder) mappingStruct(n *node, out reflect.Value) (good bool) { } value := reflect.New(elemType).Elem() d.unmarshal(n.children[i+1], value) - inlineMap.SetMapIndex(name, value) + d.setMapIndex(n.children[i+1], inlineMap, name, value) } else if d.strict { - d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s not found in struct %s", ni.line+1, name.String(), out.Type())) + d.terrors = append(d.terrors, fmt.Sprintf("line %d: field %s not found in type %s", ni.line+1, name.String(), out.Type())) } } return true diff --git a/decode_test.go b/decode_test.go index 84515f37..cefeb757 100644 --- a/decode_test.go +++ b/decode_test.go @@ -1065,15 +1065,87 @@ func (s *S) TestUnmarshalSliceOnPreset(c *C) { c.Assert(v.A, DeepEquals, []int{2}) } +var unmarshalStrictTests = []struct { + data string + value interface{} + error string +}{{ + data: "a: 1\nc: 2\n", + value: struct{ A, B int }{A: 1}, + error: `yaml: unmarshal errors:\n line 2: field c not found in type struct { A int; B int }`, +}, { + data: "a: 1\nb: 2\na: 3\n", + value: struct{ A, B int }{A: 3, B: 2}, + error: `yaml: unmarshal errors:\n line 3: field a already set in type struct { A int; B int }`, +}, { + data: "c: 3\na: 1\nb: 2\nc: 4\n", + value: struct { + A int + inlineB `yaml:",inline"` + }{ + A: 1, + inlineB: inlineB{ + B: 2, + inlineC: inlineC{ + C: 4, + }, + }, + }, + error: `yaml: unmarshal errors:\n line 4: field c already set in type struct { A int; yaml_test.inlineB "yaml:\\",inline\\"" }`, +}, { + data: "c: 0\na: 1\nb: 2\nc: 1\n", + value: struct { + A int + inlineB `yaml:",inline"` + }{ + A: 1, + inlineB: inlineB{ + B: 2, + inlineC: inlineC{ + C: 1, + }, + }, + }, + error: `yaml: unmarshal errors:\n line 4: field c already set in type struct { A int; yaml_test.inlineB "yaml:\\",inline\\"" }`, +}, { + data: "c: 1\na: 1\nb: 2\nc: 3\n", + value: struct { + A int + M map[string]interface{} `yaml:",inline"` + }{ + A: 1, + M: map[string]interface{}{ + "b": 2, + "c": 3, + }, + }, + error: `yaml: unmarshal errors:\n line 4: key "c" already set in map`, +}, { + data: "a: 1\n9: 2\nnull: 3\n9: 4", + value: map[interface{}]interface{}{ + "a": 1, + nil: 3, + 9: 4, + }, + error: `yaml: unmarshal errors:\n line 4: key 9 already set in map`, +}} + func (s *S) TestUnmarshalStrict(c *C) { - v := struct{ A, B int }{} - - err := yaml.UnmarshalStrict([]byte("a: 1\nb: 2"), &v) - c.Check(err, IsNil) - err = yaml.Unmarshal([]byte("a: 1\nb: 2\nc: 3"), &v) - c.Check(err, IsNil) - err = yaml.UnmarshalStrict([]byte("a: 1\nb: 2\nc: 3"), &v) - c.Check(err, ErrorMatches, "yaml: unmarshal errors:\n line 3: field c not found in struct struct { A int; B int }") + for i, item := range unmarshalStrictTests { + c.Logf("test %d: %q", i, item.data) + // First test that normal Unmarshal unmarshals to the expected value. + t := reflect.ValueOf(item.value).Type() + value := reflect.New(t) + err := yaml.Unmarshal([]byte(item.data), value.Interface()) + c.Assert(err, Equals, nil) + c.Assert(value.Elem().Interface(), DeepEquals, item.value) + + // Then test that UnmarshalStrict fails on the same thing. + t = reflect.ValueOf(item.value).Type() + value = reflect.New(t) + err = yaml.UnmarshalStrict([]byte(item.data), value.Interface()) + c.Assert(err, ErrorMatches, item.error) + } } //var data []byte diff --git a/yaml.go b/yaml.go index c9a2af1d..eb769d42 100644 --- a/yaml.go +++ b/yaml.go @@ -82,7 +82,8 @@ func Unmarshal(in []byte, out interface{}) (err error) { } // UnmarshalStrict is like Unmarshal except that any fields that are found -// in the data that do not have corresponding struct members will result in +// in the data that do not have corresponding struct members, or mapping +// keys that are duplicates, will result in // an error. func UnmarshalStrict(in []byte, out interface{}) (err error) { return unmarshal(in, out, true) @@ -105,7 +106,7 @@ func NewDecoder(r io.Reader) *Decoder { } // SetStrict sets whether strict decoding behaviour is enabled when -// decoding items in the data. By default, decoding is not strict. +// decoding items in the data (see UnmarshalStrict). By default, decoding is not strict. func (dec *Decoder) SetStrict(strict bool) { dec.strict = strict } @@ -257,6 +258,9 @@ type fieldInfo struct { Num int OmitEmpty bool Flow bool + // Id holds the unique field identifier, so we can cheaply + // check for field duplicates without maintaining an extra map. + Id int // Inline holds the field index if the field is part of an inlined struct. Inline []int @@ -336,6 +340,7 @@ func getStructInfo(st reflect.Type) (*structInfo, error) { } else { finfo.Inline = append([]int{i}, finfo.Inline...) } + finfo.Id = len(fieldsList) fieldsMap[finfo.Key] = finfo fieldsList = append(fieldsList, finfo) } @@ -357,11 +362,16 @@ func getStructInfo(st reflect.Type) (*structInfo, error) { return nil, errors.New(msg) } + info.Id = len(fieldsList) fieldsList = append(fieldsList, info) fieldsMap[info.Key] = info } - sinfo = &structInfo{fieldsMap, fieldsList, inlineMap} + sinfo = &structInfo{ + FieldsMap: fieldsMap, + FieldsList: fieldsList, + InlineMap: inlineMap, + } fieldMapMutex.Lock() structMap[st] = sinfo