From 035d56409f7d33c0b42d086fc1082f258dd3a30b Mon Sep 17 00:00:00 2001 From: James McGill Date: Wed, 14 Mar 2018 17:50:41 -0400 Subject: [PATCH] helper/schema: handle TypeMap elem consistently with other collection types For historical reasons, the handling of element types for maps is inconsistent with other collection types. Here we begin a multi-step process to make it consistent, starting by supporting both the "consistent" form of using a schema.Schema and an existing erroneous form of using a schema.Type directly. In subsequent commits we will phase out the erroneous form and require the schema.Schema approach, the same as we do for TypeList and TypeSet. --- helper/schema/field_reader.go | 11 +++---- helper/schema/field_reader_config.go | 2 +- helper/schema/field_reader_config_test.go | 4 +++ helper/schema/field_reader_diff.go | 3 +- helper/schema/field_reader_diff_test.go | 13 ++++++++ helper/schema/field_reader_map.go | 2 +- helper/schema/field_reader_map_test.go | 4 +++ helper/schema/field_reader_test.go | 20 +++++++++++++ helper/schema/schema.go | 9 ++---- helper/schema/schema_test.go | 36 +++++++++++++++++++++++ 10 files changed, 90 insertions(+), 14 deletions(-) diff --git a/helper/schema/field_reader.go b/helper/schema/field_reader.go index 1660a6702..b80b223a2 100644 --- a/helper/schema/field_reader.go +++ b/helper/schema/field_reader.go @@ -126,6 +126,8 @@ func addrToSchema(addr []string, schemaMap map[string]*Schema) []*Schema { switch v := current.Elem.(type) { case ValueType: current = &Schema{Type: v} + case *Schema: + current, _ = current.Elem.(*Schema) default: // maps default to string values. This is all we can have // if this is nested in another list or map. @@ -249,11 +251,10 @@ func readObjectField( } // convert map values to the proper primitive type based on schema.Elem -func mapValuesToPrimitive(m map[string]interface{}, schema *Schema) error { - - elemType := TypeString - if et, ok := schema.Elem.(ValueType); ok { - elemType = et +func mapValuesToPrimitive(k string, m map[string]interface{}, schema *Schema) error { + elemType, err := getValueType(k, schema) + if err != nil { + return err } switch elemType { diff --git a/helper/schema/field_reader_config.go b/helper/schema/field_reader_config.go index f958bbcb1..55a301d81 100644 --- a/helper/schema/field_reader_config.go +++ b/helper/schema/field_reader_config.go @@ -206,7 +206,7 @@ func (r *ConfigFieldReader) readMap(k string, schema *Schema) (FieldReadResult, panic(fmt.Sprintf("unknown type: %#v", mraw)) } - err := mapValuesToPrimitive(result, schema) + err := mapValuesToPrimitive(k, result, schema) if err != nil { return FieldReadResult{}, nil } diff --git a/helper/schema/field_reader_config_test.go b/helper/schema/field_reader_config_test.go index 5db643d75..6f50c340f 100644 --- a/helper/schema/field_reader_config_test.go +++ b/helper/schema/field_reader_config_test.go @@ -39,6 +39,10 @@ func TestConfigFieldReader(t *testing.T) { "one": "1", "two": "2", }, + "mapIntNestedSchema": map[string]interface{}{ + "one": "1", + "two": "2", + }, "mapFloat": map[string]interface{}{ "oneDotTwo": "1.2", }, diff --git a/helper/schema/field_reader_diff.go b/helper/schema/field_reader_diff.go index 644b93e6a..d558a5bc9 100644 --- a/helper/schema/field_reader_diff.go +++ b/helper/schema/field_reader_diff.go @@ -122,7 +122,8 @@ func (r *DiffFieldReader) readMap( result[k] = v.New } - err = mapValuesToPrimitive(result, schema) + key := address[len(address)-1] + err = mapValuesToPrimitive(key, result, schema) if err != nil { return FieldReadResult{}, nil } diff --git a/helper/schema/field_reader_diff_test.go b/helper/schema/field_reader_diff_test.go index 537e62e55..49b05e862 100644 --- a/helper/schema/field_reader_diff_test.go +++ b/helper/schema/field_reader_diff_test.go @@ -433,6 +433,19 @@ func TestDiffFieldReader(t *testing.T) { New: "2", }, + "mapIntNestedSchema.%": &terraform.ResourceAttrDiff{ + Old: "", + New: "2", + }, + "mapIntNestedSchema.one": &terraform.ResourceAttrDiff{ + Old: "", + New: "1", + }, + "mapIntNestedSchema.two": &terraform.ResourceAttrDiff{ + Old: "", + New: "2", + }, + "mapFloat.%": &terraform.ResourceAttrDiff{ Old: "", New: "1", diff --git a/helper/schema/field_reader_map.go b/helper/schema/field_reader_map.go index 95339810b..054efe08f 100644 --- a/helper/schema/field_reader_map.go +++ b/helper/schema/field_reader_map.go @@ -61,7 +61,7 @@ func (r *MapFieldReader) readMap(k string, schema *Schema) (FieldReadResult, err return true }) - err := mapValuesToPrimitive(result, schema) + err := mapValuesToPrimitive(k, result, schema) if err != nil { return FieldReadResult{}, nil } diff --git a/helper/schema/field_reader_map_test.go b/helper/schema/field_reader_map_test.go index 4fa78e25c..2723674a3 100644 --- a/helper/schema/field_reader_map_test.go +++ b/helper/schema/field_reader_map_test.go @@ -46,6 +46,10 @@ func TestMapFieldReader(t *testing.T) { "mapInt.one": "1", "mapInt.two": "2", + "mapIntNestedSchema.%": "2", + "mapIntNestedSchema.one": "1", + "mapIntNestedSchema.two": "2", + "mapFloat.%": "1", "mapFloat.oneDotTwo": "1.2", diff --git a/helper/schema/field_reader_test.go b/helper/schema/field_reader_test.go index fb0030722..2c62eb0a8 100644 --- a/helper/schema/field_reader_test.go +++ b/helper/schema/field_reader_test.go @@ -215,6 +215,13 @@ func testFieldReader(t *testing.T, f func(map[string]*Schema) FieldReader) { Type: TypeMap, Elem: TypeInt, }, + + // This is used to verify that the type of a Map can be specified using the + // same syntax as for lists (as a nested *Schema passed to Elem) + "mapIntNestedSchema": &Schema{ + Type: TypeMap, + Elem: &Schema{Type: TypeInt}, + }, "mapFloat": &Schema{ Type: TypeMap, Elem: TypeFloat, @@ -360,6 +367,19 @@ func testFieldReader(t *testing.T, f func(map[string]*Schema) FieldReader) { false, }, + "mapIntNestedSchema": { + []string{"mapIntNestedSchema"}, + FieldReadResult{ + Value: map[string]interface{}{ + "one": 1, + "two": 2, + }, + Exists: true, + Computed: false, + }, + false, + }, + "mapFloat": { []string{"mapFloat"}, FieldReadResult{ diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 4c9ecd61a..c94b694ff 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -1461,13 +1461,10 @@ func getValueType(k string, schema *Schema) (ValueType, error) { return vt, nil } + // If a Schema is provided to a Map, we use the Type of that schema + // as the type for each element in the Map. if s, ok := schema.Elem.(*Schema); ok { - if s.Elem == nil { - return TypeString, nil - } - if vt, ok := s.Elem.(ValueType); ok { - return vt, nil - } + return s.Type, nil } if _, ok := schema.Elem.(*Resource); ok { diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 7c4d20f0a..4f50f9215 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -4528,6 +4528,42 @@ func TestSchemaMap_Validate(t *testing.T) { }, }, + "Map with type specified as value type": { + Schema: map[string]*Schema{ + "user_data": &Schema{ + Type: TypeMap, + Optional: true, + Elem: TypeBool, + }, + }, + + Config: map[string]interface{}{ + "user_data": map[string]interface{}{ + "foo": "not_a_bool", + }, + }, + + Err: true, + }, + + "Map with type specified as nested Schema": { + Schema: map[string]*Schema{ + "user_data": &Schema{ + Type: TypeMap, + Optional: true, + Elem: &Schema{Type: TypeBool}, + }, + }, + + Config: map[string]interface{}{ + "user_data": map[string]interface{}{ + "foo": "not_a_bool", + }, + }, + + Err: true, + }, + "Bad map: just a slice": { Schema: map[string]*Schema{ "user_data": &Schema{