From c156716d0154c58ef49c2a2c6793b064de506cca Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Wed, 3 Jul 2024 21:19:40 +0100 Subject: [PATCH] configstruct: fix parsing of invalid booleans in the config Apparently fmt.Sscanln doesn't parse bool's properly and this isn't likely to be fixed by the Go team who regard sscanf as a mistake. This only uses sscan for integers and uses the correct routine for everything else. This also implements parsing time.Duration See: https://github.com/golang/go/issues/43306 --- fs/config/configstruct/configstruct.go | 45 ++++++++++++------ fs/config/configstruct/configstruct_test.go | 51 +++++++++++++++++++++ fs/config/configstruct/internal_test.go | 38 --------------- 3 files changed, 83 insertions(+), 51 deletions(-) diff --git a/fs/config/configstruct/configstruct.go b/fs/config/configstruct/configstruct.go index e8692b1b6..7a548682e 100644 --- a/fs/config/configstruct/configstruct.go +++ b/fs/config/configstruct/configstruct.go @@ -6,7 +6,9 @@ import ( "fmt" "reflect" "regexp" + "strconv" "strings" + "time" "github.com/rclone/rclone/fs/config/configmap" ) @@ -24,22 +26,39 @@ func camelToSnake(in string) string { // StringToInterface turns in into an interface{} the same type as def func StringToInterface(def interface{}, in string) (newValue interface{}, err error) { typ := reflect.TypeOf(def) - if typ.Kind() == reflect.String && typ.Name() == "string" { - // Pass strings unmodified - return in, nil - } - // Otherwise parse with Sscanln - // - // This means any types we use here must implement fmt.Scanner o := reflect.New(typ) - n, err := fmt.Sscanln(in, o.Interface()) + switch def.(type) { + case string: + // return strings unmodified + newValue = in + case int, int8, int16, int32, int64, + uint, uint8, uint16, uint32, uint64, uintptr, + float32, float64: + // As per Rob Pike's advice in https://github.com/golang/go/issues/43306 + // we only use Sscan for numbers + var n int + n, err = fmt.Sscanln(in, o.Interface()) + if err == nil && n != 1 { + err = errors.New("no items parsed") + } + newValue = o.Elem().Interface() + case bool: + newValue, err = strconv.ParseBool(in) + case time.Duration: + newValue, err = time.ParseDuration(in) + default: + // Try using a Set method + if do, ok := o.Interface().(interface{ Set(s string) error }); ok { + err = do.Set(in) + } else { + err = errors.New("don't know how to parse this type") + } + newValue = o.Elem().Interface() + } if err != nil { - return newValue, fmt.Errorf("parsing %q as %T failed: %w", in, def, err) + return nil, fmt.Errorf("parsing %q as %T failed: %w", in, def, err) } - if n != 1 { - return newValue, errors.New("no items parsed") - } - return o.Elem().Interface(), nil + return newValue, nil } // Item describes a single entry in the options structure diff --git a/fs/config/configstruct/configstruct_test.go b/fs/config/configstruct/configstruct_test.go index 92af006fc..803371ba5 100644 --- a/fs/config/configstruct/configstruct_test.go +++ b/fs/config/configstruct/configstruct_test.go @@ -1,6 +1,7 @@ package configstruct_test import ( + "fmt" "testing" "time" @@ -114,3 +115,53 @@ func TestSetFull(t *testing.T) { require.NoError(t, err) assert.Equal(t, want, in) } + +func TestStringToInterface(t *testing.T) { + item := struct{ A int }{2} + for _, test := range []struct { + in string + def interface{} + want interface{} + err string + }{ + {"", string(""), "", ""}, + {" string ", string(""), " string ", ""}, + {"123", int(0), int(123), ""}, + {"0x123", int(0), int(0x123), ""}, + {" 0x123 ", int(0), int(0x123), ""}, + {"-123", int(0), int(-123), ""}, + {"0", false, false, ""}, + {"1", false, true, ""}, + {"7", false, true, `parsing "7" as bool failed: strconv.ParseBool: parsing "7": invalid syntax`}, + {"FALSE", false, false, ""}, + {"true", false, true, ""}, + {"123", uint(0), uint(123), ""}, + {"123", int64(0), int64(123), ""}, + {"123x", int64(0), nil, "parsing \"123x\" as int64 failed: expected newline"}, + {"truth", false, nil, "parsing \"truth\" as bool failed: strconv.ParseBool: parsing \"truth\": invalid syntax"}, + {"struct", item, nil, "parsing \"struct\" as struct { A int } failed: don't know how to parse this type"}, + {"1s", fs.Duration(0), fs.Duration(time.Second), ""}, + {"1m1s", fs.Duration(0), fs.Duration(61 * time.Second), ""}, + {"1potato", fs.Duration(0), nil, `parsing "1potato" as fs.Duration failed: parsing time "1potato" as "2006-01-02": cannot parse "1potato" as "2006"`}, + {``, []string{}, []string{}, ""}, + {`[]`, []string(nil), []string{}, ""}, + {`["hello"]`, []string{}, []string{"hello"}, ""}, + {`["hello","world!"]`, []string(nil), []string{"hello", "world!"}, ""}, + {"1s", time.Duration(0), time.Second, ""}, + {"1m1s", time.Duration(0), 61 * time.Second, ""}, + {"1potato", time.Duration(0), nil, `parsing "1potato" as time.Duration failed: time: unknown unit "potato" in duration "1potato"`}, + {"1M", fs.SizeSuffix(0), fs.Mebi, ""}, + {"1G", fs.SizeSuffix(0), fs.Gibi, ""}, + {"1potato", fs.SizeSuffix(0), nil, `parsing "1potato" as fs.SizeSuffix failed: bad suffix 'o'`}, + } { + what := fmt.Sprintf("parse %q as %T", test.in, test.def) + got, err := configstruct.StringToInterface(test.def, test.in) + if test.err == "" { + require.NoError(t, err, what) + assert.Equal(t, test.want, got, what) + } else { + assert.Nil(t, got, what) + assert.EqualError(t, err, test.err, what) + } + } +} diff --git a/fs/config/configstruct/internal_test.go b/fs/config/configstruct/internal_test.go index 3918303e5..2f3727d1e 100644 --- a/fs/config/configstruct/internal_test.go +++ b/fs/config/configstruct/internal_test.go @@ -1,11 +1,9 @@ package configstruct import ( - "fmt" "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestCamelToSnake(t *testing.T) { @@ -22,39 +20,3 @@ func TestCamelToSnake(t *testing.T) { assert.Equal(t, test.want, got, test.in) } } - -func TestStringToInterface(t *testing.T) { - item := struct{ A int }{2} - for _, test := range []struct { - in string - def interface{} - want interface{} - err string - }{ - {"", string(""), "", ""}, - {" string ", string(""), " string ", ""}, - {"123", int(0), int(123), ""}, - {"0x123", int(0), int(0x123), ""}, - {" 0x123 ", int(0), int(0x123), ""}, - {"-123", int(0), int(-123), ""}, - {"0", false, false, ""}, - {"1", false, true, ""}, - {"FALSE", false, false, ""}, - {"true", false, true, ""}, - {"123", uint(0), uint(123), ""}, - {"123", int64(0), int64(123), ""}, - {"123x", int64(0), nil, "parsing \"123x\" as int64 failed: expected newline"}, - {"truth", false, nil, "parsing \"truth\" as bool failed: syntax error scanning boolean"}, - {"struct", item, nil, "parsing \"struct\" as struct { A int } failed: can't scan type: *struct { A int }"}, - } { - what := fmt.Sprintf("parse %q as %T", test.in, test.def) - got, err := StringToInterface(test.def, test.in) - if test.err == "" { - require.NoError(t, err, what) - assert.Equal(t, test.want, got, what) - } else { - assert.Nil(t, got) - assert.EqualError(t, err, test.err, what) - } - } -}