function: Missing underlying type validation and refactor parameter n…

…ame validation (#991) Reference: #965 Reference: #967 The framework will now raise implementation errors if a function parameter or return definition is missing underlying type information (e.g. collection element type or object attribute type). Only framework types are considered in the initial implementation.
hashicorp · Apr 24, 2024 · a150eef · a150eef
1 parent cc4a6c7
commit a150eef
Show file tree

Hide file tree

Showing 34 changed files with 3,660 additions and 86 deletions.
diff --git a/.changes/unreleased/ENHANCEMENTS-20240423-165354.yaml b/.changes/unreleased/ENHANCEMENTS-20240423-165354.yaml
@@ -0,0 +1,6 @@
+kind: ENHANCEMENTS
+body: 'function: Introduced implementation errors for collection and object parameters
+  and returns which are missing type information'
+time: 2024-04-23T16:53:54.509459-04:00
+custom:
+  Issue: "991"
diff --git a/function/bool_parameter.go b/function/bool_parameter.go
@@ -4,13 +4,17 @@
 package function
 
 import (
+	"context"
+
 	"github.com/hashicorp/terraform-plugin-framework/attr"
+	"github.com/hashicorp/terraform-plugin-framework/internal/fwfunction"
 	"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
 )
 
 // Ensure the implementation satisifies the desired interfaces.
 var _ Parameter = BoolParameter{}
 var _ ParameterWithBoolValidators = BoolParameter{}
+var _ fwfunction.ParameterWithValidateImplementation = BoolParameter{}
 
 // BoolParameter represents a function parameter that is a boolean.
 //
@@ -115,3 +119,9 @@ func (p BoolParameter) GetType() attr.Type {
 
 	return basetypes.BoolType{}
 }
+
+func (p BoolParameter) ValidateImplementation(ctx context.Context, req fwfunction.ValidateParameterImplementationRequest, resp *fwfunction.ValidateParameterImplementationResponse) {
+	if p.GetName() == "" {
+		resp.Diagnostics.Append(fwfunction.MissingParameterNameDiag(req.FunctionName, req.ParameterPosition))
+	}
+}
diff --git a/function/bool_parameter_test.go b/function/bool_parameter_test.go
@@ -4,12 +4,15 @@
 package function_test
 
 import (
+	"context"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 
 	"github.com/hashicorp/terraform-plugin-framework/attr"
+	"github.com/hashicorp/terraform-plugin-framework/diag"
 	"github.com/hashicorp/terraform-plugin-framework/function"
+	"github.com/hashicorp/terraform-plugin-framework/internal/fwfunction"
 	"github.com/hashicorp/terraform-plugin-framework/internal/testing/testtypes"
 	"github.com/hashicorp/terraform-plugin-framework/internal/testing/testvalidator"
 	"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
@@ -284,3 +287,58 @@ func TestBoolParameterBoolValidators(t *testing.T) {
 		})
 	}
 }
+
+func TestBoolParameterValidateImplementation(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		param    function.BoolParameter
+		request  fwfunction.ValidateParameterImplementationRequest
+		expected *fwfunction.ValidateParameterImplementationResponse
+	}{
+		"name": {
+			param: function.BoolParameter{
+				Name: "testparam",
+			},
+			request: fwfunction.ValidateParameterImplementationRequest{
+				FunctionName:      "testfunc",
+				ParameterPosition: pointer(int64(0)),
+			},
+			expected: &fwfunction.ValidateParameterImplementationResponse{},
+		},
+		"name-missing": {
+			param: function.BoolParameter{
+				// Name intentionally missing
+			},
+			request: fwfunction.ValidateParameterImplementationRequest{
+				FunctionName:      "testfunc",
+				ParameterPosition: pointer(int64(0)),
+			},
+			expected: &fwfunction.ValidateParameterImplementationResponse{
+				Diagnostics: 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"+
+							"Function \"testfunc\" - Parameter at position 0 does not have a name",
+					),
+				},
+			},
+		},
+	}
+
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
+
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			got := &fwfunction.ValidateParameterImplementationResponse{}
+			testCase.param.ValidateImplementation(context.Background(), testCase.request, got)
+
+			if diff := cmp.Diff(got, testCase.expected); diff != "" {
+				t.Errorf("unexpected difference: %s", diff)
+			}
+		})
+	}
+}
diff --git a/function/definition.go b/function/definition.go
@@ -81,20 +81,10 @@ func (d Definition) ValidateImplementation(ctx context.Context, req DefinitionVa
 	paramNames := make(map[string]int, len(d.Parameters))
 	for pos, param := range d.Parameters {
 		parameterPosition := int64(pos)
-		name := param.GetName()
-		// If name is not set, add an error diagnostic, parameter names are mandatory.
-		if name == "" {
-			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"+
-					fmt.Sprintf("Function %q - Parameter at position %d does not have a name", req.FuncName, pos),
-			)
-		}
 
 		if paramWithValidateImplementation, ok := param.(fwfunction.ParameterWithValidateImplementation); ok {
 			req := fwfunction.ValidateParameterImplementationRequest{
-				Name:              name,
+				FunctionName:      req.FuncName,
 				ParameterPosition: &parameterPosition,
 			}
 			resp := &fwfunction.ValidateParameterImplementationResponse{}
@@ -104,7 +94,9 @@ func (d Definition) ValidateImplementation(ctx context.Context, req DefinitionVa
 			diags.Append(resp.Diagnostics...)
 		}
 
+		name := param.GetName()
 		conflictPos, exists := paramNames[name]
+
 		if exists && name != "" {
 			diags.AddError(
 				"Invalid Function Definition",
@@ -120,20 +112,9 @@ func (d Definition) ValidateImplementation(ctx context.Context, req DefinitionVa
 	}
 
 	if d.VariadicParameter != nil {
-		name := d.VariadicParameter.GetName()
-		// If name is not set, add an error diagnostic, parameter names are mandatory.
-		if name == "" {
-			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"+
-					fmt.Sprintf("Function %q - The variadic parameter does not have a name", req.FuncName),
-			)
-		}
-
 		if paramWithValidateImplementation, ok := d.VariadicParameter.(fwfunction.ParameterWithValidateImplementation); ok {
 			req := fwfunction.ValidateParameterImplementationRequest{
-				Name: name,
+				FunctionName: req.FuncName,
 			}
 			resp := &fwfunction.ValidateParameterImplementationResponse{}
 
@@ -142,7 +123,9 @@ func (d Definition) ValidateImplementation(ctx context.Context, req DefinitionVa
 			diags.Append(resp.Diagnostics...)
 		}
 
+		name := d.VariadicParameter.GetName()
 		conflictPos, exists := paramNames[name]
+
 		if exists && name != "" {
 			diags.AddError(
 				"Invalid Function Definition",

diff --git a/function/dynamic_parameter.go b/function/dynamic_parameter.go
@@ -4,13 +4,17 @@
 package function
 
 import (
+	"context"
+
 	"github.com/hashicorp/terraform-plugin-framework/attr"
+	"github.com/hashicorp/terraform-plugin-framework/internal/fwfunction"
 	"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
 )
 
 // Ensure the implementation satisifies the desired interfaces.
 var _ Parameter = DynamicParameter{}
 var _ ParameterWithDynamicValidators = DynamicParameter{}
+var _ fwfunction.ParameterWithValidateImplementation = DynamicParameter{}
 
 // DynamicParameter represents a function parameter that is a dynamic, rather
 // than a static type. Static types are always preferable over dynamic
@@ -110,3 +114,9 @@ func (p DynamicParameter) GetType() attr.Type {
 
 	return basetypes.DynamicType{}
 }
+
+func (p DynamicParameter) ValidateImplementation(ctx context.Context, req fwfunction.ValidateParameterImplementationRequest, resp *fwfunction.ValidateParameterImplementationResponse) {
+	if p.GetName() == "" {
+		resp.Diagnostics.Append(fwfunction.MissingParameterNameDiag(req.FunctionName, req.ParameterPosition))
+	}
+}
diff --git a/function/dynamic_parameter_test.go b/function/dynamic_parameter_test.go
@@ -4,12 +4,15 @@
 package function_test
 
 import (
+	"context"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 
 	"github.com/hashicorp/terraform-plugin-framework/attr"
+	"github.com/hashicorp/terraform-plugin-framework/diag"
 	"github.com/hashicorp/terraform-plugin-framework/function"
+	"github.com/hashicorp/terraform-plugin-framework/internal/fwfunction"
 	"github.com/hashicorp/terraform-plugin-framework/internal/testing/testtypes"
 	"github.com/hashicorp/terraform-plugin-framework/internal/testing/testvalidator"
 	"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
@@ -284,3 +287,58 @@ func TestDynamicParameterDynamicValidators(t *testing.T) {
 		})
 	}
 }
+
+func TestDynamicParameterValidateImplementation(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		param    function.DynamicParameter
+		request  fwfunction.ValidateParameterImplementationRequest
+		expected *fwfunction.ValidateParameterImplementationResponse
+	}{
+		"name": {
+			param: function.DynamicParameter{
+				Name: "testparam",
+			},
+			request: fwfunction.ValidateParameterImplementationRequest{
+				FunctionName:      "testfunc",
+				ParameterPosition: pointer(int64(0)),
+			},
+			expected: &fwfunction.ValidateParameterImplementationResponse{},
+		},
+		"name-missing": {
+			param: function.DynamicParameter{
+				// Name intentionally missing
+			},
+			request: fwfunction.ValidateParameterImplementationRequest{
+				FunctionName:      "testfunc",
+				ParameterPosition: pointer(int64(0)),
+			},
+			expected: &fwfunction.ValidateParameterImplementationResponse{
+				Diagnostics: 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"+
+							"Function \"testfunc\" - Parameter at position 0 does not have a name",
+					),
+				},
+			},
+		},
+	}
+
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
+
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			got := &fwfunction.ValidateParameterImplementationResponse{}
+			testCase.param.ValidateImplementation(context.Background(), testCase.request, got)
+
+			if diff := cmp.Diff(got, testCase.expected); diff != "" {
+				t.Errorf("unexpected difference: %s", diff)
+			}
+		})
+	}
+}
diff --git a/function/float64_parameter.go b/function/float64_parameter.go
@@ -4,13 +4,17 @@
 package function
 
 import (
+	"context"
+
 	"github.com/hashicorp/terraform-plugin-framework/attr"
+	"github.com/hashicorp/terraform-plugin-framework/internal/fwfunction"
 	"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
 )
 
 // Ensure the implementation satisifies the desired interfaces.
 var _ Parameter = Float64Parameter{}
 var _ ParameterWithFloat64Validators = Float64Parameter{}
+var _ fwfunction.ParameterWithValidateImplementation = Float64Parameter{}
 
 // Float64Parameter represents a function parameter that is a 64-bit floating
 // point number.
@@ -112,3 +116,9 @@ func (p Float64Parameter) GetType() attr.Type {
 
 	return basetypes.Float64Type{}
 }
+
+func (p Float64Parameter) ValidateImplementation(ctx context.Context, req fwfunction.ValidateParameterImplementationRequest, resp *fwfunction.ValidateParameterImplementationResponse) {
+	if p.GetName() == "" {
+		resp.Diagnostics.Append(fwfunction.MissingParameterNameDiag(req.FunctionName, req.ParameterPosition))
+	}
+}
diff --git a/function/float64_parameter_test.go b/function/float64_parameter_test.go
@@ -4,12 +4,15 @@
 package function_test
 
 import (
+	"context"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
 
 	"github.com/hashicorp/terraform-plugin-framework/attr"
+	"github.com/hashicorp/terraform-plugin-framework/diag"
 	"github.com/hashicorp/terraform-plugin-framework/function"
+	"github.com/hashicorp/terraform-plugin-framework/internal/fwfunction"
 	"github.com/hashicorp/terraform-plugin-framework/internal/testing/testtypes"
 	"github.com/hashicorp/terraform-plugin-framework/internal/testing/testvalidator"
 	"github.com/hashicorp/terraform-plugin-framework/types/basetypes"
@@ -284,3 +287,58 @@ func TestFloat64ParameterFloat64Validators(t *testing.T) {
 		})
 	}
 }
+
+func TestFloat64ParameterValidateImplementation(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		param    function.Float64Parameter
+		request  fwfunction.ValidateParameterImplementationRequest
+		expected *fwfunction.ValidateParameterImplementationResponse
+	}{
+		"name": {
+			param: function.Float64Parameter{
+				Name: "testparam",
+			},
+			request: fwfunction.ValidateParameterImplementationRequest{
+				FunctionName:      "testfunc",
+				ParameterPosition: pointer(int64(0)),
+			},
+			expected: &fwfunction.ValidateParameterImplementationResponse{},
+		},
+		"name-missing": {
+			param: function.Float64Parameter{
+				// Name intentionally missing
+			},
+			request: fwfunction.ValidateParameterImplementationRequest{
+				FunctionName:      "testfunc",
+				ParameterPosition: pointer(int64(0)),
+			},
+			expected: &fwfunction.ValidateParameterImplementationResponse{
+				Diagnostics: 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"+
+							"Function \"testfunc\" - Parameter at position 0 does not have a name",
+					),
+				},
+			},
+		},
+	}
+
+	for name, testCase := range testCases {
+		name, testCase := name, testCase
+
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			got := &fwfunction.ValidateParameterImplementationResponse{}
+			testCase.param.ValidateImplementation(context.Background(), testCase.request, got)
+
+			if diff := cmp.Diff(got, testCase.expected); diff != "" {
+				t.Errorf("unexpected difference: %s", diff)
+			}
+		})
+	}
+}