Regression bug: sub tree configs failing to use environment variables

[redkite127]

Preflight Checklist

• I have searched the issue tracker for an issue that matches the one I want to file, without success.
• I am not looking for support or already pursued the available support channels without success.
• I have checked the troubleshooting guide for my problem, without success.

Viper Version

1.16.0

Go Version

1.19.4

Config Source

Environment variables, Files

Format

YAML

Repl.it link

No response

Code reproducing the issue

package main

import (
	"fmt"
	"log"
	"os"

	"github.com/spf13/viper"
)

type Config struct {
	WorkerCount int            `mapstructure:"workercount"`
	Database    DatabaseConfig `mapstructure:"database"`
}

type DatabaseConfig struct {
	Host     string `mapstructure:"host"`
	Port     int    `mapstructure:"port"`
	Username string `mapstructure:"username"`
	Password string `mapstructure:"password"`
	Database string `mapstructure:"database"`
}

func main() {
	os.Setenv("WORKERCOUNT", "7")
	os.Setenv("DATABASE_PASSWORD", "MyPassword")

	viper.SetConfigName("config")
	viper.AddConfigPath(".")
	err := viper.ReadInConfig()
	if err != nil {
		panic(fmt.Errorf("reading config: %w", err))
	}

	viper.AutomaticEnv()
	log.Println("workercount:", viper.Get("workercount")) // expecting 7, got 7

	subViper := viper.Sub("database")
	subViper.SetEnvPrefix("database")
	subViper.AutomaticEnv()
	log.Println("database_password:", subViper.Get("password")) // expecting "MyPassword", got "1234"
}

Expected Behavior

The value of the password of the database is supposed to be taken from the environment variable, set at the beginning of the main(). I should have MyPassword.

Actual Behavior

I have the value from the configuration file and not the one from the environment variable: admin

Steps To Reproduce

1. I used this config.yaml:

workercount: 4
database:
  host: localhost
  port: 3306
  username: admin
  password: admin
  database: 1234

2. Simply executed the code above in v1.16.0.

Additional Information

It works with viper:v1.15.0, but it doesn't with viper:v1.16.0.

[github-actions]

👋 Thanks for reporting!

A maintainer will take a look at your issue 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! ❤️

[sumit-anantwar]

Any update on this?

[Slitherings]

`
package main

import (
"fmt"
"log"
"os"

"github.com/spf13/viper"

)

type Config struct {
WorkerCount int mapstructure:"workercount"
Database DatabaseConfig mapstructure:"database"
}

type DatabaseConfig struct {
Host string mapstructure:"host"
Port int mapstructure:"port"
Username string mapstructure:"username"
Password string mapstructure:"password"
Database string mapstructure:"database"
}

func main() {
viper.SetConfigName("config")
viper.AddConfigPath(".")

viper.AutomaticEnv()

err := viper.ReadInConfig()
if err != nil {
	panic(fmt.Errorf("reading config: %w", err))
}

os.Setenv("WORKERCOUNT", "7")
os.Setenv("DATABASE_PASSWORD", "MyPassword")

log.Println("workercount:", viper.Get("workercount"))

subViper := viper.Sub("database")
subViper.AutomaticEnv()

log.Println("database_password:", subViper.Get("password"))

}
`

You could try this code, and see if this resolves your problem? You're manually setting environment variables using os.Setenv() before calling viper.ReadInConfig(). This means that the values from the environment variables set manually will be overwritten by the values from the configuration file during the viper.ReadInConfig() step. You're also setting the DATABASE_PASSWORD to "MyPassword", but the configuration file likely contains a different value for the password field. Since viper.ReadInConfig() is executed after you set the environment variables, the configuration file's value is taking priority over the environment variable.

[redkite127]

Thank you for your reply! Changing the env after using readInConfig doesn't make much sense, most of the time (if not in all situations) you will have your environment set-up before launching your app!

Again, the behavior in v1.15 is perfectly fine, but not anymore in v1.16.0.

[robdesbois]

Getting hit by this issue after trying the v1.16.0 upgrade too.

The fix for sub tree inheritance has broken the automatic env var behaviour. Specifically, the change to env var name used for lookup here: https://github.com/spf13/viper/blob/v1.16.0/viper.go#L1307

If I patch viper to use lcaseKey as before everything works fine, otherwise the tree name is being prepended. For sub-tree named logging, with SetEnvPrefix("LOGGING") and key env it used to look up env var named LOGGING_ENV. Now it looks up LOGGING_LOGGING_ENV.

I don't know this issue or library well enough to understand if our usage pattern is bad or this is a genuine regression that needs fixing. The current behaviour doesn't match this part of the readme though:

AutomaticEnv...will check for an environment variable with a name matching the key uppercased and prefixed with the EnvPrefix if set.

[sagikazarmark]

@redkite127 @robdesbois if I understand correctly the problem appears when you set an env prefix after a viper.Sub call.

Honestly, I find that a bit strange. Although, the API allows that, I would expect that a Viper instance after calling Sub would append the provided prefix to the env key prefix.

In pseudo code:

v := viper.New()
v.AutomaticEnv()
v.SetEnvPrefix("myapp")

os.Setenv("MYAPP_DATABASE_PASSWORD", "verysecret")

database := v.Sub("database")

database.GetString("password") // Should return "verysecret"

Notice, I don't set an env prefix on database. In this case the env lookup appends the Viper prefix ("database") to the env prefix.

I believe that is the intended behavior and I believe this test proves that.

From what I can tell, you can just drop the following from your code and it should work:

	subViper.SetEnvPrefix("database")
	subViper.AutomaticEnv()

viper.SetEnvPrefix("LOGGING")

(Note: you may need to set an env key replacer for it to work)

I don't know this issue or library well enough to understand if our usage pattern is bad or this is a genuine regression that needs fixing. The current behaviour doesn't match this part of the readme though:

AutomaticEnv...will check for an environment variable with a name matching the key uppercased and prefixed with the EnvPrefix if set.

It may need clarification, but I don't think it's incorrect. The env var is prefixed with the env prefixed, but then the additional segments (passed to Sub) are also added to the key.

Sadly the Sub feature is very confusing, because it exposes Viper's full API doing all kinds of weird things, like changing settings and even writing.

Sub Vipers are only supposed to be used for reading and nothing else.

In light of these, I believe what you consider a regression is actually bug well fixed in Viper. But there is a chance I missed something in your examples or didn't understand the issue.

Can you provide a failing test that highlights the problem? Maybe that would help getting closer the truth.

Thanks!

[redkite127]

Thank you for the reply @sagikazarmark, those 2 lines were more an attempt to force it to work than anything else. Without those, it still fails and takes the value from the config instead of the ENV.

I will try to provide you a failing test. In the meantime, someone else provided a fix proposal: #1617 might you be interested in his suggested fix?