From 24525d71d3186123f0b169ea316560b0d5e7dab0 Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Thu, 26 May 2022 22:10:37 +0200 Subject: [PATCH] Fix parsing of boolean config parameters. Other file formats like JSON or YAML would provide more type information, and unmarshalling would create correct types. Properties files aren't that useful, so we have to enforce some types by ourselves. We have to keep an eye on other types beside boolean, but this one seems sufficient for now. Relates to https://github.com/urfave/cli/pull/1376, which made our previous workaround (see #81 and #83) fail due to a private function in the InputSourceContext interface. --- fileutil/map_input_source.go | 255 ------------------------ fileutil/properties_file_loader.go | 21 +- fileutil/properties_file_loader_test.go | 2 +- 3 files changed, 21 insertions(+), 257 deletions(-) delete mode 100644 fileutil/map_input_source.go diff --git a/fileutil/map_input_source.go b/fileutil/map_input_source.go deleted file mode 100644 index fb1848e8..00000000 --- a/fileutil/map_input_source.go +++ /dev/null @@ -1,255 +0,0 @@ -// Heavily copied and inspired by urfave/cli and https://stackoverflow.com/a/46860900/372019 - -package fileutil - -import ( - "fmt" - "reflect" - "strconv" - "strings" - "time" - - "github.com/urfave/cli/v2" -) - -// MapInputSource implements InputSourceContext to return -// data from the map that is loaded. -type MapInputSource struct { - file string - valueMap map[interface{}]interface{} -} - -// nestedVal checks if the name has '.' delimiters. -// If so, it tries to traverse the tree by the '.' delimited sections to find -// a nested value for the key. -func nestedVal(name string, tree map[interface{}]interface{}) (interface{}, bool) { - if sections := strings.Split(name, "."); len(sections) > 1 { - node := tree - for _, section := range sections[:len(sections)-1] { - child, ok := node[section] - if !ok { - return nil, false - } - ctype, ok := child.(map[interface{}]interface{}) - if !ok { - return nil, false - } - node = ctype - } - if val, ok := node[sections[len(sections)-1]]; ok { - return val, true - } - } - return nil, false -} - -// Source returns the path of the source file -func (fsm *MapInputSource) Source() string { - return fsm.file -} - -// Int returns an int from the map if it exists otherwise returns 0 -func (fsm *MapInputSource) Int(name string) (int, error) { - otherGenericValue, exists := fsm.valueMap[name] - if exists { - otherValue, isType := otherGenericValue.(int) - if !isType { - return 0, incorrectTypeForFlagError(name, "int", otherGenericValue) - } - return otherValue, nil - } - nestedGenericValue, exists := nestedVal(name, fsm.valueMap) - if exists { - otherValue, isType := nestedGenericValue.(int) - if !isType { - return 0, incorrectTypeForFlagError(name, "int", nestedGenericValue) - } - return otherValue, nil - } - - return 0, nil -} - -// Duration returns a duration from the map if it exists otherwise returns 0 -func (fsm *MapInputSource) Duration(name string) (time.Duration, error) { - otherGenericValue, exists := fsm.valueMap[name] - if exists { - return castDuration(name, otherGenericValue) - } - nestedGenericValue, exists := nestedVal(name, fsm.valueMap) - if exists { - return castDuration(name, nestedGenericValue) - } - - return 0, nil -} - -func castDuration(name string, value interface{}) (time.Duration, error) { - if otherValue, isType := value.(time.Duration); isType { - return otherValue, nil - } - otherStringValue, isType := value.(string) - parsedValue, err := time.ParseDuration(otherStringValue) - if !isType || err != nil { - return 0, incorrectTypeForFlagError(name, "duration", value) - } - return parsedValue, nil -} - -// Float64 returns an float64 from the map if it exists otherwise returns 0 -func (fsm *MapInputSource) Float64(name string) (float64, error) { - otherGenericValue, exists := fsm.valueMap[name] - if exists { - otherValue, isType := otherGenericValue.(float64) - if !isType { - return 0, incorrectTypeForFlagError(name, "float64", otherGenericValue) - } - return otherValue, nil - } - nestedGenericValue, exists := nestedVal(name, fsm.valueMap) - if exists { - otherValue, isType := nestedGenericValue.(float64) - if !isType { - return 0, incorrectTypeForFlagError(name, "float64", nestedGenericValue) - } - return otherValue, nil - } - - return 0, nil -} - -// String returns a string from the map if it exists otherwise returns an empty string -func (fsm *MapInputSource) String(name string) (string, error) { - otherGenericValue, exists := fsm.valueMap[name] - if exists { - otherValue, isType := otherGenericValue.(string) - if !isType { - return "", incorrectTypeForFlagError(name, "string", otherGenericValue) - } - return otherValue, nil - } - nestedGenericValue, exists := nestedVal(name, fsm.valueMap) - if exists { - otherValue, isType := nestedGenericValue.(string) - if !isType { - return "", incorrectTypeForFlagError(name, "string", nestedGenericValue) - } - return otherValue, nil - } - - return "", nil -} - -// StringSlice returns an []string from the map if it exists otherwise returns nil -func (fsm *MapInputSource) StringSlice(name string) ([]string, error) { - otherGenericValue, exists := fsm.valueMap[name] - if !exists { - otherGenericValue, exists = nestedVal(name, fsm.valueMap) - if !exists { - return nil, nil - } - } - - otherValue, isType := otherGenericValue.([]interface{}) - if !isType { - return nil, incorrectTypeForFlagError(name, "[]interface{}", otherGenericValue) - } - - var stringSlice = make([]string, 0, len(otherValue)) - for i, v := range otherValue { - stringValue, isType := v.(string) - - if !isType { - return nil, incorrectTypeForFlagError(fmt.Sprintf("%s[%d]", name, i), "string", v) - } - - stringSlice = append(stringSlice, stringValue) - } - - return stringSlice, nil -} - -// IntSlice returns an []int from the map if it exists otherwise returns nil -func (fsm *MapInputSource) IntSlice(name string) ([]int, error) { - otherGenericValue, exists := fsm.valueMap[name] - if !exists { - otherGenericValue, exists = nestedVal(name, fsm.valueMap) - if !exists { - return nil, nil - } - } - - otherValue, isType := otherGenericValue.([]interface{}) - if !isType { - return nil, incorrectTypeForFlagError(name, "[]interface{}", otherGenericValue) - } - - var intSlice = make([]int, 0, len(otherValue)) - for i, v := range otherValue { - intValue, isType := v.(int) - - if !isType { - return nil, incorrectTypeForFlagError(fmt.Sprintf("%s[%d]", name, i), "int", v) - } - - intSlice = append(intSlice, intValue) - } - - return intSlice, nil -} - -// Generic returns an cli.Generic from the map if it exists otherwise returns nil -func (fsm *MapInputSource) Generic(name string) (cli.Generic, error) { - otherGenericValue, exists := fsm.valueMap[name] - if exists { - otherValue, isType := otherGenericValue.(cli.Generic) - if !isType { - return nil, incorrectTypeForFlagError(name, "cli.Generic", otherGenericValue) - } - return otherValue, nil - } - nestedGenericValue, exists := nestedVal(name, fsm.valueMap) - if exists { - otherValue, isType := nestedGenericValue.(cli.Generic) - if !isType { - return nil, incorrectTypeForFlagError(name, "cli.Generic", nestedGenericValue) - } - return otherValue, nil - } - - return nil, nil -} - -// Bool returns an bool from the map otherwise returns false -func (fsm *MapInputSource) Bool(name string) (bool, error) { - otherGenericValue, exists := fsm.valueMap[name] - if exists { - return fsm.asBool(name, otherGenericValue) - } - nestedGenericValue, exists := nestedVal(name, fsm.valueMap) - if exists { - return fsm.asBool(name, nestedGenericValue) - } - return false, nil -} - -func (fsm *MapInputSource) asBool(name string, value interface{}) (bool, error) { - switch v := value.(type) { - case bool: - return v, nil - case string: - return strconv.ParseBool(v) - default: - return false, incorrectTypeForFlagError(name, "bool", value) - } -} - -func incorrectTypeForFlagError(name, expectedTypeName string, value interface{}) error { - valueType := reflect.TypeOf(value) - valueTypeName := "" - if valueType != nil { - valueTypeName = valueType.Name() - } - - return fmt.Errorf("mismatched type for flag '%s'. Expected '%s' but actual is '%s'", name, expectedTypeName, valueTypeName) -} diff --git a/fileutil/properties_file_loader.go b/fileutil/properties_file_loader.go index 1e0036c1..17104d2f 100644 --- a/fileutil/properties_file_loader.go +++ b/fileutil/properties_file_loader.go @@ -12,6 +12,7 @@ import ( "net/url" "os" "runtime" + "strconv" "strings" "github.com/urfave/cli/v2" @@ -50,9 +51,27 @@ func readPropertiesFile(filename string) (map[interface{}]interface{}, error) { return nil, err } + // Other file formats would provide more type information, and unmarshalling would + // create correct types. Properties files aren't that useful, so we have to enforce + // some types by ourselves. We have to keep an eye on other types beside boolean, + // but this one seems good enough for now. + coerceBooleanValues(config) + return config, nil } +func coerceBooleanValues(m map[interface{}]interface{}) { + for k, v := range m { + switch v := v.(type) { + case string: + parsed, err := strconv.ParseBool(v) + if err == nil { + m[k] = parsed + } + } + } +} + type propertiesSourceContext struct { FilePath string } @@ -66,7 +85,7 @@ func NewPropertiesSourceFromFile(file string) (altsrc.InputSourceContext, error) return nil, fmt.Errorf("Unable to load Properties file '%s': inner error: \n'%v'", ysc.FilePath, err.Error()) } - return &MapInputSource{valueMap: results}, nil + return altsrc.NewMapInputSource(file, results), nil } // NewPropertiesSourceFromFlagFunc creates a new Properties InputSourceContext from a provided flag name and source context. diff --git a/fileutil/properties_file_loader_test.go b/fileutil/properties_file_loader_test.go index 1596bcb9..48b279dd 100644 --- a/fileutil/properties_file_loader_test.go +++ b/fileutil/properties_file_loader_test.go @@ -14,7 +14,7 @@ func TestReadPropertiesFile(t *testing.T) { t.FailNow() } - if props["host"] != "localhost" || props["proxyHost"] != "test" || props["protocol"] != "https://" || props["chunk"] != "" || props["boolean"] != "true" { + if props["host"] != "localhost" || props["proxyHost"] != "test" || props["protocol"] != "https://" || props["chunk"] != "" || props["boolean"] != true { fmt.Printf("props: %q", props) t.Error("Error properties not loaded correctly") t.Fail()