From 6502ffef22494b1251814c83a3c25accd7de0897 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 13 Sep 2016 16:59:55 -0400 Subject: [PATCH 1/7] Add failing test for missing fields when decoding A JSON object with a single entry containing a single object fails to decode. --- decoder_test.go | 50 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/decoder_test.go b/decoder_test.go index 39ea046..6476ed8 100644 --- a/decoder_test.go +++ b/decoder_test.go @@ -851,3 +851,53 @@ func TestDecode_topLevelKeys(t *testing.T) { t.Errorf("bad source: %s", templates.Templates[1].Source) } } + +func TestDecode_structureMixedFields(t *testing.T) { + jsonConfig := `{ + "vars": { + "var_1": { + "default": { + "key1": "a", + "key2": "b" + } + }, + "var_2": { + "description": "an extra field is required", + "default": { + "key1": "a", + "key2": "b" + } + } + } +} +` + + type V struct { + Name string `hcl:",key"` + Description string + Default map[string]string + } + + type Root struct { + Vars []*V + } + + var r Root + + err := Decode(&r, jsonConfig) + if err != nil { + t.Fatalf("err: %s", err) + } + + if len(r.Vars) != 2 { + t.Fatalf("expected 2 Vars, got %d", len(r.Vars)) + } + + if r.Vars[1].Default == nil { + t.Fatalf("failed to decode Default field in %#v", r.Vars[1]) + } + + if !reflect.DeepEqual(r.Vars[0].Default, r.Vars[1].Default) { + t.Fatalf("defaults should match\n[0]: %#v\n[1]: %#v\n", r.Vars[0], r.Vars[1]) + } +} From 77eac88c9f17cb011f331b572be0ebf0f60aa9f3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 14 Sep 2016 12:43:40 -0400 Subject: [PATCH 2/7] Simplify the failing test Remove a level of nesting and separate the passing and failing cases. --- decoder_test.go | 59 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/decoder_test.go b/decoder_test.go index 6476ed8..a2fc7e0 100644 --- a/decoder_test.go +++ b/decoder_test.go @@ -852,21 +852,25 @@ func TestDecode_topLevelKeys(t *testing.T) { } } -func TestDecode_structureMixedFields(t *testing.T) { - jsonConfig := `{ - "vars": { - "var_1": { - "default": { - "key1": "a", - "key2": "b" - } - }, - "var_2": { - "description": "an extra field is required", - "default": { - "key1": "a", - "key2": "b" - } +func TestDecode_structureFlattened(t *testing.T) { + jsonA := ` +{ + "var_1": { + "default": { + "key1": "a", + "key2": "b" + } + } +} +` + + jsonB := ` +{ + "var_2": { + "description": "an extra field is required", + "default": { + "key1": "a", + "key2": "b" } } } @@ -878,26 +882,27 @@ func TestDecode_structureMixedFields(t *testing.T) { Default map[string]string } - type Root struct { - Vars []*V - } + var vA, vB []*V - var r Root - - err := Decode(&r, jsonConfig) + err := Decode(&vA, jsonA) if err != nil { t.Fatalf("err: %s", err) } - if len(r.Vars) != 2 { - t.Fatalf("expected 2 Vars, got %d", len(r.Vars)) + err = Decode(&vB, jsonB) + if err != nil { + t.Fatalf("err: %s", err) } - if r.Vars[1].Default == nil { - t.Fatalf("failed to decode Default field in %#v", r.Vars[1]) + if len(vA) == 0 { + t.Fatal("failed to decode vA") } - if !reflect.DeepEqual(r.Vars[0].Default, r.Vars[1].Default) { - t.Fatalf("defaults should match\n[0]: %#v\n[1]: %#v\n", r.Vars[0], r.Vars[1]) + if len(vB) == 0 { + t.Fatal("failed to decode vB") + } + + if !reflect.DeepEqual(vA[0].Default, vB[0].Default) { + t.Fatalf("defaults should match\n[0]: %#v\n[1]: %#v\n", vA[0], vB[0]) } } From d3228f113dbcb6de02b73dd1d9607194a5a11717 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 14 Sep 2016 17:25:14 -0400 Subject: [PATCH 3/7] Unflatten single objects from a list A single json object with nested objects is flattened by the json parser to a list of objects sharing the parent key. If we're decoding into struct this was likely a mistake, and we need to re-expand the ast. --- decoder.go | 95 ++++++++++++++++++++++++++++++++++++++++++++++++- decoder_test.go | 32 ++++++++++++----- 2 files changed, 118 insertions(+), 9 deletions(-) diff --git a/decoder.go b/decoder.go index a6938fe..f733052 100644 --- a/decoder.go +++ b/decoder.go @@ -409,7 +409,6 @@ func (d *decoder) decodeSlice(name string, node ast.Node, result reflect.Value) if result.Kind() == reflect.Interface { result = result.Elem() } - // Create the slice if it isn't nil resultType := result.Type() resultElemType := resultType.Elem() @@ -419,6 +418,20 @@ func (d *decoder) decodeSlice(name string, node ast.Node, result reflect.Value) resultSliceType, 0, 0) } + // if node is an object that was decoded from ambiguous JSON and flattened + // as a list, decode into a single value in the slice. + if objAsList(node, resultElemType) { + val := reflect.Indirect(reflect.New(resultElemType)) + err := d.decodeFlatObj(name+"[0]", node, val) + if err != nil { + return err + } + + result = reflect.Append(result, val) + set.Set(result) + return nil + } + // Figure out the items we'll be copying into the slice var items []ast.Node switch n := node.(type) { @@ -455,6 +468,85 @@ func (d *decoder) decodeSlice(name string, node ast.Node, result reflect.Value) return nil } +// decode a flattened object into a single struct +func (d decoder) decodeFlatObj(name string, node ast.Node, result reflect.Value) error { + list := node.(*ast.ObjectList) + var keyToken token.Token + // get the key token, and strip it out of the key-val nodes + for _, item := range list.Items { + keyToken = item.Keys[0].Token + item.Keys = item.Keys[1:] + } + + // we need to un-flatten the ast enough to decode + newNode := &ast.ObjectItem{ + Keys: []*ast.ObjectKey{ + &ast.ObjectKey{ + Token: keyToken, + }, + }, + Val: &ast.ObjectType{ + List: list, + }, + } + + if err := d.decode(name, newNode, result); err != nil { + return err + } + + return nil +} + +// objAsList detects if an ambiguous JSON object was flattened to a List which +// should be decoded into a struct. +func objAsList(node ast.Node, elemType reflect.Type) bool { + objList, ok := node.(*ast.ObjectList) + if !ok { + return false + } + + // our target type must be a struct + switch elemType.Kind() { + case reflect.Ptr: + switch elemType.Elem().Kind() { + case reflect.Struct: + //OK + default: + return false + } + case reflect.Struct: + //OK + default: + return false + } + + fieldNames := make(map[string]bool) + prevKey := "" + + for _, item := range objList.Items { + // a list value will have a key and field name + if len(item.Keys) != 2 { + return false + } + + key := item.Keys[0].Token.Value().(string) + if prevKey != "" && key != prevKey { + // the same object will have all the same key + return false + } + prevKey = key + + fieldName := item.Keys[1].Token.Value().(string) + + if ok := fieldNames[fieldName]; ok { + // A single struct won't have duplicate fields. + return false + } + } + + return true +} + func (d *decoder) decodeString(name string, node ast.Node, result reflect.Value) error { switch n := node.(type) { case *ast.LiteralType: @@ -606,6 +698,7 @@ func (d *decoder) decodeStruct(name string, node ast.Node, result reflect.Value) // match (only object with the field), then we decode it exactly. // If it is a prefix match, then we decode the matches. filter := list.Filter(fieldName) + prefixMatches := filter.Children() matches := filter.Elem() if len(matches.Items) == 0 && len(prefixMatches.Items) == 0 { diff --git a/decoder_test.go b/decoder_test.go index a2fc7e0..218d3f6 100644 --- a/decoder_test.go +++ b/decoder_test.go @@ -867,7 +867,7 @@ func TestDecode_structureFlattened(t *testing.T) { jsonB := ` { "var_2": { - "description": "an extra field is required", + "description": "Described", "default": { "key1": "a", "key2": "b" @@ -876,6 +876,7 @@ func TestDecode_structureFlattened(t *testing.T) { } ` + // make sure we can also correctly extract the Name key type V struct { Name string `hcl:",key"` Description string @@ -888,21 +889,36 @@ func TestDecode_structureFlattened(t *testing.T) { if err != nil { t.Fatalf("err: %s", err) } + if len(vA) != 1 { + t.Fatal("failed to decode jsonA") + } err = Decode(&vB, jsonB) if err != nil { t.Fatalf("err: %s", err) } - - if len(vA) == 0 { - t.Fatal("failed to decode vA") + if len(vB) != 1 { + t.Fatal("failed to decode jsonB") } - if len(vB) == 0 { - t.Fatal("failed to decode vB") + expectedA := []*V{ + &V{ + Name: "var_1", + Default: map[string]string{"key1": "a", "key2": "b"}, + }, + } + expectedB := []*V{ + &V{ + Name: "var_2", + Description: "Described", + Default: map[string]string{"key1": "a", "key2": "b"}, + }, } - if !reflect.DeepEqual(vA[0].Default, vB[0].Default) { - t.Fatalf("defaults should match\n[0]: %#v\n[1]: %#v\n", vA[0], vB[0]) + if !reflect.DeepEqual(vA, expectedA) { + t.Fatalf("\nexpected: %#v\ngot: %#v\n", expectedA[0], vA[0]) + } + if !reflect.DeepEqual(vB, expectedB) { + t.Fatalf("\nexpected: %#v\ngot: %#v\n", expectedB[0], vB[0]) } } From 769aa724365409ca20102251f99ded03ccea6b09 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 15 Sep 2016 11:56:28 -0400 Subject: [PATCH 4/7] Added more tests for decoding JSON->maps Make TestDecode_flattenedJSON table driven to add more test cases as reproduced in terraform. --- decoder_test.go | 203 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 155 insertions(+), 48 deletions(-) diff --git a/decoder_test.go b/decoder_test.go index 218d3f6..14abb5c 100644 --- a/decoder_test.go +++ b/decoder_test.go @@ -6,6 +6,7 @@ import ( "reflect" "testing" + "github.com/davecgh/go-spew/spew" "github.com/hashicorp/hcl/hcl/ast" "github.com/hashicorp/hcl/testhelper" ) @@ -852,21 +853,46 @@ func TestDecode_topLevelKeys(t *testing.T) { } } -func TestDecode_structureFlattened(t *testing.T) { - jsonA := ` +func TestDecode_flattenedJSON(t *testing.T) { + // make sure we can also correctly extract a Name key too + type V struct { + Name string `hcl:",key"` + Description string + Default map[string]string + } + type Vars struct { + Variable []*V + } + + cases := []struct { + JSON string + Out interface{} + Expected interface{} + }{ + { // Nested object, no sibling keys + JSON: ` { - "var_1": { + "var_name": { "default": { "key1": "a", "key2": "b" } } } -` + `, + Out: &[]*V{}, + Expected: &[]*V{ + &V{ + Name: "var_name", + Default: map[string]string{"key1": "a", "key2": "b"}, + }, + }, + }, - jsonB := ` + { // Nested object with a sibling key (this worked previously) + JSON: ` { - "var_2": { + "var_name": { "description": "Described", "default": { "key1": "a", @@ -874,51 +900,132 @@ func TestDecode_structureFlattened(t *testing.T) { } } } -` - - // make sure we can also correctly extract the Name key - type V struct { - Name string `hcl:",key"` - Description string - Default map[string]string - } - - var vA, vB []*V - - err := Decode(&vA, jsonA) - if err != nil { - t.Fatalf("err: %s", err) - } - if len(vA) != 1 { - t.Fatal("failed to decode jsonA") - } - - err = Decode(&vB, jsonB) - if err != nil { - t.Fatalf("err: %s", err) - } - if len(vB) != 1 { - t.Fatal("failed to decode jsonB") - } - - expectedA := []*V{ - &V{ - Name: "var_1", - Default: map[string]string{"key1": "a", "key2": "b"}, + `, + Out: &[]*V{}, + Expected: &[]*V{ + &V{ + Name: "var_name", + Description: "Described", + Default: map[string]string{"key1": "a", "key2": "b"}, + }, + }, }, - } - expectedB := []*V{ - &V{ - Name: "var_2", - Description: "Described", - Default: map[string]string{"key1": "a", "key2": "b"}, + + { // Multiple nested objects, one with a sibling key + JSON: ` +{ + "variable": { + "var_1": { + "default": { + "key1": "a", + "key2": "b" + } + }, + "var_2": { + "description": "Described", + "default": { + "key1": "a", + "key2": "b" + } + } + } +} + `, + Out: &Vars{}, + Expected: &Vars{ + Variable: []*V{ + &V{ + Name: "var_1", + Default: map[string]string{"key1": "a", "key2": "b"}, + }, + &V{ + Name: "var_2", + Description: "Described", + Default: map[string]string{"key1": "a", "key2": "b"}, + }, + }, + }, + }, + + { // Nested object to maps + JSON: ` +{ + "variable": { + "var_name": { + "description": "Described", + "default": { + "key1": "a", + "key2": "b" + } + } + } +} + `, + Out: &[]map[string]interface{}{}, + Expected: &[]map[string]interface{}{ + { + "variable": []map[string]interface{}{ + { + "var_name": []map[string]interface{}{ + { + "description": "Described", + "default": []map[string]interface{}{ + { + "key1": "a", + "key2": "b", + }, + }, + }, + }, + }, + }, + }, + }, + }, + + { // Nested object to maps without a sibling key should decode the same as above + JSON: ` +{ + "variable": { + "var_name": { + "default": { + "key1": "a", + "key2": "b" + } + } + } +} + `, + Out: &[]map[string]interface{}{}, + Expected: &[]map[string]interface{}{ + { + "variable": []map[string]interface{}{ + { + "var_name": []map[string]interface{}{ + { + "default": []map[string]interface{}{ + { + "key1": "a", + "key2": "b", + }, + }, + }, + }, + }, + }, + }, + }, }, } - if !reflect.DeepEqual(vA, expectedA) { - t.Fatalf("\nexpected: %#v\ngot: %#v\n", expectedA[0], vA[0]) - } - if !reflect.DeepEqual(vB, expectedB) { - t.Fatalf("\nexpected: %#v\ngot: %#v\n", expectedB[0], vB[0]) + for i, tc := range cases { + err := Decode(tc.Out, tc.JSON) + if err != nil { + t.Fatalf("[%d] err: %s", i, err) + } + + if !reflect.DeepEqual(tc.Out, tc.Expected) { + t.Fatalf("[%d]\ngot: %s\nexpected: %s\n", i, spew.Sdump(tc.Out), spew.Sdump(tc.Expected)) + } } } From bdd93440d82c6b6fc4818758faaee8e8e0f212cc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 15 Sep 2016 15:36:40 -0400 Subject: [PATCH 5/7] Simplify the code to expand objects Now that we know only individual items in a slice need to be expanded, we can simplify the code flow to expand the ast in place while decoding. --- decoder.go | 120 +++++++++++++++++------------------------------- decoder_test.go | 58 +++++++++++++++++++++++ 2 files changed, 100 insertions(+), 78 deletions(-) diff --git a/decoder.go b/decoder.go index f733052..c8a077d 100644 --- a/decoder.go +++ b/decoder.go @@ -418,20 +418,6 @@ func (d *decoder) decodeSlice(name string, node ast.Node, result reflect.Value) resultSliceType, 0, 0) } - // if node is an object that was decoded from ambiguous JSON and flattened - // as a list, decode into a single value in the slice. - if objAsList(node, resultElemType) { - val := reflect.Indirect(reflect.New(resultElemType)) - err := d.decodeFlatObj(name+"[0]", node, val) - if err != nil { - return err - } - - result = reflect.Append(result, val) - set.Set(result) - return nil - } - // Figure out the items we'll be copying into the slice var items []ast.Node switch n := node.(type) { @@ -456,6 +442,12 @@ func (d *decoder) decodeSlice(name string, node ast.Node, result reflect.Value) // Decode val := reflect.Indirect(reflect.New(resultElemType)) + + // if item is an object that was decoded from ambiguous JSON and + // flattened, make sure it's expanded if it needs to decode into a + // defined structure. + item := expandObject(item, val) + if err := d.decode(fieldName, item, val); err != nil { return err } @@ -468,16 +460,40 @@ func (d *decoder) decodeSlice(name string, node ast.Node, result reflect.Value) return nil } -// decode a flattened object into a single struct -func (d decoder) decodeFlatObj(name string, node ast.Node, result reflect.Value) error { - list := node.(*ast.ObjectList) - var keyToken token.Token - // get the key token, and strip it out of the key-val nodes - for _, item := range list.Items { - keyToken = item.Keys[0].Token - item.Keys = item.Keys[1:] +// expandObject detects if an ambiguous JSON object was flattened to a List which +// should be decoded into a struct, and expands the ast to properly deocode. +func expandObject(node ast.Node, result reflect.Value) ast.Node { + item, ok := node.(*ast.ObjectItem) + if !ok { + return node } + elemType := result.Type() + + // our target type must be a struct + switch elemType.Kind() { + case reflect.Ptr: + switch elemType.Elem().Kind() { + case reflect.Struct: + //OK + default: + return node + } + case reflect.Struct: + //OK + default: + return node + } + + // A list value will have a key and field name. If it had more fields, + // it wouldn't have been flattened. + if len(item.Keys) != 2 { + return node + } + + keyToken := item.Keys[0].Token + item.Keys = item.Keys[1:] + // we need to un-flatten the ast enough to decode newNode := &ast.ObjectItem{ Keys: []*ast.ObjectKey{ @@ -486,65 +502,13 @@ func (d decoder) decodeFlatObj(name string, node ast.Node, result reflect.Value) }, }, Val: &ast.ObjectType{ - List: list, + List: &ast.ObjectList{ + Items: []*ast.ObjectItem{item}, + }, }, } - if err := d.decode(name, newNode, result); err != nil { - return err - } - - return nil -} - -// objAsList detects if an ambiguous JSON object was flattened to a List which -// should be decoded into a struct. -func objAsList(node ast.Node, elemType reflect.Type) bool { - objList, ok := node.(*ast.ObjectList) - if !ok { - return false - } - - // our target type must be a struct - switch elemType.Kind() { - case reflect.Ptr: - switch elemType.Elem().Kind() { - case reflect.Struct: - //OK - default: - return false - } - case reflect.Struct: - //OK - default: - return false - } - - fieldNames := make(map[string]bool) - prevKey := "" - - for _, item := range objList.Items { - // a list value will have a key and field name - if len(item.Keys) != 2 { - return false - } - - key := item.Keys[0].Token.Value().(string) - if prevKey != "" && key != prevKey { - // the same object will have all the same key - return false - } - prevKey = key - - fieldName := item.Keys[1].Token.Value().(string) - - if ok := fieldNames[fieldName]; ok { - // A single struct won't have duplicate fields. - return false - } - } - - return true + return newNode } func (d *decoder) decodeString(name string, node ast.Node, result reflect.Value) error { diff --git a/decoder_test.go b/decoder_test.go index 14abb5c..5a8404c 100644 --- a/decoder_test.go +++ b/decoder_test.go @@ -1016,6 +1016,64 @@ func TestDecode_flattenedJSON(t *testing.T) { }, }, }, + + { // Nested objects, one with a sibling key, and one without + JSON: ` +{ + "variable": { + "var_1": { + "default": { + "key1": "a", + "key2": "b" + } + }, + "var_2": { + "description": "Described", + "default": { + "key1": "a", + "key2": "b" + } + } + } +} + `, + Out: &[]map[string]interface{}{}, + Expected: &[]map[string]interface{}{ + { + "variable": []map[string]interface{}{ + { + "var_1": []map[string]interface{}{ + { + "default": []map[string]interface{}{ + { + "key1": "a", + "key2": "b", + }, + }, + }, + }, + }, + }, + }, + { + "variable": []map[string]interface{}{ + { + "var_2": []map[string]interface{}{ + { + "description": "Described", + "default": []map[string]interface{}{ + { + "key1": "a", + "key2": "b", + }, + }, + }, + }, + }, + }, + }, + }, + }, } for i, tc := range cases { From 88422edf64d1ae3c0fc4baa520a57fd910c41901 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 15 Sep 2016 16:19:29 -0400 Subject: [PATCH 6/7] Update Makefile with new deps, go1.7 in travis --- .travis.yml | 2 +- Makefile | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 83dc540..a785444 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,3 +1,3 @@ sudo: false language: go -go: 1.5 +go: 1.7 diff --git a/Makefile b/Makefile index ad404a8..84fd743 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,7 @@ fmt: generate go fmt ./... test: generate + go get -t ./... go test $(TEST) $(TESTARGS) generate: From 0bcadf025b23fcff3e35ba31e7356214a0b5d272 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 15 Sep 2016 16:24:51 -0400 Subject: [PATCH 7/7] install test deps in appveyor --- appveyor.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/appveyor.yml b/appveyor.yml index e70f03b..3c8cdf8 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -12,5 +12,8 @@ install: go version go env + + go get -t ./... + build_script: - cmd: go test -v ./...