1.18.0 Unmarshal() Breaking Our Tests With: '' expected a map, got 'interface' #1706
Comments
|
👋 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, https://twitter.com/sagikazarmark/status/1306904078967074816 Thank you! ❤️ |
|
We're seeing the same here. |
|
me too |
|
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 Fwiw, I fixed this by ensuring that I just pass a pointer to |
|
Have also been bitten by this, but in my case we're not passing a pointer to a pointer, but rather a generic type: The actual type we pass in to this is a plain struct. |
|
Related issue here: after upgrading to 0.18.1 (which changes /cc @krakowski in case this related to #1429 or #1704 |
|
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) |
@shane-ns1 could you provide a snippet of your code. I have no idea how unmarshaling to an interface would work. |
|
Here is a rough first workaround, but I believe mapstructure should take care of this: #1707 |
Apologies, I wasn't clear on my side. In my particular case, the code that invoked 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 :). |
|
Hello, same here, I encountered a bug when using 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. |
|
A couple thoughts:
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 |
|
@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 |
@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
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{}
} |
|
@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. |
|
Absolutely, please excuse me if I gave the impression of feeling blamed. I‘m super thankful for your work! |
|
@andig no worries! ;) |
|
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 |
|
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 I had to change two of our tests to reflect other behavioral changes in |
|
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 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 |
|
@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 |
@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? |
|
@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?
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. |
What I had in mind here is a simple
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.
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) |
|
@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. |
|
I ended up forking mapstructure and will maintain it under go-viper, so we may have a more permanent solution soon. |
|
As discussed above, I tagged 1.18.2 disabling the new bind struct feature. It can be enabled with the I will add a new interface for enabling the feature runtime in 1.19. |
|
#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 I may still add the runtime optional feature interface in 2023. |
|
@sagikazarmark I'd still prefer if we could re-enable this without new api. The existing 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? |
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.
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.gopackage 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)
} |
## 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
|
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 Just waiting for this to be released. |
|
@cedws you can give it a try by upgrading to v1.20.0-alpha.1 |
Preflight Checklist
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:
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.
The text was updated successfully, but these errors were encountered: