diff --git a/errors.go b/errors.go index 6920eda..3ed9080 100644 --- a/errors.go +++ b/errors.go @@ -14,6 +14,10 @@ var ( // add a flag with the same name as a pre-existing flag. ErrDuplicateFlag = errors.New("duplicate flag") + // ErrAmbiguousFlag should be returned when a flag name is ambiguous and + // matches more than one defined flag. + ErrAmbiguousFlag = errors.New("ambiguous flag") + // ErrNotParsed may be returned by flag set methods which require the flag // set to have been successfully parsed, and that condition isn't satisfied. ErrNotParsed = errors.New("not parsed") diff --git a/ffenv/ffenv.go b/ffenv/ffenv.go index 93e0409..bbec3db 100644 --- a/ffenv/ffenv.go +++ b/ffenv/ffenv.go @@ -10,16 +10,29 @@ import ( "strings" ) -// Parse is a parser for .env files. Each line is tokenized on the first `=` -// character. The first token is interpreted as the env var representation of -// the flag name, and the second token is interpreted as the value. Both tokens -// are trimmed of leading and trailing whitespace. If the value is "double -// quoted", control characters like `\n` are expanded. Lines beginning with `#` -// are interpreted as comments. End-of-line comments are not supported. +// Parse is a parser for .env files. // -// The parser respects the [ff.WithEnvVarPrefix] option. For example, if parse -// is called with an env var prefix MYPROG, then both FOO=bar and MYPROG_FOO=bar -// would set a flag named foo. +// Each line in the .env file is tokenized on the first `=` character. The first +// token is interpreted as the env var representation of the flag name, and the +// second token is interpreted as the value. Both tokens are trimmed of leading +// and trailing whitespace. If the value is "double quoted", control characters +// like `\n` are expanded. Lines beginning with `#` are interpreted as comments. +// End-of-line comments are not supported. +// +// Parse options related to environment variables, like [ff.WithEnvVarPrefix], +// [ff.WithEnvVarShortNames], and [ff.WithEnvVarCaseSensitive], also apply to +// .env files. For example, WithEnvVarPrefix("MYPROG") means that the keys in an +// .env file must begin with MYPROG_. +// +// If for any reason any key in an .env file matches multiple flags, parse will +// return [ff.ErrDuplicateFlag]. This can happen if you have flags with names +// that differ only in capitalization, e.g. `-v` and `-V`. If you want to +// support setting these flags from an .env file, either use discrete long +// names, or [ff.WithEnvVarCaseSensitive]. +// +// Using the .env config file parser doesn't automatically enable parsing of +// actual environment variables. To do so, callers must still explicitly provide +// e.g. [ff.WithEnvVars] or [ff.WithEnvVarPrefix]. func Parse(r io.Reader, set func(name, value string) error) error { s := bufio.NewScanner(r) for s.Scan() { diff --git a/ffenv/ffenv_test.go b/ffenv/ffenv_test.go index 9defc0d..2c68dc7 100644 --- a/ffenv/ffenv_test.go +++ b/ffenv/ffenv_test.go @@ -6,6 +6,7 @@ import ( "time" "github.com/peterbourgon/ff/v4" + "github.com/peterbourgon/ff/v4/ffenv" "github.com/peterbourgon/ff/v4/fftest" ) @@ -18,46 +19,65 @@ func TestEnvFileParser(t *testing.T) { Want: fftest.Vars{}, }, { - ConfigFile: "testdata/basic.env", - Want: fftest.Vars{S: "bar", I: 99, B: true, D: time.Hour}, + ConfigFile: "testdata/basic.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Want: fftest.Vars{S: "bar", I: 99, B: true, D: time.Hour}, }, { - ConfigFile: "testdata/prefix.env", - Options: []ff.Option{ff.WithEnvVarPrefix("MYPROG")}, - Want: fftest.Vars{S: "bingo", I: 123}, + ConfigFile: "testdata/prefix.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Options: []ff.Option{ff.WithEnvVarPrefix("MYPROG")}, + Want: fftest.Vars{S: "bingo", I: 123}, }, { - ConfigFile: "testdata/prefix-undef.env", - Options: []ff.Option{ff.WithEnvVarPrefix("MYPROG"), ff.WithConfigIgnoreUndefinedFlags()}, - Want: fftest.Vars{S: "bango", I: 9}, + ConfigFile: "testdata/prefix-undef.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Options: []ff.Option{ff.WithEnvVarPrefix("MYPROG"), ff.WithConfigIgnoreUndefinedFlags()}, + Want: fftest.Vars{S: "bango", I: 9}, }, { - ConfigFile: "testdata/quotes.env", - Want: fftest.Vars{S: "", I: 32, X: []string{"1", "2 2", "3 3 3"}}, + ConfigFile: "testdata/quotes.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Want: fftest.Vars{S: "", I: 32, X: []string{"1", "2 2", "3 3 3"}}, }, { - ConfigFile: "testdata/no-value.env", - Want: fftest.Vars{WantParseErrorString: "D: parse error"}, + ConfigFile: "testdata/no-value.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Want: fftest.Vars{WantParseErrorString: "DUR: parse error"}, }, { - ConfigFile: "testdata/spaces.env", - Want: fftest.Vars{X: []string{"1", "2", "3", "4", "5", " 6", " 7 ", " 8 ", "9"}}, + ConfigFile: "testdata/spaces.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Want: fftest.Vars{X: []string{"1", "2", "3", "4", "5", " 6", " 7 ", " 8 ", "9"}}, }, { - ConfigFile: "testdata/newlines.env", - Want: fftest.Vars{S: "one\ntwo\nthree\n\n", X: []string{`A\nB\n\n`}}, + ConfigFile: "testdata/newlines.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Want: fftest.Vars{S: "one\ntwo\nthree\n\n", X: []string{`A\nB\n\n`}}, }, { - ConfigFile: "testdata/capitalization.env", - Want: fftest.Vars{S: "hello", I: 12345}, + ConfigFile: "testdata/comments.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Want: fftest.Vars{S: "abc # def"}, }, { - ConfigFile: "testdata/comments.env", - Want: fftest.Vars{S: "abc # def"}, + ConfigFile: "testdata/short.env", + Constructors: fftest.DefaultConstructors, + Options: []ff.Option{ff.WithEnvVarShortNames()}, + Want: fftest.Vars{S: "hello", I: 99, D: 8 * time.Millisecond}, + }, + { + ConfigFile: "testdata/case-sensitive.env", + Constructors: []fftest.Constructor{fftest.CoreConstructor}, + Options: []ff.Option{ff.WithEnvVarPrefix("MYPREFIX"), ff.WithEnvVarCaseSensitive(), ff.WithConfigIgnoreUndefinedFlags()}, + Want: fftest.Vars{S: "hello", I: 12345, D: 1*time.Minute + 30*time.Second}, }, } for i := range testcases { + testcases[i].Constructors = []fftest.Constructor{ + fftest.CoreConstructor, + } if testcases[i].Name == "" { testcases[i].Name = filepath.Base(testcases[i].ConfigFile) } @@ -65,3 +85,145 @@ func TestEnvFileParser(t *testing.T) { testcases.Run(t) } + +func TestAmbiguous(t *testing.T) { + t.Parallel() + + fs := ff.NewFlagSet(t.Name()) + verboseFlag := fs.Bool('v', "verbose", "verbose output") + versionFlag := fs.Bool('V', "version", "print version") + + for _, tc := range []struct { + name string + options []ff.Option + wantErr bool + wantVerbose bool + wantVersion bool + }{ + { + // With just the config file parser, env vars aren't enabled, so we + // don't do duplicate detection up-front. And because we don't + // provide WithEnvVarShortNames(), we're only using long names to + // populate env2flags, so there is no v/V ambiguity. Also important: + // the `V=true` doesn't match to anything in the env, but it *does* + // match to a (short) flag name, because we didn't provide + // WithConfigIgnoreFlagNames(). + name: "ambiguous-1.env WithConfigFile", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env")}, + wantErr: false, + wantVerbose: true, + wantVersion: true, // V=true should match to `-V, --version` + }, + { + // This is the same as above, but WithConfigIgnoreFlagNames() means + // the `V=true` doesn't match to anything any more, and because we + // didn't provide WithConfigIgnoreUndefinedFlags() that's an error. + name: "ambiguous-1.env WithConfigIgnoreFlagNames", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithConfigIgnoreFlagNames()}, + wantErr: true, + }, + { + // Same as above, but passing WithConfigIgnoreUndefinedFlags() means + // it can now ignore `V=true` and succeed. + name: "ambiguous-1.env WithConfigIgnoreFlagNames", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithConfigIgnoreFlagNames(), ff.WithConfigIgnoreUndefinedFlags()}, + wantErr: false, + wantVerbose: true, + wantVersion: false, + }, + { + // WithEnvVarShortNames() by itself doesn't enable env var parsing, + // and so doesn't trigger duplicate detection. + name: "ambiguous-1.env WithEnvVarShortNames", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithEnvVarShortNames()}, + wantErr: false, + wantVerbose: true, + wantVersion: true, + }, + { + // WithEnvVarShortNames() combined with WithEnvVars() triggers + // duplicate detection up-front. Without WithEnvVarCaseSensitive(), + // `-v` and `-V` both map to `V` and so are ambiguous, which results + // in an error. + name: "ambiguous-1.env WithEnvVarShortNames WithEnvVars", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithEnvVarShortNames(), ff.WithEnvVars()}, + wantErr: true, + }, + { + // Same as above, but passing WithEnvVarCaseSensitive() means that + // `-v` and `-V` are no longer ambiguous, and that `V=true` now + // matches to `-V, --version`. But it also means that `VERSION=true` + // is invalid, since it would need to be `version=true`. So, another + // error. + name: "ambiguous-1.env WithEnvVarShortNames WithEnvVarCaseSensitive", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithEnvVarShortNames(), ff.WithEnvVarCaseSensitive()}, + wantErr: true, + }, + { + // But if we ignore undefined flags, then the parse can succeed. + name: "ambiguous-1.env WithEnvVarShortNames WithEnvVarCaseSensitive WithConfigIgnoreUndefinedFlags", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-1.env"), ff.WithEnvVarShortNames(), ff.WithEnvVarCaseSensitive(), ff.WithConfigIgnoreUndefinedFlags()}, + wantErr: false, + wantVerbose: false, + wantVersion: true, + }, + { + name: "ambiguous-2.env matches both short names", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env")}, + wantErr: false, + wantVerbose: true, + wantVersion: true, + }, + { + name: "ambiguous-2.env WithConfigIgnoreFlagNames no match", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames()}, + wantErr: true, + }, + { + name: "ambiguous-2.env WithConfigIgnoreFlagNames WithEnvVars no match", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames(), ff.WithEnvVars()}, + wantErr: true, + }, + { + name: "ambiguous-2.env WithConfigIgnoreFlagNames WithEnvVars WithEnvVarShortNames duplicate", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames(), ff.WithEnvVars(), ff.WithEnvVarShortNames()}, + wantErr: true, + }, + { + name: "ambiguous-2.env WithConfigIgnoreFlagNames WithEnvVarShortNames ambiguous", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames(), ff.WithEnvVarShortNames()}, + wantErr: true, + }, + { + name: "ambiguous-2.env WithConfigIgnoreFlagNames WithEnvVarShortNames WithEnvVarCaseSensitive OK", + options: []ff.Option{ff.WithConfigFile("testdata/ambiguous-2.env"), ff.WithConfigIgnoreFlagNames(), ff.WithEnvVarShortNames(), ff.WithEnvVarCaseSensitive()}, + wantErr: false, + wantVerbose: true, + wantVersion: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + fs.Reset() + + options := append([]ff.Option{ff.WithConfigFileParser(ffenv.Parse)}, tc.options...) + + err := ff.Parse(fs, []string{}, options...) + t.Logf("--verbose=%v --version=%v error=%v", *verboseFlag, *versionFlag, err) + + switch { + case tc.wantErr: + if want, have := tc.wantErr, err != nil; want != have { + t.Errorf("error: want %v, have %v", want, have) + } + + default: + if want, have := tc.wantVerbose, *verboseFlag; want != have { + t.Errorf("verbose: want %v, have %v", want, have) + } + if want, have := tc.wantVersion, *versionFlag; want != have { + t.Errorf("version: want %v, have %v", want, have) + } + } + }) + } +} diff --git a/ffenv/testdata/ambiguous-1.env b/ffenv/testdata/ambiguous-1.env new file mode 100644 index 0000000..b3b5c19 --- /dev/null +++ b/ffenv/testdata/ambiguous-1.env @@ -0,0 +1,6 @@ +# should be OK +VERBOSE=true + +# matches flag name -V by default +# with IgnoreFlagNames -> error +V=true diff --git a/ffenv/testdata/ambiguous-2.env b/ffenv/testdata/ambiguous-2.env new file mode 100644 index 0000000..996ef47 --- /dev/null +++ b/ffenv/testdata/ambiguous-2.env @@ -0,0 +1,2 @@ +V=true +v=true diff --git a/ffenv/testdata/basic.env b/ffenv/testdata/basic.env index 98a34bf..e5e04d1 100644 --- a/ffenv/testdata/basic.env +++ b/ffenv/testdata/basic.env @@ -1,4 +1,4 @@ -S=bar -I=99 -B=true -D=1h +STR=bar +INT=99 +BFLAG=true +DUR=1h diff --git a/ffenv/testdata/capitalization.env b/ffenv/testdata/capitalization.env deleted file mode 100644 index 218bdea..0000000 --- a/ffenv/testdata/capitalization.env +++ /dev/null @@ -1,2 +0,0 @@ -s=hello -i=12345 diff --git a/ffenv/testdata/case-sensitive.env b/ffenv/testdata/case-sensitive.env new file mode 100644 index 0000000..a631df5 --- /dev/null +++ b/ffenv/testdata/case-sensitive.env @@ -0,0 +1,7 @@ +# works with CaseSensitive, fails without +MYPREFIX_str=hello +MYPREFIX_int=12345 +MYPREFIX_dur=1m30s + +# works with IgnoreUndefined, fails without +MYPREFIX_FLT=3.33 diff --git a/ffenv/testdata/comments.env b/ffenv/testdata/comments.env index 276a426..a7e8bea 100644 --- a/ffenv/testdata/comments.env +++ b/ffenv/testdata/comments.env @@ -3,4 +3,4 @@ # comment 3 # S will be set to `abc # def` -S=abc # def +STR=abc # def diff --git a/ffenv/testdata/newlines.env b/ffenv/testdata/newlines.env index fb603a0..25c56fe 100644 --- a/ffenv/testdata/newlines.env +++ b/ffenv/testdata/newlines.env @@ -1,2 +1,2 @@ -S="one\ntwo\nthree\n\n" -X=A\nB\n\n +STR="one\ntwo\nthree\n\n" +XXX=A\nB\n\n diff --git a/ffenv/testdata/no-value.env b/ffenv/testdata/no-value.env index 2cc4f9b..5246ae3 100644 --- a/ffenv/testdata/no-value.env +++ b/ffenv/testdata/no-value.env @@ -1,3 +1,3 @@ -I=32 -D= -S=this is fine +INT=32 +DUR= +STR=this is fine diff --git a/ffenv/testdata/prefix-undef.env b/ffenv/testdata/prefix-undef.env index bbb8753..c7dcb05 100644 --- a/ffenv/testdata/prefix-undef.env +++ b/ffenv/testdata/prefix-undef.env @@ -1,6 +1,6 @@ -MYPROG_I=9 -OTHERPREFIX_B=true -MYPROG_S=bango -D=32m +MYPROG_INT=9 +OTHERPREFIX_BFLAG=true +MYPROG_STR=bango +DUR=32m diff --git a/ffenv/testdata/prefix.env b/ffenv/testdata/prefix.env index b8a2574..7bbdc7e 100644 --- a/ffenv/testdata/prefix.env +++ b/ffenv/testdata/prefix.env @@ -1,4 +1,4 @@ -MYPROG_S=bingo +MYPROG_STR=bingo -MYPROG_I=123 +MYPROG_INT=123 diff --git a/ffenv/testdata/quotes.env b/ffenv/testdata/quotes.env index b1625e7..69b3722 100644 --- a/ffenv/testdata/quotes.env +++ b/ffenv/testdata/quotes.env @@ -1,5 +1,5 @@ -S="" -X=1 -X=2 2 -X="3 3 3" -I="32" +STR="" +XXX=1 +XXX=2 2 +XXX="3 3 3" +INT="32" diff --git a/ffenv/testdata/short.env b/ffenv/testdata/short.env new file mode 100644 index 0000000..05dd1e9 --- /dev/null +++ b/ffenv/testdata/short.env @@ -0,0 +1,5 @@ +S=hello +I=99 + +# long names are still valid +DUR=8ms diff --git a/ffenv/testdata/spaces.env b/ffenv/testdata/spaces.env index 5b5d8d0..caf6400 100644 --- a/ffenv/testdata/spaces.env +++ b/ffenv/testdata/spaces.env @@ -1,9 +1,9 @@ -X = 1 -X= 2 -X =3 -X= 4 - X = 5 -X=" 6" -X= " 7 " -X = " 8 " - X = 9 +XXX = 1 +XXX= 2 +XXX =3 +XXX= 4 + XXX = 5 +XXX=" 6" +XXX= " 7 " +XXX = " 8 " + XXX = 9 diff --git a/flag_set_test.go b/flag_set_test.go index 209e47f..39493cf 100644 --- a/flag_set_test.go +++ b/flag_set_test.go @@ -737,3 +737,85 @@ func TestFlagSet_Func(t *testing.T) { check(t, fs, f) }) } + +func TestFlagSet_duplicates(t *testing.T) { + t.Parallel() + + // --foo; --Foo = OK + // --foo; --Foo + WithEnvVar = error + // --foo; --Foo + WithEnvVar + WithEnvVarCaseSensitive = OK + // -v, --verbose; -V, --version = OK + // -v, --verbose; -V, --version + WithEnvVar = OK + // -v, --verbose; -V, --version + WithEnvVar + WithEnvVarShortNames = error + // -v, --verbose; -V, --version + WithEnvVar + WithEnvVarShortNames + WithEnvVarCaseSensitive = OK + + var ( + fooFlag = ff.FlagConfig{LongName: "foo", Value: &ffval.String{}} + FooFlag = ff.FlagConfig{LongName: "Foo", Value: &ffval.String{}} + verboseFlag = ff.FlagConfig{ShortName: 'v', LongName: "verbose", Value: &ffval.Bool{}} + versionFlag = ff.FlagConfig{ShortName: 'V', LongName: "version", Value: &ffval.Bool{}} + ) + + _, _ = verboseFlag, versionFlag + + type testcase struct { + name string + configs []ff.FlagConfig + options []ff.Option + wantErr bool + } + + tests := []testcase{ + { + name: "--foo; --Foo", + configs: []ff.FlagConfig{fooFlag, FooFlag}, + }, + { + name: "--foo; --Foo + WithEnvVars", + configs: []ff.FlagConfig{fooFlag, FooFlag}, + options: []ff.Option{ff.WithEnvVars()}, + wantErr: true, + }, + { + name: "--foo; --Foo + WithEnvVars + WithEnvVarCaseSensitive", + configs: []ff.FlagConfig{fooFlag, FooFlag}, + options: []ff.Option{ff.WithEnvVars(), ff.WithEnvVarCaseSensitive()}, + }, + { + name: "-v, --verbose; -V, --version", + configs: []ff.FlagConfig{verboseFlag, versionFlag}, + }, + { + name: "-v, --verbose; -V, --version + WithEnvVars", + configs: []ff.FlagConfig{verboseFlag, versionFlag}, + options: []ff.Option{ff.WithEnvVars()}, + }, + { + name: "-v, --verbose; -V, --version + WithEnvVars + WithEnvVarShortNames", + configs: []ff.FlagConfig{verboseFlag, versionFlag}, + options: []ff.Option{ff.WithEnvVars(), ff.WithEnvVarShortNames()}, + wantErr: true, + }, + { + name: "-v, --verbose; -V, --version + WithEnvVars + WithEnvVarShortNames + WithEnvVarCaseSensitive", + configs: []ff.FlagConfig{verboseFlag, versionFlag}, + options: []ff.Option{ff.WithEnvVars(), ff.WithEnvVarShortNames(), ff.WithEnvVarCaseSensitive()}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fs := ff.NewFlagSet(t.Name()) + for _, config := range tc.configs { + if _, err := fs.AddFlag(config); err != nil { + t.Errorf("AddFlag(%+v): %v", config, err) + } + } + err := ff.Parse(fs, []string{"--"}, tc.options...) + t.Logf("error: %v", err) + if want, have := tc.wantErr, err != nil; want != have { + t.Errorf("Parse: error: want %v, have %v", want, have) + } + }) + } +} diff --git a/helpers.go b/helpers.go index 8a743c9..a0f8900 100644 --- a/helpers.go +++ b/helpers.go @@ -40,17 +40,6 @@ func isValidLongName(long string) bool { return isValid } -func getNameStrings(f Flag) []string { - var names []string - if short, ok := f.GetShortName(); ok { - names = append(names, string(short)) - } - if long, ok := f.GetLongName(); ok { - names = append(names, long) - } - return names -} - func getNameString(f Flag) string { var names []string if short, ok := f.GetShortName(); ok { diff --git a/options.go b/options.go index ebd3405..4040a4a 100644 --- a/options.go +++ b/options.go @@ -10,9 +10,11 @@ type Option func(*ParseContext) // ParseContext receives and maintains parse options. type ParseContext struct { - envVarEnabled bool - envVarPrefix string - envVarSplit string + envVarEnabled bool + envVarPrefix string + envVarSplit string + envVarCaseSensitive bool + envVarShortNames bool configFileName string configFlagName string @@ -20,6 +22,8 @@ type ParseContext struct { configOpenFunc func(string) (iofs.File, error) configAllowMissingFile bool configIgnoreUndefinedFlags bool + configKeyIgnoreFlagNames bool + configKeyIgnoreEnvVars bool } // ConfigFileParseFunc is a function that consumes the provided reader as a config @@ -60,6 +64,17 @@ func WithConfigFileParser(pf ConfigFileParseFunc) Option { } } +// WithFilesystem tells [Parse] to use the provided filesystem when accessing +// files on disk, typically when reading a config file. This can be useful when +// working with embedded filesystems. +// +// By default, the host filesystem is used, via [os.Open]. +func WithFilesystem(fs iofs.FS) Option { + return func(pc *ParseContext) { + pc.configOpenFunc = fs.Open + } +} + // WithConfigAllowMissingFile tells [Parse] to ignore config files that are // specified but don't exist. // @@ -81,9 +96,41 @@ func WithConfigIgnoreUndefinedFlags() Option { } } -// WithEnvVars tells [Parse] to set flags from environment variables. Flags are -// matched to environment variables by capitalizing the flag name, and replacing -// separator characters like periods or hyphens with underscores. +// WithConfigIgnoreFlagNames tells [Parse] to ignore the short and long names, +// when matching keys from config files to valid flags. +// +// By default, config file keys are matched to flags by short name, long name, +// and/or environment variable name(s). +// +// In practice, this option is really only useful when parsing .env files, to +// avoid ambiguity or unintentional matches. +func WithConfigIgnoreFlagNames() Option { + return func(pc *ParseContext) { + pc.configKeyIgnoreFlagNames = true + } +} + +// WithConfigIgnoreEnvVars tells [Parse] to ignore environment variables, when +// matching keys from config files to valid flags. +// +// By default, config file keys are matched to flags by short name, long name, +// and/or environment variable name(s). +// +// This option can be useful in situations where invalid config file keys need +// to be reliably detected and rejected, as it helps to prevent unintentional +// matches from the environment. +func WithConfigIgnoreEnvVars() Option { + return func(pc *ParseContext) { + pc.configKeyIgnoreEnvVars = true + } +} + +// WithEnvVars tells [Parse] to set flags from environment variables. +// +// Flags are matched to environment variables by capitalizing the flag's long +// name, and replacing separator characters like periods or hyphens with +// underscores. For example, the flag `-f, --foo-bar` would match the +// environment variable `FOO_BAR`. // // By default, flags are not parsed from environment variables at all. func WithEnvVars() Option { @@ -108,7 +155,8 @@ func WithEnvVarPrefix(prefix string) Option { // WithEnvVarSplit tells [Parse] to split environment variable values on the // given delimiter, and to set the flag multiple times, once for each delimited -// token. Values produced in this way are not trimmed of whitespace. +// token. Values produced in this way are not trimmed of whitespace. By default, +// no splitting of environment variable values occurs. // // For example, the env var `FOO=a,b,c` would by default set a flag named `foo` // one time, with the value `a,b,c`. Providing WithEnvVarSplit with a comma @@ -120,20 +168,45 @@ func WithEnvVarPrefix(prefix string) Option { // `a` and `b,c`. Or, `FOO=axxxb\xxxc` with a delimiter of `xxx` would yield // values `a` and `bxxxc`. // -// By default, no splitting of environment variable values occurs. +// For historical reasons, WithEnvVarSplit automatically enables environment +// variable parsing. This will change in a future release. Callers should always +// explicitly provide [WithEnvVars] or [WithEnvVarPrefix] to parse flags from +// the environment. func WithEnvVarSplit(delimiter string) Option { return func(pc *ParseContext) { - pc.envVarEnabled = true pc.envVarSplit = delimiter } } -// WithFilesystem tells [Parse] to use the provided filesystem when accessing -// files on disk, typically when reading a config file. +// WithEnvVarCaseSensitive tells [Parse] to match flags to environment variables +// using the exact case of the flag name, rather than the default behavior fo +// transforming the flag name to uppercase. // -// By default, the host filesystem is used, via [os.Open]. -func WithFilesystem(fs iofs.FS) Option { +// For example, using WithEnvVarPrefix("MYPREFIX"), the flag `foo` would +// normally match the environment variable `MYPREFIX_FOO`. With this option, it +// would instead match the environment variable `MYPREFIX_foo`. +// +// WithEnvVarCaseSensitive does NOT automatically enable environment variable +// parsing. Callers must explicitly provide [WithEnvVars] or [WithEnvVarPrefix] +// to parse flags from the environment. +func WithEnvVarCaseSensitive() Option { return func(pc *ParseContext) { - pc.configOpenFunc = fs.Open + pc.envVarCaseSensitive = true + } +} + +// WithEnvVarShortNames tells [Parse] to match flags to environment variables +// using the short name of the flag in addition to the long name. +// +// For example, if a flag is defined as `-f, --foo`, then normally only the +// environment variable `FOO` would match. Using this option means that the +// environment variable `F` would also match. +// +// WithEnvVarShortNames does NOT automatically enable environment variable +// parsing. Callers must explicitly provide [WithEnvVars] or [WithEnvVarPrefix] +// to parse flags from the environment. +func WithEnvVarShortNames() Option { + return func(pc *ParseContext) { + pc.envVarShortNames = true } } diff --git a/parse.go b/parse.go index 5fd1e08..1bc19ce 100644 --- a/parse.go +++ b/parse.go @@ -39,20 +39,33 @@ func parse(fs Flags, args []string, options ...Option) error { } // Index valid flags by env var key, to support .env config files (below). - env2flag := map[string]Flag{} + env2flags := map[string][]Flag{} { + // First, collect flags by env var key. if err := fs.WalkFlags(func(f Flag) error { - for _, name := range getNameStrings(f) { - key := getEnvVarKey(name, pc.envVarPrefix) - if existing, ok := env2flag[key]; ok { - return fmt.Errorf("%s: %w (%s)", getNameString(f), ErrDuplicateFlag, getNameString(existing)) - } - env2flag[key] = f + for _, key := range getEnvVarKeys(f, pc) { + env2flags[key] = append(env2flags[key], f) } return nil }); err != nil { return err } + + // If env var support is enabled, to prevent ambiguity, we need to + // ensure that no two flags share the same env var key. + // + // Arguably this check should also be performed if we're using an .env + // config file parser, but we have no way of knowing that, and special + // casing *our* ffenv parser isn't a good solution. But this is fine: if + // a config file specifies a key that maps to more than 1 flag, we can + // return an error at that point. + if pc.envVarEnabled { + for key, flags := range env2flags { + if len(flags) > 1 { + return fmt.Errorf("%s: %w (%s) (%s)", getNameString(flags[0]), ErrDuplicateFlag, getNameString(flags[1]), key) + } + } + } } // After each stage of parsing, record the flags that have been provided. @@ -85,11 +98,8 @@ func parse(fs Flags, args []string, options ...Option) error { return nil } - // Look in the environment for each of the flag names. - for _, name := range getNameStrings(f) { - // Transform the flag name to an env var key. - key := getEnvVarKey(name, pc.envVarPrefix) - + // Look in the environment for each of the flag's keys. + for _, key := range getEnvVarKeys(f, pc) { // Look up the value from the environment. val, ok := os.LookupEnv(key) if !ok { @@ -153,23 +163,29 @@ func parse(fs Flags, args []string, options ...Option) error { case err == nil: defer configFile.Close() if err := pc.configParseFunc(configFile, func(name, value string) error { - // The parser calls us with a name=value pair. We want to - // allow the name to be either the actual flag name, or its - // env var representation (to support .env files). var ( - setFlag, fromSet = fs.GetFlag(name) - envFlag, fromEnv = env2flag[name] - target Flag + setFlag, fromSet = fs.GetFlag(name) + envFlags, fromEnv = env2flags[name] + target Flag ) switch { - case fromSet: + case !pc.configKeyIgnoreFlagNames && fromSet: // found in the flag set target = setFlag - case !fromSet && fromEnv: - target = envFlag - case !fromSet && !fromEnv && pc.configIgnoreUndefinedFlags: + case !pc.configKeyIgnoreEnvVars && fromEnv: // found in the environment + switch len(envFlags) { + case 0: + panic(fmt.Errorf("invalid env flag state for %s", name)) + case 1: + target = envFlags[0] + default: + return fmt.Errorf("%s: %w", name, ErrAmbiguousFlag) + } + case pc.configIgnoreUndefinedFlags: // not found, but that's OK return nil - case !fromSet && !fromEnv && !pc.configIgnoreUndefinedFlags: + case !pc.configIgnoreUndefinedFlags: // not found, and that's not OK return fmt.Errorf("%s: %w", name, ErrUnknownFlag) + default: + panic(fmt.Errorf("unexpected unreachable case for %s", name)) } // If the flag was already provided by commandline args or @@ -274,19 +290,34 @@ var envVarSeparators = strings.NewReplacer( "/", "_", ) -func getEnvVarKey(flagName, envVarPrefix string) string { +func getEnvVarKeys(f Flag, pc ParseContext) []string { + var keys []string + if longName, ok := f.GetLongName(); ok { + keys = append(keys, getEnvVarKey(longName, pc)) + } + if shortName, ok := f.GetShortName(); ok && pc.envVarShortNames { + keys = append(keys, getEnvVarKey(string(shortName), pc)) + } + return keys +} + +func getEnvVarKey(flagName string, pc ParseContext) string { var key string key = flagName key = strings.TrimLeft(key, "-") - key = strings.ToUpper(key) key = envVarSeparators.Replace(key) - key = maybePrefix(key, envVarPrefix) + if pc.envVarCaseSensitive { + key = maybePrefix(key, pc.envVarPrefix) + } else { + key = maybePrefix(key, strings.ToUpper(pc.envVarPrefix)) + key = strings.ToUpper(key) + } return key } func maybePrefix(key string, prefix string) string { if prefix != "" { - key = strings.ToUpper(prefix) + "_" + key + key = prefix + "_" + key } return key } diff --git a/parse_test.go b/parse_test.go index da0e4c9..f91362b 100644 --- a/parse_test.go +++ b/parse_test.go @@ -37,7 +37,7 @@ func TestParse(t *testing.T) { { Name: "env only", Environment: map[string]string{"TEST_PARSE_S": "baz", "TEST_PARSE_F": "0.99", "TEST_PARSE_D": "100s"}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "baz", F: 0.99, D: 100 * time.Second}, }, { @@ -50,14 +50,14 @@ func TestParse(t *testing.T) { Name: "env args", Environment: map[string]string{"TEST_PARSE_S": "should be overridden", "TEST_PARSE_B": "true"}, Args: []string{"-s", "explicit wins", "-i", "7"}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "explicit wins", I: 7, B: true}, }, { Name: "file env", ConfigFile: "testdata/3.conf", Environment: map[string]string{"TEST_PARSE_S": "env takes priority", "TEST_PARSE_B": "true"}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "env takes priority", I: 99, B: true, D: 34 * time.Second}, }, { @@ -65,7 +65,7 @@ func TestParse(t *testing.T) { ConfigFile: "testdata/4.conf", Environment: map[string]string{"TEST_PARSE_S": "from env", "TEST_PARSE_I": "300", "TEST_PARSE_F": "0.15", "TEST_PARSE_B": "true"}, Args: []string{"-s", "from arg", "-i", "100"}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "from arg", I: 100, F: 0.15, B: true, D: time.Minute}, }, { @@ -83,62 +83,62 @@ func TestParse(t *testing.T) { ConfigFile: "testdata/5.conf", Environment: map[string]string{"TEST_PARSE_S": "s.env", "TEST_PARSE_X": "x.env.1"}, Args: []string{"-s", "s.arg.1", "-s", "s.arg.2", "-x", "x.arg.1", "-x", "x.arg.2"}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "s.arg.2", X: []string{"x.arg.1", "x.arg.2"}}, // highest prio wins and no others are called }, { Name: "WithEnvVars", Environment: map[string]string{"S": "xxx", "F": "9.87"}, - Options: []ff.Option{ff.WithEnvVars()}, + Options: []ff.Option{ff.WithEnvVars(), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "xxx", F: 9.87}, }, { Name: "WithEnvVars prefix", Environment: map[string]string{"TEST_PARSE_S": "foo", "S": "bar"}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "foo"}, }, { Name: "WithEnvVars no prefix", Environment: map[string]string{"TEST_PARSE_S": "foo", "S": "bar"}, - Options: []ff.Option{ff.WithEnvVars()}, + Options: []ff.Option{ff.WithEnvVars(), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "bar"}, }, { Name: "WithEnvVarSplit", Environment: map[string]string{"TEST_PARSE_S": "one,two,three", "TEST_PARSE_X": "one,two,three"}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarSplit(",")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames(), ff.WithEnvVarSplit(",")}, Want: fftest.Vars{S: "three", X: []string{"one", "two", "three"}}, }, { Name: "env default comma behavior", Environment: map[string]string{"TEST_PARSE_S": "one,two,three", "TEST_PARSE_X": "one,two,three"}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "one,two,three", X: []string{"one,two,three"}}, }, { Name: "env var split comma whitespace", Environment: map[string]string{"TEST_PARSE_S": "one, two, three ", "TEST_PARSE_X": "one, two, three "}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarSplit(",")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames(), ff.WithEnvVarSplit(",")}, Want: fftest.Vars{S: " three ", X: []string{"one", " two", " three "}}, }, { Name: "env var split escaping", Environment: map[string]string{"TEST_PARSE_S": `a\,b`, "TEST_PARSE_X": `one,two\,three`}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarSplit(",")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames(), ff.WithEnvVarSplit(",")}, Want: fftest.Vars{S: `a,b`, X: []string{`one`, `two,three`}}, }, { Name: "env var split escaping multichar", Environment: map[string]string{"TEST_PARSE_S": `a\xxb`, "TEST_PARSE_X": `onexxtwo\xxthree`}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarSplit("xx")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames(), ff.WithEnvVarSplit("xx")}, Want: fftest.Vars{S: `axxb`, X: []string{`one`, `twoxxthree`}}, }, { Name: "env var provided but empty", Default: fftest.Vars{S: "non-empty-default", I: 42, F: 1.23}, Environment: map[string]string{"TEST_PARSE_S": ""}, - Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE")}, + Options: []ff.Option{ff.WithEnvVarPrefix("TEST_PARSE"), ff.WithEnvVarShortNames()}, Want: fftest.Vars{S: "", I: 42, F: 1.23}, }, }