Skip to content

Commit

Permalink
convert: Fix panicking and misbehavior for object types with optional…
Browse files Browse the repository at this point in the history 
… attributes
  • Loading branch information
@apparentlymart
apparentlymart committed Mar 8, 2021
2 parents fcc7075 + dfcbf96 commit c91a2bf
Show file tree
Hide file tree
Showing 5 changed files with 294 additions and 11 deletions.
27 changes: 19 additions & 8 deletions cty/convert/conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,25 @@ func getConversion(in cty.Type, out cty.Type, unsafe bool) conversion {
// Conversion to DynamicPseudoType always just passes through verbatim.
return in, nil
}
if !in.IsKnown() {
return cty.UnknownVal(out), nil
}
if in.IsNull() {
// We'll pass through nulls, albeit type converted, and let
// the caller deal with whatever handling they want to do in
// case null values are considered valid in some applications.
return cty.NullVal(out), nil
if isKnown, isNull := in.IsKnown(), in.IsNull(); !isKnown || isNull {
// Avoid constructing unknown or null values with types which
// include optional attributes. Known or non-null object values
// will be passed to a conversion function which drops the optional
// attributes from the type. Unknown and null pass through values
// must do the same to ensure that homogeneous collections have a
// single element type.
out = out.WithoutOptionalAttributesDeep()

if !isKnown {
return cty.UnknownVal(out), nil
}

if isNull {
// We'll pass through nulls, albeit type converted, and let
// the caller deal with whatever handling they want to do in
// case null values are considered valid in some applications.
return cty.NullVal(out), nil
}
}

return conv(in, path)
Expand Down
125 changes: 125 additions & 0 deletions cty/convert/public_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,45 @@ func TestConvert(t *testing.T) {
),
WantError: true, // Attribute "bar" is required
},
{
Value: cty.NullVal(cty.DynamicPseudoType),
Type: cty.ObjectWithOptionalAttrs(
map[string]cty.Type{
"foo": cty.String,
"bar": cty.String,
},
[]string{"foo"},
),
Want: cty.NullVal(cty.Object(map[string]cty.Type{
"foo": cty.String,
"bar": cty.String,
})),
},
{
Value: cty.ListVal([]cty.Value{
cty.NullVal(cty.DynamicPseudoType),
cty.ObjectVal(map[string]cty.Value{
"bar": cty.StringVal("bar value"),
}),
}),
Type: cty.List(cty.ObjectWithOptionalAttrs(
map[string]cty.Type{
"foo": cty.String,
"bar": cty.String,
},
[]string{"foo"},
)),
Want: cty.ListVal([]cty.Value{
cty.NullVal(cty.Object(map[string]cty.Type{
"foo": cty.String,
"bar": cty.String,
})),
cty.ObjectVal(map[string]cty.Value{
"foo": cty.NullVal(cty.String),
"bar": cty.StringVal("bar value"),
}),
}),
},
{
Value: cty.ObjectVal(map[string]cty.Value{
"foo": cty.True,
Expand Down Expand Up @@ -748,6 +787,92 @@ func TestConvert(t *testing.T) {
"b": cty.MapValEmpty(cty.String),
}),
},
// reduction of https://github.com/hashicorp/terraform/issues/27269
{
Value: cty.TupleVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{
"a": cty.NullVal(cty.DynamicPseudoType),
}),
cty.ObjectVal(map[string]cty.Value{
"a": cty.ObjectVal(map[string]cty.Value{
"b": cty.ListVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{
"c": cty.StringVal("d"),
}),
}),
}),
}),
}),
Type: cty.List(cty.Object(map[string]cty.Type{
"a": cty.Object(map[string]cty.Type{
"b": cty.List(cty.ObjectWithOptionalAttrs(map[string]cty.Type{
"c": cty.String,
"d": cty.String,
}, []string{"d"})),
}),
})),
Want: cty.ListVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{
"a": cty.NullVal(cty.Object(map[string]cty.Type{
"b": cty.List(cty.Object(map[string]cty.Type{
"c": cty.String,
"d": cty.String,
})),
})),
}),
cty.ObjectVal(map[string]cty.Value{
"a": cty.ObjectVal(map[string]cty.Value{
"b": cty.ListVal([]cty.Value{
cty.ObjectVal(map[string]cty.Value{
"c": cty.StringVal("d"),
"d": cty.NullVal(cty.String),
}),
}),
}),
}),
}),
},
// When converting null values into nested types which include objects
// with optional attributes, we expect the resulting value to be of a
// recursively concretized type.
{
Value: cty.NullVal(cty.DynamicPseudoType),
Type: cty.Object(
map[string]cty.Type{
"foo": cty.ObjectWithOptionalAttrs(
map[string]cty.Type{
"bar": cty.String,
},
[]string{"bar"},
),
},
),
Want: cty.NullVal(cty.Object(map[string]cty.Type{
"foo": cty.Object(map[string]cty.Type{
"bar": cty.String,
}),
})),
},
// The same nested optional attributes flattening should happen for
// unknown values, too.
{
Value: cty.UnknownVal(cty.DynamicPseudoType),
Type: cty.Object(
map[string]cty.Type{
"foo": cty.ObjectWithOptionalAttrs(
map[string]cty.Type{
"bar": cty.String,
},
[]string{"bar"},
),
},
),
Want: cty.UnknownVal(cty.Object(map[string]cty.Type{
"foo": cty.Object(map[string]cty.Type{
"bar": cty.String,
}),
})),
},
// https://github.com/hashicorp/terraform/issues/21588:
{
Value: cty.TupleVal([]cty.Value{
Expand Down
16 changes: 13 additions & 3 deletions cty/convert/unify.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,19 @@ func unify(types []cty.Type, unsafe bool) (cty.Type, []Conversion) {
// unification purposes.
{
mapCt := 0
listCt := 0
setCt := 0
objectCt := 0
tupleCt := 0
dynamicCt := 0
for _, ty := range types {
switch {
case ty.IsMapType():
mapCt++
case ty.IsListType():
listCt++
case ty.IsSetType():
setCt++
case ty.IsObjectType():
objectCt++
case ty.IsTupleType():
Expand All @@ -48,7 +54,11 @@ func unify(types []cty.Type, unsafe bool) (cty.Type, []Conversion) {
}
switch {
case mapCt > 0 && (mapCt+dynamicCt) == len(types):
return unifyMapTypes(types, unsafe, dynamicCt > 0)
return unifyCollectionTypes(cty.Map, types, unsafe, dynamicCt > 0)
case listCt > 0 && (listCt+dynamicCt) == len(types):
return unifyCollectionTypes(cty.List, types, unsafe, dynamicCt > 0)
case setCt > 0 && (setCt+dynamicCt) == len(types):
return unifyCollectionTypes(cty.Set, types, unsafe, dynamicCt > 0)
case objectCt > 0 && (objectCt+dynamicCt) == len(types):
return unifyObjectTypes(types, unsafe, dynamicCt > 0)
case tupleCt > 0 && (tupleCt+dynamicCt) == len(types):
Expand Down Expand Up @@ -100,7 +110,7 @@ Preferences:
return cty.NilType, nil
}

func unifyMapTypes(types []cty.Type, unsafe bool, hasDynamic bool) (cty.Type, []Conversion) {
func unifyCollectionTypes(collectionType func(cty.Type) cty.Type, types []cty.Type, unsafe bool, hasDynamic bool) (cty.Type, []Conversion) {
// If we had any dynamic types in the input here then we can't predict
// what path we'll take through here once these become known types, so
// we'll conservatively produce DynamicVal for these.
Expand All @@ -117,7 +127,7 @@ func unifyMapTypes(types []cty.Type, unsafe bool, hasDynamic bool) (cty.Type, []
return cty.NilType, nil
}

retTy := cty.Map(retElemType)
retTy := collectionType(retElemType)

conversions := make([]Conversion, len(types))
for i, ty := range types {
Expand Down
38 changes: 38 additions & 0 deletions cty/type.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,44 @@ func (t Type) HasDynamicTypes() bool {
}
}

// WithoutOptionalAttributesDeep returns a type equivalent to the receiver but
// with any objects with optional attributes converted into fully concrete
// object types. This operation is applied recursively.
func (t Type) WithoutOptionalAttributesDeep() Type {
switch {
case t == DynamicPseudoType, t.IsPrimitiveType(), t.IsCapsuleType():
return t
case t.IsMapType():
return Map(t.ElementType().WithoutOptionalAttributesDeep())
case t.IsListType():
return List(t.ElementType().WithoutOptionalAttributesDeep())
case t.IsSetType():
return Set(t.ElementType().WithoutOptionalAttributesDeep())
case t.IsTupleType():
originalElemTypes := t.TupleElementTypes()
elemTypes := make([]Type, len(originalElemTypes))
for i, et := range originalElemTypes {
elemTypes[i] = et.WithoutOptionalAttributesDeep()
}
return Tuple(elemTypes)
case t.IsObjectType():
originalAttrTypes := t.AttributeTypes()
attrTypes := make(map[string]Type, len(originalAttrTypes))
for k, t := range originalAttrTypes {
attrTypes[k] = t.WithoutOptionalAttributesDeep()
}

// This is the subtle line which does all the work of this function: by
// constructing a new Object type with these attribute types, we drop
// the list of optional attributes (if present). This results in a
// concrete Object type which requires all of the original attributes.
return Object(attrTypes)
default:
// Should never happen, since above should be exhaustive
panic("WithoutOptionalAttributesDeep does not support the given type")
}
}

type friendlyTypeNameMode rune

const (
Expand Down
99 changes: 99 additions & 0 deletions cty/type_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,105 @@ func TestHasDynamicTypes(t *testing.T) {
}
}

func TestWithoutOptionalAttributesDeep(t *testing.T) {
tests := []struct {
ty Type
expected Type
}{
{
DynamicPseudoType,
DynamicPseudoType,
},
{
List(DynamicPseudoType),
List(DynamicPseudoType),
},
{
Tuple([]Type{String, DynamicPseudoType}),
Tuple([]Type{String, DynamicPseudoType}),
},
{
Object(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}),
Object(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}),
},
{
ObjectWithOptionalAttrs(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}, []string{"a"}),
Object(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}),
},
{
Map(ObjectWithOptionalAttrs(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}, []string{"a"})),
Map(Object(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
})),
},
{
Set(ObjectWithOptionalAttrs(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}, []string{"a"})),
Set(Object(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
})),
},
{
List(ObjectWithOptionalAttrs(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}, []string{"a"})),
List(Object(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
})),
},
{
Tuple([]Type{
ObjectWithOptionalAttrs(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}, []string{"a"}),
ObjectWithOptionalAttrs(map[string]Type{
"b": Number,
}, []string{"b"}),
}),
Tuple([]Type{
Object(map[string]Type{
"a": String,
"unknown": DynamicPseudoType,
}),
Object(map[string]Type{
"b": Number,
}),
}),
},
}

for _, test := range tests {
t.Run(fmt.Sprintf("%#v.HasDynamicTypes()", test.ty), func(t *testing.T) {
got := test.ty.WithoutOptionalAttributesDeep()
if !test.expected.Equals(got) {
t.Errorf("Equals returned %#v; want %#v", got, test.expected)
}
})
}
}

func TestNilTypeEquals(t *testing.T) {
var typ Type
if !typ.Equals(NilType) {
Expand Down

0 comments on commit c91a2bf

Please sign in to comment.