From 8f7d614268bcfe5aa3960b19c208db09a44d22a6 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Wed, 28 Feb 2024 15:09:22 -0500 Subject: [PATCH 1/6] add validation and refactor defaulting logic --- function/bool_parameter.go | 13 +- function/bool_parameter_test.go | 8 +- function/definition.go | 57 ++++ function/definition_test.go | 310 +++++++++++++++++- function/float64_parameter.go | 13 +- function/float64_parameter_test.go | 8 +- function/int64_parameter.go | 13 +- function/int64_parameter_test.go | 8 +- function/list_parameter.go | 13 +- function/list_parameter_test.go | 8 +- function/map_parameter.go | 13 +- function/map_parameter_test.go | 8 +- function/number_parameter.go | 13 +- function/number_parameter_test.go | 8 +- function/object_parameter.go | 13 +- function/object_parameter_test.go | 8 +- function/parameter.go | 13 +- function/set_parameter.go | 13 +- function/set_parameter_test.go | 8 +- function/string_parameter.go | 13 +- function/string_parameter_test.go | 8 +- internal/toproto5/function.go | 32 +- internal/toproto5/function_test.go | 73 ++++- internal/toproto5/getfunctions_test.go | 8 +- internal/toproto5/getproviderschema_test.go | 8 +- internal/toproto6/function.go | 32 +- internal/toproto6/function_test.go | 73 ++++- internal/toproto6/getfunctions_test.go | 8 +- internal/toproto6/getproviderschema_test.go | 8 +- .../framework/functions/documentation.mdx | 4 +- 30 files changed, 605 insertions(+), 210 deletions(-) diff --git a/function/bool_parameter.go b/function/bool_parameter.go index 02910f518..4c6ec94a9 100644 --- a/function/bool_parameter.go +++ b/function/bool_parameter.go @@ -60,8 +60,11 @@ type BoolParameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -91,11 +94,7 @@ func (p BoolParameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p BoolParameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/bool_parameter_test.go b/function/bool_parameter_test.go index 322c821c4..f82113cf7 100644 --- a/function/bool_parameter_test.go +++ b/function/bool_parameter_test.go @@ -182,13 +182,7 @@ func TestBoolParameterGetName(t *testing.T) { }{ "unset": { parameter: function.BoolParameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.BoolParameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.BoolParameter{ diff --git a/function/definition.go b/function/definition.go index b95ee29bc..bec53892a 100644 --- a/function/definition.go +++ b/function/definition.go @@ -85,6 +85,27 @@ func (d Definition) Parameter(ctx context.Context, position int) (Parameter, dia return d.Parameters[position], nil } +// ParameterName returns the Parameter name for a given argument position. This will be the return +// from the `(Parameter).GetName` function or a default value. An error diagnostic is raised if the +// position is outside the expected arguments. +func (d Definition) ParameterName(ctx context.Context, position int) (string, diag.Diagnostics) { + parameter, diags := d.Parameter(ctx, position) + if diags.HasError() { + return "", diags + } + + definedName := parameter.GetName() + if definedName != "" { + return definedName, nil + } + + if position >= len(d.Parameters) { + return DefaultVariadicParameterName, nil + } + + return fmt.Sprintf("%s%d", DefaultParameterNamePrefix, position+1), nil +} + // ValidateImplementation contains logic for validating the provider-defined // implementation of the definition to prevent unexpected errors or panics. This // logic runs during the GetProviderSchema RPC, or via provider-defined unit @@ -108,6 +129,42 @@ func (d Definition) ValidateImplementation(ctx context.Context) diag.Diagnostics ) } + paramNames := make(map[string]int, len(d.Parameters)) + for pos := range d.Parameters { + // TODO: what should we do with the diags? This should never happen + name, _ := d.ParameterName(ctx, pos) + + conflictPos, exists := paramNames[name] + if exists { + diags.AddError( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + fmt.Sprintf("Parameters at position %d and %d have the same name %q", conflictPos, pos, name), + ) + continue + } + + paramNames[name] = pos + } + + if d.VariadicParameter != nil { + // TODO: what should we do with the diags? This should never happen + name, _ := d.ParameterName(ctx, len(d.Parameters)+1) + + conflictPos, exists := paramNames[name] + if exists { + diags.AddError( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + fmt.Sprintf("Parameter at position %d and the variadic parameter have the same name %q", conflictPos, name), + ) + } + } + return diags } diff --git a/function/definition_test.go b/function/definition_test.go index 4aae5bf77..fe502d8f2 100644 --- a/function/definition_test.go +++ b/function/definition_test.go @@ -145,6 +145,143 @@ func TestDefinitionParameter(t *testing.T) { } } +func TestDefinitionParameterName(t *testing.T) { + t.Parallel() + + testCases := map[string]struct { + definition function.Definition + position int + expected string + expectedDiagnostics diag.Diagnostics + }{ + "none": { + definition: function.Definition{ + // no Parameters or VariadicParameter + }, + position: 0, + expected: "", + expectedDiagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid Parameter Position for Definition", + "When determining the parameter for the given argument position, an invalid value was given. "+ + "This is always an issue in the provider code and should be reported to the provider developers.\n\n"+ + "Function does not implement parameters.\n"+ + "Given position: 0", + ), + }, + }, + "parameters-default-first": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{}, + function.Int64Parameter{}, + function.StringParameter{}, + }, + }, + position: 0, + expected: "param1", + }, + "parameters-default--last": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{}, + function.Int64Parameter{}, + function.StringParameter{}, + }, + }, + position: 2, + expected: "param3", + }, + "parameters-default--middle": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{}, + function.Int64Parameter{}, + function.StringParameter{}, + }, + }, + position: 1, + expected: "param2", + }, + "parameters-defined": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{ + Name: "myspecialparam", + }, + }, + }, + position: 0, + expected: "myspecialparam", + }, + "parameters-over": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{}, + }, + }, + position: 1, + expected: "", + expectedDiagnostics: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid Parameter Position for Definition", + "When determining the parameter for the given argument position, an invalid value was given. "+ + "This is always an issue in the provider code and should be reported to the provider developers.\n\n"+ + "Max argument position: 0\n"+ + "Given position: 1", + ), + }, + }, + "variadicparameter-and-parameters-select-parameter": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{}, + }, + VariadicParameter: function.StringParameter{}, + }, + position: 0, + expected: "param1", + }, + "variadicparameter-and-parameters-select-variadicparameter-default": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{}, + }, + VariadicParameter: function.StringParameter{}, + }, + position: 1, + expected: function.DefaultVariadicParameterName, + }, + "variadicparameter-defined": { + definition: function.Definition{ + VariadicParameter: function.StringParameter{ + Name: "myspecialvariadic", + }, + }, + position: 0, + expected: "myspecialvariadic", + }, + } + + for name, testCase := range testCases { + name, testCase := name, testCase + + t.Run(name, func(t *testing.T) { + t.Parallel() + + got, diags := testCase.definition.ParameterName(context.Background(), testCase.position) + + if diff := cmp.Diff(got, testCase.expected); diff != "" { + t.Errorf("unexpected difference: %s", diff) + } + + if diff := cmp.Diff(diags, testCase.expectedDiagnostics); diff != "" { + t.Errorf("unexpected diagnostics difference: %s", diff) + } + }) + } +} + func TestDefinitionValidateImplementation(t *testing.T) { t.Parallel() @@ -152,11 +289,35 @@ func TestDefinitionValidateImplementation(t *testing.T) { definition function.Definition expected diag.Diagnostics }{ - "valid": { + "valid-no-params": { + definition: function.Definition{ + Return: function.StringReturn{}, + }, + }, + "valid-only-variadic": { definition: function.Definition{ + VariadicParameter: function.StringParameter{}, + Return: function.StringReturn{}, + }, + }, + "valid-param-name-defaults": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.StringParameter{}, + function.StringParameter{}, + }, Return: function.StringReturn{}, }, }, + "valid-param-names-defaults-with-variadic": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.StringParameter{}, + }, + VariadicParameter: function.NumberParameter{}, + Return: function.StringReturn{}, + }, + }, "result-missing": { definition: function.Definition{}, expected: diag.Diagnostics{ @@ -168,6 +329,153 @@ func TestDefinitionValidateImplementation(t *testing.T) { ), }, }, + "conflicting-param-names": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.StringParameter{ + Name: "string-param", + }, + function.Float64Parameter{ + Name: "float-param", + }, + function.Int64Parameter{ + Name: "param-dup", + }, + function.NumberParameter{ + Name: "number-param", + }, + function.BoolParameter{ + Name: "param-dup", + }, + }, + Return: function.StringReturn{}, + }, + expected: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + "Parameters at position 2 and 4 have the same name \"param-dup\"", + ), + }, + }, + "conflicting-param-name-with-default": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.StringParameter{ + Name: "param2", + }, + function.Float64Parameter{}, // defaults to param2 + }, + Return: function.StringReturn{}, + }, + expected: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + "Parameters at position 0 and 1 have the same name \"param2\"", + ), + }, + }, + "conflicting-param-names-variadic": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.Float64Parameter{ + Name: "float-param", + }, + function.Int64Parameter{ + Name: "param-dup", + }, + function.NumberParameter{ + Name: "number-param", + }, + }, + VariadicParameter: function.BoolParameter{ + Name: "param-dup", + }, + Return: function.StringReturn{}, + }, + expected: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + "Parameter at position 1 and the variadic parameter have the same name \"param-dup\"", + ), + }, + }, + "conflicting-param-names-variadic-multiple": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.StringParameter{ + Name: "param-dup", + }, + function.Float64Parameter{ + Name: "float-param", + }, + function.Int64Parameter{ + Name: "param-dup", + }, + function.NumberParameter{ + Name: "number-param", + }, + function.BoolParameter{ + Name: "param-dup", + }, + }, + VariadicParameter: function.BoolParameter{ + Name: "param-dup", + }, + Return: function.StringReturn{}, + }, + expected: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + "Parameters at position 0 and 2 have the same name \"param-dup\"", + ), + diag.NewErrorDiagnostic( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + "Parameters at position 0 and 4 have the same name \"param-dup\"", + ), + diag.NewErrorDiagnostic( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + "Parameter at position 0 and the variadic parameter have the same name \"param-dup\"", + ), + }, + }, + "conflicting-param-name-with-variadic-default": { + definition: function.Definition{ + Parameters: []function.Parameter{ + function.Float64Parameter{ + Name: function.DefaultVariadicParameterName, + }, + }, + VariadicParameter: function.BoolParameter{}, // defaults to varparam + Return: function.StringReturn{}, + }, + expected: diag.Diagnostics{ + diag.NewErrorDiagnostic( + "Invalid Function Definition", + "When validating the function definition, an implementation issue was found. "+ + "This is always an issue with the provider and should be reported to the provider developers.\n\n"+ + "Parameter names must be unique. "+ + "Parameter at position 0 and the variadic parameter have the same name \"varparam\"", + ), + }, + }, } for name, testCase := range testCases { diff --git a/function/float64_parameter.go b/function/float64_parameter.go index b0b7bdb8a..221fb4f22 100644 --- a/function/float64_parameter.go +++ b/function/float64_parameter.go @@ -57,8 +57,11 @@ type Float64Parameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -88,11 +91,7 @@ func (p Float64Parameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p Float64Parameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/float64_parameter_test.go b/function/float64_parameter_test.go index 6cd6eaedb..479d06437 100644 --- a/function/float64_parameter_test.go +++ b/function/float64_parameter_test.go @@ -182,13 +182,7 @@ func TestFloat64ParameterGetName(t *testing.T) { }{ "unset": { parameter: function.Float64Parameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.Float64Parameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.Float64Parameter{ diff --git a/function/int64_parameter.go b/function/int64_parameter.go index 6a4d7abb7..3fd6d8852 100644 --- a/function/int64_parameter.go +++ b/function/int64_parameter.go @@ -56,8 +56,11 @@ type Int64Parameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -87,11 +90,7 @@ func (p Int64Parameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p Int64Parameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/int64_parameter_test.go b/function/int64_parameter_test.go index 4651270e2..5422dcd69 100644 --- a/function/int64_parameter_test.go +++ b/function/int64_parameter_test.go @@ -182,13 +182,7 @@ func TestInt64ParameterGetName(t *testing.T) { }{ "unset": { parameter: function.Int64Parameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.Int64Parameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.Int64Parameter{ diff --git a/function/list_parameter.go b/function/list_parameter.go index 006e3750e..f2c60d9c3 100644 --- a/function/list_parameter.go +++ b/function/list_parameter.go @@ -60,8 +60,11 @@ type ListParameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -91,11 +94,7 @@ func (p ListParameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p ListParameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/list_parameter_test.go b/function/list_parameter_test.go index cf83fa09d..ae383fd1e 100644 --- a/function/list_parameter_test.go +++ b/function/list_parameter_test.go @@ -182,13 +182,7 @@ func TestListParameterGetName(t *testing.T) { }{ "unset": { parameter: function.ListParameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.ListParameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.ListParameter{ diff --git a/function/map_parameter.go b/function/map_parameter.go index fdde26d4d..2c7a7256f 100644 --- a/function/map_parameter.go +++ b/function/map_parameter.go @@ -60,8 +60,11 @@ type MapParameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -91,11 +94,7 @@ func (p MapParameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p MapParameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/map_parameter_test.go b/function/map_parameter_test.go index 092fe5880..83e6b8625 100644 --- a/function/map_parameter_test.go +++ b/function/map_parameter_test.go @@ -182,13 +182,7 @@ func TestMapParameterGetName(t *testing.T) { }{ "unset": { parameter: function.MapParameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.MapParameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.MapParameter{ diff --git a/function/number_parameter.go b/function/number_parameter.go index 05575cfc7..a4899fe79 100644 --- a/function/number_parameter.go +++ b/function/number_parameter.go @@ -55,8 +55,11 @@ type NumberParameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -86,11 +89,7 @@ func (p NumberParameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p NumberParameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/number_parameter_test.go b/function/number_parameter_test.go index 2847dc974..dcc097c1e 100644 --- a/function/number_parameter_test.go +++ b/function/number_parameter_test.go @@ -182,13 +182,7 @@ func TestNumberParameterGetName(t *testing.T) { }{ "unset": { parameter: function.NumberParameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.NumberParameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.NumberParameter{ diff --git a/function/object_parameter.go b/function/object_parameter.go index b51cf8095..8b8791e46 100644 --- a/function/object_parameter.go +++ b/function/object_parameter.go @@ -62,8 +62,11 @@ type ObjectParameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -93,11 +96,7 @@ func (p ObjectParameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p ObjectParameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/object_parameter_test.go b/function/object_parameter_test.go index fce67ae0c..10358943b 100644 --- a/function/object_parameter_test.go +++ b/function/object_parameter_test.go @@ -182,13 +182,7 @@ func TestObjectParameterGetName(t *testing.T) { }{ "unset": { parameter: function.ObjectParameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.ObjectParameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.ObjectParameter{ diff --git a/function/parameter.go b/function/parameter.go index 4a7c89ff1..dc396423f 100644 --- a/function/parameter.go +++ b/function/parameter.go @@ -7,9 +7,16 @@ import ( "github.com/hashicorp/terraform-plugin-framework/attr" ) -// DefaultParameterName is the name given to parameters which do not declare -// a name. Use this to prevent Terraform errors for missing names. -const DefaultParameterName = "param" +const ( + // DefaultParameterNamePrefix is the prefix used to default the name of parameters which do not declare + // a name. Use this to prevent Terraform errors for missing names. This prefix is used with the parameter + // position in a function definition to create a unique name (param1, param2, etc.) + DefaultParameterNamePrefix = "param" + + // DefaultVariadicParameterName is the default name given to a variadic parameter that does not declare + // a name. Use this to prevent Terraform errors for missing names. + DefaultVariadicParameterName = "varparam" +) // Parameter is the interface for defining function parameters. type Parameter interface { diff --git a/function/set_parameter.go b/function/set_parameter.go index a028d2109..f920f05ef 100644 --- a/function/set_parameter.go +++ b/function/set_parameter.go @@ -60,8 +60,11 @@ type SetParameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -91,11 +94,7 @@ func (p SetParameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p SetParameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/set_parameter_test.go b/function/set_parameter_test.go index 1328119f3..29fd2820a 100644 --- a/function/set_parameter_test.go +++ b/function/set_parameter_test.go @@ -182,13 +182,7 @@ func TestSetParameterGetName(t *testing.T) { }{ "unset": { parameter: function.SetParameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.SetParameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.SetParameter{ diff --git a/function/string_parameter.go b/function/string_parameter.go index b8a3454d7..8584f94d4 100644 --- a/function/string_parameter.go +++ b/function/string_parameter.go @@ -56,8 +56,11 @@ type StringParameter struct { // Name is a short usage name for the parameter, such as "data". This name // is used in documentation, such as generating a function signature, - // however its usage may be extended in the future. If no name is provided, - // this will default to "param". + // however its usage may be extended in the future. + // + // If no name is provided, this will default to "param" with a suffix of the + // position the parameter is in the function definition. ("param1", "param2", etc.) + // If the parameter is variadic, the default name will be "varparam". // // This must be a valid Terraform identifier, such as starting with an // alphabetical character and followed by alphanumeric or underscore @@ -87,11 +90,7 @@ func (p StringParameter) GetMarkdownDescription() string { // GetName returns the parameter name. func (p StringParameter) GetName() string { - if p.Name != "" { - return p.Name - } - - return DefaultParameterName + return p.Name } // GetType returns the parameter data type. diff --git a/function/string_parameter_test.go b/function/string_parameter_test.go index 2602fa525..1cf9bbcbf 100644 --- a/function/string_parameter_test.go +++ b/function/string_parameter_test.go @@ -182,13 +182,7 @@ func TestStringParameterGetName(t *testing.T) { }{ "unset": { parameter: function.StringParameter{}, - expected: function.DefaultParameterName, - }, - "Name-empty": { - parameter: function.StringParameter{ - Name: "", - }, - expected: function.DefaultParameterName, + expected: "", }, "Name-nonempty": { parameter: function.StringParameter{ diff --git a/internal/toproto5/function.go b/internal/toproto5/function.go index 07388f07a..e414a0bd5 100644 --- a/internal/toproto5/function.go +++ b/internal/toproto5/function.go @@ -19,7 +19,6 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov5.Function { Parameters: make([]*tfprotov5.FunctionParameter, 0, len(fw.Parameters)), Return: FunctionReturn(ctx, fw.Return), Summary: fw.Summary, - VariadicParameter: FunctionParameter(ctx, fw.VariadicParameter), } if fw.MarkdownDescription != "" { @@ -30,8 +29,12 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov5.Function { proto.DescriptionKind = tfprotov5.StringKindPlain } - for _, fwParameter := range fw.Parameters { - proto.Parameters = append(proto.Parameters, FunctionParameter(ctx, fwParameter)) + for i, fwParameter := range fw.Parameters { + proto.Parameters = append(proto.Parameters, FunctionParameter(ctx, fw, fwParameter, i)) + } + + if fw.VariadicParameter != nil { + proto.VariadicParameter = FunctionParameter(ctx, fw, fw.VariadicParameter, len(fw.Parameters)+1) } return proto @@ -39,23 +42,26 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov5.Function { // FunctionParameter returns the *tfprotov5.FunctionParameter for a // function.Parameter. -func FunctionParameter(ctx context.Context, fw function.Parameter) *tfprotov5.FunctionParameter { - if fw == nil { +func FunctionParameter(ctx context.Context, def function.Definition, param function.Parameter, position int) *tfprotov5.FunctionParameter { + if param == nil { return nil } + // TODO: what should we do with the diags? This should never happen + name, _ := def.ParameterName(ctx, position) + proto := &tfprotov5.FunctionParameter{ - AllowNullValue: fw.GetAllowNullValue(), - AllowUnknownValues: fw.GetAllowUnknownValues(), - Name: fw.GetName(), - Type: fw.GetType().TerraformType(ctx), + AllowNullValue: param.GetAllowNullValue(), + AllowUnknownValues: param.GetAllowUnknownValues(), + Name: name, + Type: param.GetType().TerraformType(ctx), } - if fw.GetMarkdownDescription() != "" { - proto.Description = fw.GetMarkdownDescription() + if param.GetMarkdownDescription() != "" { + proto.Description = param.GetMarkdownDescription() proto.DescriptionKind = tfprotov5.StringKindMarkdown - } else if fw.GetDescription() != "" { - proto.Description = fw.GetDescription() + } else if param.GetDescription() != "" { + proto.Description = param.GetDescription() proto.DescriptionKind = tfprotov5.StringKindPlain } diff --git a/internal/toproto5/function_test.go b/internal/toproto5/function_test.go index 0f10ea6b9..a0e1a69d3 100644 --- a/internal/toproto5/function_test.go +++ b/internal/toproto5/function_test.go @@ -78,15 +78,15 @@ func TestFunction(t *testing.T) { expected: &tfprotov5.Function{ Parameters: []*tfprotov5.FunctionParameter{ { - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, { - Name: function.DefaultParameterName, + Name: "param2", Type: tftypes.Number, }, { - Name: function.DefaultParameterName, + Name: "param3", Type: tftypes.String, }, }, @@ -95,6 +95,40 @@ func TestFunction(t *testing.T) { }, }, }, + "parameters-with-variadic": { + fw: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{}, + function.Int64Parameter{}, + function.StringParameter{}, + }, + VariadicParameter: function.Float64Parameter{}, + Return: function.StringReturn{}, + }, + expected: &tfprotov5.Function{ + Parameters: []*tfprotov5.FunctionParameter{ + { + Name: "param1", + Type: tftypes.Bool, + }, + { + Name: "param2", + Type: tftypes.Number, + }, + { + Name: "param3", + Type: tftypes.String, + }, + }, + VariadicParameter: &tfprotov5.FunctionParameter{ + Name: function.DefaultVariadicParameterName, + Type: tftypes.Number, + }, + Return: &tfprotov5.FunctionReturn{ + Type: tftypes.String, + }, + }, + }, "result": { fw: function.Definition{ Return: function.StringReturn{}, @@ -130,7 +164,7 @@ func TestFunction(t *testing.T) { Type: tftypes.String, }, VariadicParameter: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: function.DefaultVariadicParameterName, Type: tftypes.String, }, }, @@ -201,7 +235,7 @@ func TestFunctionParameter(t *testing.T) { }, expected: &tfprotov5.FunctionParameter{ AllowNullValue: true, - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, @@ -211,7 +245,7 @@ func TestFunctionParameter(t *testing.T) { }, expected: &tfprotov5.FunctionParameter{ AllowUnknownValues: true, - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, @@ -222,7 +256,7 @@ func TestFunctionParameter(t *testing.T) { expected: &tfprotov5.FunctionParameter{ Description: "test description", DescriptionKind: tfprotov5.StringKindPlain, - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, @@ -233,7 +267,7 @@ func TestFunctionParameter(t *testing.T) { expected: &tfprotov5.FunctionParameter{ Description: "test description", DescriptionKind: tfprotov5.StringKindMarkdown, - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, @@ -249,21 +283,21 @@ func TestFunctionParameter(t *testing.T) { "type-bool": { fw: function.BoolParameter{}, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, "type-float64": { fw: function.Float64Parameter{}, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Number, }, }, "type-int64": { fw: function.Int64Parameter{}, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Number, }, }, @@ -272,7 +306,7 @@ func TestFunctionParameter(t *testing.T) { ElementType: basetypes.StringType{}, }, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.List{ ElementType: tftypes.String, }, @@ -283,7 +317,7 @@ func TestFunctionParameter(t *testing.T) { ElementType: basetypes.StringType{}, }, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Map{ ElementType: tftypes.String, }, @@ -292,7 +326,7 @@ func TestFunctionParameter(t *testing.T) { "type-number": { fw: function.NumberParameter{}, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Number, }, }, @@ -305,7 +339,7 @@ func TestFunctionParameter(t *testing.T) { }, }, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Object{ AttributeTypes: map[string]tftypes.Type{ "bool": tftypes.Bool, @@ -320,7 +354,7 @@ func TestFunctionParameter(t *testing.T) { ElementType: basetypes.StringType{}, }, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Set{ ElementType: tftypes.String, }, @@ -329,7 +363,7 @@ func TestFunctionParameter(t *testing.T) { "type-string": { fw: function.StringParameter{}, expected: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.String, }, }, @@ -341,7 +375,10 @@ func TestFunctionParameter(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - got := toproto5.FunctionParameter(context.Background(), testCase.fw) + def := function.Definition{ + Parameters: []function.Parameter{testCase.fw}, + } + got := toproto5.FunctionParameter(context.Background(), def, testCase.fw, 0) if diff := cmp.Diff(got, testCase.expected); diff != "" { t.Errorf("unexpected difference: %s", diff) diff --git a/internal/toproto5/getfunctions_test.go b/internal/toproto5/getfunctions_test.go index 524566d9e..58bec8735 100644 --- a/internal/toproto5/getfunctions_test.go +++ b/internal/toproto5/getfunctions_test.go @@ -138,15 +138,15 @@ func TestGetFunctionsResponse(t *testing.T) { "testfunction": { Parameters: []*tfprotov5.FunctionParameter{ { - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, { - Name: function.DefaultParameterName, + Name: "param2", Type: tftypes.Number, }, { - Name: function.DefaultParameterName, + Name: "param3", Type: tftypes.String, }, }, @@ -214,7 +214,7 @@ func TestGetFunctionsResponse(t *testing.T) { Type: tftypes.String, }, VariadicParameter: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: function.DefaultVariadicParameterName, Type: tftypes.String, }, }, diff --git a/internal/toproto5/getproviderschema_test.go b/internal/toproto5/getproviderschema_test.go index ddce83357..a54fd1138 100644 --- a/internal/toproto5/getproviderschema_test.go +++ b/internal/toproto5/getproviderschema_test.go @@ -1072,15 +1072,15 @@ func TestGetProviderSchemaResponse(t *testing.T) { "testfunction": { Parameters: []*tfprotov5.FunctionParameter{ { - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, { - Name: function.DefaultParameterName, + Name: "param2", Type: tftypes.Number, }, { - Name: function.DefaultParameterName, + Name: "param3", Type: tftypes.String, }, }, @@ -1154,7 +1154,7 @@ func TestGetProviderSchemaResponse(t *testing.T) { Type: tftypes.String, }, VariadicParameter: &tfprotov5.FunctionParameter{ - Name: function.DefaultParameterName, + Name: function.DefaultVariadicParameterName, Type: tftypes.String, }, }, diff --git a/internal/toproto6/function.go b/internal/toproto6/function.go index e77be9245..e54450515 100644 --- a/internal/toproto6/function.go +++ b/internal/toproto6/function.go @@ -19,7 +19,6 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov6.Function { Parameters: make([]*tfprotov6.FunctionParameter, 0, len(fw.Parameters)), Return: FunctionReturn(ctx, fw.Return), Summary: fw.Summary, - VariadicParameter: FunctionParameter(ctx, fw.VariadicParameter), } if fw.MarkdownDescription != "" { @@ -30,8 +29,12 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov6.Function { proto.DescriptionKind = tfprotov6.StringKindPlain } - for _, fwParameter := range fw.Parameters { - proto.Parameters = append(proto.Parameters, FunctionParameter(ctx, fwParameter)) + for i, fwParameter := range fw.Parameters { + proto.Parameters = append(proto.Parameters, FunctionParameter(ctx, fw, fwParameter, i)) + } + + if fw.VariadicParameter != nil { + proto.VariadicParameter = FunctionParameter(ctx, fw, fw.VariadicParameter, len(fw.Parameters)+1) } return proto @@ -39,23 +42,26 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov6.Function { // FunctionParameter returns the *tfprotov6.FunctionParameter for a // function.Parameter. -func FunctionParameter(ctx context.Context, fw function.Parameter) *tfprotov6.FunctionParameter { - if fw == nil { +func FunctionParameter(ctx context.Context, def function.Definition, param function.Parameter, position int) *tfprotov6.FunctionParameter { + if param == nil { return nil } + // TODO: what should we do with the diags? This should never happen + name, _ := def.ParameterName(ctx, position) + proto := &tfprotov6.FunctionParameter{ - AllowNullValue: fw.GetAllowNullValue(), - AllowUnknownValues: fw.GetAllowUnknownValues(), - Name: fw.GetName(), - Type: fw.GetType().TerraformType(ctx), + AllowNullValue: param.GetAllowNullValue(), + AllowUnknownValues: param.GetAllowUnknownValues(), + Name: name, + Type: param.GetType().TerraformType(ctx), } - if fw.GetMarkdownDescription() != "" { - proto.Description = fw.GetMarkdownDescription() + if param.GetMarkdownDescription() != "" { + proto.Description = param.GetMarkdownDescription() proto.DescriptionKind = tfprotov6.StringKindMarkdown - } else if fw.GetDescription() != "" { - proto.Description = fw.GetDescription() + } else if param.GetDescription() != "" { + proto.Description = param.GetDescription() proto.DescriptionKind = tfprotov6.StringKindPlain } diff --git a/internal/toproto6/function_test.go b/internal/toproto6/function_test.go index 9e7e88bd2..dd8d0ed24 100644 --- a/internal/toproto6/function_test.go +++ b/internal/toproto6/function_test.go @@ -78,15 +78,15 @@ func TestFunction(t *testing.T) { expected: &tfprotov6.Function{ Parameters: []*tfprotov6.FunctionParameter{ { - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, { - Name: function.DefaultParameterName, + Name: "param2", Type: tftypes.Number, }, { - Name: function.DefaultParameterName, + Name: "param3", Type: tftypes.String, }, }, @@ -95,6 +95,40 @@ func TestFunction(t *testing.T) { }, }, }, + "parameters-with-variadic": { + fw: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{}, + function.Int64Parameter{}, + function.StringParameter{}, + }, + VariadicParameter: function.Float64Parameter{}, + Return: function.StringReturn{}, + }, + expected: &tfprotov6.Function{ + Parameters: []*tfprotov6.FunctionParameter{ + { + Name: "param1", + Type: tftypes.Bool, + }, + { + Name: "param2", + Type: tftypes.Number, + }, + { + Name: "param3", + Type: tftypes.String, + }, + }, + VariadicParameter: &tfprotov6.FunctionParameter{ + Name: function.DefaultVariadicParameterName, + Type: tftypes.Number, + }, + Return: &tfprotov6.FunctionReturn{ + Type: tftypes.String, + }, + }, + }, "result": { fw: function.Definition{ Return: function.StringReturn{}, @@ -130,7 +164,7 @@ func TestFunction(t *testing.T) { Type: tftypes.String, }, VariadicParameter: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: function.DefaultVariadicParameterName, Type: tftypes.String, }, }, @@ -201,7 +235,7 @@ func TestFunctionParameter(t *testing.T) { }, expected: &tfprotov6.FunctionParameter{ AllowNullValue: true, - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, @@ -211,7 +245,7 @@ func TestFunctionParameter(t *testing.T) { }, expected: &tfprotov6.FunctionParameter{ AllowUnknownValues: true, - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, @@ -222,7 +256,7 @@ func TestFunctionParameter(t *testing.T) { expected: &tfprotov6.FunctionParameter{ Description: "test description", DescriptionKind: tfprotov6.StringKindPlain, - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, @@ -233,7 +267,7 @@ func TestFunctionParameter(t *testing.T) { expected: &tfprotov6.FunctionParameter{ Description: "test description", DescriptionKind: tfprotov6.StringKindMarkdown, - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, @@ -249,21 +283,21 @@ func TestFunctionParameter(t *testing.T) { "type-bool": { fw: function.BoolParameter{}, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, }, "type-float64": { fw: function.Float64Parameter{}, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Number, }, }, "type-int64": { fw: function.Int64Parameter{}, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Number, }, }, @@ -272,7 +306,7 @@ func TestFunctionParameter(t *testing.T) { ElementType: basetypes.StringType{}, }, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.List{ ElementType: tftypes.String, }, @@ -283,7 +317,7 @@ func TestFunctionParameter(t *testing.T) { ElementType: basetypes.StringType{}, }, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Map{ ElementType: tftypes.String, }, @@ -292,7 +326,7 @@ func TestFunctionParameter(t *testing.T) { "type-number": { fw: function.NumberParameter{}, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Number, }, }, @@ -305,7 +339,7 @@ func TestFunctionParameter(t *testing.T) { }, }, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Object{ AttributeTypes: map[string]tftypes.Type{ "bool": tftypes.Bool, @@ -320,7 +354,7 @@ func TestFunctionParameter(t *testing.T) { ElementType: basetypes.StringType{}, }, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Set{ ElementType: tftypes.String, }, @@ -329,7 +363,7 @@ func TestFunctionParameter(t *testing.T) { "type-string": { fw: function.StringParameter{}, expected: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.String, }, }, @@ -341,7 +375,10 @@ func TestFunctionParameter(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - got := toproto6.FunctionParameter(context.Background(), testCase.fw) + def := function.Definition{ + Parameters: []function.Parameter{testCase.fw}, + } + got := toproto6.FunctionParameter(context.Background(), def, testCase.fw, 0) if diff := cmp.Diff(got, testCase.expected); diff != "" { t.Errorf("unexpected difference: %s", diff) diff --git a/internal/toproto6/getfunctions_test.go b/internal/toproto6/getfunctions_test.go index 789eab18c..3b4dfa9f1 100644 --- a/internal/toproto6/getfunctions_test.go +++ b/internal/toproto6/getfunctions_test.go @@ -138,15 +138,15 @@ func TestGetFunctionsResponse(t *testing.T) { "testfunction": { Parameters: []*tfprotov6.FunctionParameter{ { - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, { - Name: function.DefaultParameterName, + Name: "param2", Type: tftypes.Number, }, { - Name: function.DefaultParameterName, + Name: "param3", Type: tftypes.String, }, }, @@ -214,7 +214,7 @@ func TestGetFunctionsResponse(t *testing.T) { Type: tftypes.String, }, VariadicParameter: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: function.DefaultVariadicParameterName, Type: tftypes.String, }, }, diff --git a/internal/toproto6/getproviderschema_test.go b/internal/toproto6/getproviderschema_test.go index 20cd7df10..b53854275 100644 --- a/internal/toproto6/getproviderschema_test.go +++ b/internal/toproto6/getproviderschema_test.go @@ -1120,15 +1120,15 @@ func TestGetProviderSchemaResponse(t *testing.T) { "testfunction": { Parameters: []*tfprotov6.FunctionParameter{ { - Name: function.DefaultParameterName, + Name: "param1", Type: tftypes.Bool, }, { - Name: function.DefaultParameterName, + Name: "param2", Type: tftypes.Number, }, { - Name: function.DefaultParameterName, + Name: "param3", Type: tftypes.String, }, }, @@ -1202,7 +1202,7 @@ func TestGetProviderSchemaResponse(t *testing.T) { Type: tftypes.String, }, VariadicParameter: &tfprotov6.FunctionParameter{ - Name: function.DefaultParameterName, + Name: function.DefaultVariadicParameterName, Type: tftypes.String, }, }, diff --git a/website/docs/plugin/framework/functions/documentation.mdx b/website/docs/plugin/framework/functions/documentation.mdx index 94c38ee3a..40bc94833 100644 --- a/website/docs/plugin/framework/functions/documentation.mdx +++ b/website/docs/plugin/framework/functions/documentation.mdx @@ -47,11 +47,11 @@ Each [parameter type](/terraform/plugin/framework/functions/parameters), whether | Field Name | Description | |---|---| -| `Name` | Single word or abbreviation of parameter for function signature generation, defaults to `param`. | +| `Name` | Single word or abbreviation of parameter for function signature generation. If name is not provided, will default to the prefix "param" with a suffix of the position the parameter is in the function definition. (`param1`, `param2`, etc.). If the parameter is variadic, the default name will be `varparam`. | | `Description` | Documentation about the parameter and its expected values in plaintext format. | | `MarkdownDescription` | Documentation about the parameter and its expected values in Markdown format. | -The name is only for documentation purposes and helpful when there is a need to disambiguate between multiple parameters, such as the words `cidr` and `ip` in a generated function signature like `cidr_contains_ip(cidr string, ip string) bool`. +The name must be unique in the context of the [function definition](/terraform/plugin/framework/functions/implementation#definition-method) and is only for documentation purposes. The name can be helpful when there is a need to disambiguate between multiple parameters, such as the words `cidr` and `ip` in a generated function signature like `cidr_contains_ip(cidr string, ip string) bool`. If there are no description formatting differences, set only one of `Description` or `MarkdownDescription`. When Terraform has not sent a preference for the description formatting, the framework will return `MarkdownDescription` if both are defined. From 5d5baf66baf700b3110776450f8ad1a6b3f30ba7 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Wed, 28 Feb 2024 15:22:58 -0500 Subject: [PATCH 2/6] add changelogs --- .changes/unreleased/BUG FIXES-20240228-151959.yaml | 6 ++++++ .changes/unreleased/BUG FIXES-20240228-152155.yaml | 6 ++++++ 2 files changed, 12 insertions(+) create mode 100644 .changes/unreleased/BUG FIXES-20240228-151959.yaml create mode 100644 .changes/unreleased/BUG FIXES-20240228-152155.yaml diff --git a/.changes/unreleased/BUG FIXES-20240228-151959.yaml b/.changes/unreleased/BUG FIXES-20240228-151959.yaml new file mode 100644 index 000000000..59b5f8420 --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20240228-151959.yaml @@ -0,0 +1,6 @@ +kind: BUG FIXES +body: 'function: Added implementation validation to `function.Definition` to ensure + all parameter names (including the variadic parameter) are unique.' +time: 2024-02-28T15:19:59.843244-05:00 +custom: + Issue: "926" diff --git a/.changes/unreleased/BUG FIXES-20240228-152155.yaml b/.changes/unreleased/BUG FIXES-20240228-152155.yaml new file mode 100644 index 000000000..6c384fd91 --- /dev/null +++ b/.changes/unreleased/BUG FIXES-20240228-152155.yaml @@ -0,0 +1,6 @@ +kind: BUG FIXES +body: 'function: Updated the default parameter name to include the position of the + parameter (i.e. `param1`, `param2`, etc.). Variadic parameters will default to `varparam`.' +time: 2024-02-28T15:21:55.182389-05:00 +custom: + Issue: "926" From 86dd2a9436642c6df462c28e88012092ef2d4273 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Wed, 28 Feb 2024 15:28:37 -0500 Subject: [PATCH 3/6] test fix --- function/definition_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/function/definition_test.go b/function/definition_test.go index fe502d8f2..cafb52fe6 100644 --- a/function/definition_test.go +++ b/function/definition_test.go @@ -181,7 +181,7 @@ func TestDefinitionParameterName(t *testing.T) { position: 0, expected: "param1", }, - "parameters-default--last": { + "parameters-default-last": { definition: function.Definition{ Parameters: []function.Parameter{ function.BoolParameter{}, @@ -192,7 +192,7 @@ func TestDefinitionParameterName(t *testing.T) { position: 2, expected: "param3", }, - "parameters-default--middle": { + "parameters-default-middle": { definition: function.Definition{ Parameters: []function.Parameter{ function.BoolParameter{}, From 97ffc56dfd229a9d9652466cd5ce03aa36fb76da Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Wed, 28 Feb 2024 16:11:41 -0500 Subject: [PATCH 4/6] Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Brian Flad --- website/docs/plugin/framework/functions/documentation.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/plugin/framework/functions/documentation.mdx b/website/docs/plugin/framework/functions/documentation.mdx index 40bc94833..4d58cdcd4 100644 --- a/website/docs/plugin/framework/functions/documentation.mdx +++ b/website/docs/plugin/framework/functions/documentation.mdx @@ -51,7 +51,7 @@ Each [parameter type](/terraform/plugin/framework/functions/parameters), whether | `Description` | Documentation about the parameter and its expected values in plaintext format. | | `MarkdownDescription` | Documentation about the parameter and its expected values in Markdown format. | -The name must be unique in the context of the [function definition](/terraform/plugin/framework/functions/implementation#definition-method) and is only for documentation purposes. The name can be helpful when there is a need to disambiguate between multiple parameters, such as the words `cidr` and `ip` in a generated function signature like `cidr_contains_ip(cidr string, ip string) bool`. +The name must be unique in the context of the [function definition](/terraform/plugin/framework/functions/implementation#definition-method). It is used for documentation purposes and displayed in error diagnostics presented to practitioners. The name should delineate the purpose of the parameter, especially to disambiguate between multiple parameters, such as the words `cidr` and `ip` in a generated function signature like `cidr_contains_ip(cidr string, ip string) bool`. If there are no description formatting differences, set only one of `Description` or `MarkdownDescription`. When Terraform has not sent a preference for the description formatting, the framework will return `MarkdownDescription` if both are defined. From 858ddbb0662c5223614de73b952033018cb6f2ec Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Wed, 28 Feb 2024 16:59:37 -0500 Subject: [PATCH 5/6] refactor the logic, tests and docs for defaulting --- function/definition.go | 37 +++----- function/definition_test.go | 137 ----------------------------- function/parameter.go | 4 + internal/toproto5/function.go | 42 +++++---- internal/toproto5/function_test.go | 126 ++++++++++++++++++++------ internal/toproto6/function.go | 42 +++++---- internal/toproto6/function_test.go | 128 +++++++++++++++++++++------ 7 files changed, 266 insertions(+), 250 deletions(-) diff --git a/function/definition.go b/function/definition.go index bec53892a..39c1a408e 100644 --- a/function/definition.go +++ b/function/definition.go @@ -85,27 +85,6 @@ func (d Definition) Parameter(ctx context.Context, position int) (Parameter, dia return d.Parameters[position], nil } -// ParameterName returns the Parameter name for a given argument position. This will be the return -// from the `(Parameter).GetName` function or a default value. An error diagnostic is raised if the -// position is outside the expected arguments. -func (d Definition) ParameterName(ctx context.Context, position int) (string, diag.Diagnostics) { - parameter, diags := d.Parameter(ctx, position) - if diags.HasError() { - return "", diags - } - - definedName := parameter.GetName() - if definedName != "" { - return definedName, nil - } - - if position >= len(d.Parameters) { - return DefaultVariadicParameterName, nil - } - - return fmt.Sprintf("%s%d", DefaultParameterNamePrefix, position+1), nil -} - // ValidateImplementation contains logic for validating the provider-defined // implementation of the definition to prevent unexpected errors or panics. This // logic runs during the GetProviderSchema RPC, or via provider-defined unit @@ -130,9 +109,12 @@ func (d Definition) ValidateImplementation(ctx context.Context) diag.Diagnostics } paramNames := make(map[string]int, len(d.Parameters)) - for pos := range d.Parameters { - // TODO: what should we do with the diags? This should never happen - name, _ := d.ParameterName(ctx, pos) + for pos, param := range d.Parameters { + name := param.GetName() + // If name is not set, default the param name based on position: "param1", "param2", etc. + if name == "" { + name = fmt.Sprintf("%s%d", DefaultParameterNamePrefix, pos+1) + } conflictPos, exists := paramNames[name] if exists { @@ -150,8 +132,11 @@ func (d Definition) ValidateImplementation(ctx context.Context) diag.Diagnostics } if d.VariadicParameter != nil { - // TODO: what should we do with the diags? This should never happen - name, _ := d.ParameterName(ctx, len(d.Parameters)+1) + name := d.VariadicParameter.GetName() + // If name is not set, default the variadic param name + if name == "" { + name = DefaultVariadicParameterName + } conflictPos, exists := paramNames[name] if exists { diff --git a/function/definition_test.go b/function/definition_test.go index cafb52fe6..8c1a9387d 100644 --- a/function/definition_test.go +++ b/function/definition_test.go @@ -145,143 +145,6 @@ func TestDefinitionParameter(t *testing.T) { } } -func TestDefinitionParameterName(t *testing.T) { - t.Parallel() - - testCases := map[string]struct { - definition function.Definition - position int - expected string - expectedDiagnostics diag.Diagnostics - }{ - "none": { - definition: function.Definition{ - // no Parameters or VariadicParameter - }, - position: 0, - expected: "", - expectedDiagnostics: diag.Diagnostics{ - diag.NewErrorDiagnostic( - "Invalid Parameter Position for Definition", - "When determining the parameter for the given argument position, an invalid value was given. "+ - "This is always an issue in the provider code and should be reported to the provider developers.\n\n"+ - "Function does not implement parameters.\n"+ - "Given position: 0", - ), - }, - }, - "parameters-default-first": { - definition: function.Definition{ - Parameters: []function.Parameter{ - function.BoolParameter{}, - function.Int64Parameter{}, - function.StringParameter{}, - }, - }, - position: 0, - expected: "param1", - }, - "parameters-default-last": { - definition: function.Definition{ - Parameters: []function.Parameter{ - function.BoolParameter{}, - function.Int64Parameter{}, - function.StringParameter{}, - }, - }, - position: 2, - expected: "param3", - }, - "parameters-default-middle": { - definition: function.Definition{ - Parameters: []function.Parameter{ - function.BoolParameter{}, - function.Int64Parameter{}, - function.StringParameter{}, - }, - }, - position: 1, - expected: "param2", - }, - "parameters-defined": { - definition: function.Definition{ - Parameters: []function.Parameter{ - function.BoolParameter{ - Name: "myspecialparam", - }, - }, - }, - position: 0, - expected: "myspecialparam", - }, - "parameters-over": { - definition: function.Definition{ - Parameters: []function.Parameter{ - function.BoolParameter{}, - }, - }, - position: 1, - expected: "", - expectedDiagnostics: diag.Diagnostics{ - diag.NewErrorDiagnostic( - "Invalid Parameter Position for Definition", - "When determining the parameter for the given argument position, an invalid value was given. "+ - "This is always an issue in the provider code and should be reported to the provider developers.\n\n"+ - "Max argument position: 0\n"+ - "Given position: 1", - ), - }, - }, - "variadicparameter-and-parameters-select-parameter": { - definition: function.Definition{ - Parameters: []function.Parameter{ - function.BoolParameter{}, - }, - VariadicParameter: function.StringParameter{}, - }, - position: 0, - expected: "param1", - }, - "variadicparameter-and-parameters-select-variadicparameter-default": { - definition: function.Definition{ - Parameters: []function.Parameter{ - function.BoolParameter{}, - }, - VariadicParameter: function.StringParameter{}, - }, - position: 1, - expected: function.DefaultVariadicParameterName, - }, - "variadicparameter-defined": { - definition: function.Definition{ - VariadicParameter: function.StringParameter{ - Name: "myspecialvariadic", - }, - }, - position: 0, - expected: "myspecialvariadic", - }, - } - - for name, testCase := range testCases { - name, testCase := name, testCase - - t.Run(name, func(t *testing.T) { - t.Parallel() - - got, diags := testCase.definition.ParameterName(context.Background(), testCase.position) - - if diff := cmp.Diff(got, testCase.expected); diff != "" { - t.Errorf("unexpected difference: %s", diff) - } - - if diff := cmp.Diff(diags, testCase.expectedDiagnostics); diff != "" { - t.Errorf("unexpected diagnostics difference: %s", diff) - } - }) - } -} - func TestDefinitionValidateImplementation(t *testing.T) { t.Parallel() diff --git a/function/parameter.go b/function/parameter.go index dc396423f..14a9fbb5d 100644 --- a/function/parameter.go +++ b/function/parameter.go @@ -37,6 +37,10 @@ type Parameter interface { // GetName should return a usage name for the parameter. Parameters are // positional, so this name has no meaning except documentation. + // + // If the name is returned as an empty string, a default name will be used to prevent Terraform errors for missing names. + // The default name will be the prefix "param" with a suffix of the position the parameter is in the function definition. (`param1`, `param2`, etc.) + // If the parameter is variadic, the default name will be `varparam`. GetName() string // GetType should return the data type for the parameter, which determines diff --git a/internal/toproto5/function.go b/internal/toproto5/function.go index e414a0bd5..7b4cfc071 100644 --- a/internal/toproto5/function.go +++ b/internal/toproto5/function.go @@ -5,6 +5,7 @@ package toproto5 import ( "context" + "fmt" "github.com/hashicorp/terraform-plugin-go/tfprotov5" @@ -30,11 +31,25 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov5.Function { } for i, fwParameter := range fw.Parameters { - proto.Parameters = append(proto.Parameters, FunctionParameter(ctx, fw, fwParameter, i)) + protoParam := FunctionParameter(ctx, fwParameter) + + // If name is not set, default the param name based on position: "param1", "param2", etc. + if protoParam.Name == "" { + protoParam.Name = fmt.Sprintf("%s%d", function.DefaultParameterNamePrefix, i+1) + } + + proto.Parameters = append(proto.Parameters, protoParam) } if fw.VariadicParameter != nil { - proto.VariadicParameter = FunctionParameter(ctx, fw, fw.VariadicParameter, len(fw.Parameters)+1) + protoParam := FunctionParameter(ctx, fw.VariadicParameter) + + // If name is not set, default the variadic param name + if protoParam.Name == "" { + protoParam.Name = function.DefaultVariadicParameterName + } + + proto.VariadicParameter = protoParam } return proto @@ -42,26 +57,23 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov5.Function { // FunctionParameter returns the *tfprotov5.FunctionParameter for a // function.Parameter. -func FunctionParameter(ctx context.Context, def function.Definition, param function.Parameter, position int) *tfprotov5.FunctionParameter { - if param == nil { +func FunctionParameter(ctx context.Context, fw function.Parameter) *tfprotov5.FunctionParameter { + if fw == nil { return nil } - // TODO: what should we do with the diags? This should never happen - name, _ := def.ParameterName(ctx, position) - proto := &tfprotov5.FunctionParameter{ - AllowNullValue: param.GetAllowNullValue(), - AllowUnknownValues: param.GetAllowUnknownValues(), - Name: name, - Type: param.GetType().TerraformType(ctx), + AllowNullValue: fw.GetAllowNullValue(), + AllowUnknownValues: fw.GetAllowUnknownValues(), + Name: fw.GetName(), + Type: fw.GetType().TerraformType(ctx), } - if param.GetMarkdownDescription() != "" { - proto.Description = param.GetMarkdownDescription() + if fw.GetMarkdownDescription() != "" { + proto.Description = fw.GetMarkdownDescription() proto.DescriptionKind = tfprotov5.StringKindMarkdown - } else if param.GetDescription() != "" { - proto.Description = param.GetDescription() + } else if fw.GetDescription() != "" { + proto.Description = fw.GetDescription() proto.DescriptionKind = tfprotov5.StringKindPlain } diff --git a/internal/toproto5/function_test.go b/internal/toproto5/function_test.go index a0e1a69d3..65dfbd0ae 100644 --- a/internal/toproto5/function_test.go +++ b/internal/toproto5/function_test.go @@ -69,24 +69,30 @@ func TestFunction(t *testing.T) { "parameters": { fw: function.Definition{ Parameters: []function.Parameter{ - function.BoolParameter{}, - function.Int64Parameter{}, - function.StringParameter{}, + function.BoolParameter{ + Name: "bool", + }, + function.Int64Parameter{ + Name: "int64", + }, + function.StringParameter{ + Name: "string", + }, }, Return: function.StringReturn{}, }, expected: &tfprotov5.Function{ Parameters: []*tfprotov5.FunctionParameter{ { - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, { - Name: "param2", + Name: "int64", Type: tftypes.Number, }, { - Name: "param3", + Name: "string", Type: tftypes.String, }, }, @@ -96,6 +102,48 @@ func TestFunction(t *testing.T) { }, }, "parameters-with-variadic": { + fw: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{ + Name: "bool", + }, + function.Int64Parameter{ + Name: "int64", + }, + function.StringParameter{ + Name: "string", + }, + }, + VariadicParameter: function.Float64Parameter{ + Name: "variadic_float64", + }, + Return: function.StringReturn{}, + }, + expected: &tfprotov5.Function{ + Parameters: []*tfprotov5.FunctionParameter{ + { + Name: "bool", + Type: tftypes.Bool, + }, + { + Name: "int64", + Type: tftypes.Number, + }, + { + Name: "string", + Type: tftypes.String, + }, + }, + VariadicParameter: &tfprotov5.FunctionParameter{ + Name: "variadic_float64", + Type: tftypes.Number, + }, + Return: &tfprotov5.FunctionReturn{ + Type: tftypes.String, + }, + }, + }, + "parameters-defaults": { fw: function.Definition{ Parameters: []function.Parameter{ function.BoolParameter{}, @@ -231,43 +279,47 @@ func TestFunctionParameter(t *testing.T) { }, "allownullvalue": { fw: function.BoolParameter{ + Name: "bool", AllowNullValue: true, }, expected: &tfprotov5.FunctionParameter{ AllowNullValue: true, - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, }, "allowunknownvalues": { fw: function.BoolParameter{ + Name: "bool", AllowUnknownValues: true, }, expected: &tfprotov5.FunctionParameter{ AllowUnknownValues: true, - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, }, "description": { fw: function.BoolParameter{ + Name: "bool", Description: "test description", }, expected: &tfprotov5.FunctionParameter{ Description: "test description", DescriptionKind: tfprotov5.StringKindPlain, - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, }, "description-markdown": { fw: function.BoolParameter{ + Name: "bool", MarkdownDescription: "test description", }, expected: &tfprotov5.FunctionParameter{ Description: "test description", DescriptionKind: tfprotov5.StringKindMarkdown, - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, }, @@ -280,33 +332,47 @@ func TestFunctionParameter(t *testing.T) { Type: tftypes.Bool, }, }, - "type-bool": { + "name-empty": { fw: function.BoolParameter{}, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "", // default is applied by the toproto5.Function method + Type: tftypes.Bool, + }, + }, + "type-bool": { + fw: function.BoolParameter{ + Name: "bool", + }, + expected: &tfprotov5.FunctionParameter{ + Name: "bool", Type: tftypes.Bool, }, }, "type-float64": { - fw: function.Float64Parameter{}, + fw: function.Float64Parameter{ + Name: "float64", + }, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "float64", Type: tftypes.Number, }, }, "type-int64": { - fw: function.Int64Parameter{}, + fw: function.Int64Parameter{ + Name: "int64", + }, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "int64", Type: tftypes.Number, }, }, "type-list": { fw: function.ListParameter{ + Name: "list", ElementType: basetypes.StringType{}, }, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "list", Type: tftypes.List{ ElementType: tftypes.String, }, @@ -314,24 +380,28 @@ func TestFunctionParameter(t *testing.T) { }, "type-map": { fw: function.MapParameter{ + Name: "map", ElementType: basetypes.StringType{}, }, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "map", Type: tftypes.Map{ ElementType: tftypes.String, }, }, }, "type-number": { - fw: function.NumberParameter{}, + fw: function.NumberParameter{ + Name: "number", + }, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "number", Type: tftypes.Number, }, }, "type-object": { fw: function.ObjectParameter{ + Name: "object", AttributeTypes: map[string]attr.Type{ "bool": basetypes.BoolType{}, "int64": basetypes.Int64Type{}, @@ -339,7 +409,7 @@ func TestFunctionParameter(t *testing.T) { }, }, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "object", Type: tftypes.Object{ AttributeTypes: map[string]tftypes.Type{ "bool": tftypes.Bool, @@ -351,19 +421,22 @@ func TestFunctionParameter(t *testing.T) { }, "type-set": { fw: function.SetParameter{ + Name: "set", ElementType: basetypes.StringType{}, }, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "set", Type: tftypes.Set{ ElementType: tftypes.String, }, }, }, "type-string": { - fw: function.StringParameter{}, + fw: function.StringParameter{ + Name: "string", + }, expected: &tfprotov5.FunctionParameter{ - Name: "param1", + Name: "string", Type: tftypes.String, }, }, @@ -375,10 +448,7 @@ func TestFunctionParameter(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - def := function.Definition{ - Parameters: []function.Parameter{testCase.fw}, - } - got := toproto5.FunctionParameter(context.Background(), def, testCase.fw, 0) + got := toproto5.FunctionParameter(context.Background(), testCase.fw) if diff := cmp.Diff(got, testCase.expected); diff != "" { t.Errorf("unexpected difference: %s", diff) diff --git a/internal/toproto6/function.go b/internal/toproto6/function.go index e54450515..90925be10 100644 --- a/internal/toproto6/function.go +++ b/internal/toproto6/function.go @@ -5,6 +5,7 @@ package toproto6 import ( "context" + "fmt" "github.com/hashicorp/terraform-plugin-go/tfprotov6" @@ -30,11 +31,25 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov6.Function { } for i, fwParameter := range fw.Parameters { - proto.Parameters = append(proto.Parameters, FunctionParameter(ctx, fw, fwParameter, i)) + protoParam := FunctionParameter(ctx, fwParameter) + + // If name is not set, default the param name based on position: "param1", "param2", etc. + if protoParam.Name == "" { + protoParam.Name = fmt.Sprintf("%s%d", function.DefaultParameterNamePrefix, i+1) + } + + proto.Parameters = append(proto.Parameters, protoParam) } if fw.VariadicParameter != nil { - proto.VariadicParameter = FunctionParameter(ctx, fw, fw.VariadicParameter, len(fw.Parameters)+1) + protoParam := FunctionParameter(ctx, fw.VariadicParameter) + + // If name is not set, default the variadic param name + if protoParam.Name == "" { + protoParam.Name = function.DefaultVariadicParameterName + } + + proto.VariadicParameter = protoParam } return proto @@ -42,26 +57,23 @@ func Function(ctx context.Context, fw function.Definition) *tfprotov6.Function { // FunctionParameter returns the *tfprotov6.FunctionParameter for a // function.Parameter. -func FunctionParameter(ctx context.Context, def function.Definition, param function.Parameter, position int) *tfprotov6.FunctionParameter { - if param == nil { +func FunctionParameter(ctx context.Context, fw function.Parameter) *tfprotov6.FunctionParameter { + if fw == nil { return nil } - // TODO: what should we do with the diags? This should never happen - name, _ := def.ParameterName(ctx, position) - proto := &tfprotov6.FunctionParameter{ - AllowNullValue: param.GetAllowNullValue(), - AllowUnknownValues: param.GetAllowUnknownValues(), - Name: name, - Type: param.GetType().TerraformType(ctx), + AllowNullValue: fw.GetAllowNullValue(), + AllowUnknownValues: fw.GetAllowUnknownValues(), + Name: fw.GetName(), + Type: fw.GetType().TerraformType(ctx), } - if param.GetMarkdownDescription() != "" { - proto.Description = param.GetMarkdownDescription() + if fw.GetMarkdownDescription() != "" { + proto.Description = fw.GetMarkdownDescription() proto.DescriptionKind = tfprotov6.StringKindMarkdown - } else if param.GetDescription() != "" { - proto.Description = param.GetDescription() + } else if fw.GetDescription() != "" { + proto.Description = fw.GetDescription() proto.DescriptionKind = tfprotov6.StringKindPlain } diff --git a/internal/toproto6/function_test.go b/internal/toproto6/function_test.go index dd8d0ed24..00f914c58 100644 --- a/internal/toproto6/function_test.go +++ b/internal/toproto6/function_test.go @@ -69,24 +69,30 @@ func TestFunction(t *testing.T) { "parameters": { fw: function.Definition{ Parameters: []function.Parameter{ - function.BoolParameter{}, - function.Int64Parameter{}, - function.StringParameter{}, + function.BoolParameter{ + Name: "bool", + }, + function.Int64Parameter{ + Name: "int64", + }, + function.StringParameter{ + Name: "string", + }, }, Return: function.StringReturn{}, }, expected: &tfprotov6.Function{ Parameters: []*tfprotov6.FunctionParameter{ { - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, { - Name: "param2", + Name: "int64", Type: tftypes.Number, }, { - Name: "param3", + Name: "string", Type: tftypes.String, }, }, @@ -96,6 +102,48 @@ func TestFunction(t *testing.T) { }, }, "parameters-with-variadic": { + fw: function.Definition{ + Parameters: []function.Parameter{ + function.BoolParameter{ + Name: "bool", + }, + function.Int64Parameter{ + Name: "int64", + }, + function.StringParameter{ + Name: "string", + }, + }, + VariadicParameter: function.Float64Parameter{ + Name: "variadic_float64", + }, + Return: function.StringReturn{}, + }, + expected: &tfprotov6.Function{ + Parameters: []*tfprotov6.FunctionParameter{ + { + Name: "bool", + Type: tftypes.Bool, + }, + { + Name: "int64", + Type: tftypes.Number, + }, + { + Name: "string", + Type: tftypes.String, + }, + }, + VariadicParameter: &tfprotov6.FunctionParameter{ + Name: "variadic_float64", + Type: tftypes.Number, + }, + Return: &tfprotov6.FunctionReturn{ + Type: tftypes.String, + }, + }, + }, + "parameters-defaults": { fw: function.Definition{ Parameters: []function.Parameter{ function.BoolParameter{}, @@ -231,43 +279,47 @@ func TestFunctionParameter(t *testing.T) { }, "allownullvalue": { fw: function.BoolParameter{ + Name: "bool", AllowNullValue: true, }, expected: &tfprotov6.FunctionParameter{ AllowNullValue: true, - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, }, - "allowunknownvalue": { + "allowunknownvalues": { fw: function.BoolParameter{ + Name: "bool", AllowUnknownValues: true, }, expected: &tfprotov6.FunctionParameter{ AllowUnknownValues: true, - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, }, "description": { fw: function.BoolParameter{ + Name: "bool", Description: "test description", }, expected: &tfprotov6.FunctionParameter{ Description: "test description", DescriptionKind: tfprotov6.StringKindPlain, - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, }, "description-markdown": { fw: function.BoolParameter{ + Name: "bool", MarkdownDescription: "test description", }, expected: &tfprotov6.FunctionParameter{ Description: "test description", DescriptionKind: tfprotov6.StringKindMarkdown, - Name: "param1", + Name: "bool", Type: tftypes.Bool, }, }, @@ -280,33 +332,47 @@ func TestFunctionParameter(t *testing.T) { Type: tftypes.Bool, }, }, - "type-bool": { + "name-empty": { fw: function.BoolParameter{}, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "", // default is applied by the toproto6.Function method + Type: tftypes.Bool, + }, + }, + "type-bool": { + fw: function.BoolParameter{ + Name: "bool", + }, + expected: &tfprotov6.FunctionParameter{ + Name: "bool", Type: tftypes.Bool, }, }, "type-float64": { - fw: function.Float64Parameter{}, + fw: function.Float64Parameter{ + Name: "float64", + }, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "float64", Type: tftypes.Number, }, }, "type-int64": { - fw: function.Int64Parameter{}, + fw: function.Int64Parameter{ + Name: "int64", + }, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "int64", Type: tftypes.Number, }, }, "type-list": { fw: function.ListParameter{ + Name: "list", ElementType: basetypes.StringType{}, }, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "list", Type: tftypes.List{ ElementType: tftypes.String, }, @@ -314,24 +380,28 @@ func TestFunctionParameter(t *testing.T) { }, "type-map": { fw: function.MapParameter{ + Name: "map", ElementType: basetypes.StringType{}, }, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "map", Type: tftypes.Map{ ElementType: tftypes.String, }, }, }, "type-number": { - fw: function.NumberParameter{}, + fw: function.NumberParameter{ + Name: "number", + }, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "number", Type: tftypes.Number, }, }, "type-object": { fw: function.ObjectParameter{ + Name: "object", AttributeTypes: map[string]attr.Type{ "bool": basetypes.BoolType{}, "int64": basetypes.Int64Type{}, @@ -339,7 +409,7 @@ func TestFunctionParameter(t *testing.T) { }, }, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "object", Type: tftypes.Object{ AttributeTypes: map[string]tftypes.Type{ "bool": tftypes.Bool, @@ -351,19 +421,22 @@ func TestFunctionParameter(t *testing.T) { }, "type-set": { fw: function.SetParameter{ + Name: "set", ElementType: basetypes.StringType{}, }, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "set", Type: tftypes.Set{ ElementType: tftypes.String, }, }, }, "type-string": { - fw: function.StringParameter{}, + fw: function.StringParameter{ + Name: "string", + }, expected: &tfprotov6.FunctionParameter{ - Name: "param1", + Name: "string", Type: tftypes.String, }, }, @@ -375,10 +448,7 @@ func TestFunctionParameter(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - def := function.Definition{ - Parameters: []function.Parameter{testCase.fw}, - } - got := toproto6.FunctionParameter(context.Background(), def, testCase.fw, 0) + got := toproto6.FunctionParameter(context.Background(), testCase.fw) if diff := cmp.Diff(got, testCase.expected); diff != "" { t.Errorf("unexpected difference: %s", diff) From cc2f7d6b5e5429fad2eadef777e2412a130d2d44 Mon Sep 17 00:00:00 2001 From: Austin Valle Date: Thu, 29 Feb 2024 09:48:13 -0500 Subject: [PATCH 6/6] Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Benjamin Bennett --- website/docs/plugin/framework/functions/documentation.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/plugin/framework/functions/documentation.mdx b/website/docs/plugin/framework/functions/documentation.mdx index 4d58cdcd4..91b1c293c 100644 --- a/website/docs/plugin/framework/functions/documentation.mdx +++ b/website/docs/plugin/framework/functions/documentation.mdx @@ -47,7 +47,7 @@ Each [parameter type](/terraform/plugin/framework/functions/parameters), whether | Field Name | Description | |---|---| -| `Name` | Single word or abbreviation of parameter for function signature generation. If name is not provided, will default to the prefix "param" with a suffix of the position the parameter is in the function definition. (`param1`, `param2`, etc.). If the parameter is variadic, the default name will be `varparam`. | +| `Name` | Single word or abbreviation of parameter for function signature generation. If name is not provided, will default to the prefix "param" with a suffix of the position the parameter is in the function definition (e.g., `param1`, `param2`). If the parameter is variadic, the default name will be `varparam`. | | `Description` | Documentation about the parameter and its expected values in plaintext format. | | `MarkdownDescription` | Documentation about the parameter and its expected values in Markdown format. |