From 8920f69f8e4371f2fbf0b3a65f77dc4ee3bfa242 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Apr 2025 12:29:53 -0500 Subject: [PATCH 1/5] chore: implement validation in an exported function Allows dynamic params to reuse the same logic --- provider/parameter.go | 133 ++++++++++++++++++++++++------------------ 1 file changed, 75 insertions(+), 58 deletions(-) diff --git a/provider/parameter.go b/provider/parameter.go index 4eb8a28..396f23e 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -153,11 +153,7 @@ 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) } @@ -166,60 +162,11 @@ func parameterDataSource() *schema.Resource { // 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{ @@ -459,6 +406,76 @@ 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, formType, err := ValidateFormType(v.Type, len(v.Option), v.FormType) + if err != nil { + return diag.FromErr(err) + } + + if formType != v.FormType { + return diag.FromErr(fmt.Errorf("form_type %q is not valid for type %q", v.FormType, v.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 diag.FromErr(fmt.Errorf("multiple options cannot have the same name %q", option.Name)) + } + _, exists = optionValues[option.Value] + if exists { + return diag.FromErr(fmt.Errorf("multiple options cannot have the same value %q", option.Value)) + } + diags := valueIsType(optionType, option.Value) + if diags.HasError() { + return diags + } + optionValues[option.Value] = nil + optionNames[option.Name] = nil + } + } + + if v.Default != "" { + 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. + var defaultValues []string + // TODO: We do this unmarshal in a few spots. It should be standardized. + err = json.Unmarshal([]byte(v.Default), &defaultValues) + if err != nil { + return diag.FromErr(fmt.Errorf("default value %q is not a list of strings", v.Default)) + } + + // 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 diag.FromErr(fmt.Errorf( + "default value %q is not a valid option, values %q are missing from the option", + v.Default, strings.Join(missing, ", "), + )) + } + } else { + _, defaultIsValid := optionValues[v.Default] + if !defaultIsValid { + return diag.FromErr(fmt.Errorf("%q default value %q must be defined as one of options", v.FormType, v.Default)) + } + } + } + + return nil +} + func (v *Validation) Valid(typ OptionType, value string) error { if typ != OptionTypeNumber { if !v.MinDisabled { From 84f0a89b27b2ea0a4d01ed93aec3d09ff3ba1d1f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Apr 2025 13:30:58 -0500 Subject: [PATCH 2/5] chore: enhance parameter validation error messages Validation code is exported for external use --- provider/formtype.go | 28 +++++++++++-- provider/parameter.go | 95 +++++++++++++++++++++++++++++++------------ 2 files changed, 93 insertions(+), 30 deletions(-) diff --git a/provider/formtype.go b/provider/formtype.go index 753f894..474b629 100644 --- a/provider/formtype.go +++ b/provider/formtype.go @@ -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 { @@ -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 diff --git a/provider/parameter.go b/provider/parameter.go index 396f23e..c64eb2a 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -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 } @@ -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 := ¶meter.Validation[0] err = validation.Valid(parameter.Type, value) @@ -155,7 +157,12 @@ func parameterDataSource() *schema.Resource { // Validate options _, 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 @@ -380,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) @@ -393,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! @@ -409,13 +415,14 @@ func valueIsType(typ OptionType, value string) diag.Diagnostics { 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, formType, err := ValidateFormType(v.Type, len(v.Option), v.FormType) + optionType, _, err := ValidateFormType(v.Type, len(v.Option), v.FormType) if err != nil { - return diag.FromErr(err) - } - - if formType != v.FormType { - return diag.FromErr(fmt.Errorf("form_type %q is not valid for type %q", v.FormType, v.Type)) + 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{}{} @@ -424,13 +431,21 @@ func (v *Parameter) Valid() diag.Diagnostics { for _, option := range v.Option { _, exists := optionNames[option.Name] if exists { - return diag.FromErr(fmt.Errorf("multiple options cannot have the same name %q", option.Name)) + 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 diag.FromErr(fmt.Errorf("multiple options cannot have the same value %q", option.Value)) + 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) + diags := valueIsType(optionType, option.Value, cty.Path{}) if diags.HasError() { return diags } @@ -439,15 +454,13 @@ func (v *Parameter) Valid() diag.Diagnostics { } } - if v.Default != "" { + 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. - var defaultValues []string - // TODO: We do this unmarshal in a few spots. It should be standardized. - err = json.Unmarshal([]byte(v.Default), &defaultValues) - if err != nil { - return diag.FromErr(fmt.Errorf("default value %q is not a list of strings", v.Default)) + 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 @@ -460,15 +473,25 @@ func (v *Parameter) Valid() diag.Diagnostics { } if len(missing) > 0 { - return diag.FromErr(fmt.Errorf( - "default value %q is not a valid option, values %q are missing from the option", - v.Default, strings.Join(missing, ", "), - )) + 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 diag.FromErr(fmt.Errorf("%q default value %q must be defined as one of options", v.FormType, v.Default)) + 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"}}, + }) } } } @@ -536,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. @@ -563,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} +} From 8342a18908c1ab9ef1982306e24932758a4210df Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 11 Apr 2025 15:23:28 -0500 Subject: [PATCH 3/5] fix error assertions --- provider/parameter_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/provider/parameter_test.go b/provider/parameter_test.go index f817280..4b52d94 100644 --- a/provider/parameter_test.go +++ b/provider/parameter_test.go @@ -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: ` @@ -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: ` From c7e7b4e7c3f4ce5afe9b82a66bc2036f83a824cc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Apr 2025 08:44:31 -0500 Subject: [PATCH 4/5] remove singleDiag helper --- provider/parameter.go | 97 +++++++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 44 deletions(-) diff --git a/provider/parameter.go b/provider/parameter.go index c64eb2a..3b6070d 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -157,12 +157,14 @@ func parameterDataSource() *schema.Resource { // Validate options _, parameter.FormType, err = ValidateFormType(parameter.Type, len(parameter.Option), parameter.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"}}, - }) + 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 @@ -417,12 +419,14 @@ func (v *Parameter) Valid() diag.Diagnostics { // 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"}}, - }) + return diag.Diagnostics{ + { + Severity: diag.Error, + Summary: "Invalid form_type for parameter", + Detail: err.Error(), + AttributePath: cty.Path{cty.GetAttrStep{Name: "form_type"}}, + }, + } } optionNames := map[string]interface{}{} @@ -431,19 +435,22 @@ func (v *Parameter) Valid() diag.Diagnostics { for _, option := range v.Option { _, exists := optionNames[option.Name] if exists { - return singleDiag(diag.Diagnostic{ + return diag.Diagnostics{{ 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), - }) + return diag.Diagnostics{ + { + 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() { @@ -473,25 +480,29 @@ func (v *Parameter) Valid() diag.Diagnostics { } 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"}}, - }) + return diag.Diagnostics{ + { + 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 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"}}, + }, + } } } } @@ -563,12 +574,14 @@ 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 nil, diag.Diagnostics{ + { + 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 } @@ -600,7 +613,3 @@ 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} -} From f353685d1554ab871d102995e5dc1f46b0618edb Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 14 Apr 2025 08:50:38 -0500 Subject: [PATCH 5/5] PR comments, mostly typos --- provider/formtype.go | 2 +- provider/parameter.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/provider/formtype.go b/provider/formtype.go index 474b629..75d32c4 100644 --- a/provider/formtype.go +++ b/provider/formtype.go @@ -142,7 +142,7 @@ func ValidateFormType(paramType OptionType, optionCount int, specifiedFormType P if optionsExist { optionMsg = " when options exist" } else { - optionMsg = " when options do no exist" + optionMsg = " when options do not exist" } } return paramType, specifiedFormType, diff --git a/provider/parameter.go b/provider/parameter.go index 3b6070d..2f7dc66 100644 --- a/provider/parameter.go +++ b/provider/parameter.go @@ -429,8 +429,8 @@ func (v *Parameter) Valid() diag.Diagnostics { } } - optionNames := map[string]interface{}{} - optionValues := map[string]interface{}{} + optionNames := map[string]any{} + optionValues := map[string]any{} if len(v.Option) > 0 { for _, option := range v.Option { _, exists := optionNames[option.Name] @@ -577,7 +577,7 @@ func valueIsListString(value string, path cty.Path) ([]string, diag.Diagnostics) return nil, diag.Diagnostics{ { Severity: diag.Error, - Summary: fmt.Sprintf("When using list(string) type, value must be a json encoded list of strings"), + 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, },