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
This commit is contained in:
Nick Craig-Wood 2024-09-11 15:42:47 +01:00
parent 3af757e26d
commit 874d66658e
6 changed files with 101 additions and 24 deletions

View File

@ -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))
}

View File

@ -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

View File

@ -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

View File

@ -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"`},

View File

@ -143,6 +143,25 @@ 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 {
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)
@ -150,6 +169,7 @@ func installFlag(flags *pflag.FlagSet, name string, groupsString string) {
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
if groupsString != "" && flags == pflag.CommandLine {

View File

@ -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)
}