From d7fbec9c90327c91afed08941ed7dd7c24d05941 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Apr 2025 12:56:53 -0500 Subject: [PATCH 1/4] validate input vales --- provider/parameter.go | 126 +++++++++++++++++++++++++++--------------- 1 file changed, 82 insertions(+), 44 deletions(-) diff --git a/provider/parameter.go b/provider/parameter.go index 2f7dc66..05cd57d 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -21,6 +21,10 @@ import ( "golang.org/x/xerrors" ) +var ( + defaultValuePath = cty.Path{cty.GetAttrStep{Name: "default"}} +) + type Option struct { Name string Description string @@ -124,7 +128,7 @@ func parameterDataSource() *schema.Resource { } var value string if parameter.Default != "" { - err := valueIsType(parameter.Type, parameter.Default, cty.Path{cty.GetAttrStep{Name: "default"}}) + err := valueIsType(parameter.Type, parameter.Default, defaultValuePath) if err != nil { return err } @@ -144,34 +148,22 @@ func parameterDataSource() *schema.Resource { return diag.Errorf("ephemeral parameter requires the default property") } - // TODO: Should we move this into the Valid() function on - // Parameter? - if len(parameter.Validation) == 1 { - validation := ¶meter.Validation[0] - err = validation.Valid(parameter.Type, value) - if err != nil { - return diag.FromErr(err) - } + // Do ValidateFormType up front. If there is no error, update the + // 'parameter.FormType' value to the new value. This is to handle default cases, + // since the default logic is more advanced than the sdk provider schema + // supports. + _, newFT, err := ValidateFormType(parameter.Type, len(parameter.Option), parameter.FormType) + if err == nil { + // If there is an error, parameter.Valid will catch it. + parameter.FormType = newFT + + // Set the form_type back in case the value was changed. + // Eg via a default. If a user does not specify, a default value + // is used and saved. + rd.Set("form_type", parameter.FormType) } - // Validate options - _, parameter.FormType, err = ValidateFormType(parameter.Type, len(parameter.Option), parameter.FormType) - if err != nil { - return diag.Diagnostics{ - { - Severity: diag.Error, - Summary: "Invalid form_type for parameter", - Detail: err.Error(), - AttributePath: cty.Path{cty.GetAttrStep{Name: "form_type"}}, - }, - } - } - // Set the form_type back in case the value was changed. - // Eg via a default. If a user does not specify, a default value - // is used and saved. - rd.Set("form_type", parameter.FormType) - - diags := parameter.Valid() + diags := parameter.Valid(value) if diags.HasError() { return diags } @@ -414,10 +406,13 @@ func valueIsType(typ OptionType, value string, attrPath cty.Path) diag.Diagnosti return nil } -func (v *Parameter) Valid() diag.Diagnostics { +func (v *Parameter) Valid(value string) diag.Diagnostics { + var err error + var optionType OptionType + // optionType might differ from parameter.Type. This is ok, and parameter.Type // should be used for the value type, and optionType for options. - optionType, _, err := ValidateFormType(v.Type, len(v.Option), v.FormType) + optionType, v.FormType, err = ValidateFormType(v.Type, len(v.Option), v.FormType) if err != nil { return diag.Diagnostics{ { @@ -458,24 +453,52 @@ func (v *Parameter) Valid() diag.Diagnostics { } optionValues[option.Value] = nil optionNames[option.Name] = nil + + // TODO: Option values should also be validated. + // v.validValue(option.Value, optionType, nil, cty.Path{}) + } + } + + // Validate the default value + if v.Default != "" { + d := v.validValue(v.Default, optionType, optionValues, defaultValuePath) + if d.HasError() { + return d } } - if v.Default != "" && len(v.Option) > 0 { + // Value must always be validated + d := v.validValue(value, optionType, optionValues, cty.Path{}) + if d.HasError() { + return d + } + + return nil +} + +func (v *Parameter) validValue(value string, optionType OptionType, optionValues map[string]any, path cty.Path) diag.Diagnostics { + // name is used for constructing more precise error messages. + name := "Value" + if path.Equals(defaultValuePath) { + name = "Default value" + } + + // First validate if the value is a valid option + if len(optionValues) > 0 { if v.Type == OptionTypeListString && optionType == OptionTypeString { // If the type is list(string) and optionType is string, we have // to ensure all elements of the default exist as options. - defaultValues, diags := valueIsListString(v.Default, cty.Path{cty.GetAttrStep{Name: "default"}}) + listValues, diags := valueIsListString(value, defaultValuePath) if diags.HasError() { return diags } // missing is used to construct a more helpful error message var missing []string - for _, defaultValue := range defaultValues { - _, defaultIsValid := optionValues[defaultValue] - if !defaultIsValid { - missing = append(missing, defaultValue) + for _, listValue := range listValues { + _, isValid := optionValues[listValue] + if !isValid { + missing = append(missing, listValue) } } @@ -483,30 +506,45 @@ func (v *Parameter) Valid() diag.Diagnostics { return diag.Diagnostics{ { Severity: diag.Error, - Summary: "Default values must be a valid option", + Summary: fmt.Sprintf("%ss must be a valid option", name), Detail: fmt.Sprintf( - "default value %q is not a valid option, values %q are missing from the options", - v.Default, strings.Join(missing, ", "), + "%s %q is not a valid option, values %q are missing from the options", + name, value, strings.Join(missing, ", "), ), - AttributePath: cty.Path{cty.GetAttrStep{Name: "default"}}, + AttributePath: defaultValuePath, }, } } } else { - _, defaultIsValid := optionValues[v.Default] - if !defaultIsValid { + _, isValid := optionValues[value] + if !isValid { return diag.Diagnostics{ { Severity: diag.Error, - Summary: "Default value must be a valid option", - Detail: fmt.Sprintf("the value %q must be defined as one of options", v.Default), - AttributePath: cty.Path{cty.GetAttrStep{Name: "default"}}, + Summary: fmt.Sprintf("%s must be a valid option", name), + Detail: fmt.Sprintf("the value %q must be defined as one of options", value), + AttributePath: path, }, } } } } + if len(v.Validation) == 1 { + validCheck := &v.Validation[0] + err := validCheck.Valid(v.Type, value) + if err != nil { + return diag.Diagnostics{ + { + Severity: diag.Error, + Summary: fmt.Sprintf("Invalid parameter %s according to 'validation' block", strings.ToLower(name)), + Detail: err.Error(), + AttributePath: path, + }, + } + } + } + return nil } From c640b02427d377cac92c21053a249a9419e6a982 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Apr 2025 16:35:09 -0500 Subject: [PATCH 2/4] begin adding invalid unit tests --- provider/parameter.go | 85 ++++++++++++++++---------- provider/parameter_test.go | 122 +++++++++++++++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 33 deletions(-) diff --git a/provider/parameter.go b/provider/parameter.go index 05cd57d..f28524e 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -50,7 +50,6 @@ const ( ) type Parameter struct { - Value string Name string DisplayName string `mapstructure:"display_name"` Description string @@ -86,7 +85,6 @@ func parameterDataSource() *schema.Resource { var parameter Parameter err = mapstructure.Decode(struct { - Value interface{} Name interface{} DisplayName interface{} Description interface{} @@ -101,7 +99,6 @@ func parameterDataSource() *schema.Resource { Order interface{} Ephemeral interface{} }{ - Value: rd.Get("value"), Name: rd.Get("name"), DisplayName: rd.Get("display_name"), Description: rd.Get("description"), @@ -126,14 +123,7 @@ func parameterDataSource() *schema.Resource { if err != nil { return diag.Errorf("decode parameter: %s", err) } - var value string - if parameter.Default != "" { - err := valueIsType(parameter.Type, parameter.Default, defaultValuePath) - if err != nil { - return err - } - value = parameter.Default - } + value := parameter.Default envValue, ok := os.LookupEnv(ParameterEnvironmentVariable(parameter.Name)) if ok { value = envValue @@ -381,27 +371,27 @@ func fixValidationResourceData(rawConfig cty.Value, validation interface{}) (int return vArr, nil } -func valueIsType(typ OptionType, value string, attrPath cty.Path) diag.Diagnostics { +func valueIsType(typ OptionType, value string) error { switch typ { case OptionTypeNumber: _, err := strconv.ParseFloat(value, 64) if err != nil { - return diag.Errorf("%q is not a number", value) + return fmt.Errorf("%q is not a number", value) } case OptionTypeBoolean: _, err := strconv.ParseBool(value) if err != nil { - return diag.Errorf("%q is not a bool", value) + return fmt.Errorf("%q is not a bool", value) } case OptionTypeListString: - _, diags := valueIsListString(value, attrPath) - if diags.HasError() { - return diags + _, err := valueIsListString(value) + if err != nil { + return err } case OptionTypeString: // Anything is a string! default: - return diag.Errorf("invalid type %q", typ) + return fmt.Errorf("invalid type %q", typ) } return nil } @@ -447,9 +437,15 @@ func (v *Parameter) Valid(value string) diag.Diagnostics { }, } } - diags := valueIsType(optionType, option.Value, cty.Path{}) - if diags.HasError() { - return diags + err = valueIsType(optionType, option.Value) + if err != nil { + return diag.Diagnostics{ + { + Severity: diag.Error, + Summary: fmt.Sprintf("Option %q with value=%q is not of type %q", option.Name, option.Value, optionType), + Detail: err.Error(), + }, + } } optionValues[option.Value] = nil optionNames[option.Name] = nil @@ -461,6 +457,18 @@ func (v *Parameter) Valid(value string) diag.Diagnostics { // Validate the default value if v.Default != "" { + err := valueIsType(v.Type, v.Default) + if err != nil { + return diag.Diagnostics{ + { + Severity: diag.Error, + Summary: fmt.Sprintf("Default value is not of type %q", v.Type), + Detail: err.Error(), + AttributePath: defaultValuePath, + }, + } + } + d := v.validValue(v.Default, optionType, optionValues, defaultValuePath) if d.HasError() { return d @@ -473,6 +481,17 @@ func (v *Parameter) Valid(value string) diag.Diagnostics { return d } + err = valueIsType(v.Type, value) + if err != nil { + return diag.Diagnostics{ + { + Severity: diag.Error, + Summary: fmt.Sprintf("Parameter value is not of type %q", v.Type), + Detail: err.Error(), + }, + } + } + return nil } @@ -488,9 +507,16 @@ func (v *Parameter) validValue(value string, optionType OptionType, optionValues if v.Type == OptionTypeListString && optionType == OptionTypeString { // If the type is list(string) and optionType is string, we have // to ensure all elements of the default exist as options. - listValues, diags := valueIsListString(value, defaultValuePath) - if diags.HasError() { - return diags + listValues, err := valueIsListString(value) + if err != nil { + return diag.Diagnostics{ + { + Severity: diag.Error, + Summary: "When using list(string) type, value must be a json encoded list of strings", + Detail: err.Error(), + AttributePath: defaultValuePath, + }, + } } // missing is used to construct a more helpful error message @@ -608,18 +634,11 @@ func (v *Validation) Valid(typ OptionType, value string) error { return nil } -func valueIsListString(value string, path cty.Path) ([]string, diag.Diagnostics) { +func valueIsListString(value string) ([]string, error) { var items []string err := json.Unmarshal([]byte(value), &items) if err != nil { - return nil, diag.Diagnostics{ - { - Severity: diag.Error, - Summary: "When using list(string) type, value must be a json encoded list of strings", - Detail: fmt.Sprintf("value %q is not a valid list of strings", value), - AttributePath: path, - }, - } + return nil, fmt.Errorf("value %q is not a valid list of strings", value) } return items, nil } diff --git a/provider/parameter_test.go b/provider/parameter_test.go index 4b52d94..302d45e 100644 --- a/provider/parameter_test.go +++ b/provider/parameter_test.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/coder/terraform-provider-coder/v2/provider" @@ -686,6 +687,108 @@ data "coder_parameter" "region" { } } +func TestParameterValidation(t *testing.T) { + t.Parallel() + opts := func(vals ...string) []provider.Option { + options := make([]provider.Option, 0, len(vals)) + for _, val := range vals { + options = append(options, provider.Option{ + Name: val, + Value: val, + }) + } + return options + } + + for _, tc := range []struct { + Name string + Parameter provider.Parameter + Value string + ExpectError *regexp.Regexp + }{ + { + Name: "ValidStringParameter", + Parameter: provider.Parameter{ + Type: "string", + }, + Value: "alpha", + }, + // Test invalid states + { + Name: "InvalidFormType", + Parameter: provider.Parameter{ + Type: "string", + Option: opts("alpha", "bravo", "charlie"), + FormType: provider.ParameterFormTypeSlider, + }, + Value: "alpha", + ExpectError: regexp.MustCompile("Invalid form_type for parameter"), + }, + { + Name: "NotInOptions", + Parameter: provider.Parameter{ + Type: "string", + Option: opts("alpha", "bravo", "charlie"), + }, + Value: "delta", // not in option set + ExpectError: regexp.MustCompile("Value must be a valid option"), + }, + { + Name: "NonUniqueOptionNames", + Parameter: provider.Parameter{ + Type: "string", + Option: opts("alpha", "alpha"), + }, + Value: "alpha", + ExpectError: regexp.MustCompile("Option names must be unique"), + }, + { + Name: "NonUniqueOptionValues", + Parameter: provider.Parameter{ + Type: "string", + Option: []provider.Option{ + {Name: "Alpha", Value: "alpha"}, + {Name: "AlphaAgain", Value: "alpha"}, + }, + }, + Value: "alpha", + ExpectError: regexp.MustCompile("Option values must be unique"), + }, + { + Name: "IncorrectValueTypeOption", + Parameter: provider.Parameter{ + Type: "number", + Option: opts("not-a-number"), + }, + Value: "5", + ExpectError: regexp.MustCompile("is not a number"), + }, + { + Name: "IncorrectValueType", + Parameter: provider.Parameter{ + Type: "number", + }, + Value: "not-a-number", + ExpectError: regexp.MustCompile("Parameter value is not of type \"number\""), + }, + } { + tc := tc + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + diags := tc.Parameter.Valid(tc.Value) + if tc.ExpectError != nil { + require.True(t, diags.HasError()) + errMsg := fmt.Sprintf("%+v", diags[0]) // close enough + require.Truef(t, tc.ExpectError.MatchString(errMsg), "got: %s", errMsg) + } else { + if !assert.False(t, diags.HasError()) { + t.Logf("got: %+v", diags[0]) + } + } + }) + } +} + func TestValueValidatesType(t *testing.T) { t.Parallel() for _, tc := range []struct { @@ -798,6 +901,25 @@ func TestValueValidatesType(t *testing.T) { Value: `[]`, MinDisabled: true, MaxDisabled: true, + }, { + Name: "ValidListOfStrings", + Type: "list(string)", + Value: `["first","second","third"]`, + MinDisabled: true, + MaxDisabled: true, + }, { + Name: "InvalidListOfStrings", + Type: "list(string)", + Value: `["first","second","third"`, + MinDisabled: true, + MaxDisabled: true, + Error: regexp.MustCompile("is not valid list of strings"), + }, { + Name: "EmptyListOfStrings", + Type: "list(string)", + Value: `[]`, + MinDisabled: true, + MaxDisabled: true, }} { tc := tc t.Run(tc.Name, func(t *testing.T) { From e6a529d271fcb91cebf904f571849e33af2b20d0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Apr 2025 16:57:39 -0500 Subject: [PATCH 3/4] add more validation tests --- provider/parameter_test.go | 58 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/provider/parameter_test.go b/provider/parameter_test.go index 302d45e..7231e35 100644 --- a/provider/parameter_test.go +++ b/provider/parameter_test.go @@ -733,6 +733,15 @@ func TestParameterValidation(t *testing.T) { Value: "delta", // not in option set ExpectError: regexp.MustCompile("Value must be a valid option"), }, + { + Name: "NumberNotInOptions", + Parameter: provider.Parameter{ + Type: "number", + Option: opts("1", "2", "3"), + }, + Value: "0", // not in option set + ExpectError: regexp.MustCompile("Value must be a valid option"), + }, { Name: "NonUniqueOptionNames", Parameter: provider.Parameter{ @@ -771,6 +780,55 @@ func TestParameterValidation(t *testing.T) { Value: "not-a-number", ExpectError: regexp.MustCompile("Parameter value is not of type \"number\""), }, + { + Name: "NotListStringDefault", + Parameter: provider.Parameter{ + Type: "list(string)", + Default: "not-a-list", + }, + ExpectError: regexp.MustCompile("not a valid list of strings"), + }, + { + Name: "NotListStringDefault", + Parameter: provider.Parameter{ + Type: "list(string)", + }, + Value: "not-a-list", + ExpectError: regexp.MustCompile("not a valid list of strings"), + }, + { + Name: "DefaultListStringNotInOptions", + Parameter: provider.Parameter{ + Type: "list(string)", + Default: `["red", "yellow", "black"]`, + Option: opts("red", "blue", "green"), + FormType: provider.ParameterFormTypeMultiSelect, + }, + ExpectError: regexp.MustCompile("is not a valid option, values \"yellow, black\" are missing from the options"), + }, + { + Name: "ListStringNotInOptions", + Parameter: provider.Parameter{ + Type: "list(string)", + Default: `["red"]`, + Option: opts("red", "blue", "green"), + FormType: provider.ParameterFormTypeMultiSelect, + }, + Value: `["red", "yellow", "black"]`, + ExpectError: regexp.MustCompile("is not a valid option, values \"yellow, black\" are missing from the options"), + }, + { + Name: "InvalidMiniumum", + Parameter: provider.Parameter{ + Type: "number", + Default: "5", + Validation: []provider.Validation{{ + Min: 10, + Error: "must be greater than 10", + }}, + }, + ExpectError: regexp.MustCompile("must be greater than 10"), + }, } { tc := tc t.Run(tc.Name, func(t *testing.T) { From a61664c59309d296dd480de19ebccf5995c3e8af Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 15 Apr 2025 10:05:35 -0500 Subject: [PATCH 4/4] add better error message --- provider/parameter.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/provider/parameter.go b/provider/parameter.go index f28524e..03ca419 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -544,10 +544,14 @@ func (v *Parameter) validValue(value string, optionType OptionType, optionValues } else { _, isValid := optionValues[value] if !isValid { + extra := "" + if value == "" { + extra = ". The value is empty, did you forget to set it with a default or from user input?" + } return diag.Diagnostics{ { Severity: diag.Error, - Summary: fmt.Sprintf("%s must be a valid option", name), + Summary: fmt.Sprintf("%s must be a valid option%s", name, extra), Detail: fmt.Sprintf("the value %q must be defined as one of options", value), AttributePath: path, },