Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
3 tasks done
jcburley opened this issue Dec 7, 2023 · 33 comments
Open
3 tasks done
Labels
kind/bug Something isn't working

Comments

@jcburley
Copy link

jcburley commented Dec 7, 2023

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

@jcburley jcburley added the kind/bug Something isn't working label Dec 7, 2023
Copy link

github-actions bot commented Dec 7, 2023

👋 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
Copy link

We're seeing the same here.

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

@uouuou
Copy link

uouuou commented Dec 8, 2023

me too

@juliusbachnick
Copy link

juliusbachnick commented Dec 8, 2023

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
Copy link

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
Copy link
Contributor

andig commented Dec 8, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@juliusbachnick
Copy link

juliusbachnick commented Dec 8, 2023

(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
Copy link

ELMcode commented Dec 8, 2023

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
Copy link
Collaborator

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 #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
Copy link
Contributor

andig commented Dec 9, 2023

@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 #1429 or #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, #1707 seems to work for me. No more

'' expected a map, got 'ptr'

@andig
Copy link
Contributor

andig commented Dec 9, 2023

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
Copy link
Collaborator

@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
Copy link
Contributor

andig commented Dec 9, 2023

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

@sagikazarmark
Copy link
Collaborator

@andig no worries! ;)

@shane-ns1
Copy link

shane-ns1 commented Dec 12, 2023

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
Copy link
Author

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
Copy link
Collaborator

sagikazarmark commented Dec 15, 2023

I've opened a PR to disable struct binding by default for now until we feel relatively confident that we found all backwards incompatibilities: #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
Copy link
Contributor

@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
Copy link
Contributor

andig commented Dec 15, 2023

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, #1707 would already fix the issue.

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

@sagikazarmark
Copy link
Collaborator

@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
Copy link
Contributor

@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 #1429 (comment)

@sagikazarmark
Copy link
Collaborator

@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
Copy link
Collaborator

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

@sagikazarmark
Copy link
Collaborator

sagikazarmark commented Dec 18, 2023

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
Copy link
Collaborator

#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
Copy link
Contributor

andig commented Dec 21, 2023

@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 added a commit to mazenharake/mapstructure that referenced this issue Mar 20, 2024
This issue is related to spf13/viper#1706
It solves the problem by shaving off any pointers or interfaces to
reach the concrete types which are then decoded accordingly.
@mazenharake
Copy link

#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: go-viper/mapstructure@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
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)
}

evan-forbes added a commit to celestiaorg/celestia-core that referenced this issue Apr 18, 2024
## Description

I think the aws dep bumped viper in the last tracing PR, and 1.18 has
this issue spf13/viper#1706.

we can do this or celestiaorg/cosmos-sdk#389

main uses v1.13 of viper, but 1.15 is the lowest we can use without a
replace statement on this branch
@sagikazarmark
Copy link
Collaborator

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
Copy link

cedws commented Jun 7, 2024

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.