Viper: Unmarshal non-bound environment variables

[sagikazarmark]

This is going to be an umbrella issue for other open issues for this problem.

These are:

• #725
• #688
• #584
• #212
• #188
• #140

Problem

Currently Viper cannot unmarshal automatically loaded environment variables.

Here is a failing test case reproducing the issue:

package viper

import (
    "os"
    "testing"
)

func TestUnmarshal_AutomaticEnv(t *testing.T) {
    os.Setenv("MYKEY", "myvalue")

    type Config struct {
        MyKey string `mapstructure:"mykey"`
    }

    var c Config

    AutomaticEnv()

    if err := Unmarshal(&c); err != nil {
        t.Fatal(err)
    }

    if c.MyKey != "myvalue" {
        t.Error("failed to unmarshal automatically loaded environment variable")
    }

    os.Clearenv()
}

Root cause

Automatic environment variable loading works as follows:

1. User requests a value from Viper: viper.GetString("mykey")
2. Viper runs the env key replacer on the key
3. Viper compares the key (case insensitively) with the list of environment variables
4. Returns the value if one is found

Unmarshal, however, doesn't work by requesting a key, quite the opposite: it tries to unmarshal existing keys onto the given structure. Since automatically loaded environment variables are not in the list of existing keys (unlike defaults and bind env vars), these variables won't be unmarshaled.

Workaround

1. Use BindEnv to manually bind environment variables
2. Use SetDefault to set a default value (even an empty one), so Viper can find the key and try to find a matching env var

Possible solutions

The linked issues list a number of workarounds to fix this issue.

Personally, I'm in favor of the following:

1. Unmarshal the given struct onto a map[string]interface{}. That will give use a nested structure of all the keys otherwise mapstructure expects.
2. Flatten the structure, so we get a list of expected keys
3. Check that every key is defined. If not, check if there is a matching environment variable.
4. Merge the available settings with the matching, automatically loaded environment variables.
5. Use mapstructure as usual

Although I think that this is currently the best available solution, it doesn't mean we can't do better, so please do suggest if you have an idea.

[ktong]

Below is a wapper to unmarshal config with environment variables.

func Unmarshal(v View, key string, rawVal interface{}) error {
    cfg := v.(*viper.Viper)
    bindEnvs(cfg, key, rawVal)

    return cfg.UnmarshalKey(key, rawVal)
}

// Workaround because viper does not treat env vars the same as other config.
// See https://github.com/spf13/viper/issues/761.
func bindEnvs(cfg *viper.Viper, key string, rawVal interface{}) {
    for _, k := range allKeys(key, rawVal) {
        val := cfg.Get(k)
        cfg.Set(k, val)
    }
}

func allKeys(key string, rawVal interface{}) []string {
    b, err := yaml.Marshal(
        map[string]interface{}{
            key: rawVal,
        },
    )
    if err != nil {
        return nil
    }

    v := viper.New()
    v.SetConfigType("yaml")
    if err := v.ReadConfig(bytes.NewReader(b)); err != nil {
        return nil
    }

    return v.AllKeys()
}

[celian-garcia]

Hi guys, do you have a plan to support it ? I struggled with this as well and I used a solution inspired from one given by @krak3n but supporting pointers and ,squash feature of mapstructure.

Would be cool if mapstructure was exposing the feature of giving a tree of names based on the mapstructure tags. Then it would be easy to browse the tree and call the viper.BindEnv function with all the leaves.

For the moment, here is the solution I mentioned:

// bindenv: workaround to make the unmarshal work with environment variables
// Inspired from solution found here : https://github.com/spf13/viper/issues/188#issuecomment-399884438
func (b *viperConfigBuilder) bindenvs(iface interface{}, parts ...string) {
    ifv := reflect.ValueOf(iface)
    if ifv.Kind() == reflect.Ptr {
        ifv = ifv.Elem()
    }
    for i := 0; i < ifv.NumField(); i++ {
        v := ifv.Field(i)
        t := ifv.Type().Field(i)
        tv, ok := t.Tag.Lookup("mapstructure")
        if !ok {
            continue
        }
        if tv == ",squash" {
            b.bindenvs(v.Interface(), parts...)
            continue
        }
        switch v.Kind() {
        case reflect.Struct:
            b.bindenvs(v.Interface(), append(parts, tv)...)
        default:
            b.v.BindEnv(strings.Join(append(parts, tv), "."))
        }
    }
}

[iamolegga]

Hi all, in case someone need ready-to-use-package for this feature, I've just created one based on @celian-garcia and @krak3n examples, thank you guys!

[celian-garcia]

Thanks @iamolegga, I will use it :)

[segevfiner]

If mapstructure either returned metadata about fields not unmarshalled into in the target struct (Rather then the source), or otherwise had a callback to handle such fields, it could have been an easy way for viper to fix this. Otherwise using reflection in a similar way to what mapstructure does to handle the struct seems like the most viable approach, though it will have to correctly handle the various mapstructure tags. You can find various snippets of code doing just that around the issue tracker and the internet. Even then, it still won't work with dynamic parts of the struct, e.g. map[string]interface{} and so on, without further cleverness from viper rather then it just doing something akin to BindEnv.

Serializing the struct won't work correctly with omitempty.

[Kafei59]

I've also use a custom method for binding env vars without config file nor flags like so:

import (
    "github.com/fatih/structs"
    "github.com/jeremywohl/flatten"
)

func SetupConfigFromEnv() error {
    // Setup automatic env binding
    viper.AutomaticEnv()
    viper.SetEnvPrefix("")
    viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))

    // Transform config struct to map
    confMap := structs.Map(Config{})

    // Flatten nested conf map
    flat, err := flatten.Flatten(confMap, "", flatten.DotStyle)
    if err != nil {
        return errors.Wrap(err, "Unable to flatten config")
    }

    // Bind each conf fields to environment vars
    for key, _ := range flat {
        err := viper.BindEnv(key)
        if err != nil {
            return errors.Wrapf(err, "Unable to bind env var: %s", key)
        }
    }

    return nil
}

It minimises "home-made" custom code but it has to use 2 other librairies to work.

[Blquinn]

@sagikazarmark Any news on this? Is it just a matter of a pull request?

[nikunjy]

Any update here ? Having to use BindEnv doesn't make sense. I have to write a constant in the mapstructure and specify the same string in BindEnv

[sagikazarmark]

Here is a rough solution:

type Config struct {
    MyKey string `mapstructure:"mykey"`
}

// extract the keys form a struct and flatten them using a dot separator
keys := viper.ExtractKeysFromStruct(Config{}, ".")

// define keys in Viper
// this would essentially let Viper know about a key without setting a default or any value for it
// since these keys would not have a value initially, IsSet would return false, AllKeys would not return them
// AutomaticEnv detection could use these keys though
viper.Define(keys...)

// or
// BindEnvs would be the same as BindEnv for multiple keys
// though this would render AutomaticEnv less useful when used with Unmarshal
viper.BindEnvs(keys...)

It would use mapstructure under the hood, so that it can stay close to the unmarshal behavior.

Cases when it could fall short:

type Config struct {
    MyKey string `mapstructure:"mykey"`

    // The default value here is nil, so fields of the config would probably not show up by default?
    SubConfig *SubConfig
}

type SubConfig struct {
    MyKey string `mapstructure:"mykey"`
}

WDYT?

[segevfiner]

If you use mapstructure as it is now to implement ExtractKeysFromStruct it would miss omitempty fields. (Telling users not to use them is weird, error prone and they might want to use them, e.g. When writing the config to a file).

Personally I believe it's best if Unmarshal just handled this internally so it just works with no side effects to viper like calling BindEnvs from it.

[sagikazarmark]

@segevfiner

If you use mapstructure as it is now to implement ExtractKeysFromStruct it would miss omitempty fields. (Telling users not to use them is weird, error prone and they might want to use them, e.g. When writing the config to a file).

That's entirely correct, we would have to workaround that issue somehow. See the issue above I opened in the mapstructure repo.

Personally I believe it's best if Unmarshal just handled this internally so it just works with no side effects to viper like calling BindEnvs from it.

I think it would be very hard to do, if possible at all. The way it works currently, is Unmarshal gets a list of values from the Viper instance and tries to unmarshal them onto the struct. At this point environment variables are already evaluated.

I don't think that side effect is actually a problem, because it's exactly the same thing when you set a default for example: you define a key in one of the config sources and that key in this case would be exactly the same for all sources (if you were to call Set manually for example, to override a default or an env var for that matter, you would use this exact same key).

What BindEnvs would do is essentially telling Viper to expect these keys if it doesn't expect them already. It feels like a better approach to me, because it fits nicely into how config sources are evaluated.

[GiancarlosIO]

👀