Skip to content

Commit 2f64589

Browse files
authored
Merge pull request #2451 from abo3losh1/fix/empty-env-source-not-set
fix: skip empty source values for flags that cannot parse them Tested-by: Eng Zer Jun <engzerjun@gmail.com> Reviewed-by: Eng Zer Jun <engzerjun@gmail.com>
2 parents f6f38ed + 72a6466 commit 2f64589

3 files changed

Lines changed: 138 additions & 8 deletions

File tree

‎docs/v3/examples/flags/value-sources.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,11 @@ func main() {
136136
Note that default values are set in the same order as they are defined in the
137137
`Sources` param. This allows the user to choose order of priority
138138

139+
An empty value from any source, such as an environment variable that is set but empty
140+
or an empty file, only counts as a value for string and bool flags, where it means an
141+
empty string and `false`. For any other flag it is skipped, so it does not satisfy
142+
`Required`, does not make `IsSet` true, and does not hide a later source.
143+
139144
#### Values from alternate input sources (YAML, TOML, and others)
140145

141146
There is a separate package [altsrc](https://github.com/urfave/cli-altsrc) that adds support for getting flag values

‎flag_impl.go‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -135,13 +135,24 @@ func (f *FlagBase[T, C, V]) PostParse() error {
135135
tracef("postparse (flag=%[1]q)", f.Name)
136136

137137
if !f.hasBeenSet {
138-
if val, source, found := f.Sources.LookupWithSource(); found {
139-
// reflect.TypeOf yields nil when T is an interface type (e.g.
140-
// GenericFlag) and the value is nil, so the kind has to be
141-
// derived defensively.
142-
kind := reflect.Invalid
143-
if ty := reflect.TypeOf(f.Value); ty != nil {
144-
kind = ty.Kind()
138+
// reflect.TypeOf yields nil when T is an interface type (e.g.
139+
// GenericFlag) and the value is nil, so the kind has to be
140+
// derived defensively.
141+
kind := reflect.Invalid
142+
if ty := reflect.TypeOf(f.Value); ty != nil {
143+
kind = ty.Kind()
144+
}
145+
146+
// An empty value is a value only for a string, and reads as false
147+
// for a bool. Any other kind has nothing to parse from it, so an
148+
// empty source is skipped: it neither marks the flag as set nor
149+
// hides a later source in the chain.
150+
emptyIsValue := kind == reflect.String || kind == reflect.Bool
151+
152+
for _, source := range f.Sources.Chain {
153+
val, found := source.Lookup()
154+
if !found || (val == "" && !emptyIsValue) {
155+
continue
145156
}
146157

147158
if val != "" || kind == reflect.String {
@@ -151,12 +162,13 @@ func (f *FlagBase[T, C, V]) PostParse() error {
151162
val, f.Value, source, f.Name, err,
152163
)
153164
}
154-
} else if val == "" && kind == reflect.Bool {
165+
} else {
155166
_ = f.Set(f.Name, "false")
156167
}
157168

158169
f.hasBeenSet = true
159170
f.fromSource = true
171+
break
160172
}
161173
}
162174

‎flag_test.go‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"flag"
77
"fmt"
88
"os"
9+
"path/filepath"
910
"reflect"
1011
"regexp"
1112
"runtime"
@@ -668,6 +669,118 @@ func TestStringFlagDefaultText(t *testing.T) {
668669
assert.Equal(t, expected, fl.String())
669670
}
670671

672+
// An empty value from a source can only be a value for a string, and reads as
673+
// false for a bool. For any other kind there is nothing to parse, so the source
674+
// must not count as setting the flag: that would satisfy Required with a zero
675+
// value, report IsSet for a flag that only holds its default, and hide a later
676+
// source in the chain that does carry a value.
677+
func TestFlagsFromEmptyEnv(t *testing.T) {
678+
testCases := []struct {
679+
name string
680+
env map[string]string
681+
fl Flag
682+
wantValue any
683+
wantIsSet bool
684+
errContains string
685+
}{
686+
{
687+
name: "required int from an empty env var is not set",
688+
env: map[string]string{"PORT": ""},
689+
fl: &IntFlag{Name: "port", Required: true, Sources: EnvVars("PORT")},
690+
errContains: `Required flag "port" not set`,
691+
},
692+
{
693+
name: "required duration from an empty env var is not set",
694+
env: map[string]string{"TIMEOUT": ""},
695+
fl: &DurationFlag{Name: "timeout", Required: true, Sources: EnvVars("TIMEOUT")},
696+
errContains: `Required flag "timeout" not set`,
697+
},
698+
{
699+
name: "int keeps its default and is not set",
700+
env: map[string]string{"PORT": ""},
701+
fl: &IntFlag{Name: "port", Value: 80, Sources: EnvVars("PORT")},
702+
wantValue: 80,
703+
},
704+
{
705+
name: "string slice keeps its default and is not set",
706+
env: map[string]string{"TAGS": ""},
707+
fl: &StringSliceFlag{Name: "tags", Value: []string{"a"}, Sources: EnvVars("TAGS")},
708+
wantValue: []string{"a"},
709+
},
710+
{
711+
name: "an empty env var does not hide a later one",
712+
env: map[string]string{"LEGACY_PORT": "", "PORT": "8080"},
713+
fl: &IntFlag{Name: "port", Value: 80, Sources: EnvVars("LEGACY_PORT", "PORT")},
714+
wantValue: 8080,
715+
wantIsSet: true,
716+
},
717+
{
718+
name: "an unparsable env var still fails before a later one",
719+
env: map[string]string{"LEGACY_PORT": "abc", "PORT": "8080"},
720+
fl: &IntFlag{Name: "port", Sources: EnvVars("LEGACY_PORT", "PORT")},
721+
errContains: `could not parse "abc" as int value from environment variable "LEGACY_PORT"`,
722+
},
723+
{
724+
name: "an empty string is still a value",
725+
env: map[string]string{"NAME": ""},
726+
fl: &StringFlag{Name: "name", Value: "def", Sources: EnvVars("NAME")},
727+
wantValue: "",
728+
wantIsSet: true,
729+
},
730+
{
731+
name: "an empty bool still reads as false",
732+
env: map[string]string{"DEBUG": ""},
733+
fl: &BoolFlag{Name: "debug", Value: true, Sources: EnvVars("DEBUG")},
734+
wantValue: false,
735+
wantIsSet: true,
736+
},
737+
}
738+
739+
for _, tc := range testCases {
740+
t.Run(tc.name, func(t *testing.T) {
741+
for k, v := range tc.env {
742+
t.Setenv(k, v)
743+
}
744+
745+
name := tc.fl.Names()[0]
746+
var gotValue any
747+
var gotIsSet bool
748+
cmd := &Command{
749+
Flags: []Flag{tc.fl},
750+
Action: func(_ context.Context, cmd *Command) error {
751+
gotValue = cmd.Value(name)
752+
gotIsSet = cmd.IsSet(name)
753+
return nil
754+
},
755+
}
756+
757+
err := cmd.Run(buildTestContext(t), []string{"run"})
758+
if tc.errContains != "" {
759+
require.ErrorContains(t, err, tc.errContains)
760+
return
761+
}
762+
763+
require.NoError(t, err)
764+
assert.Equal(t, tc.wantValue, gotValue)
765+
assert.Equal(t, tc.wantIsSet, gotIsSet)
766+
})
767+
}
768+
}
769+
770+
// The skip applies to every source in the chain, not only to environment
771+
// variables, so an empty file is treated the same way as an empty variable.
772+
func TestFlagFromEmptyFileSource(t *testing.T) {
773+
path := filepath.Join(t.TempDir(), "port")
774+
require.NoError(t, os.WriteFile(path, nil, 0o600))
775+
776+
cmd := &Command{
777+
Flags: []Flag{&IntFlag{Name: "port", Required: true, Sources: Files(path)}},
778+
}
779+
780+
err := cmd.Run(buildTestContext(t), []string{"run"})
781+
require.ErrorContains(t, err, `Required flag "port" not set`)
782+
}
783+
671784
func TestStringFlagWithEnvVarHelpOutput(t *testing.T) {
672785
t.Setenv("APP_FOO", "derp")
673786

0 commit comments

Comments
 (0)