Skip to content

chore: enhance parameter validation error messages #379

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 14, 2025
Merged

chore: enhance parameter validation error messages #379

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8920f69
chore: implement validation in an exported function
Emyrk Apr 11, 2025
84f0a89
chore: enhance parameter validation error messages
Emyrk Apr 11, 2025
8342a18
fix error assertions
Emyrk Apr 11, 2025
c7e7b4e
remove singleDiag helper
Emyrk Apr 14, 2025
f353685
PR comments, mostly typos
Emyrk Apr 14, 2025
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
28 changes: 25 additions & 3 deletions provider/formtype.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,11 @@ var formTypeTruthTable = map[OptionType]map[bool][]ParameterFormType{
// The use case is when using multi-select. The options are 'string' and the
// value is 'list(string)'.
func ValidateFormType(paramType OptionType, optionCount int, specifiedFormType ParameterFormType) (OptionType, ParameterFormType, error) {
allowed, ok := formTypeTruthTable[paramType][optionCount > 0]
optionsExist := optionCount > 0
allowed, ok := formTypeTruthTable[paramType][optionsExist]
if !ok || len(allowed) == 0 {
return paramType, specifiedFormType, xerrors.Errorf("value type %q is not supported for 'form_types'", paramType)
// This error should really never be hit, as the provider sdk does an enum validation.
return paramType, specifiedFormType, xerrors.Errorf("\"type\" attribute=%q is not supported, choose one of %v", paramType, OptionTypes())
}

if specifiedFormType == ParameterFormTypeDefault {
Expand All @@ -126,7 +128,27 @@ func ValidateFormType(paramType OptionType, optionCount int, specifiedFormType P
}

if !slices.Contains(allowed, specifiedFormType) {
return paramType, specifiedFormType, xerrors.Errorf("value type %q is not supported for 'form_types'", specifiedFormType)
optionMsg := ""
opposite := formTypeTruthTable[paramType][!optionsExist]

// This extra message tells a user if they are using a valid form_type
// for a 'type', but it is invalid because options do/do-not exist.
// It serves as a more helpful error message.
//
// Eg: form_type=slider is valid for type=number, but invalid if options exist.
// And this error message is more accurate than just saying "form_type=slider is
// not valid for type=number".
if slices.Contains(opposite, specifiedFormType) {
if optionsExist {
optionMsg = " when options exist"
} else {
optionMsg = " when options do no exist"
}
}
return paramType, specifiedFormType,
xerrors.Errorf("\"form_type\" attribute=%q is not supported for \"type\"=%q%s, choose one of %v",
specifiedFormType, paramType,
optionMsg, toStrings(allowed))
}

// This is the only current special case. If 'multi-select' is selected, the type
Expand Down
188 changes: 123 additions & 65 deletions provider/parameter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func parameterDataSource() *schema.Resource {
}
var value string
if parameter.Default != "" {
err := valueIsType(parameter.Type, parameter.Default)
err := valueIsType(parameter.Type, parameter.Default, cty.Path{cty.GetAttrStep{Name: "default"}})
if err != nil {
return err
}
Expand All @@ -144,6 +144,8 @@ 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 := &parameter.Validation[0]
err = validation.Valid(parameter.Type, value)
Expand All @@ -153,73 +155,25 @@ func parameterDataSource() *schema.Resource {
}

// Validate options

// optionType might differ from parameter.Type. This is ok, and parameter.Type
// should be used for the value type, and optionType for options.
var optionType OptionType
optionType, parameter.FormType, err = ValidateFormType(parameter.Type, len(parameter.Option), parameter.FormType)
_, parameter.FormType, err = ValidateFormType(parameter.Type, len(parameter.Option), parameter.FormType)
if err != nil {
return diag.FromErr(err)
return singleDiag(diag.Diagnostic{
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)

if len(parameter.Option) > 0 {
names := map[string]interface{}{}
values := map[string]interface{}{}
for _, option := range parameter.Option {
_, exists := names[option.Name]
if exists {
return diag.Errorf("multiple options cannot have the same name %q", option.Name)
}
_, exists = values[option.Value]
if exists {
return diag.Errorf("multiple options cannot have the same value %q", option.Value)
}
err := valueIsType(optionType, option.Value)
if err != nil {
return err
}
values[option.Value] = nil
names[option.Name] = nil
}

if parameter.Default != "" {
if parameter.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.
var defaultValues []string
// TODO: We do this unmarshal in a few spots. It should be standardized.
err = json.Unmarshal([]byte(parameter.Default), &defaultValues)
if err != nil {
return diag.Errorf("default value %q is not a list of strings", parameter.Default)
}

// missing is used to construct a more helpful error message
var missing []string
for _, defaultValue := range defaultValues {
_, defaultIsValid := values[defaultValue]
if !defaultIsValid {
missing = append(missing, defaultValue)
}
}

if len(missing) > 0 {
return diag.Errorf(
"default value %q is not a valid option, values %q are missing from the option",
parameter.Default, strings.Join(missing, ", "),
)
}
} else {
_, defaultIsValid := values[parameter.Default]
if !defaultIsValid {
return diag.Errorf("%q default value %q must be defined as one of options", parameter.FormType, parameter.Default)
}
}
}
diags := parameter.Valid()
if diags.HasError() {
return diags
}

return nil
},
Schema: map[string]*schema.Schema{
Expand Down Expand Up @@ -433,7 +387,7 @@ func fixValidationResourceData(rawConfig cty.Value, validation interface{}) (int
return vArr, nil
}

func valueIsType(typ OptionType, value string) diag.Diagnostics {
func valueIsType(typ OptionType, value string, attrPath cty.Path) diag.Diagnostics {
switch typ {
case OptionTypeNumber:
_, err := strconv.ParseFloat(value, 64)
Expand All @@ -446,10 +400,9 @@ func valueIsType(typ OptionType, value string) diag.Diagnostics {
return diag.Errorf("%q is not a bool", value)
}
case OptionTypeListString:
var items []string
err := json.Unmarshal([]byte(value), &items)
if err != nil {
return diag.Errorf("%q is not an array of strings", value)
_, diags := valueIsListString(value, attrPath)
if diags.HasError() {
return diags
}
case OptionTypeString:
// Anything is a string!
Expand All @@ -459,6 +412,93 @@ func valueIsType(typ OptionType, value string) diag.Diagnostics {
return nil
}

func (v *Parameter) Valid() diag.Diagnostics {
// 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)
if err != nil {
return singleDiag(diag.Diagnostic{
Severity: diag.Error,
Summary: "Invalid form_type for parameter",
Detail: err.Error(),
AttributePath: cty.Path{cty.GetAttrStep{Name: "form_type"}},
})
}

optionNames := map[string]interface{}{}
optionValues := map[string]interface{}{}
if len(v.Option) > 0 {
for _, option := range v.Option {
_, exists := optionNames[option.Name]
if exists {
return singleDiag(diag.Diagnostic{
Severity: diag.Error,
Summary: "Option names must be unique.",
Detail: fmt.Sprintf("multiple options found with the same name %q", option.Name),
})
}
_, exists = optionValues[option.Value]
if exists {
return singleDiag(diag.Diagnostic{
Severity: diag.Error,
Summary: "Option values must be unique.",
Detail: fmt.Sprintf("multiple options found with the same value %q", option.Value),
})
}
diags := valueIsType(optionType, option.Value, cty.Path{})
if diags.HasError() {
return diags
}
optionValues[option.Value] = nil
optionNames[option.Name] = nil
}
}

if v.Default != "" && len(v.Option) > 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"}})
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)
}
}

if len(missing) > 0 {
return singleDiag(diag.Diagnostic{
Severity: diag.Error,
Summary: "Default values must be a valid option",
Detail: fmt.Sprintf(
"default value %q is not a valid option, values %q are missing from the options",
v.Default, strings.Join(missing, ", "),
),
AttributePath: cty.Path{cty.GetAttrStep{Name: "default"}},
})
}
} else {
_, defaultIsValid := optionValues[v.Default]
if !defaultIsValid {
return singleDiag(diag.Diagnostic{
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"}},
})
}
}
}

return nil
}

func (v *Validation) Valid(typ OptionType, value string) error {
if typ != OptionTypeNumber {
if !v.MinDisabled {
Expand Down Expand Up @@ -519,6 +559,20 @@ func (v *Validation) Valid(typ OptionType, value string) error {
return nil
}

func valueIsListString(value string, path cty.Path) ([]string, diag.Diagnostics) {
var items []string
err := json.Unmarshal([]byte(value), &items)
if err != nil {
return nil, singleDiag(diag.Diagnostic{
Severity: diag.Error,
Summary: fmt.Sprintf("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 items, nil
}

// ParameterEnvironmentVariable returns the environment variable to specify for
// a parameter by it's name. It's hashed because spaces and special characters
// can be used in parameter names that may not be valid in env vars.
Expand Down Expand Up @@ -546,3 +600,7 @@ func (v *Validation) errorRendered(value string) error {
"{value}", value)
return xerrors.Errorf(r.Replace(v.Error))
}

func singleDiag(diagnostic diag.Diagnostic) diag.Diagnostics {
return diag.Diagnostics{diagnostic}
}
4 changes: 2 additions & 2 deletions provider/parameter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ func TestParameter(t *testing.T) {
}
}
`,
ExpectError: regexp.MustCompile("cannot have the same name"),
ExpectError: regexp.MustCompile("Option names must be unique"),
}, {
Name: "DuplicateOptionValue",
Config: `
Expand All @@ -308,7 +308,7 @@ func TestParameter(t *testing.T) {
}
}
`,
ExpectError: regexp.MustCompile("cannot have the same value"),
ExpectError: regexp.MustCompile("Option values must be unique"),
}, {
Name: "RequiredParameterNoDefault",
Config: `
Expand Down
Loading