Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

convert: Fix converting to nested dynamic map #47

Merged
merged 3 commits into from
Mar 3, 2020
Merged

convert: Fix converting to nested dynamic map #47

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8be809f
convert: Fix converting to nested dynamic map
alisdair Feb 25, 2020
65f6e09
Update cty/convert/public_test.go
alisdair Mar 3, 2020
e1048fb
Fix potential panic in unreachable code
alisdair Mar 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
132 changes: 110 additions & 22 deletions cty/convert/conversion_collection.go
View file
Open in desktop
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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes throughout are for clarity, now that the path parameter can be used multiple times (including in the call to conversionUnifyCollectionElements). Always mutating a copy seemed clearer to me than repeatedly mutating the passed parameter.

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
Open in desktop
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/terraform/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