Viper: Broken YAML Unmarshal

[joaolpinho]

Broken YAML Unmarshal

Since commit bep@51f23d1f1c56a7773ae8f2cfd038f7996ecc9ac2 viper fails to Unmarshal yaml data.

[bep]

Can you be more specific? (as in: I have tested that commit with lots of YAML so there must be something I don't get)

[joaolpinho]

Honestly I couldn't find a specific change, therefore I've only picked the commit that started the issue. With this version I've unsuccessfully tried to unmarshal the keys into a struct, resulting in zeroed values.

Before trying older versions I've traced to func unmarshallConfigReader (v.ReadRemoteConfig() -> v.getRemoteConfig -> v.unmarshalReader -> unmarshallConfigReader) within util.go and verified that the Reader did contain the correct data, apart from that I'm currently not able to give you more detailed information.

[moorereason]

Can you provide some YAML that triggers the issue?

[bep]

I think the hint is in the "keys into a struct". We have no tests for that, I guess.

But I would be surprised if the issue was introduced in my commit and not the ec4eb2fa8549869ae7a2accd4fcc83d1c0555c15 from @benoitmasson -- but we must fix it anyhow.

[benoitmasson]

With the YAML example it would be easier to debug... if you could send it here we could investigate.

One potential lead though: insensitiviseMap() has always been buggy (in the latest version on master, it does not lowercase keys nested inside a lowercased key, for example foo.BAR will not be lowercased, while Foo.BAR will).

Since @bep's commit, Get() relies on the fact that keys are lowercased, which could explain why they are not found in your case. I already have a fix ready for that, if that's useful I can open a specific PR for it.

[benoitmasson]

I haven't been able to reproduce the bug while playing with lower- and upper-cased nested keys. It may be something else (probably related to my commit, indeed).

I'm afraid I really need a config sample (and the associated structure with tags) to check that…

[joaolpinho]

Please take a look @ https://github.com/joaolpinho/viper--258

I've included the glide.lock file containing all used packages and their version, hopefully its a problem on my end, but I'm unable to confirm that since changing versions do resolve my issue.

[bep]

I have merged https://github.com/spf13/viper/pull/256 (my objection in that one wasn't important) that may or may not have solved the issue.

[benoitmasson]

Got it!

Your structure fields begin with a lower-case letter, thus are unexported… other packages, like viper or mapstructure (which performs unmarshalling) cannot see them and set them.

Structure fields and configuration keys are case-insensitive, hence you should just define your structure as follows:

    type Config struct {
        Hacker   bool
        Name     string
        Hobbies  string
        Clothing string
        Age      int
    }

regardless of the case in your YAML file.

(I cannot explain, however, how you could have it work once…)

[joaolpinho]

@benoitmasson problem still persists, i've even tried to make YAML and struct keys identical to your suggestion but without any good results.

If the sample I've provided works well on both of you then I can only point out dependencies and/or system, Yesterday this issue occurred to a colleague of mine, found out that both of us updated viper thus being very weird that this issue only occurred to us. As for dependencies I've also tried installing using go get instead of glide with the same results.

For the time being, if you cannot reproduce this issue, lets assume that its a problem on my end and freeze this issue. I'll try to figure out on my end and I'll let you know if anything comes up.

[benoitmasson]

I didn't mention it in my previous post because I thought it was pretty obvious, but are you aware that in your code sample, the line v.ReadConfig(bytes.NewBuffer(yamlExample)) fails ?

It returns an error (you should check it and handle the non-nil case). Here, the error simply comes from the fact that you should remove indentation in your YAML string, so I suspect that this is not your real-life data, which is probably loaded from a syntactically valid YAML file… But just in case, check the return of ReadConfig()…

[joaolpinho]

@benoitmasson You're absolutely right, I've first tried with my actual remote data, and then tried with this bad example and thought the problem was the same. I'll refactor my example.

[joaolpinho]

While testing with real-life data I stopped being able to reproduce the issue (go figure :P) so I've started testing it with previous commits and concluded that @bep merge #256 did the trick, using the most recent version resolved all my issues and I'm now able to map all values to the intended struct correctly.

Thank you for being all over it ;)
best regards