Skip to content

Commit 52a64bb

Browse files
liamcervanteliamcervante
authored
Verify type assumptions when retrieving child default values (#594)
* remove type assumptions when retrieving child default values * fix imports
1 parent eb5a6ef commit 52a64bb

File tree

2 files changed

+130
-9
lines changed

2 files changed

+130
-9
lines changed

‎ext/typeexpr/defaults.go

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -139,16 +139,58 @@ func (d *Defaults) applyAsMap(value cty.Value) map[string]cty.Value {
139139
}
140140

141141
func (d *Defaults) getChild(key interface{}) *Defaults {
142-
switch {
143-
case d.Type.IsMapType(), d.Type.IsSetType(), d.Type.IsListType():
144-
return d.Children[""]
145-
case d.Type.IsTupleType():
146-
return d.Children[strconv.Itoa(key.(int))]
147-
case d.Type.IsObjectType():
148-
return d.Children[key.(string)]
149-
default:
150-
return nil
142+
// Children for tuples are keyed by an int.
143+
// Children for objects are keyed by a string.
144+
// Children for maps, lists, and sets are always keyed by the empty string.
145+
//
146+
// For maps and objects the supplied key type is a string type.
147+
// For lists, sets, and tuples the supplied key type is an int type.
148+
//
149+
// The callers of the defaults package could, in theory, pass a value in
150+
// where the types expected based on the defaults do not match the actual
151+
// type in the value. In this case, we get a mismatch between what the
152+
// defaults package expects the key to be, and which type it actually is.
153+
//
154+
// In the event of such a mismatch, we just won't apply defaults. Instead,
155+
// relying on the user later calling go-cty.Convert to detect this same
156+
// error (as documented). In this case we'd just want to return nil to
157+
// indicate either there are no defaults or we can't work out how to apply
158+
// them. Both of these outcomes are treated the same by the rest of the
159+
// package.
160+
//
161+
// For the above reasons it isn't necessarily safe to just rely on a single
162+
// metric for working out how we should retrieve the key. If the defaults
163+
// type is a tuple we can't just assume the supplied key will be an int (as
164+
// the concrete value actually supplied by the user could be an object or a
165+
// map). Similarly, if the supplied key is an int we can't just assume we
166+
// should treat the type as a tuple (as a list would also specify an int,
167+
// but we should return the children keyed by the empty string rather than
168+
// the index).
169+
170+
switch concrete := key.(type) {
171+
case int:
172+
if d.Type.IsTupleType() {
173+
// If the type is an int, and our defaults are expecting a tuple
174+
// then we return the children for the tuple at the index.
175+
return d.Children[strconv.Itoa(concrete)]
176+
}
177+
case string:
178+
if d.Type.IsObjectType() {
179+
// If the type is a string, and our defaults are expecting an object
180+
// then we return the children for the object at the key.
181+
return d.Children[concrete]
182+
}
151183
}
184+
185+
// Otherwise, either our defaults are expecting this to be a map, list, or
186+
// set or the type our defaults expecting didn't line up with something we
187+
// can convert between. So, either we want to return the child keyed by
188+
// the empty string (in the first case) or nil (in the second case).
189+
// Luckily, Golang maps return nil when referencing something that doesn't
190+
// exist. So, we can just try and retrieve the child at the empty key and
191+
// if it doesn't exist then that's fine since we'd just return nil anyway.
192+
193+
return d.Children[""]
152194
}
153195

154196
func (d *Defaults) unifyAsSlice(values []cty.Value) []cty.Value {

‎ext/typeexpr/defaults_test.go

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -751,6 +751,85 @@ func TestDefaults_Apply(t *testing.T) {
751751
}),
752752
}),
753753
},
754+
"applies default safely where possible when types mismatch": {
755+
defaults: &Defaults{
756+
Type: cty.Map(cty.ObjectWithOptionalAttrs(map[string]cty.Type{
757+
"description": cty.String,
758+
"rules": cty.Map(cty.ObjectWithOptionalAttrs(map[string]cty.Type{
759+
"description": cty.String,
760+
"destination_ports": cty.List(cty.String),
761+
"destination_addresses": cty.List(cty.String),
762+
"translated_address": cty.String,
763+
"translated_port": cty.String,
764+
}, []string{"destination_addresses"})),
765+
}, []string{"description"})),
766+
Children: map[string]*Defaults{
767+
"": {
768+
Type: cty.ObjectWithOptionalAttrs(map[string]cty.Type{
769+
"description": cty.String,
770+
"rules": cty.Map(cty.ObjectWithOptionalAttrs(map[string]cty.Type{
771+
"description": cty.String,
772+
"destination_ports": cty.List(cty.String),
773+
"destination_addresses": cty.List(cty.String),
774+
"translated_address": cty.String,
775+
"translated_port": cty.String,
776+
}, []string{"destination_addresses"})),
777+
}, []string{"description"}),
778+
DefaultValues: map[string]cty.Value{
779+
"description": cty.StringVal("unknown"),
780+
},
781+
Children: map[string]*Defaults{
782+
"rules": {
783+
Type: cty.Map(cty.ObjectWithOptionalAttrs(map[string]cty.Type{
784+
"description": cty.String,
785+
"destination_ports": cty.List(cty.String),
786+
"destination_addresses": cty.List(cty.String),
787+
"translated_address": cty.String,
788+
"translated_port": cty.String,
789+
}, []string{"destination_addresses"})),
790+
Children: map[string]*Defaults{
791+
"": {
792+
Type: cty.ObjectWithOptionalAttrs(map[string]cty.Type{
793+
"description": cty.String,
794+
"destination_ports": cty.List(cty.String),
795+
"destination_addresses": cty.List(cty.String),
796+
"translated_address": cty.String,
797+
"translated_port": cty.String,
798+
}, []string{"destination_addresses"}),
799+
DefaultValues: map[string]cty.Value{
800+
"destination_addresses": cty.ListValEmpty(cty.String),
801+
},
802+
},
803+
},
804+
},
805+
},
806+
},
807+
},
808+
},
809+
value: cty.MapVal(map[string]cty.Value{
810+
"mysql": cty.ObjectVal(map[string]cty.Value{
811+
"rules": cty.ObjectVal(map[string]cty.Value{
812+
"description": cty.StringVal("Port forward"),
813+
"destination_ports": cty.ListVal([]cty.Value{cty.StringVal("3306")}),
814+
"destination_addresses": cty.ListVal([]cty.Value{cty.StringVal("192.168.0.1")}),
815+
"translated_address": cty.StringVal("192.168.0.1"),
816+
"translated_port": cty.StringVal("3306"),
817+
}),
818+
}),
819+
}),
820+
want: cty.MapVal(map[string]cty.Value{
821+
"mysql": cty.ObjectVal(map[string]cty.Value{
822+
"description": cty.StringVal("unknown"),
823+
"rules": cty.ObjectVal(map[string]cty.Value{
824+
"description": cty.StringVal("Port forward"),
825+
"destination_ports": cty.ListVal([]cty.Value{cty.StringVal("3306")}),
826+
"destination_addresses": cty.ListVal([]cty.Value{cty.StringVal("192.168.0.1")}),
827+
"translated_address": cty.StringVal("192.168.0.1"),
828+
"translated_port": cty.StringVal("3306"),
829+
}),
830+
}),
831+
}),
832+
},
754833
}
755834

756835
for name, tc := range testCases {

0 commit comments

Comments
 (0)