AutomaticEnv unexpected behavior with custom types

alfa-alex commented 2 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

master

Go Version

1.22

Config Source

Environment variables

Format

No response

Repl.it link

No response

Code reproducing the issue

package main

import (
    "reflect"
    "strings"
    "testing"

    "github.com/mitchellh/mapstructure"
    "github.com/spf13/viper"
)

type CustomTypeWithUnexportedField struct {
    unexportedField []byte
}

func NewCustomTypeWithUnexportedField(value string) CustomTypeWithUnexportedField {
    return CustomTypeWithUnexportedField{unexportedField: []byte(value)}
}

func (c CustomTypeWithUnexportedField) String() string {
    return string(c.unexportedField)
}

type TestConfig struct {
    Custom CustomTypeWithUnexportedField `mapstructure:"custom"`
}

func TestAutomaticEnvBehavior(t *testing.T) {
    v := viper.New()
    v.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
    v.AutomaticEnv()

    t.Run("when the field with custom type is set solely in the environment", func(t *testing.T) {
        t.Setenv("CUSTOM", "a custom value set via the environment")

        var testConfig TestConfig
        if err := v.Unmarshal(&testConfig, func(dc *mapstructure.DecoderConfig) {
            dc.DecodeHook = customTypeHook()
        }); err != nil {
            t.Fatalf("unable to decode into struct, %v", err)
        }

        if testConfig.Custom.String() != "a custom value set via the environment" {
            t.Fatalf("wanted 'a custom value set via the environment', but got '%s'", testConfig.Custom.String())
        }
    })

    t.Run("when the field with custom type has a default", func(t *testing.T) {
        t.Setenv("CUSTOM", "a custom value set via the environment")
        v.SetDefault("custom", "defaultValue") // alternatively, this value could be read from a config value (or any other source)

        var testConfig TestConfig
        if err := v.Unmarshal(&testConfig, func(dc *mapstructure.DecoderConfig) { dc.DecodeHook = customTypeHook() }); err != nil {
            t.Fatalf("unable to decode into struct, %v", err)
        }

        if testConfig.Custom.String() != "a custom value set via the environment" {
            t.Fatalf("wanted 'a custom value set via the environment', but got '%s'", testConfig.Custom.String())
        }
    })
}

func customTypeHook() mapstructure.DecodeHookFuncType {
    return func(dataType reflect.Type, targetType reflect.Type, rawData any) (any, error) {
        if dataType.Kind() != reflect.String {
            return rawData, nil
        }
        if targetType != reflect.TypeOf(CustomTypeWithUnexportedField{}) {
            return rawData, nil
        }

        val, ok := rawData.(string)
        if !ok {
            return rawData, nil
        }

        return NewCustomTypeWithUnexportedField(val), nil
    }
}

Expected Behavior

I would expect the custom definition of a type like CustomTypeWithUnexportedField in the example to be decodeable with environment variables alone when the BindStruct feature flag is turned on (or v1.18.1 is used, for simplicity).

Actual Behavior

The decoding of the field only succeeds if I specify a default manually (like in the example) or load the value at least once from some config file (e.g. JSON). In other words, the recent fix to AutomaticEnv() doesn't work in this case.

Steps To Reproduce

Turn on the BindStruct feature flag.
Run the example code in a test file and notice that the first sub-test fails, whereas the second doesn't.

Additional Information

I noticed that in the scenario without any other config file / default etc. (i.e., the scenario that https://github.com/spf13/viper/pull/1429 was supposed to solve), the structKeyMap passed to flattenAndMergeMap still contains the value, but it is removed during that call.

The decodeStructKeys cannot properly use the custom decoder hook function (customTypeHook() in the example here) because the target type is a map[string]interface{} here, not a string (the latter is the case when viper does the decoding for the defaults / config files etc.).

I'm not sure whether we're hitting a limitation of mapstructure here or of viper and how hard it would be to fix this. In any case, it's unexpected because the BindStruct feature suggests that the behavior with additional config data and the behavior with only environment variables are the same.

(PS: On a side note, the issue tracker still suggests providing the code via repl.it, but the Go version there is limited to 1.17, which makes it impossible to run recent versions of viper there.)

github-actions[bot] commented 2 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! ❤️

sagikazarmark commented 1 month ago

Thanks for the detailed report @alfa-alex!

It seems to me that you are right in your analysis of why it doesn't work properly.

I think we are hitting an edge case here where by default structs are translated to map[string]interface{}.

One thing I would try is implementing a decode hook in the opposite direction: convert the struct into string. (No need to check the target in that case since it's just interface)

I realize it may not be an elegant long-term solution, but it may uncover why it isn't working.

spf13 / viper