Implement viper.BindStruct for automatic unmarshalling from environment variables

[krakowski]

Description

The current viper implementation does not allow to viper.Unmarshal environment variables into structs without binding them first (using viper.BindEnv). This behavior results in an "empty" struct (i.e no values filled) if no configuration file is provided with matching keys.

This pull request adds a new viper.BindStruct function which takes a struct pointer and binds each (nested) struct field to an environment variable using the struct field's name or the user-specified mapstructure tag. The implementation is quite simple, since I could reuse some functions:

func (v *Viper) BindStruct(input interface{}) error {
	envKeysMap := map[string]interface{}{}
	if err := mapstructure.Decode(input, &envKeysMap); err != nil {
		return err
	}

	structKeys := v.flattenAndMergeMap(map[string]bool{}, envKeysMap, "")
	for key, _ := range structKeys {
		if err := v.BindEnv(key); err != nil {
			return err
		}
	}

	return nil
}

A simple example could look like this:

type Configuration struct {
    Value int
}

os.Setenv("VALUE", 42)

var config Configuration
viper.BindStruct(&config)
viper.Unmarshal(&config)

Please see the TestBindStruct unit test for a more complete example.

Related Issues

• Fixes Consider env vars when unmarshalling #188
• Fixes Does viper require a config file in order to use ENV variables? #584
• Fixes Unmarshal using ENV without binding #688
• Fixes Commented out property in YAML yields inconsistencies between Get and Unmarshal when overriding using environment variables #725
• Fixes Unmarshal non-bound environment variables #761
• Fixes UnmarshalKey / Sub only uses read config, neglects environment variables  #1012
• Fixes Viper does not unmarshal values passed from env or pflag #1365
• Fixes AutomaticEnv function failing to read environment variables in Go code #1522
• Fixes The env attribute cannot be specified as an attribute of a struct #1545
• Fixes Use environment variables with no need to have config file (yml,josn,etc...) #1587

[CLAassistant]

All committers have signed the CLA.

[github-actions]

👋 Thanks for contributing to Viper! You are awesome! 🎉

A maintainer will take a look at your pull request shortly. 👀

In the meantime: We are working on Viper v2 and we would love to hear your thoughts about what you like or don't like about Viper, so we can improve or fix those issues.

⏰ If you have a couple minutes, please take some time and share your thoughts: https://forms.gle/R6faU74qPRPAzchZ9

📣 If you've already given us your feedback, you can still help by spreading the news,
either by sharing the above link or telling people about this on Twitter:

https://twitter.com/sagikazarmark/status/1306904078967074816

Thank you! ❤️

[sagikazarmark]

Thanks for sending this PR!

I'll try to prioritize reviewing it, especially since it resolves a long standing problem.

[swengineer404]

Can we get an update on this?

[bkubiak]

@sagikazarmark It would be great to get this PR merged. Can you check it, please?

[na-ji]

Hello,

I tried this PR quickly on my personal configuration, and can tell that it does not work with slices/arrays. It might be a problem with mapstructure though.

[krakowski]

Hi @na-ji,

There are test cases with slices included in this PR which work fine. See here for example. What exactly does not work in your case?

[na-ji]

Hi @na-ji,

There are test cases with slices included in this PR which work fine. See here for example. What exactly does not work in your case?

Hi, sorry for not adding an example. I will try to add one later. I was talking about slices of structs. This kind of structure is fully supported on other formats, like JSON, Yaml, Toml etc. But I don't know if it will be with environment variables.

[heraju]

Any update on this would be appreciated, is there any work around on this, stuck with deployment on this now :(

[maufranchini]

Any update on this issue? I'm currently stuck with deployment due to this problem. Is there a potential workaround?

One approach that has worked for me is to utilize the BindEnv function provided by Viper.

For instance, if you have a configuration key server.env and you're setting an environment variable named SERVER_ENV, you can link them using:

viper.BindEnv("server.env", "SERVER_ENV")

After this binding, you should be able to access the value of the SERVER_ENV environment variable using the corresponding configuration key in your application.

[mathnogueira]

Any update on this PR? Having to add an extra BindEnv for each new config we add to the struct is error-prone. This new function would help a lot of us.

[sagikazarmark]

Apologies for the late review everyone.

Using BindEnv as a workaround sounds OK, but maybe we could come up with a better solution specifically for Unmarshal.

The original reason why Unmarshal does not work with AutomaticEnv is this: internally, Unmarshall gets a list of keys already registered in Viper and builds the input for mapstructure from those keys.

Implementation: https://github.com/spf13/viper/blob/master/viper.go#L2098

Here is what I propose:

• BindStruct should register a list of keys in an internal key list (deduplicated)
• When Unmarshal calls AllSettings (and transitively GetKeys), it should merge the bound list with the available keys

Practically, we probably need an internal implementation of AllSettings that does that just for Unmarshal (we may be able to merge those back together later, but technically it could be a BC break since those keys don't necessarily have any values).

I'm a little weary to accept this PR as it is, because I don't think BindEnv is the right solution, it's rather a workaround.

I'm happy to give this a go in the next coming weeks, but if anyone wants to pick it up first, I'm happy to review PRs too.

[mathnogueira]

Hey @sagikazarmark ! I just opened a PR to move the implementation out of a new method to the viper.Unmarshal function. I would love your review on that.

[krakowski]

Hey @sagikazarmark,

thanks for taking a look at this! Just want to make sure I fully understand what you propose.

• BindStruct should register a list of keys in an internal key list (deduplicated)
• When Unmarshal calls AllSettings (and transitively GetKeys), it should merge the bound list with the available keys

Do you mean BindStruct should create a list of keys here?

viper/viper.go

Lines 210 to 216 in c4dcd31

	config map[string]any
	override map[string]any
	defaults map[string]any
	kvstore map[string]any
	pflags map[string]FlagValue
	env map[string][]string
	aliases map[string]string

If so, this would mean those keys need to be merged with existing keys according to some priority within AllKeys.

viper/viper.go

Lines 2018 to 2035 in c4dcd31

	func (v *Viper) AllKeys() []string {
	m := map[string]bool{}
	// add all paths, by order of descending priority to ensure correct shadowing
	m = v.flattenAndMergeMap(m, castMapStringToMapInterface(v.aliases), "")
	m = v.flattenAndMergeMap(m, v.override, "")
	m = v.mergeFlatMap(m, castMapFlagToMapInterface(v.pflags))
	m = v.mergeFlatMap(m, castMapStringSliceToMapInterface(v.env))
	m = v.flattenAndMergeMap(m, v.config, "")
	m = v.flattenAndMergeMap(m, v.kvstore, "")
	m = v.flattenAndMergeMap(m, v.defaults, "")
	// convert set of paths to list
	a := make([]string, 0, len(m))
	for x := range m {
	a = append(a, x)
	}
	return a
	}

Since both BindStruct and BindEnv are used for binding environment variables, I'm not really sure which should be prioritized at this point. Do you have some thoughts on this?

Also, do you have any examples in mind where internally using BindEnv is disadvantageous? That might make it clearer to me why it is supposed to be a workaround.

Thanks!

[mathnogueira]

Since @krakowski is taking a look at this. I'll close my PR for now.

[sagikazarmark]

@krakowski

Do you mean BindStruct should create a list of keys here?

viper/viper.go

Lines 210 to 216 in c4dcd31

	config map[string]any
	override map[string]any
	defaults map[string]any
	kvstore map[string]any
	pflags map[string]FlagValue
	env map[string][]string
	aliases map[string]string

Yes

If so, this would mean those keys need to be merged with existing keys according to some priority within AllKeys.

viper/viper.go

Lines 2018 to 2035 in c4dcd31

	func (v *Viper) AllKeys() []string {
	m := map[string]bool{}
	// add all paths, by order of descending priority to ensure correct shadowing
	m = v.flattenAndMergeMap(m, castMapStringToMapInterface(v.aliases), "")
	m = v.flattenAndMergeMap(m, v.override, "")
	m = v.mergeFlatMap(m, castMapFlagToMapInterface(v.pflags))
	m = v.mergeFlatMap(m, castMapStringSliceToMapInterface(v.env))
	m = v.flattenAndMergeMap(m, v.config, "")
	m = v.flattenAndMergeMap(m, v.kvstore, "")
	m = v.flattenAndMergeMap(m, v.defaults, "")
	// convert set of paths to list
	a := make([]string, 0, len(m))
	for x := range m {
	a = append(a, x)
	}
	return a
	}

I would say, BindStruct should come with the lowest priority. Any call to SetDefault, BindEnv or any other source should have a higher priority. In my mind BindStruct basically provides a schema for Viper. Any additional modification or config should have a higher priority.

Since both BindStruct and BindEnv are used for binding environment variables, I'm not really sure which should be prioritized at this point. Do you have some thoughts on this?

In my mind, BindStruct provides a schema to Viper. The reason why AutomaticEnv doesn't work with Unmarshal today is because internally Unmarshal grabs all keys defined in Viper and maps those to the provided struct. Currently, only AutomaticEnv suffers from this, but in the future there might be other supported config sources that may not load all keys upfront.

Also, do you have any examples in mind where internally using BindEnv is disadvantageous? That might make it clearer to me why it is supposed to be a workaround.

See my answer above. Currently only AutomaticEnv is the problematic part.

---

Do you mean BindStruct should create a list of keys here?

On second thought, I'm thinking maybe we should merge BindStruct's functionality into Unmarshal. Instead of a separate BindStruct call, we could just do all that within Unmarshal. We wouldn't have to store the keys in Viper either.

[krakowski]

On second thought, I'm thinking maybe we should merge BindStruct's functionality into Unmarshal. Instead of a separate BindStruct call, we could just do all that within Unmarshal. We wouldn't have to store the keys in Viper either.

I will refactor this PR today to reflect this change making BindStruct a private function (bindStruct) inside viper and calling it from Unmarshal 👍

That way, from the view of a user, only Unmarshal has to be called. This is what most people expect looking at the issues I referenced in this PR's description.

[krakowski]

@sagikazarmark This is now implemented. Please have a look 🙂

One thing I noticed: There's also UnmarshalKey.

viper/viper.go

Lines 1103 to 1105 in c4dcd31

	func (v *Viper) UnmarshalKey(key string, rawVal any, opts ...DecoderConfigOption) error {
	return decode(v.Get(key), defaultDecoderConfig(rawVal, opts...))
	}

Calling bindStruct there would traverse the whole struct every time UnmarshalKey is called. My proposal is to call BindEnv with the user-provided key like so:

func (v *Viper) UnmarshalKey(key string, rawVal any, opts ...DecoderConfigOption) error {
	if err := v.BindEnv(key); err != nil {
		return err
	}
	
	return decode(v.Get(key), defaultDecoderConfig(rawVal, opts...))
}

What do you think?

[sagikazarmark]

I don't think we need to do this for UnmarshalKey since the key in that case is already known and is passed to the Get call which will resolve the right env var. There is no need to call BindEnv.

[sagikazarmark]

These keys should be merged with the list of AllKeys instead. No need to call BindEnv.

[sagikazarmark]

@krakowski Thanks for updating the PR!

I thought we agreed not to call BindEnv, but to change the way AllSettings is called and merge the list of keys with AllKeys.

[sagikazarmark]

I quickly made my own version: #1699

@krakowski I'd love to get your thoughts on it (if you can give it a try, even better).

[xsteadfastx]

You mean the map stored at index 0 should then contain a TITLE key with the value assigned to the EVCC_LOADPOINTS_0_TITLE environment variable?

Exactly.

Does BindEnv support this kind of unmarshal operation?

@krakowski seems it cannot, that would actually be #935.

so this doesnt work? any ideas for workarounds?

[xsteadfastx]

@sagikazarmark @krakowski this still does not unmarshal something like EVCC_LOADPOINTS_0_TITLE into []map[string]interface{}, right?

You mean the map stored at index 0 should then contain a TITLE key with the value assigned to the EVCC_LOADPOINTS_0_TITLE environment variable? I can try it out. Does BindEnv support this kind of unmarshal operation?

tried it with the latest version with no luck.

[sagikazarmark]

@xsteadfastx you should use mapstructure struct tag instead of yaml. Not sure if that's the only problem, but that is one.

[xsteadfastx]

im using mapstructure.

…

On Do, 14. Dez 05:08, Márk Sági-Kazár wrote: @xsteadfastx you should use `mapstructure` struct tag instead of `yaml`. Not sure if that's the only problem, but that is one. -- Reply to this email directly or view it on GitHub: #1429 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

[krakowski]

@maxisam It seems mapstructure does not follow pointer types when decoding fields.

type StorageConfig struct {
  Size int `mapstructure:"size"`
}

type Configuration struct {
  Storage StorageConfig `mapstructure:"filesystem"`
}

Decoding the Configuration struct using mapstructure will yield a filesystem.size key. If you change the Storage field's type to *StorageConfig (pointer type) it yields a filesystem key. The code you posted does not work, because in your case test is used as a key instead of test.env.

A simple workaround you can use is to bind this key manually using BindEnv.

...
v.AutomaticEnv()

os.Setenv("EVCC_DATABASE_TYPE", "sqlite")
os.Setenv("EVCC_DATABASE_DSN", "dsn")
os.Setenv("EVCC_TEST_ENV", "test-env")

v.BindEnv("test.env")
...

Another option is to not use pointer fields inside the configuration structs.

[maxisam]

@krakowski Thanks for explaining, I knew it must be pointer type. I think using pointer type should be pretty common, but maybe I am wrong. (I only use Golang in this project)

I also wanna point it out this is actually a breaking change. After this, we can't no longer use pointer type with environment variable. But it was fine before.

I like the idea of this change but I think the pointer type is a big gotcha and limit the usage a lot

[xsteadfastx]

im using mapstructure.

You're using yaml tags, not mapstrucure. It shouldn't matter though as long as the member name is a straightforward version of the actual config. That seems to be the case afaikt.

That said, #1429 (comment) is actually OT as was my post since it referred to slices which were/are not supported via ENV.

why do you think im using yaml? i mean i didnt event posted any code.

[sagikazarmark]

@maxisam to be precise: mapstructure doesn't support multiple indirections. In fact, it requires you to pass a pointer to your desired output, but in case of the input it only dereferences a pointer once which results in the error seen before.

I submitted a PR to mapstructure to fix that. No idea when or if it' going to be accepted.

I have another PR in Viper as a workaround: #1707

@xsteadfastx this is what makes me believe you use yaml as struct tags: https://github.com/maxisam/mgob/blob/144a21f8275314f74b9ff3a279fbfea22cda06d3/pkg/config/plan.go#L14-L29

It works, as long as you tell mapstructure to use that instead of mapstructure.

But as @andig pointed out, slices may be another issue.

[krakowski]

@krakowski Thanks for explaining, I knew it must be pointer type. I think using pointer type should be pretty common, but maybe I am wrong. (I only use Golang in this project)

I also wanna point it out this is actually a breaking change. After this, we can't no longer use pointer type with environment variable. But it was fine before.

I like the idea of this change but I think the pointer type is a big gotcha and limit the usage a lot

@maxisam The code you posted did not work before this change (or ever) either. Please see the following replit based on viper v1.17.0: https://replit.com/@krakowski1/Viper-Automatic-Env#main.go

Before this change you needed to call BindEnv for every key. Now it is required for pointer type fields only because of the way mapstructure decoding works (which is what I mentioned as a workaround).

[maxisam]

@krakowski it did work before. I had an unit test for that. I load config first and load env variable. I meant not the test code I post here but the real code in my repo.

[maxisam]

@sagikazarmark Thanks for explaining. I hope your PR will work. In the meantime I will just use <1.8.0 with a workaround.
Also you points to my repo not @xsteadfastx

[preslavrachev]

Released v1.18.0 with this change.

Has this been made publicly available? In the code, I am seeing a check, which in my case defaults to false:

const BindStruct = false

How do I turn this on within my project?

[preslavrachev]

Whoever arrives here looking for how to enable this feature, you need to add the tags=viper_bind_struct option when running go build, go run, or go test. This is rather unfortunate, as it now needs me to create makefile to ensure that my app is started correctly (unfortunately, Go does not have default build options, as to my knowledge).

You can still make your life easier by enabling those build flags in your VS Code configuration too:

{
  "gopls": {
    "build.buildFlags": ["-tags=viper_bind_struct"]
  },
  "go.buildFlags": ["-tags=viper_bind_struct"],
  "go.testFlags": ["-tags=viper_bind_struct"]
}

@sagikazarmark any idea on whether this feature will be enabled by default in future releases?

[poundifdef]

@preslavrachev There is much more context for why this was put behind a build flag here: #1706

Basically, that code caused regressions in specific cases and so it was put behind a flag until it is fully backwards-compatible.

[alfa-alex]

I have a question about the expected behavior, I hope it's fine to ask this here (not sure yet if this is a bug or intended): Is it correct that fields tagged with omitempty cannot be set by environment variable only?

For example, when I change this line to be

		Secret string `mapstructure:"secret,omitempty"`

instead, the test fails.

[andig]

Is omitempty even a valid mapstructure tag?

[alfa-alex]

Is omitempty even a valid mapstructure tag?

Looks like it is: mitchellh/mapstructure#145