Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require provider-defined function parameter naming #964

Merged
merged 13 commits into from
Mar 21, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Switching to request-response pattern for function definition validation
bendbennett committed Mar 20, 2024
commit 11433b97e4a99d6bf959f079c78ceecd21c22fd9
34 changes: 26 additions & 8 deletions function/definition.go
Original file line number Diff line number Diff line change
@@ -89,22 +89,22 @@ func (d Definition) Parameter(ctx context.Context, position int) (Parameter, dia
// implementation of the definition to prevent unexpected errors or panics. This
// logic runs during the GetProviderSchema RPC, or via provider-defined unit
// testing, and should never include false positives.
func (d Definition) ValidateImplementation(ctx context.Context, funcName string) diag.Diagnostics {
func (d Definition) ValidateImplementation(ctx context.Context, req DefinitionValidateRequest, resp *DefinitionValidateResponse) {
var diags diag.Diagnostics

if d.Return == nil {
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 - Definition Return field is undefined", funcName),
fmt.Sprintf("Function %q - Definition Return field is undefined", req.FuncName),
)
} else if d.Return.GetType() == nil {
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 - Definition return data type is undefined", funcName),
fmt.Sprintf("Function %q - Definition return data type is undefined", req.FuncName),
)
}

@@ -117,7 +117,7 @@ func (d Definition) ValidateImplementation(ctx context.Context, funcName string)
"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", funcName, pos),
fmt.Sprintf("Function %q - Parameter at position %d does not have a name", req.FuncName, pos),
)
}

@@ -128,7 +128,7 @@ func (d Definition) ValidateImplementation(ctx context.Context, funcName string)
"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("Function %q - Parameters at position %d and %d have the same name %q", funcName, conflictPos, pos, name),
fmt.Sprintf("Function %q - Parameters at position %d and %d have the same name %q", req.FuncName, conflictPos, pos, name),
)
continue
}
@@ -144,7 +144,7 @@ func (d Definition) ValidateImplementation(ctx context.Context, funcName string)
"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", funcName),
fmt.Sprintf("Function %q - The variadic parameter does not have a name", req.FuncName),
)
}

@@ -155,12 +155,12 @@ func (d Definition) ValidateImplementation(ctx context.Context, funcName string)
"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("Function %q - Parameter at position %d and the variadic parameter have the same name %q", funcName, conflictPos, name),
fmt.Sprintf("Function %q - Parameter at position %d and the variadic parameter have the same name %q", req.FuncName, conflictPos, name),
)
}
}

return diags
resp.Diagnostics.Append(diags...)
}

// DefinitionRequest represents a request for the Function to return its
@@ -180,3 +180,21 @@ type DefinitionResponse struct {
// An empty slice indicates success, with no warnings or errors generated.
Diagnostics diag.Diagnostics
}

// DefinitionValidateRequest represents a request for the Function to validate its
// definition. An instance of this request struct is supplied as an argument to
// the Definition type ValidateImplementation method.
type DefinitionValidateRequest struct {
// FuncName is the name of the function definition being validated.
FuncName string
}

// DefinitionValidateResponse represents a response to a DefinitionValidateRequest.
// An instance of this response struct is supplied as an argument to the Definition
// type ValidateImplementation method.
type DefinitionValidateResponse struct {
// Diagnostics report errors or warnings related to validation of a function
// definition. An empty slice indicates success, with no warnings or errors
// generated.
Diagnostics diag.Diagnostics
}
177 changes: 97 additions & 80 deletions function/definition_test.go
Original file line number Diff line number Diff line change
@@ -151,25 +151,28 @@ func TestDefinitionValidateImplementation(t *testing.T) {

testCases := map[string]struct {
definition function.Definition
expected diag.Diagnostics
expected function.DefinitionValidateResponse
}{
"valid-no-params": {
definition: function.Definition{
Return: function.StringReturn{},
},
expected: function.DefinitionValidateResponse{},
},
"missing-variadic-param-name": {
definition: function.Definition{
VariadicParameter: function.StringParameter{},
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"+
"Function \"test-function\" - The variadic parameter does not have a name",
),
expected: function.DefinitionValidateResponse{
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 \"test-function\" - The variadic parameter does not have a name",
),
},
},
},
"missing-param-names": {
@@ -180,19 +183,21 @@ func TestDefinitionValidateImplementation(t *testing.T) {
},
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"+
"Function \"test-function\" - Parameter at position 0 does not have a name",
),
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 \"test-function\" - Parameter at position 1 does not have a name",
),
expected: function.DefinitionValidateResponse{
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 \"test-function\" - Parameter at position 0 does not have a name",
),
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 \"test-function\" - Parameter at position 1 does not have a name",
),
},
},
},
"missing-param-names-with-variadic": {
@@ -203,30 +208,34 @@ func TestDefinitionValidateImplementation(t *testing.T) {
VariadicParameter: function.NumberParameter{},
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"+
"Function \"test-function\" - Parameter at position 0 does not have a name",
),
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 \"test-function\" - The variadic parameter does not have a name",
),
expected: function.DefinitionValidateResponse{
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 \"test-function\" - Parameter at position 0 does not have a name",
),
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 \"test-function\" - The variadic parameter does not have a name",
),
},
},
},
"result-missing": {
definition: function.Definition{},
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"+
"Function \"test-function\" - Definition Return field is undefined",
),
expected: function.DefinitionValidateResponse{
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 \"test-function\" - Definition Return field is undefined",
),
},
},
},
"conflicting-param-names": {
@@ -250,14 +259,16 @@ func TestDefinitionValidateImplementation(t *testing.T) {
},
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. "+
"Function \"test-function\" - Parameters at position 2 and 4 have the same name \"param-dup\"",
),
expected: function.DefinitionValidateResponse{
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"+
"Parameter names must be unique. "+
"Function \"test-function\" - Parameters at position 2 and 4 have the same name \"param-dup\"",
),
},
},
},
"conflicting-param-names-variadic": {
@@ -278,14 +289,16 @@ func TestDefinitionValidateImplementation(t *testing.T) {
},
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. "+
"Function \"test-function\" - Parameter at position 1 and the variadic parameter have the same name \"param-dup\"",
),
expected: function.DefinitionValidateResponse{
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"+
"Parameter names must be unique. "+
"Function \"test-function\" - Parameter at position 1 and the variadic parameter have the same name \"param-dup\"",
),
},
},
},
"conflicting-param-names-variadic-multiple": {
@@ -312,28 +325,30 @@ func TestDefinitionValidateImplementation(t *testing.T) {
},
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. "+
"Function \"test-function\" - 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. "+
"Function \"test-function\" - 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. "+
"Function \"test-function\" - Parameter at position 0 and the variadic parameter have the same name \"param-dup\"",
),
expected: function.DefinitionValidateResponse{
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"+
"Parameter names must be unique. "+
"Function \"test-function\" - 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. "+
"Function \"test-function\" - 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. "+
"Function \"test-function\" - Parameter at position 0 and the variadic parameter have the same name \"param-dup\"",
),
},
},
},
}
@@ -344,7 +359,9 @@ func TestDefinitionValidateImplementation(t *testing.T) {
t.Run(name, func(t *testing.T) {
t.Parallel()

got := testCase.definition.ValidateImplementation(context.Background(), "test-function")
got := function.DefinitionValidateResponse{}

testCase.definition.ValidateImplementation(context.Background(), function.DefinitionValidateRequest{FuncName: "test-function"}, &got)

if diff := cmp.Diff(got, testCase.expected); diff != "" {
t.Errorf("unexpected difference: %s", diff)
12 changes: 9 additions & 3 deletions internal/fwserver/server_functions.go
Original file line number Diff line number Diff line change
@@ -98,11 +98,17 @@ func (s *Server) FunctionDefinitions(ctx context.Context) (map[string]function.D
continue
}

validateDiags := definitionResp.Definition.ValidateImplementation(ctx, name)
validateReq := function.DefinitionValidateRequest{
FuncName: name,
}

validateResp := function.DefinitionValidateResponse{}

definitionResp.Definition.ValidateImplementation(ctx, validateReq, &validateResp)

diags.Append(validateDiags...)
diags.Append(validateResp.Diagnostics...)

if validateDiags.HasError() {
if validateResp.Diagnostics.HasError() {
continue
}