diff --git a/provider/formtype.go b/provider/formtype.go index 753f894..75d32c4 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 not 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 4eb8a28..2f7dc66 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) @@ -153,73 +155,27 @@ 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 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) - 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{ @@ -433,7 +389,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) @@ -446,10 +402,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! @@ -459,6 +414,102 @@ 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 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]any{} + optionValues := map[string]any{} + if len(v.Option) > 0 { + for _, option := range v.Option { + _, exists := optionNames[option.Name] + if exists { + 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 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() { + 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 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 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"}}, + }, + } + } + } + } + + return nil +} + func (v *Validation) Valid(typ OptionType, value string) error { if typ != OptionTypeNumber { if !v.MinDisabled { @@ -519,6 +570,22 @@ 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, 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 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. 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: `