1.18.0 Unmarshal() Breaking Our Tests With: '' expected a map, got 'interface'

[jcburley]

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.0

Go Version

1.21.5

Config Source

Manual set

Format

Other (specify below)

Repl.it link

No response

Code reproducing the issue

No response

Expected Behavior

For now, due to the urgency of this new release having come out only yesterday and limited time (until maybe this weekend?), I can just say that our unit tests (for two modules, within a package we have to wrap/provide common Golang functions) have been running fine for awhile, and with this v1.18.0 release we're getting these errors from Viper.Unmarshal().

Actual Behavior

Sample error:

    config_test.go:146: 
            Error Trace:    /Users/c/go/src/github.com/namely/go-common/config/config_test.go:146
            Error:          Received unexpected error:
                            Viper.Unmarshal returned: '' expected a map, got 'interface'
            Test:           TestConfigTestSuite/TestBind

Steps To Reproduce

No response

Additional Information

I checked the release notes and didn't see anything obvious, but the code diffs between 1.17.0 and 1.18.0 do show changes in the decoding code that seems to be involved here.

I'm hoping this limited (information-poor) Issue submission might help someone quickly figure out what's wrong, if anything is indeed wrong with the new release (versus, say, a latent bug in our code, or at least test code).

If not, I'll endeavor to reduce our layers of wrapping to a simple test case showing the issue, and perhaps "get lucky", find the bug, and submit a fix as well.

[github-actions[bot]]

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

[shane-ns1]

We're seeing the same here.

config_test.go:42: '' expected a map, got 'interface'

[uouuou]

me too

[juliusbachnick]

I had a similar issue which I think is due to the additional step when unmarshalling as introduced in #1429 which tries to decode the provided interface{} into a map using mapstructure's decode which later on fails here when trying to decode the data (i.e. the input interface{} to val i.e. the output value which is the map. The failure happens because the input interface{}'s kind can only be one of the supported types of the switch statement and a pointer is not supported.

Fwiw, I fixed this by ensuring that I just pass a pointer to Unmarshal (and not a pointer to a pointer or something similar).

[fabiomargarido]

Have also been bitten by this, but in my case we're not passing a pointer to a pointer, but rather a generic type:

    var config T
    if err = c.Unmarshal(&config); err != nil {
        panic(fmt.Errorf("failed to unmarshal configs: %w", err))
    }

The actual type we pass in to this is a plain struct.

[andig]

Related issue here: after upgrading to 0.18.1 (which changes UnmarshalExact) I'm seeing:

'' expected a map, got 'ptr'

/cc @krakowski in case this related to #1429 or #1704

[sagikazarmark]

Thanks for the analysis @juliusbachnick !

As a first course of action, we could disable the new struct binding feature by default to make sure Viper continues working.

It would also help if people with issues using the new struct binding bit could share what they are trying to unmarshal to.

From what I understand pointer of a pointer doesn't work. Are there others?

(I'd also like to understand what the use case is for passing a pointer to a pointer)

[sagikazarmark]

We're seeing the same here.

config_test.go:42: '' expected a map, got 'interface'

@shane-ns1 could you provide a snippet of your code. I have no idea how unmarshaling to an interface would work.

[sagikazarmark]

Here is a rough first workaround, but I believe mapstructure should take care of this: #1707

[juliusbachnick]

(I'd also like to understand what the use case is for passing a pointer to a pointer)

Apologies, I wasn't clear on my side. In my particular case, the code that invoked viper.Unmarshal used the pointer to an implementation of a provided interface but the passed argument already was a pointer, similar to

package main

import (
    "fmt"

    "github.com/spf13/viper"
)

type Echo struct{}

func (e *Echo) String() string { return "echo" }

func main() {
    e := &Echo{}
    if err := doSomething(e); err != nil {
        panic(err)
    }
}

func doSomething(s fmt.Stringer) error {
    return viper.Unmarshal(&s)
}

So this actually was unintended, but somehow this worked previously. If anything, this change revealed the bug in my code :).

[ELMcode]

Hello, same here, I encountered a bug when using viper.Unmarshal, with 1.18 & 1.18.1 versions.

The error :

mustReadConfig: '' expected a map, got 'ptr'

The issue arises when I have the following code:

// C is a concrete implementation of Module interface
C struct {
parser string
cfg    *models.Cfg
logger logger.Logger

// mustReadConfig reads a config file.
func (c *C) mustReadConfig(reader io.Reader) {
    switch c.parser {
    case "json", "yaml", "yml", "toml":
        viper.SetConfigType(c.parser)
    default:
        c.logger.Fatalf("unsupported parsing format for config file %v\n", c.parser)
    }

    if err := viper.ReadConfig(reader); err != nil {
        c.logger.Fatalf("mustReadConfig: %v", err)
    }

    if err := viper.Unmarshal(&c.cfg); err != nil {
        c.logger.Fatalf("mustReadConfig: %v", err)
    }
}

The bug occurs with the pointer models.Cfg in the C struct. The issue is related to the use of Unmarshal. However, the bug disappears when I remove the pointer from models.Cfg. Alternatively, using viper.UnmarshalExact instead of viper.Unmarshal also resolves the issue & working with the pointer.

[sagikazarmark]

A couple thoughts:

• passing a pointer of a pointer does not make much sense to me, but your example @juliusbachnick, actually shows you may not even be aware that you are doing so, so I'm inclined to say we should handle it gracefully
• given Unmarshal is not fatally broken, I don't feel the need to revert the change for an immediate fix. 1.18 does not actually come with that many changes, and there are workarounds for the problem.
• I think the pointer-of-pointer situation should be handled by mapstructure. I'm gonna open an issue/PR to address that, but given the last release of mapstructure was more than a year ago, we will probably have to workaround it.

I have a very rudimentary quick fix in https://github.com/spf13/viper/pull/1707. If you could give that a try, that would be great.

It would also be great to understand if there are other cases where mapstructure fails.

The '' expected a map, got 'interface' seems strange to me, I'm not sure how to replicate it and how it would have worked before.

[andig]

@sagikazarmark I think the issue here is that nothing has changed with regards to the decoding target (i.e. from user side). What has imho changed is additional inputs (from ENV) into the decoding process. Hence I think that this must somehow be due to https://github.com/spf13/viper/pull/1429 or https://github.com/spf13/viper/pull/1704 and not due to mistakes on the part of Viper users?

Where does that pointer-to-pointer come from that did not present a problem before?

That said, https://github.com/spf13/viper/pull/1707 seems to work for me. No more

'' expected a map, got 'ptr'

[andig]

The '' expected a map, got 'interface' seems strange to me, I'm not sure how to replicate it and how it would have worked before.

@sagikazarmark Here's a repro for the error:

func demoConfig(conf *globalConfig) error {
    viper.SetConfigType("yaml")
    if err := viper.ReadConfig(strings.NewReader(demoYaml)); err != nil {
        return fmt.Errorf("failed decoding demo config: %w", err)
    }

    if err := viper.UnmarshalExact(&conf); err != nil {
        return fmt.Errorf("failed loading demo config: %w", err)
    }

    return nil
}

I used to pass a pointer to the global config struct and indirected that once more when calling Viper. Seems that is unnecessary. Passing to viper.UnmarshalExact by value should be ok since a copy of the conf pointer would still point to the same globalConfig. However- that worked before.

globalConfig looks like this:

type globalConfig struct {
    Network      networkConfig
    Log          string
    SponsorToken string
    Plant        string // telemetry plant id
    Telemetry    bool
    Metrics      bool
    Profile      bool
    Levels       map[string]string
    Interval     time.Duration
    Database     dbConfig
    Mqtt         mqttConfig
    ModbusProxy  []proxyConfig
    Javascript   []javascriptConfig
    Go           []goConfig
    Influx       server.InfluxConfig
    EEBus        map[string]interface{}
    HEMS         config.Typed
    Messaging    messagingConfig
    Meters       []config.Named
    Chargers     []config.Named
    Vehicles     []config.Named
    Tariffs      tariffConfig
    Site         map[string]interface{}
    Loadpoints   []map[string]interface{}
}

[sagikazarmark]

@andig it's definitely related to #1429, but that doesn't mean it's not the result of some nonsensical usage, that worked as side-effect so far.

But I'm not trying to blame it on Viper users if that's what you are getting at. I'm simply trying to gather information to decide the best course of action.

[andig]

Absolutely, please excuse me if I gave the impression of feeling blamed. I‘m super thankful for your work!

[sagikazarmark]

@andig no worries! ;)

[shane-ns1]

I was able to work around this in our code by changing from:

    if err = viper.Unmarshal(&dst); err != nil {

To:

    if err = viper.Unmarshal(dst); err != nil {

The dst variable there is of type interface{}.

[jcburley]

Thanks @shane-ns1 et al — though I haven't investigated this fully myself so as to understand it completely, your comments have given me some idea of what to try.

Since our code that calls viper.Unmarshal() is itself a wrapper called in a variety of scenarios (certainly by our unit tests), removing the leading & fixed some tests and broke others. I'm hesitant to simply dismiss (EOL) the newly failing tests, since they might represent real code elsewhere in our org, and came up with this (paraphrased a bit) instead:

// Support callers that pass pointers to structs as well as
// just structs to be populated, without doing
// double-indirection that ran afoul of viper 1.18.0.
val := reflect.ValueOf(dst)
if val.Kind() != reflect.Ptr && val.CanAddr() {
    dst = val.Addr().Interface()
}

// Unmarshal the config into the supplied struct, using `config` as the tag
// name
err = viper.Unmarshal(dst, func(...) { ... })

I had to change two of our tests to reflect other behavioral changes in viper. One such change seemed entirely reasonable; the other totally mystifies me as to how it ever worked. I might post more about either/both here or, if I feel strongly they might represent regressions, as new Issue(s).

[sagikazarmark]

I've opened a PR to disable struct binding by default for now until we feel relatively confident that we found all backwards incompatibilities: https://github.com/spf13/viper/pull/1715

You can enable struct binding by adding the viper_bind_struct to the list of your build tags.

Would this work for everyone while we figure out a safe way to enable the feature again?

If so, I'm going to release it as 1.18.2

cc @krakowski

[krakowski]

@sagikazarmark Is it possible to make this a runtime option (defaulting to false)? Something like experimental opt-in flags. This way people who want to use it don't have to recompile the library themselves.

In the long run a function for extracting all fields using reflect will be needed for this to work reliably, as mapstructure's decode function can't be used to extract nested fields which must be accessed through a pointer.

[andig]

Would this work for everyone while we figure out a safe way to enable the feature again?

@sagikazarmark situation is a bit unfortunate, but still: feature/breakage was introduced with 1.18.0, which would already indicate some attention when upgrading. Root cause is unnecessary use of pointer to pointer (at least in my case). Plus, https://github.com/spf13/viper/pull/1707 would already fix the issue.

I would like to propose to merge #1707 instead of adding a build tag and release that?

[sagikazarmark]

@krakowski adding a runtime flag would require introducing something to the API. Removing that would be considered a BC break. Some could argue that using build tags for the same purpose is no different, but so far it seemed like a better solution.

Also, it is temporary until we figure out how to resolve the BC problems.

Do you have a scenario in mind where build tags would be suboptimal?

In the long run a function for extracting all fields using reflect will be needed for this to work reliably, as mapstructure's decode function can't be used to extract nested fields which must be accessed through a pointer.

Can you elaborate? A single pointer is not a problem. A pointer to a pointer is. I agree this should be solved in mapstructure though.

@andig My concern is that #1707 doesn't resolve the problem entirely. I've seen other error messages reported in this issue. Until we can figure those out, I'm more comfortable with not enabling the new feature by default.

[krakowski]

@krakowski adding a runtime flag would require introducing something to the API. Removing that would be considered a BC break. Some could argue that using build tags for the same purpose is no different, but so far it seemed like a better solution.

What I had in mind here is a simple map[string]bool and viper.EnableFeature(string). This could allow to enable (experimental) features dynamically at runtime in the future and get feedback on them from the community.

Also, it is temporary until we figure out how to resolve the BC problems.

Do you have a scenario in mind where build tags would be suboptimal?

Personally, this would work for me so I could only come up with an artificial scenario here: Using two viper instances where just one of them has the feature enabled.

In the long run a function for extracting all fields using reflect will be needed for this to work reliably, as mapstructure's decode function can't be used to extract nested fields which must be accessed through a pointer.

Can you elaborate? A single pointer is not a problem. A pointer to a pointer is. I agree this should be solved in mapstructure though.

Sure. I made a comment within the original PR describing what goes wrong, when using pointer fields inside structs. Please have a look at https://github.com/spf13/viper/pull/1429#issuecomment-1856118406

[sagikazarmark]

@krakowski thanks for the feedback.

How about this: Let’s release 1.18.2 with the build flag solution so that people can go back to the original functionality ASAP.

Then we can tag 1.19 with a runtime version. I’d like to sleep on the API though.

[sagikazarmark]

I ended up forking mapstructure and will maintain it under go-viper, so we may have a more permanent solution soon.

[sagikazarmark]

As discussed above, I tagged 1.18.2 disabling the new bind struct feature. It can be enabled with the viper_bind_struct tag.

I will add a new interface for enabling the feature runtime in 1.19.

[sagikazarmark]

1723 uses a mapstructure version that should fix the multiple indirection problem.

I'd still like to look into what causes the same error with interface{} before enabling this feature by default again, but I don't think it's gonna happen this year.

I may still add the runtime optional feature interface in 2023.

[andig]

@sagikazarmark I'd still prefer if we could re-enable this without new api. The existing AutomaticEnv should already cover this case.

I used to get the desired behavior by pre-registering all keys like this:

// register all known config keys
flat, _ := flatten.Flatten(structs.Map(conf), "", flatten.DotStyle)
for _, v := range maps.Keys(flat) {
    _ = viper.BindEnv(v)
}

What I'm totally failing to understand is why we're apparently triggering errors on the target struct side now (depending on the target struct) when the change should really modify what gets decoded, so on the input data side?

[mazenharake]

1723 uses a mapstructure version that should fix the multiple indirection problem.

I'd still like to look into what causes the same error with interface{} before enabling this feature by default again, but I don't think it's gonna happen this year.

I may still add the runtime optional feature interface in 2023.

I ran into this problem yesterday and noticed that the fix in go-viper/mapstructure doesn't really go all the way. I propose the following change instead which seems to work for any input of pointers and interface nesting.

See: https://github.com/go-viper/mapstructure/compare/main...mazenharake:mapstructure:deepshave

I might be missing something but I tested it with the code below (warning: monstrosity incoming). Note: I did a local replace in the go.mod to point at the local go-viper/mapstructure package so dont trust the import path. Also, all tests in mapstructure and viper runs without errors.

if we can get #1723 in then I can create a pull request or you can just copy-paste it in since it is simple enough (haven't looked into your pull request routines yet).

Anyway, this should fix this issue.

viperUnmarshal.go

```golang package main import ( "fmt" "os" "strings" "github.com/mitchellh/mapstructure" "github.com/spf13/viper" ) type Person struct { Name string `mapstructure:"name"` Age int `mapstructure:"age"` Job *Job `mapstructure:"job"` Hobbies []string `mapstructure:"hobbies"` XJob **Job `mapstructure:"xjob"` } type Job struct { Title string `mapstructure:"title"` Company string `mapstructure:"company"` } func (p *Person) String() string { return fmt.Sprintf("person:%s:%d:%s:%v:%s:%s", p.Name, p.Age, p.Job.ToString(), p.Hobbies, (*p.XJob).Title, (*p.XJob).Company) } func (j *Job) ToString() string { return fmt.Sprintf("%s:%s", j.Title, j.Company) } type JobMapI interface { ToString() string } type JobMap map[string]*Job func (j JobMap) ToString() string { return j.ToString() } func main() { viper.SetEnvPrefix("VTEST") viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) viper.AutomaticEnv() viper.NewWithOptions() os.Setenv("VTEST_NAME", "john") os.Setenv("VTEST_AGE", "99") os.Setenv("VTEST_JOB_TITLE", "architect") os.Setenv("VTEST_JOB_COMPANY", "somecompany") os.Setenv("VTEST_HOBBIES", "1 2 3") os.Setenv("VTEST__XJOB_TITLE", "xtitle") os.Setenv("VTEST_XJOB_COMPANY", "xcompany") xjob := &Job{} p := &Person{ Job: &Job{}, XJob: &xjob, } if err := doSomething(p); err != nil { panic(err) } fmt.Println("P1", p.String()) os.Setenv("VTEST_NAME", "jane") os.Setenv("VTEST_JOB_TITLE", "developer") if err := doSomethingElse(p); err != nil { panic(err) } fmt.Println("P2", p.String()) os.Setenv("VTEST_NAME", "bob") os.Setenv("VTEST_JOB_TITLE", "carpenter") pp := &p if err := doSomethingAnon(pp); err != nil { panic(err) } fmt.Println("P3", p.String()) outputMap := map[string]any{} inputMap := map[string]*Job{ "foo": { Title: "sometitle", Company: "somecompany", }, } if err := mapstructure.Decode(&inputMap, &outputMap); err != nil { panic(err) } for k, v := range outputMap { fmt.Println(k, ":", v) } outputMap = map[string]any{} inputJobMap := JobMap{ "foo": { Title: "someothertitle", Company: "someothercompany", }, } if err := doSomethingJobMapI(inputJobMap, &outputMap); err != nil { panic(err) } for k, v := range outputMap { fmt.Println(k, ":", v) } outputMap = map[string]any{} inputArray := []any{map[string]string{"foo": "bar"}} ms, _ := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ Metadata: nil, Result: &outputMap, WeaklyTypedInput: true, DecodeHook: mapstructure.ComposeDecodeHookFunc( mapstructure.StringToTimeDurationHookFunc(), mapstructure.StringToSliceHookFunc(","), ), }) if err := ms.Decode(&inputArray); err != nil { panic(err) } for k, v := range outputMap { fmt.Println(k, ":", v) } } func doSomething(s fmt.Stringer) error { return viper.Unmarshal(&s) } func doSomethingElse(p *Person) error { return viper.Unmarshal(p) } func doSomethingAnon(i any) error { return viper.Unmarshal(i) } func doSomethingJobMapI(jm JobMapI, output *map[string]any) error { return mapstructure.Decode(jm, output) } ```

[sagikazarmark]

How do you all feel about #1854?

I'd imagine it would become the default behavior at some point, so the experimental flag would become noop.

[cedws]

How do you all feel about #1854?

I'd imagine it would become the default behavior at some point, so the experimental flag would become noop.

This looks good, I'm glad that I will be able to remove the build tag from our pipelines/Dockerfiles/etc. It means our programs will behave correctly with plain go build, which improves local development experience.

Just waiting for this to be released.

[sagikazarmark]

@cedws you can give it a try by upgrading to v1.20.0-alpha.1