Nested maps #195

benoitmasson · 2016-06-03T08:25:41Z

This PR should fix #168 and #190, among others, by updating the Set*() functions so that they change “deep” values in the configuration maps.

Not ready for merge: no code yet, only unit tests in file overrides_test.go (corresponding to the cases discussed in #168), to make sure we agree on the final behavior.

Please comment if you wish to change anything.

CLAassistant · 2016-06-03T08:25:42Z

All committers have signed the CLA.

benoitmasson · 2016-07-16T20:50:17Z

[EDIT: outdated list, see this post for the up-to-date version]

PR rebased onto master and completed:

commit 7e6dd39 fixes Issue Overriding nested keys #168 (Set() and SetDefault() now handle nested keys correctly, and according to the tests from overrides_test.go)
commit 6457afe fixes Issue Binding flags and unmarshalling to nested structures #190 (AllSettings() takes nested flags into account, so that Unmarshal handles correctly overriding with flags)
commit 344a2cc fixes Get(), which used to return a value for foo.bar.baz when foo.bar was defined but foo.bar.baz wasn't
commit 2b7b6ce fixes case insensivity for nested config elements (insensitiviseMaps() used to stop at depth-1 keys, now recurses through the whole structure)

It should now be ready for merge, but feel free to comment...

rybit · 2016-08-22T23:27:18Z

Am I understanding this PR right. It will address issues when you have code like this:

type Config struct {
  Port      int
  LogConfig struct {
    Level string
  } `mapstructure:"log_config"`
}

func LoadConfig()(*Config, error) {
  viper.SetEnvPrefix("NF")
  viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
  viper.AutomaticEnv()

  config := new(Config)
  if err := viper.Marshal(config); err != nil {
    return nil, err
  }
  return config, nil
}

and let it be overriden by env vars like NF_LOG_CONFIG_LEVEL? Just like you can override port with NF_PORT?

benoitmasson · 2016-08-24T13:50:40Z

Hi @rybit,

You got it right, that's the idea for different contexts (overrides and flags). I didn't deal with env vars since I don't have a use for them, but that's interesting too.

I've tried to execute your code sample, and unfortunately the answer is no, it doesn't work... I'm investigating this and I'll let you know if it can be fixed easily.

benoitmasson · 2016-08-25T09:32:55Z

I've added a new commit 9141a0a, which replaces 6457afe in that it provides a more general solution to the unmarshalling of overridden nested values.

It now works with both flags and environment variables, and probably everything else, since viper.AllSettings() now exclusively relies on the result of the viper.Get(key) function for all nested keys found in the configuration.

@rybit your code sample now works, just make sure to use viper.Unmarshal() function, and to set a default value to port and log_config.level, with viper.SetDefault() for example (otherwise viper doesn't know these config values exist and doesn't look for corresponding values in the environment).

rybit · 2016-08-25T20:56:48Z

This is great! I have been poking at it!

Before I have been using a (terrible) method to walk the structure and populate the fields by calling the 'GetXXX' methods. But it did mean that you loaded each value, including zero values.

I was hoping that you wouldn't have to set defaults, that it would just return the zero value or the overridden value. It means you can add a value to the struct and it will get the zero or the right override. For instance, if you're setting defaults in the pflag declaration. Or if you're going to default to a zero value.

Thanks for the work though! I am excited for when this goes in :)

awfm9 · 2016-09-20T08:29:42Z

I'll try to take a look at this soon, could you rebase the branch in the meantime?

benoitmasson · 2016-09-20T11:38:40Z

Great, thanks!

The rebase is complete, however the test you recently added fails: func TestShadowedNestedValue(t *testing.T) (commit 2f6a414)

--- FAIL: TestShadowedNestedValue (0.00s)
        Error Trace:    viper_test.go:923
    Error:      Not equal: "" (expected)
                    != "polyester" (actual)

(oh, and by the way, it seems you have used the wrong parameter order for assert.Equal(), “actual” instead of “expected” and vice-versa, tell me if you want me to change that)

Indeed, I followed the convention written in the README file, which stipulates that:

This obeys the precedence rules established above; the search for the root key
(in this example, datastore) will cascade through the remaining configuration
registries until found. The search for the sub-keys (metric and host),
however, will not.

For example, if the metric key was not defined in the configuration loaded
from file, but was defined in the defaults, Viper would return the zero value.

i.e., since clothing is first found in the config map, all sub-elements will be looked for in this map. Since clothing.shirt exists only in the defaults map, it should remain undefined.

This test worked previously (before my commits), because the whole “nested maps” stuff was buggy. My changes make Viper.find() work as described and TestShadowedNestedValue() fails.

I have two options to choose from before patching and pushing:

either change the test in TestShadowedNestedValue() to assert.Nil(t, Get("clothing.shirt"));
or update the behavior of find() (and the message in the README) to make it look for clothing.shirt in all maps.

What should I do? I'd go for solution 1, which has a much lower impact on the package (and on its possible usages).

awfm9 · 2016-09-21T12:52:00Z

You bring up a good point, I'll try to get some feedback about this, since I can't decide this on my own.

awfm9 · 2016-09-21T18:56:32Z

It's hard to argue with the README, but after getting some feedback, it seems my initial understanding is correct and the documentation should probably be updated. Basically, the precedence should overwrite at times and replace at times.

Quoting the response from @spf13 :

Default = {
    x: foo,
    y: {
        z: bar
   }
}

If I specify in the config:

y: yellow

then the final result would be

{
    x: foo,
    y: yellow
}

If I specify in the config:

y.n:yellow

then the final result would be:

{
    x: foo,
    y: {
        n: yellow,
        z: bar
   }
}

So this means that solution 2 would be the correct approach. Apologies about the confusion.

benoitmasson · 2016-09-21T19:05:20Z

No problem, that was my first understanding too, until I saw the README :-)

However, that means changing the find() function accordingly, which may be quite a bit of work from what I remember. I'll fix that as soon as possible, but that will probably take a few days. I'll keep you informed in this thread.

Thanks for the clarification anyway, in the end it will probably more logical that way.

benoitmasson · 2016-09-29T14:56:05Z

Now, this is finally looking good!

To make it short, I've made a bunch of fixes, with 2 major improvements:

Set*() functions correctly insert nested values in “deep” maps, and Get() retrieves them using the correct order of priority (issues Default value for nested key #71, Trouble accessing nested StringSlice #93, Default values doesn't work (SetDefault function) #158, Overriding nested keys #168, The default settings do not work correctly for nested key #209). This was achieved by a partial rewrite of the underlying find() function.
It takes into account shadowing, as you requested earlier, and precedence of keys containing dots, as specified in the README file.
AllKeys() and AllSettings() have been rewritten, they now perform correctly the “configuration merge” (the “final result” you mentioned), taking into account all configuration levels with their priority, shadowing, etc.
The main consequence is that Unmarshal() works well for nested structures, even for overridden values (issues Unmarshal doesn't behave correctly for bound pflags #141, Nested keys don't work with AutomaticEnv #160, defaults for nested keys don't work with AllSettings or Unmarshal #162, Binding flags and unmarshalling to nested structures #190)

README has been updated, too, according to our discussion, and should now be consistent with this implementation.

More detailed overview of the most significant changes:

commit 4db0eb0 fixes Get(), which used to return a value for foo.bar.baz when foo.bar was defined but foo.bar.baz wasn't
commit a60edc6 fixes case insensivity for nested config elements (insensitiviseMaps() used to stop at depth-1 keys, now recurses through the whole structure)
commits 63a39fe, 17581d7 and 64b6d29 add more tests, to reflect what is claimed in the README file
commits 86fffbd and 4190078 fix Issues Default value for nested key #71, Trouble accessing nested StringSlice #93, Default values doesn't work (SetDefault function) #158, Overriding nested keys #168, The default settings do not work correctly for nested key #209 (Set() and SetDefault() now handle nested keys correctly, and according to the tests from overrides_test.go)
commit 0afe404 ensures that properties containing dots generate nested maps when parsed.
commit 59609bb performs all the heavy work about find() refactoring: nested keys reachable in any map, shadowing, … to comply with this comment
commit 74f9cd8 fixes Issues Unmarshal doesn't behave correctly for bound pflags #141, Nested keys don't work with AutomaticEnv #160, defaults for nested keys don't work with AllSettings or Unmarshal #162, Binding flags and unmarshalling to nested structures #190 (AllSettings() takes nested flags into account, so that Unmarshal() handles correctly the different configuration levels)

From what I've seen, it is very likely that these patches solve the following issues too: #183, #188, #234, #237

I hope this can be merged shortly, it's becoming harder and harder to rebase… So please let me know if I can be of any help.

Thanks a lot!

niondir · 2016-10-06T13:49:21Z

Please merge this Pull Request, I could also need this. I guess it will also fix missing Environment variable overrides in viper.AllSettings().

Removed extra "Aliases"

If foo.bar is defined but foo.bar.baz isn't, then v.Get("foo.bar.baz") returns nil. v.searchMap() modified for this purpose.

So that nested keys are lowercased

overrides (elements added with Set()) should have highest priority, checked first (was after PFlags). From README.md: > Viper uses the following precedence order. Each item takes precedence > over the item below it: > • explicit call to Set > • flag > • env > • config > • key/value store > • default + fix TestBindPFlags() to use a new Viper instance (otherwise, "port" was defined in an earlier test function with Set() and took precedence)

if config["foo"]["bar"] and config["foo.bar"] coexist, the latter value should be used (as described in README).

"clothing.jacket.price" exists in defaults map, but "clothing.jacket" exists and is a value in config map => should remain undefined (“shadowed” by the config)

In function TestNestedOverrides(), in file overrides_test.go More involved complement to TestOverrides() in viper_test.go, as suggested in Issue spf13#168 (spf13#168)

Set() and SetDefault() fixed, so that they insert (or substitute) correctly "nested values" (= values identified by a complete path, i.e. containing dots; e.g. "foo.bar"). They scan (and create if needed) intermediate maps, so that the value reaches the innermost map. Examples : * v.override = make(map[string]interface{}) v.Set("foo.bar", 10) v.Set("foo.baz", 20) fmt.Println(v.override["foo"]["bar"]) // displays "10" fmt.Println(v.override["foo"]["baz"]) // displays "20" Furthermore, pre-existing non-map value are erased: * v.Set("foo.bar", 10) v.Set("foo.bar.baz", 20) fmt.Println(v.override["foo"]["bar"]) // displays "map["baz": 20]" The low-level work is performed by function deepSearch(), it scans the given map according to a given key path, creating intermediate maps if necessary.

If a property name contains a ".", it is split and generates sub-maps in the "config" map, as for the other configuration sources. Tests updated accordingly, to reflect these nested values.

Rewriting of find(), which now checks for in-depth values, and ignores shadowed ones. * When looking for "foo.bar": - before: if "foo" was defined in some map, "foo.*" was looked for only in this map - now: "foo.*" is looked for in all maps, the first one with a value is used (even if "foo" was defined elsewhere, in a higher-priority map). * Also, find() tests that the requested key is not shadowed somewhere on its path. e.g., {"foo.bar": 1} in the config map shadows "foo.bar.baz" in the defaults map * Lastly, if in the config map, config["foo"]["bar"] and config["foo.bar"] coexist, the latter value is used. (this should not be necessary for other maps, since by construction their keys should not contain dots) => Get() and IsSet() corrected and simplified, since all the logic about value retrieval has been put in find() + README.md modified accordingly: In Section “Accessing nested keys”, to reflect the corrected behavior of find(): - paths searched for at all levels, not only the one of the first sub-key; - paths may be shadowed by a shorter, higher priority, path. + tests modified accordingly

…d values * Function AllKeys() now returns all keys holding a value (for nested values, the nested key is the full path, i.e., a sequence of dot-separated keys). Previously, returned only depth-1 keys, as well as flags and environment variables: this is more generic and may be used widely. Besides, it takes into account shadowed keys (key ignored if shadowed by a path at a higher-priority level). * Function AllSettings() now returns nested maps for all keys holding a value, as specified by AllKeys(). The value stored in the map is the one with highest priority, as returned by the Get() function (taking into account aliases, environment variables, flags, etc.). This fixes Unmarshal(): it fills in correct values for nested configuration elements overridden by flags or env variables. + tests fixed accordingly + test added to TestShadowedNestedValue(), to test Unmarshalling of shadowed keys

awfm9 · 2016-10-07T18:03:35Z

I will review this tomorrow and see if I can merge right away.

awfm9 · 2016-10-08T07:54:47Z

I checked the tests and most of the code. I also used this version in a private project and the improved behaviour is a bliss. I will go ahead and merge now.

One comment I have is whether setting a key to two different maps should maybe merge them instead of replacing the first one with the second one. This is of course a question of semantics and a design decision; I just feel it would possibly make more sense, as there is no viper.Add option to do this.

Thanks a lot for the great work.

aeneasr · 2016-10-22T09:24:14Z

@benoitmasson @awishformore this pull requests breaks viper.BindEnv, because it isn't being used in unmarshalling when no default value is set:

Does not work:

    viper.BindEnv("HOST")

    if err := viper.ReadInConfig(); err != nil {
        fmt.Printf(`Config file not found because "%s"`, err)
        fmt.Println("")
    }

    if err := viper.Unmarshal(c); err != nil {
        fatal(fmt.Sprintf("Could not read config because %s.", err))
    }

Does work:

    viper.BindEnv("HOST")
    viper.SetDefault("HOST", "")

    if err := viper.ReadInConfig(); err != nil {
        fmt.Printf(`Config file not found because "%s"`, err)
        fmt.Println("")
    }

    if err := viper.Unmarshal(c); err != nil {
        fatal(fmt.Sprintf("Could not read config because %s.", err))
    }

benoitmasson · 2016-10-22T15:41:00Z

OK @arekkas, I'll have a look.

benoitmasson mentioned this pull request Jun 3, 2016

Overriding nested keys #168

Closed

benoitmasson force-pushed the nested-maps branch from f2dc934 to bf1de71 Compare July 16, 2016 20:40

benoitmasson mentioned this pull request Jul 16, 2016

Binding flags and unmarshalling to nested structures #190

Closed

benoitmasson force-pushed the nested-maps branch from bf1de71 to 9141a0a Compare August 25, 2016 08:33

awfm9 self-assigned this Sep 20, 2016

benoitmasson force-pushed the nested-maps branch from 9141a0a to 3b8c1c9 Compare September 29, 2016 14:31

benoitmasson added 11 commits October 6, 2016 16:17

Fix indentation in comment

01e8f74

Fix order of expected<=>actual in assert.Equal() statements

1186061

Fix: display in Debug()

fd1ed93

Removed extra "Aliases"

Fix: Get() returns nil when nested element not found

4db0eb0

If foo.bar is defined but foo.bar.baz isn't, then v.Get("foo.bar.baz") returns nil. v.searchMap() modified for this purpose.

Fix: insensitiviseMaps() made recursive

a60edc6

So that nested keys are lowercased

Tests: add a test for checking precedence of dot-containing keys.

63a39fe

if config["foo"]["bar"] and config["foo.bar"] coexist, the latter value should be used (as described in README).

Tests: add a test for shadowed nested key

17581d7

"clothing.jacket.price" exists in defaults map, but "clothing.jacket" exists and is a value in config map => should remain undefined (“shadowed” by the config)

Tests for overriding nested values

64b6d29

In function TestNestedOverrides(), in file overrides_test.go More involved complement to TestOverrides() in viper_test.go, as suggested in Issue spf13#168 (spf13#168)

Fix: properties parsing generates nested maps

0afe404

If a property name contains a ".", it is split and generates sub-maps in the "config" map, as for the other configuration sources. Tests updated accordingly, to reflect these nested values.

benoitmasson added 2 commits October 6, 2016 16:22

benoitmasson force-pushed the nested-maps branch from 892b80c to 74f9cd8 Compare October 6, 2016 14:26

benoitmasson mentioned this pull request Oct 7, 2016

Default value for nested key #71

Closed

awfm9 merged commit ec4eb2f into spf13:master Oct 8, 2016

benoitmasson mentioned this pull request Oct 12, 2016

Increase performance of nested keys search #256

Merged

rybit mentioned this pull request Oct 22, 2016

Consider env vars when unmarshalling #188

Closed

aeneasr mentioned this pull request Oct 22, 2016

0.6.0 ory/hydra#293

Merged

20 tasks

benoitmasson mentioned this pull request Oct 23, 2016

Fixed AllKeys() to include env values added with BindEnv() #264

Merged

benoitmasson mentioned this pull request Apr 11, 2017

Binding with cobra flag results on IsSet always true #276

Closed

sdurrheimer mentioned this pull request Aug 28, 2017

Switch to golang/dep + depedencies updates prometheus/promu#79

Closed

This was referenced Nov 13, 2018

Fix configuration with files and cli flags spacemeshos/go-spacemesh#238

Merged

Viper won't override nested config with cli flags spacemeshos/go-spacemesh#245

Closed

edermi mentioned this pull request Aug 14, 2019

viper.Sub() not extracting nested default values #747

Open

edznux-dd mentioned this pull request Jun 6, 2023

Add live k8s API collector DataDog/KubeHound#34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested maps #195

Nested maps #195

benoitmasson commented Jun 3, 2016 •

edited

Loading

CLAassistant commented Jun 3, 2016 •

edited

Loading

benoitmasson commented Jul 16, 2016 •

edited

Loading

rybit commented Aug 22, 2016

benoitmasson commented Aug 24, 2016

benoitmasson commented Aug 25, 2016

rybit commented Aug 25, 2016

awfm9 commented Sep 20, 2016

benoitmasson commented Sep 20, 2016 •

edited

Loading

awfm9 commented Sep 21, 2016

awfm9 commented Sep 21, 2016

benoitmasson commented Sep 21, 2016

benoitmasson commented Sep 29, 2016 •

edited

Loading

niondir commented Oct 6, 2016

awfm9 commented Oct 7, 2016

awfm9 commented Oct 8, 2016

aeneasr commented Oct 22, 2016 •

edited

Loading

benoitmasson commented Oct 22, 2016

Nested maps #195

Nested maps #195

Conversation

benoitmasson commented Jun 3, 2016 • edited Loading

CLAassistant commented Jun 3, 2016 • edited Loading

benoitmasson commented Jul 16, 2016 • edited Loading

rybit commented Aug 22, 2016

benoitmasson commented Aug 24, 2016

benoitmasson commented Aug 25, 2016

rybit commented Aug 25, 2016

awfm9 commented Sep 20, 2016

benoitmasson commented Sep 20, 2016 • edited Loading

awfm9 commented Sep 21, 2016

awfm9 commented Sep 21, 2016

benoitmasson commented Sep 21, 2016

benoitmasson commented Sep 29, 2016 • edited Loading

niondir commented Oct 6, 2016

awfm9 commented Oct 7, 2016

awfm9 commented Oct 8, 2016

aeneasr commented Oct 22, 2016 • edited Loading

benoitmasson commented Oct 22, 2016

benoitmasson commented Jun 3, 2016 •

edited

Loading

CLAassistant commented Jun 3, 2016 •

edited

Loading

benoitmasson commented Jul 16, 2016 •

edited

Loading

benoitmasson commented Sep 20, 2016 •

edited

Loading

benoitmasson commented Sep 29, 2016 •

edited

Loading

aeneasr commented Oct 22, 2016 •

edited

Loading