Viper: StringToString pflag values are not handled correctly by viper

[terev]

I bind a StringToString pflag like so

RootCmd.PersistentFlags().StringToStringP(key, conf.alias, defaultValue, conf.usage)
viper.BindPFlag(key, RootCmd.PersistentFlags().Lookup(key))

Then retrieve the value from my viper with

smap := viper.GetStringMapString(key)

The string map returned is an empty map, which seems to be the default value returned on error by spf13/cast.

I expected to get a populated string map back.

[terev]

This issue seems related to what was seen in https://github.com/spf13/viper/issues/200

[maskeda]

It looks like viper is fixed, can someone update viper?
https://github.com/spf13/viper/issues/200

[maskeda]

Created a PR, https://github.com/sensu/sensu-go/pull/2867

[kd7lxl]

I made a demo of this issue: https://github.com/kd7lxl/viper-demo

[danmx]

it seems that in v1.6.2 this issue is fixed

[amir20]

I tried it @danmx and still doesn't work. Using filters = viper.GetStringMapString("filter") with --filter name=test outputs "DEBUG filterArgs = {map[]}".

[terev]

@danmx @amir20 I've submitted a pr with a fix for this issue. Hopefully someone will have a look at some point.

[danmx]

@terev here you have how the input works:
key_a=value_1,key_b=value_2
https://github.com/spf13/pflag/blob/d929dcb/string_to_string_test.go#L143-L144

@amir20 did you add this to your code:

serverCmd.Flags().Int("filter", map[string]string{}, "desc filter")
if err := viper.BindPFlag("filter", serverCmd.Flags().Lookup("filter")); err != nil {
    return err
}

https://github.com/spf13/viper#working-with-flags

[matevz]

The issue still persists in v1.6.2.

This:

f := pflag.NewFlagSet("", pflag.ContinueOnError)
f.StringToString("filter", map[string]string{}, "desc filter")
_ = viper.BindPFlags(f)
...
filters := viper.GetStringMapString("filter")
fmt.Println(filters)

prints map[].

While using just GetString:

filters := viper.GetString("filter")

prints [a=0].

[amir20]

@matevz is correct here. This is still broken.

@danmx Not sure why you have an Int for pflags. You need to use StringToString to reproduce this. Can you try @matevz code? I get map[] too. Maybe you are doing something different.

[danmx]

@amir20 that was a poor copy-paste of code snippets from my tool.

I wrote a simple cli just to test it and you're right also now I know that I have a bug in my tool 🤦‍♂

here's the code if someone what's to quickly reproduce it:

package main

import (
    "fmt"
    "os"

    "github.com/spf13/cobra"
    "github.com/spf13/viper"
)

var (
    cfg     *viper.Viper
    rootCmd = &cobra.Command{
        PreRunE: func(cmd *cobra.Command, args []string) error {
            if err := cfg.BindPFlag("test1", cmd.Flags().Lookup("test1")); err != nil {
                return err
            }
            if err := cfg.BindPFlag("test2", cmd.Flags().Lookup("test2")); err != nil {
                return err
            }
            return nil
        },
        RunE: func(cmd *cobra.Command, args []string) error {
            a := cfg.GetStringMapString("test1")
            fmt.Printf("GetStringMapString - T: %T, v: %v, s: %s\n", a, a, a)
            b := cfg.GetStringMap("test1")
            fmt.Printf("GetStringMap - T: %T, v: %v, s: %s\n", b, b, b)
            c := cfg.Get("test1")
            fmt.Printf("Get - T: %T, v: %v, s: %s\n", c, c, c)
            arg2 := cfg.GetString("test2")
            fmt.Printf("GetString - T: %T, v: %v, s: %s\n", arg2, arg2, arg2)
            return nil
        },
    }
)

func init() {
    cfg = viper.GetViper()
    rootCmd.Flags().StringToString("test1", map[string]string{"default_key": "default_value"}, "test description")
    rootCmd.Flags().String("test2", "default_string", "test description")
}

func main() {
    if err := rootCmd.Execute(); err != nil {
        os.Exit(1) //nolint:gomnd
    }
}

$ go build -o main

$ ./main --test1 a=b,c=d
GetStringMapString - T: map[string]string, v: map[], s: map[]
GetStringMap - T: map[string]interface {}, v: map[], s: map[]
Get - T: string, v: [c=d,a=b], s: [c=d,a=b]
GetString - T: string, v: default_string, s: default_string

[terev]

@amir20 @danmx if you wouldn't mind could you try my fork? i linked the pr above

[danmx]

$ ./main --test1 a=b,c=d
GetStringMapString - T: map[string]string, v: map[a:b c:d], s: map[a:b c:d]
GetStringMap - T: map[string]interface {}, v: map[], s: map[]
Get - T: map[string]string, v: map[a:b c:d], s: map[a:b c:d]
GetString - T: string, v: default_string, s: default_string

@terev it seems to work only for GetStringMapString

[terev]

@danmx to me that output looks like Get also worked

[danmx]

@terev you're right 😂 I only glimpsed over GetStringMap (which is still broken) and GetStringMapString

[terev]

@amir20 @danmx pushed some changes https://github.com/spf13/viper/pull/874. I was able to get GetStringMap working,

GetStringMapString - T: map[string]string, v: map[a:b c:d], s: map[a:b c:d]
GetStringMap - T: map[string]interface {}, v: map[a:b c:d], s: map[a:b c:d]
Get - T: map[string]interface {}, v: map[a:b c:d], s: map[a:b c:d]
GetString - T: string, v: default_string, s: default_string

[amir20]

This _still_ doesn't work. :( @sagikazarmark I tried testing with EXAMPLE_FOO=bar=test go run main.go and it still fails.

After reading and using debugger, I see that the code eventually gets to https://github.com/spf13/cast/blob/master/cast.go#L108-L110.

func ToStringMapString(i interface{}) map[string]string {
    v, _ := ToStringMapStringE(i)
    return v
}

ToStringMapStringE returns an error because it cannot cast bar=test using JSON. :/

I can create a new bug or reopen this.

[sagikazarmark]

@amir20 This issue (and the fix) is for pflag values, not env vars. So I think it should definitely be a new feature request.

[amir20]

Interesting. That's confusing. This whole time I thought viper.GetStringMapString(key) is going to fix both env and pFalgs use case. I'd argue that as a developer it shouldn't be a different feature to know the source.

Regardless, not a big deal. I'll create a new bug.