function: Add validation for parameter name conflicts and update defa…

…ulting logic (#936) * add validation and refactor defaulting logic * add changelogs * test fix * Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Brian Flad <[email protected]> * refactor the logic, tests and docs for defaulting * Update website/docs/plugin/framework/functions/documentation.mdx Co-authored-by: Benjamin Bennett <[email protected]> --------- Co-authored-by: Brian Flad <[email protected]> Co-authored-by: Benjamin Bennett <[email protected]>
hashicorp · Feb 29, 2024 · f03ca33 · f03ca33
1 parent 87c1b41
commit f03ca33
Show file tree

Hide file tree

Showing 32 changed files with 624 additions and 201 deletions.
diff --git a/.changes/unreleased/BUG FIXES-20240228-151959.yaml 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
@@ -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"
diff --git 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
@@ -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
@@ -108,6 +108,48 @@ func (d Definition) ValidateImplementation(ctx context.Context) diag.Diagnostics
 		)
 	}
 
+	paramNames := make(map[string]int, len(d.Parameters))
+	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 {
+			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 {
+		name := d.VariadicParameter.GetName()
+		// If name is not set, default the variadic param name
+		if name == "" {
+			name = DefaultVariadicParameterName
+		}
+
+		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
@@ -152,11 +152,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 +192,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
@@ -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
@@ -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
@@ -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
@@ -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{