Skip to content

Commit

Permalink
convert: Fix converting to nested dynamic map
Browse files Browse the repository at this point in the history 
When converting an object tree to a nested dynamic map (for example,
`cty.Map(cty.Map(cty.DynamicPseudoType))` or deeper), it is necessary to
unify the types of the child elements of the map.

Previously, we would only apply this process to the innermost leaf
object's attributes, which could result in local unification but
globally incompatible types. This caused a panic when converting the
single top-level object into a map with concrete type, as the types of
the sub-trees were incompatible.

This commit addresses this class of bugs in two ways: adding type
unification for multi-level map and object structures, and fallback
error handling to prevent panics for this class of failure.

The additional type unification process has several steps:

- First, we add a second unify & convert step at the end of the process,
  immediately before constructing the final map. This second unification
  ensures that not only sibling elements but also cousins are of the
  same type.

- We also ensure that generated type information is preserved for empty
  collections, by returning an empty map of the detected type, instead
  of the dynamic type. This prevents a multi-step unification bouncing
  back and forth between concrete and dynamic element types.

- Finally, we add a specific unification procedure for map-to-map
  conversions. This inspects and unifies the element types for the maps
  which are being converted. For example, a populated map(string) and an
  empty map(any) will be unified to a map(string) type.

Tests are extended to cover reduced configurations from the three source
Terraform bugs which prompted this work.
  • Loading branch information
@alisdair @apparentlymart
alisdair authored and apparentlymart committed Mar 3, 2020
1 parent 089490b commit 8a7a7a8
Show file tree
Hide file tree
Showing 3 changed files with 221 additions and 22 deletions.
132 changes: 110 additions & 22 deletions cty/convert/conversion_collection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ func conversionCollectionToList(ety cty.Type, conv conversion) conversion {
return func(val cty.Value, path cty.Path) (cty.Value, error) {
elems := make([]cty.Value, 0, val.LengthInt())
i := int64(0)
path = append(path, nil)
elemPath := append(path.Copy(), nil)
it := val.ElementIterator()
for it.Next() {
_, val := it.Element()
var err error

path[len(path)-1] = cty.IndexStep{
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: cty.NumberIntVal(i),
}

if conv != nil {
val, err = conv(val, path)
val, err = conv(val, elemPath)
if err != nil {
return cty.NilVal, err
}
Expand All @@ -37,6 +37,9 @@ func conversionCollectionToList(ety cty.Type, conv conversion) conversion {
}

if len(elems) == 0 {
if ety == cty.DynamicPseudoType {
ety = val.Type().ElementType()
}
return cty.ListValEmpty(ety), nil
}

Expand All @@ -55,18 +58,18 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion {
return func(val cty.Value, path cty.Path) (cty.Value, error) {
elems := make([]cty.Value, 0, val.LengthInt())
i := int64(0)
path = append(path, nil)
elemPath := append(path.Copy(), nil)
it := val.ElementIterator()
for it.Next() {
_, val := it.Element()
var err error

path[len(path)-1] = cty.IndexStep{
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: cty.NumberIntVal(i),
}

if conv != nil {
val, err = conv(val, path)
val, err = conv(val, elemPath)
if err != nil {
return cty.NilVal, err
}
Expand All @@ -77,6 +80,11 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion {
}

if len(elems) == 0 {
// Prefer a concrete type over a dynamic type when returning an
// empty set
if ety == cty.DynamicPseudoType {
ety = val.Type().ElementType()
}
return cty.SetValEmpty(ety), nil
}

Expand All @@ -93,25 +101,25 @@ func conversionCollectionToSet(ety cty.Type, conv conversion) conversion {
func conversionCollectionToMap(ety cty.Type, conv conversion) conversion {
return func(val cty.Value, path cty.Path) (cty.Value, error) {
elems := make(map[string]cty.Value, 0)
path = append(path, nil)
elemPath := append(path.Copy(), nil)
it := val.ElementIterator()
for it.Next() {
key, val := it.Element()
var err error

path[len(path)-1] = cty.IndexStep{
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: key,
}

keyStr, err := Convert(key, cty.String)
if err != nil {
// Should never happen, because keys can only be numbers or
// strings and both can convert to string.
return cty.DynamicVal, path.NewErrorf("cannot convert key type %s to string for map", key.Type().FriendlyName())
return cty.DynamicVal, elemPath.NewErrorf("cannot convert key type %s to string for map", key.Type().FriendlyName())
}

if conv != nil {
val, err = conv(val, path)
val, err = conv(val, elemPath)
if err != nil {
return cty.NilVal, err
}
Expand All @@ -121,9 +129,25 @@ func conversionCollectionToMap(ety cty.Type, conv conversion) conversion {
}

if len(elems) == 0 {
// Prefer a concrete type over a dynamic type when returning an
// empty map
if ety == cty.DynamicPseudoType {
ety = val.Type().ElementType()
}
return cty.MapValEmpty(ety), nil
}

if ety.IsCollectionType() || ety.IsObjectType() {
var err error
if elems, err = conversionUnifyCollectionElements(elems, path, false); err != nil {
return cty.NilVal, err
}
}

if err := conversionCheckMapElementTypes(elems, path); err != nil {
return cty.NilVal, err
}

return cty.MapVal(elems), nil
}
}
Expand Down Expand Up @@ -171,20 +195,20 @@ func conversionTupleToSet(tupleType cty.Type, listEty cty.Type, unsafe bool) con
// element conversions in elemConvs
return func(val cty.Value, path cty.Path) (cty.Value, error) {
elems := make([]cty.Value, 0, len(elemConvs))
path = append(path, nil)
elemPath := append(path.Copy(), nil)
i := int64(0)
it := val.ElementIterator()
for it.Next() {
_, val := it.Element()
var err error

path[len(path)-1] = cty.IndexStep{
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: cty.NumberIntVal(i),
}

conv := elemConvs[i]
if conv != nil {
val, err = conv(val, path)
val, err = conv(val, elemPath)
if err != nil {
return cty.NilVal, err
}
Expand Down Expand Up @@ -241,20 +265,20 @@ func conversionTupleToList(tupleType cty.Type, listEty cty.Type, unsafe bool) co
// element conversions in elemConvs
return func(val cty.Value, path cty.Path) (cty.Value, error) {
elems := make([]cty.Value, 0, len(elemConvs))
path = append(path, nil)
elemPath := append(path.Copy(), nil)
i := int64(0)
it := val.ElementIterator()
for it.Next() {
_, val := it.Element()
var err error

path[len(path)-1] = cty.IndexStep{
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: cty.NumberIntVal(i),
}

conv := elemConvs[i]
if conv != nil {
val, err = conv(val, path)
val, err = conv(val, elemPath)
if err != nil {
return cty.NilVal, err
}
Expand Down Expand Up @@ -315,26 +339,37 @@ func conversionObjectToMap(objectType cty.Type, mapEty cty.Type, unsafe bool) co
// element conversions in elemConvs
return func(val cty.Value, path cty.Path) (cty.Value, error) {
elems := make(map[string]cty.Value, len(elemConvs))
path = append(path, nil)
elemPath := append(path.Copy(), nil)
it := val.ElementIterator()
for it.Next() {
name, val := it.Element()
var err error

path[len(path)-1] = cty.IndexStep{
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: name,
}

conv := elemConvs[name.AsString()]
if conv != nil {
val, err = conv(val, path)
val, err = conv(val, elemPath)
if err != nil {
return cty.NilVal, err
}
}
elems[name.AsString()] = val
}

if mapEty.IsCollectionType() || mapEty.IsObjectType() {
var err error
if elems, err = conversionUnifyCollectionElements(elems, path, unsafe); err != nil {
return cty.NilVal, err
}
}

if err := conversionCheckMapElementTypes(elems, path); err != nil {
return cty.NilVal, err
}

return cty.MapVal(elems), nil
}
}
Expand Down Expand Up @@ -368,7 +403,7 @@ func conversionMapToObject(mapType cty.Type, objType cty.Type, unsafe bool) conv
// element conversions in elemConvs
return func(val cty.Value, path cty.Path) (cty.Value, error) {
elems := make(map[string]cty.Value, len(elemConvs))
path = append(path, nil)
elemPath := append(path.Copy(), nil)
it := val.ElementIterator()
for it.Next() {
name, val := it.Element()
Expand All @@ -380,13 +415,13 @@ func conversionMapToObject(mapType cty.Type, objType cty.Type, unsafe bool) conv

var err error

path[len(path)-1] = cty.IndexStep{
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: name,
}

conv := elemConvs[name.AsString()]
if conv != nil {
val, err = conv(val, path)
val, err = conv(val, elemPath)
if err != nil {
return cty.NilVal, err
}
Expand All @@ -398,3 +433,56 @@ func conversionMapToObject(mapType cty.Type, objType cty.Type, unsafe bool) conv
return cty.ObjectVal(elems), nil
}
}

func conversionUnifyCollectionElements(elems map[string]cty.Value, path cty.Path, unsafe bool) (map[string]cty.Value, error) {
elemTypes := make([]cty.Type, 0, len(elems))
for _, elem := range elems {
elemTypes = append(elemTypes, elem.Type())
}
unifiedType, _ := unify(elemTypes, unsafe)
if unifiedType == cty.NilType {
}

unifiedElems := make(map[string]cty.Value)
elemPath := append(path.Copy(), nil)

for name, elem := range elems {
if elem.Type().Equals(unifiedType) {
unifiedElems[name] = elem
continue
}
conv := getConversion(elem.Type(), unifiedType, unsafe)
if conv == nil {
}
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: cty.StringVal(name),
}
val, err := conv(elem, elemPath)
if err != nil {
return nil, err
}
unifiedElems[name] = val
}

return unifiedElems, nil
}

func conversionCheckMapElementTypes(elems map[string]cty.Value, path cty.Path) error {
elementType := cty.NilType
elemPath := append(path.Copy(), nil)

for name, elem := range elems {
if elementType == cty.NilType {
elementType = elem.Type()
continue
}
if !elementType.Equals(elem.Type()) {
elemPath[len(elemPath)-1] = cty.IndexStep{
Key: cty.StringVal(name),
}
return elemPath.NewErrorf("%s is required", elementType.FriendlyName())
}
}

return nil
}
68 changes: 68 additions & 0 deletions cty/convert/public_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,16 @@ func TestConvert(t *testing.T) {
cty.StringVal("hello"),
}),
},
{
Value: cty.ListValEmpty(cty.String),
Type: cty.Set(cty.DynamicPseudoType),
Want: cty.SetValEmpty(cty.String),
},
{
Value: cty.SetValEmpty(cty.String),
Type: cty.List(cty.DynamicPseudoType),
Want: cty.ListValEmpty(cty.String),
},
{
Value: cty.ObjectVal(map[string]cty.Value{
"num": cty.NumberIntVal(5),
Expand Down Expand Up @@ -581,6 +591,64 @@ func TestConvert(t *testing.T) {
Type: cty.Object(map[string]cty.Type{"foo": cty.String}),
Want: cty.ObjectVal(map[string]cty.Value{"foo": cty.StringVal("hello")}),
},
// reduction of https://github.com/hashicorp/terraform/issues/23804
{
Value: cty.ObjectVal(map[string]cty.Value{
"a": cty.ObjectVal(map[string]cty.Value{
"x": cty.TupleVal([]cty.Value{cty.StringVal("foo")}),
}),
"b": cty.ObjectVal(map[string]cty.Value{
"x": cty.TupleVal([]cty.Value{cty.StringVal("bar")}),
}),
"c": cty.ObjectVal(map[string]cty.Value{
"x": cty.TupleVal([]cty.Value{cty.StringVal("foo"), cty.StringVal("bar")}),
}),
}),
Type: cty.Map(cty.Map(cty.DynamicPseudoType)),
Want: cty.MapVal(map[string]cty.Value{
"a": cty.MapVal(map[string]cty.Value{
"x": cty.ListVal([]cty.Value{cty.StringVal("foo")}),
}),
"b": cty.MapVal(map[string]cty.Value{
"x": cty.ListVal([]cty.Value{cty.StringVal("bar")}),
}),
"c": cty.MapVal(map[string]cty.Value{
"x": cty.ListVal([]cty.Value{cty.StringVal("foo"), cty.StringVal("bar")}),
}),
}),
},
// reduction of https://github.com/hashicorp/issues/24167
{
Value: cty.ObjectVal(map[string]cty.Value{
"a": cty.ObjectVal(map[string]cty.Value{
"x": cty.NullVal(cty.DynamicPseudoType),
}),
"b": cty.ObjectVal(map[string]cty.Value{
"x": cty.ObjectVal(map[string]cty.Value{
"c": cty.NumberIntVal(1),
"d": cty.NumberIntVal(2),
}),
}),
}),
Type: cty.Map(cty.Map(cty.Object(map[string]cty.Type{"x": cty.Map(cty.DynamicPseudoType)}))),
WantError: true,
},
// reduction of https://github.com/hashicorp/terraform/issues/23431
{
Value: cty.ObjectVal(map[string]cty.Value{
"a": cty.ObjectVal(map[string]cty.Value{
"x": cty.StringVal("foo"),
}),
"b": cty.MapValEmpty(cty.DynamicPseudoType),
}),
Type: cty.Map(cty.Map(cty.DynamicPseudoType)),
Want: cty.MapVal(map[string]cty.Value{
"a": cty.MapVal(map[string]cty.Value{
"x": cty.StringVal("foo"),
}),
"b": cty.MapValEmpty(cty.String),
}),
},
}

for _, test := range tests {
Expand Down
Loading

0 comments on commit 8a7a7a8

Please sign in to comment.