Viper: Binding with cobra flag results on IsSet always true

[ggeorgiev]

When I bind a key to a cobra flag, when I ask IsSet I always get true, even if the flag was not set in the command line flags as well.

Do I miss something or this is a real issue?

[jasonish]

I just ran into this as well. My temporary solution is to not find the particular value I need to check IsSet on, and did a custom function that first checks if the flag is set, then checks if the viper.IsSet is set on it. IMO, viper.IsSet() is not doing the right thing here.

[stephenrlouie]

I saw this problem too! isSet() returns true as soon as I Bind a flag to viper.

I used viper in the past and believe its behavior has changed. It used to return false if you just bound the flag and no actual value was provided by an input (config file, command line, or env). It would return true only if one of these inputs were provided. IMO this is the appropriate behavior since a flag usually requires a value by nature of being a flag, but I wouldn't consider that "set".

My workaround is to substitute isSet for a zero value check. (I could also set the defaults when I declare the flags)

[jasonish]

I used viper in the past and believe its behavior has changed.

I believe you are correct. It looks like e3bc06f20c7a60e8042d3f8741551ce606d31c40 added the behaviour you would expect, then ec4eb2fa8549869ae7a2accd4fcc83d1c0555c15 broke it again.

@benoitmasson Is there a reason IsSet() behaviour changed with this commit? Or was it an unintended side affect?

[benoitmasson]

Hi,

Sorry about that, this is clearly a side effect of my changes. And there should be a test for this specific case (or TestIsSet() should be extended to use flags).

However, the old code

       if val == nil {
               source := v.find(strings.ToLower(path[0]))
               if source != nil {
                       if reflect.TypeOf(source).Kind() == reflect.Map {
                               val = v.searchMap(cast.ToStringMap(source), path[1:])
                       }
               }
       }

cannot be restored as such, because it does not work properly with nested values (the source map may not be the one where the value is really, example in this post).

I'll have a look and see if I can do something.

[benoitmasson]

I added some tests to TestIsSet(), but everything works as expected:

        v.ReadConfig(bytes.NewBuffer(yamlExample)) // defines "eyes"

    v.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
    v.BindEnv("eyes")
    v.BindEnv("foo")
    v.BindEnv("clothing.hat")
    v.BindEnv("clothing.hats")
    os.Setenv("FOO", "bar")
    os.Setenv("CLOTHING_HAT", "bowler")

    assert.True(t, v.IsSet("eyes"))           // set in the config file
    assert.True(t, v.IsSet("foo"))            // set in the environment
    assert.True(t, v.IsSet("clothing.hat"))   // set in the environment
    assert.False(t, v.IsSet("clothing.hats")) // bound but not set

ok      github.com/spf13/viper  0.014s

Did I get something wrong? Or could you be more precise about the issue you are facing?

[jasonish]

How about this:

func TestIsSetFailing(t *testing.T) {
    v := New()

    flagset := pflag.NewFlagSet("testisset", pflag.ExitOnError)
    flagset.Bool("foo", false, "foo")
    v.BindPFlag("foo", flagset.Lookup("foo"))
    err := flagset.Parse([]string{})
    assert.Nil(t, err)

    // Here I would expect IsSet to be false...
    assert.False(t, v.IsSet("foo"))
}

It seems to be tied to binding a flag.

[benoitmasson]

Ooops, I must be tired, I read “flags” and tested “environment” :-( Thanks for the precision.

I see where this comes from now. If a flag is bound to Viper, a default value is automatically assigned to it, hence Get() always returns a value (the default one if not defined anywhere). IsSet() follows this behavior, but I agree this is not desired in this precise case.

I will look for a fix (probably inside the Get() and find() functions, I like the IsSet() one as it is) and submit a PR as soon as possible.

[ggeorgiev]

@benoitmasson thank you for fixing this, it is causing me to use some workarounds that I can get rid of.

[zyuyou]

331 Still not accepted?

[jacobcase]

Any idea when #331 will get accepted?