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.

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

function: Switch the representation of variadic arguments to types.Tuple #923

Merged
merged 16 commits into from
Feb 21, 2024
Merged
8 changes: 8 additions & 0 deletions .changes/unreleased/BREAKING CHANGES-20240216-142501.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
kind: BREAKING CHANGES
body: 'function: Changed the framework type for variadic parameters to `types.TupleType`,
where each element is the same element type. Provider-defined functions using a
`types.List` for retrieving variadic argument data will need to update their code
to use `types.Tuple`.'
time: 2024-02-16T14:25:01.729428-05:00
custom:
Issue: "923"
12 changes: 6 additions & 6 deletions function/arguments_data.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ func (d ArgumentsData) Equal(o ArgumentsData) bool {
// Each target type must be acceptable for the data type in the parameter
// definition.
//
// Variadic parameter argument data must be consumed by a types.List or Go slice
// Variadic parameter argument data must be consumed by a types.Tuple or Go slice
// type with an element type appropriate for the parameter definition ([]T). The
// framework automatically populates this list with elements matching the zero,
// framework automatically populates this tuple with elements matching the zero,
// one, or more arguments passed.
func (d ArgumentsData) Get(ctx context.Context, targets ...any) diag.Diagnostics {
var diags diag.Diagnostics
Expand Down Expand Up @@ -112,10 +112,10 @@ func (d ArgumentsData) Get(ctx context.Context, targets ...any) diag.Diagnostics
// position and populates the target with the value. The target type must be
// acceptable for the data type in the parameter definition.
//
// Variadic parameter argument data must be consumed by a types.List or Go slice
// Variadic parameter argument data must be consumed by a types.Tuple or Go slice
// type with an element type appropriate for the parameter definition ([]T) at
// the position after all parameters. The framework automatically populates this
// list with elements matching the zero, one, or more arguments passed.
// tuple with elements matching the zero, one, or more arguments passed.
func (d ArgumentsData) GetArgument(ctx context.Context, position int, target any) diag.Diagnostics {
var diags diag.Diagnostics

Expand All @@ -134,8 +134,8 @@ func (d ArgumentsData) GetArgument(ctx context.Context, position int, target any
diags.AddError(
"Invalid Argument Data Position",
"When attempting to fetch argument data during the function call, the provider code attempted to read a non-existent argument position. "+
"Function argument positions are 0-based and any final variadic parameter is represented as one argument position with an ordered list of the parameter data type. "+
"This is always an error in the provider code and should be reported to the provider developers.\n\n"+
"Function argument positions are 0-based and any final variadic parameter is represented as one argument position with a tuple where each element "+
"type matches the parameter data type. This is always an error in the provider code and should be reported to the provider developers.\n\n"+
fmt.Sprintf("Given argument position: %d, last argument position: %d", position, len(d.values)-1),
)

Expand Down
127 changes: 125 additions & 2 deletions function/arguments_data_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,46 @@ func TestArgumentsDataGet(t *testing.T) {
pointer(attr.Value(basetypes.NewStringValue("test"))),
},
},
"attr-value-variadic": {
argumentsData: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolNull(),
basetypes.NewInt64Unknown(),
basetypes.NewStringValue("test"),
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("test1"),
basetypes.NewStringValue("test2"),
},
),
}),
targets: []any{
new(attr.Value),
new(attr.Value),
new(attr.Value),
new(attr.Value),
},
expected: []any{
pointer(attr.Value(basetypes.NewBoolNull())),
pointer(attr.Value(basetypes.NewInt64Unknown())),
pointer(attr.Value(basetypes.NewStringValue("test"))),
pointer(attr.Value(
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("test1"),
basetypes.NewStringValue("test2"),
},
),
)),
},
},
"framework-type": {
argumentsData: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolNull(),
Expand All @@ -190,6 +230,46 @@ func TestArgumentsDataGet(t *testing.T) {
pointer(basetypes.NewStringValue("test")),
},
},
"framework-type-variadic": {
argumentsData: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolNull(),
basetypes.NewInt64Unknown(),
basetypes.NewStringValue("test"),
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("test1"),
basetypes.NewStringValue("test2"),
},
),
}),
targets: []any{
new(basetypes.BoolValue),
new(basetypes.Int64Value),
new(basetypes.StringValue),
new(basetypes.TupleValue),
},
expected: []any{
pointer(basetypes.NewBoolNull()),
pointer(basetypes.NewInt64Unknown()),
pointer(basetypes.NewStringValue("test")),
pointer(
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("test1"),
basetypes.NewStringValue("test2"),
},
),
),
},
},
"reflection": {
argumentsData: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolNull(),
Expand All @@ -204,6 +284,46 @@ func TestArgumentsDataGet(t *testing.T) {
pointer("test"),
},
},
"reflection-variadic": {
argumentsData: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolNull(),
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("test1"),
basetypes.NewStringValue("test2"),
},
),
}),
targets: []any{
new(*bool),
new([]string),
},
expected: []any{
pointer((*bool)(nil)),
pointer([]string{
"test1",
"test2",
}),
},
},
"reflection-variadic-empty": {
argumentsData: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolNull(),
basetypes.NewTupleValueMust([]attr.Type{}, []attr.Value{}),
}),
targets: []any{
new(*bool),
new([]string),
},
expected: []any{
pointer((*bool)(nil)),
pointer([]string{}),
},
},
}

for name, testCase := range testCases {
Expand All @@ -225,6 +345,9 @@ func TestArgumentsDataGet(t *testing.T) {
cmp.Transformer("StringValue", func(v *basetypes.StringValue) basetypes.StringValue {
return *v
}),
cmp.Transformer("TupleValue", func(v *basetypes.TupleValue) basetypes.TupleValue {
return *v
}),
}

if diff := cmp.Diff(testCase.targets, testCase.expected, options...); diff != "" {
Expand Down Expand Up @@ -273,8 +396,8 @@ func TestArgumentsDataGetArgument(t *testing.T) {
diag.NewErrorDiagnostic(
"Invalid Argument Data Position",
"When attempting to fetch argument data during the function call, the provider code attempted to read a non-existent argument position. "+
"Function argument positions are 0-based and any final variadic parameter is represented as one argument position with an ordered list of the parameter data type. "+
"This is always an error in the provider code and should be reported to the provider developers.\n\n"+
"Function argument positions are 0-based and any final variadic parameter is represented as one argument position with a tuple where each element "+
"type matches the parameter data type. This is always an error in the provider code and should be reported to the provider developers.\n\n"+
"Given argument position: 1, last argument position: 0",
),
},
Expand Down
2 changes: 1 addition & 1 deletion function/definition.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ type Definition struct {

// VariadicParameter is an optional final parameter which can accept zero or
// more arguments when the function is called. The argument data is sent as
// an ordered list of the associated data type.
// a tuple, where all elements are of the same associated data type.
VariadicParameter Parameter

// Return is the function call response data type.
Expand Down
24 changes: 22 additions & 2 deletions internal/fromproto5/arguments_data.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func ArgumentsData(ctx context.Context, arguments []*tfprotov5.DynamicValue, def
return function.NewArgumentsData(nil), nil
}

// Variadic values are collected as a separate list to ease developer usage.
// Variadic values are collected as a separate tuple to ease developer usage.
argumentValues := make([]attr.Value, 0, len(definition.Parameters))
variadicValues := make([]attr.Value, 0, len(arguments)-len(definition.Parameters))
var diags diag.Diagnostics
Expand Down Expand Up @@ -133,7 +133,27 @@ func ArgumentsData(ctx context.Context, arguments []*tfprotov5.DynamicValue, def
}

if definition.VariadicParameter != nil {
variadicValue, variadicValueDiags := basetypes.NewListValue(definition.VariadicParameter.GetType(), variadicValues)
// MAINTAINER NOTE: Variadic parameters are represented as individual arguments in the CallFunction RPC and Terraform core applies the variadic parameter
// type constraint to each argument individually. For developer convenience, the framework logic below, groups the variadic arguments into a
// framework Tuple where each element type of the tuple matches the variadic parameter type.
//
// Previously, this logic utilized a framework List with an element type that matched the variadic parameter type. Using a List presented an issue with dynamic
// variadic parameters, as each argument was allowed to be any type "individually", rather than having a single type constraint applied to all dynamic elements,
// like a cty.List in Terraform. This eventually results in an error attempting to create a tftypes.List with multiple element types (when unwrapping from a framework
// dynamic to a tftypes concrete value).
//
// While a framework List type can handle multiple dynamic values of different types (due to it's wrapping of dynamic values), `terraform-plugin-go` and `tftypes.List` cannot.
// Currently, the logic for retrieving argument data is dependent on the tftypes package to utilize the framework reflection logic, requiring us to apply a type constraint
// that is valid in Terraform core and `terraform-plugin-go`, which we are doing here with a Tuple.
variadicType := definition.VariadicParameter.GetType()
tupleTypes := make([]attr.Type, len(variadicValues))
tupleValues := make([]attr.Value, len(variadicValues))
for i, val := range variadicValues {
tupleTypes[i] = variadicType
tupleValues[i] = val
}

variadicValue, variadicValueDiags := basetypes.NewTupleValue(tupleTypes, tupleValues)

diags.Append(variadicValueDiags...)

Expand Down
57 changes: 37 additions & 20 deletions internal/fromproto5/arguments_data_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,8 +183,8 @@ func TestArgumentsData(t *testing.T) {
},
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolValue(true),
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{},
[]attr.Value{},
),
}),
Expand All @@ -202,8 +202,10 @@ func TestArgumentsData(t *testing.T) {
},
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolValue(true),
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("varg-arg1"),
},
Expand All @@ -224,8 +226,11 @@ func TestArgumentsData(t *testing.T) {
},
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolValue(true),
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("varg-arg1"),
basetypes.NewStringValue("varg-arg2"),
Expand Down Expand Up @@ -264,8 +269,8 @@ func TestArgumentsData(t *testing.T) {
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolValue(true),
basetypes.NewBoolValue(false),
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{},
[]attr.Value{},
),
}),
Expand All @@ -286,8 +291,10 @@ func TestArgumentsData(t *testing.T) {
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolValue(true),
basetypes.NewBoolValue(false),
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("varg-arg2"),
},
Expand All @@ -311,8 +318,11 @@ func TestArgumentsData(t *testing.T) {
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewBoolValue(true),
basetypes.NewBoolValue(false),
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("varg-arg2"),
basetypes.NewStringValue("varg-arg3"),
Expand All @@ -326,8 +336,8 @@ func TestArgumentsData(t *testing.T) {
VariadicParameter: function.StringParameter{},
},
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{},
[]attr.Value{},
),
}),
Expand All @@ -340,8 +350,10 @@ func TestArgumentsData(t *testing.T) {
VariadicParameter: function.StringParameter{},
},
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("varg-arg0"),
},
Expand All @@ -358,8 +370,10 @@ func TestArgumentsData(t *testing.T) {
},
},
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewListValueMust(
testtypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{
testtypes.StringType{},
},
[]attr.Value{
testtypes.String{
CreatedBy: testtypes.StringType{},
Expand All @@ -378,8 +392,11 @@ func TestArgumentsData(t *testing.T) {
VariadicParameter: function.StringParameter{},
},
expected: function.NewArgumentsData([]attr.Value{
basetypes.NewListValueMust(
basetypes.StringType{},
basetypes.NewTupleValueMust(
[]attr.Type{
basetypes.StringType{},
basetypes.StringType{},
},
[]attr.Value{
basetypes.NewStringValue("varg-arg0"),
basetypes.NewStringValue("varg-arg1"),
Expand Down
Loading
Loading