Setting values in pointer structs via environment not working >1.18.0

tkrop commented 6 months ago

Preflight Checklist

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

Viper Version

1.18.1

Go Version

1.21.5

Config Source

Environment variables

Format

Other (specify below)

Repl.it link

No response

Code reproducing the issue

package main_test

import (
    "testing"

    "github.com/spf13/viper"
    "github.com/stretchr/testify/assert"
    "github.com/stretchr/testify/require"
)

type ViperConfig struct {
    Test    ViperTest
    TestPtr *ViperTest
}

type ViperTest struct {
    String string
}

func TestViper(t *testing.T) {
    // Given
    v := viper.New()
    v.AutomaticEnv()
    v.SetDefault("test.string", "test-original")
    v.SetDefault("testptr.string", "test-original")

    // When
    t.Setenv("TEST.STRING", "test-changed")
    t.Setenv("TESTPTR.STRING", "test-changed")
    config := &ViperConfig{}
    err := v.Unmarshal(config)
    require.NoError(t, err)

    // Then
    assert.Equal(t, "test-changed", config.Test.String)
    assert.Equal(t, "test-changed", v.Get("test.string"))
    assert.Equal(t, "test-changed", config.TestPtr.String)
    assert.Equal(t, "test-changed", v.Get("testptr.string"))
}

Expected Behavior

Expect test code to run.

Actual Behavior

It fails to configure config.TestPtr.String with the environment variable string.

Steps To Reproduce

Run the above test code.

Additional Information

The interesting aspect of this bug is that the main loop to setup the values in viper.go#L2142 is actually called with 4 keys starting from viper 1.18.0: ["test.string","testptr.string","test.string","testptr"] (order is unstable and may change). Resolving the values for this keys works majorly as correctly, but the result is still is incorrect because of the order:

test.string => test-changed
testptr.string => test-changed
test.string=> test-changed
testptr=> {string: "test-original"}

The result is

{ test: {string: "test-changed"}, testptr: {string: "test-original"} }

To fix this, I humbly assume, the keys extracted in /viper.go#L1124 need to be sufficiently ordered. However, there may be other bugs lurking in the newly added code.

PS: I opened a new bug, because it somehow looks different than the other bugs reported for >1.18.0, but may be it is related and the key 4. should not even be testptr.

github-actions[bot] commented 6 months ago

👋 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! ❤️

sonny-semc commented 6 months ago

I am experiencing the same thing. For now I fixed the problem by stopping using pointers.

But in some case, we also use map[string]struct, and because of the ordering issue too, the value I override with env var inside the struct gets flushed in the loop because the parent container is listed at the end of the list.

Furthermore I spent time trying to reproduce the problem with the replit project provided as example to find out that the problem does not appear with go 1.17. But as @tkrop we are using go 1.21 and it does not work.

Note that with both go1.17 and go1.21, using v.AllSettings() gives me the same settings. The difference is really only on the Unmarshal.

sagikazarmark commented 6 months ago

Thank you for reporting this issue.

My guess is this is caused by a bug in the underlying mapstructure library. Indeed, the keys reported for the struct should say testptr.string, not testptr.

I've forked the library under https://github.com/go-viper/mapstructure and will maintain a fork going forward, so we can roll fixes out quicker.

If you could take a look at the existing issues or PRs to see if there is already a report/fix that would be great. If you can contribute a patch that would be even better and much appreciated!

tkrop commented 6 months ago

@sonny-semc nice quick fix, that may be helpful as safe-guard. However, if @sagikazarmark is right, than it should be fixed in the mapstructure I assume.

@sagikazarmark many thanks for your quick action on this. Viper is a great piece of code, your work is very much appreciated. I have checked on the original mapstructure project, but so far I could not find an issue or pull request matching the problem.

spf13 / viper