From 75f5b06ff72e55aa995cd6e33509e3d29e25c5de Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Wed, 11 Sep 2024 15:42:47 +0100 Subject: [PATCH] fs: fix setting stringArray config values from environment variables After the config re-organisation, the setting of stringArray config values (eg `--exclude` set with `RCLONE_EXCLUDE`) was broken and gave a message like this for `RCLONE_EXCLUDE=*.jpg`: Failed to load "filter" default values: failed to initialise "filter" options: couldn't parse config item "exclude" = "*.jpg" as []string: parsing "*.jpg" as []string failed: invalid character '/' looking for beginning of value This was caused by the parser trying to parse the input string as a JSON value. When the config was re-organised it was thought that the internal representation of stringArray values was not important as it was never visible externally, however this turned out not to be true. A defined representation was chosen - a comma separated string and this was documented and tests were introduced in this patch. This potentially introduces a very small backwards incompatibility. In rclone v1.67.0 RCLONE_EXCLUDE=a,b Would be interpreted as --exclude "a,b" Whereas this new code will interpret it as --exclude "a" --exclude "b" The benefit of being able to set multiple values with an environment variable was deemed to outweigh the very small backwards compatibility risk. If a value with a `,` is needed, then use CSV escaping, eg RCLONE_EXCLUDE="a,b" (Note this needs to have the quotes in so at the unix shell that would be RCLONE_EXCLUDE='"a,b"' Fixes #8063 --- cmdtest/environment_test.go | 40 +++++++++++++++++++++ docs/content/docs.md | 16 +++++++++ fs/config/configstruct/configstruct.go | 22 +++++++----- fs/config/configstruct/configstruct_test.go | 8 +++-- fs/config/flags/flags.go | 30 +++++++++++++--- fs/registry.go | 9 ++--- 6 files changed, 101 insertions(+), 24 deletions(-) diff --git a/cmdtest/environment_test.go b/cmdtest/environment_test.go index 3f1ad8035..2c161bf49 100644 --- a/cmdtest/environment_test.go +++ b/cmdtest/environment_test.go @@ -6,7 +6,9 @@ package cmdtest import ( "os" + "regexp" "runtime" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -344,4 +346,42 @@ func TestEnvironmentVariables(t *testing.T) { env = "" out, err = rcloneEnv(env, "version", "-vv", "--use-json-log") jsonLogOK() + + // Find all the File filter lines in out and return them + parseFileFilters := func(out string) (extensions []string) { + // Match: - (^|/)[^/]*\.jpg$ + find := regexp.MustCompile(`^- \(\^\|\/\)\[\^\/\]\*\\\.(.*?)\$$`) + for _, line := range strings.Split(out, "\n") { + if m := find.FindStringSubmatch(line); m != nil { + extensions = append(extensions, m[1]) + } + } + return extensions + } + + // Make sure that multiple valued (stringArray) environment variables are handled properly + env = `` + out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters", "--exclude", "*.gif", "--exclude", "*.tif") + require.NoError(t, err) + assert.Equal(t, []string{"gif", "tif"}, parseFileFilters(out)) + + env = `RCLONE_EXCLUDE=*.jpg` + out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters", "--exclude", "*.gif") + require.NoError(t, err) + assert.Equal(t, []string{"jpg", "gif"}, parseFileFilters(out)) + + env = `RCLONE_EXCLUDE=*.jpg,*.png` + out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters", "--exclude", "*.gif", "--exclude", "*.tif") + require.NoError(t, err) + assert.Equal(t, []string{"jpg", "png", "gif", "tif"}, parseFileFilters(out)) + + env = `RCLONE_EXCLUDE="*.jpg","*.png"` + out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters") + require.NoError(t, err) + assert.Equal(t, []string{"jpg", "png"}, parseFileFilters(out)) + + env = `RCLONE_EXCLUDE="*.,,,","*.png"` + out, err = rcloneEnv(env, "version", "-vv", "--dump", "filters") + require.NoError(t, err) + assert.Equal(t, []string{",,,", "png"}, parseFileFilters(out)) } diff --git a/docs/content/docs.md b/docs/content/docs.md index 7f6e208e5..eed34ed21 100644 --- a/docs/content/docs.md +++ b/docs/content/docs.md @@ -2911,6 +2911,22 @@ so they take exactly the same form. The options set by environment variables can be seen with the `-vv` flag, e.g. `rclone version -vv`. +Options that can appear multiple times (type `stringArray`) are +treated slighly differently as environment variables can only be +defined once. In order to allow a simple mechanism for adding one or +many items, the input is treated as a [CSV encoded](https://godoc.org/encoding/csv) +string. For example + +| Environment Variable | Equivalent options | +|----------------------|--------------------| +| `RCLONE_EXCLUDE="*.jpg"` | `--exclude "*.jpg"` | +| `RCLONE_EXCLUDE="*.jpg,*.png"` | `--exclude "*.jpg"` `--exclude "*.png"` | +| `RCLONE_EXCLUDE='"*.jpg","*.png"'` | `--exclude "*.jpg"` `--exclude "*.png"` | +| `RCLONE_EXCLUDE='"/directory with comma , in it /**"'` | `--exclude "/directory with comma , in it /**" | + +If `stringArray` options are defined as environment variables **and** +options on the command line then all the values will be used. + ### Config file ### You can set defaults for values in the config file on an individual diff --git a/fs/config/configstruct/configstruct.go b/fs/config/configstruct/configstruct.go index 4d8c941f9..78322af9a 100644 --- a/fs/config/configstruct/configstruct.go +++ b/fs/config/configstruct/configstruct.go @@ -2,7 +2,7 @@ package configstruct import ( - "encoding/json" + "encoding/csv" "errors" "fmt" "reflect" @@ -31,7 +31,7 @@ func camelToSnake(in string) string { // // Builtin types are expected to be encoding as their natural // stringificatons as produced by fmt.Sprint except for []string which -// is expected to be encoded as JSON with empty array encoded as "". +// is expected to be encoded a a CSV with empty array encoded as "". // // Any other types are expected to be encoded by their String() // methods and decoded by their `Set(s string) error` methods. @@ -58,14 +58,18 @@ func StringToInterface(def interface{}, in string) (newValue interface{}, err er case time.Duration: newValue, err = time.ParseDuration(in) case []string: - // JSON decode arrays of strings - if in != "" { - var out []string - err = json.Unmarshal([]byte(in), &out) - newValue = out - } else { - // Empty string we will treat as empty array + // CSV decode arrays of strings - ideally we would use + // fs.CommaSepList here but we can't as it would cause + // a circular import. + if len(in) == 0 { newValue = []string{} + } else { + r := csv.NewReader(strings.NewReader(in)) + newValue, err = r.Read() + switch _err := err.(type) { + case *csv.ParseError: + err = _err.Err // remove line numbers from the error message + } } default: // Try using a Set method diff --git a/fs/config/configstruct/configstruct_test.go b/fs/config/configstruct/configstruct_test.go index a31d70222..fa30684be 100644 --- a/fs/config/configstruct/configstruct_test.go +++ b/fs/config/configstruct/configstruct_test.go @@ -204,9 +204,11 @@ func TestStringToInterface(t *testing.T) { {"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!"}, ""}, + {`""`, []string(nil), []string{""}, ""}, + {`hello`, []string{}, []string{"hello"}, ""}, + {`"hello"`, []string{}, []string{"hello"}, ""}, + {`hello,world!`, []string(nil), []string{"hello", "world!"}, ""}, + {`"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"`}, diff --git a/fs/config/flags/flags.go b/fs/config/flags/flags.go index ebbd55f2a..953a3d0fd 100644 --- a/fs/config/flags/flags.go +++ b/fs/config/flags/flags.go @@ -143,12 +143,32 @@ func installFlag(flags *pflag.FlagSet, name string, groupsString string) { // Read default from environment if possible envKey := fs.OptionToEnv(name) if envValue, envFound := os.LookupEnv(envKey); envFound { - err := flags.Set(name, envValue) - if err != nil { - fs.Fatalf(nil, "Invalid value when setting --%s from environment variable %s=%q: %v", name, envKey, envValue, err) + isStringArray := false + opt, isOption := flag.Value.(*fs.Option) + if isOption { + _, isStringArray = opt.Default.([]string) + } + if isStringArray { + // Treat stringArray differently, treating the environment variable as a CSV array + var list fs.CommaSepList + err := list.Set(envValue) + if err != nil { + fs.Fatalf(nil, "Invalid value when setting stringArray --%s from environment variable %s=%q: %v", name, envKey, envValue, err) + } + // Set both the Value (so items on the command line get added) and DefValue so the help is correct + opt.Value = ([]string)(list) + flag.DefValue = list.String() + for _, v := range list { + fs.Debugf(nil, "Setting --%s %q from environment variable %s=%q", name, v, envKey, envValue) + } + } else { + err := flags.Set(name, envValue) + if err != nil { + fs.Fatalf(nil, "Invalid value when setting --%s from environment variable %s=%q: %v", name, envKey, envValue, err) + } + fs.Debugf(nil, "Setting --%s %q from environment variable %s=%q", name, flag.Value, envKey, envValue) + flag.DefValue = envValue } - fs.Debugf(nil, "Setting --%s %q from environment variable %s=%q", name, flag.Value, envKey, envValue) - flag.DefValue = envValue } // Add flag to Group if it is a global flag diff --git a/fs/registry.go b/fs/registry.go index 9d46e2602..e341bd2d5 100644 --- a/fs/registry.go +++ b/fs/registry.go @@ -264,14 +264,9 @@ func (o *Option) String() string { if len(stringArray) == 0 { return "" } - // Encode string arrays as JSON + // Encode string arrays as CSV // The default Go encoding can't be decoded uniquely - buf, err := json.Marshal(stringArray) - if err != nil { - Errorf(nil, "Can't encode default value for %q key - ignoring: %v", o.Name, err) - return "[]" - } - return string(buf) + return CommaSepList(stringArray).String() } return fmt.Sprint(v) }