From ae72cd4b995f47c60424d1c4d43c725f1de9a8a8 Mon Sep 17 00:00:00 2001 From: Benoit Masson Date: Tue, 27 Sep 2016 17:11:17 +0200 Subject: [PATCH] Fix: Get() and IsSet() work correctly on nested values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- README.md | 19 ++-- viper.go | 266 +++++++++++++++++++++++++++++++++++--------------- viper_test.go | 8 +- 3 files changed, 203 insertions(+), 90 deletions(-) diff --git a/README.md b/README.md index cf17560d81..f4e72f871d 100644 --- a/README.md +++ b/README.md @@ -458,16 +458,17 @@ Viper can access a nested field by passing a `.` delimited path of keys: GetString("datastore.metric.host") // (returns "127.0.0.1") ``` -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. +This obeys the precedence rules established above; the search for the path +will cascade through the remaining configuration registries until found. -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. +For example, given this configuration file, both `datastore.metric.host` and +`datastore.metric.port` are already defined (and may be overridden). If in addition +`datastore.metric.protocol` was defined in the defaults, Viper would also find it. -On the other hand, if the primary key was not defined, Viper would go through -the remaining registries looking for it. +However, if `datastore.metric` was overridden (by a flag, an environment variable, +the `Set()` method, …) with an immediate value, then all sub-keys of +`datastore.metric` become undefined, they are “shadowed” by the higher-priority +configuration level. Lastly, if there exists a key that matches the delimited key path, its value will be returned instead. E.g. @@ -491,7 +492,7 @@ will be returned instead. E.g. } } -GetString("datastore.metric.host") //returns "0.0.0.0" +GetString("datastore.metric.host") // returns "0.0.0.0" ``` ### Extract sub-tree diff --git a/viper.go b/viper.go index a3064acf7e..5881dc1904 100644 --- a/viper.go +++ b/viper.go @@ -399,8 +399,9 @@ func (v *Viper) providerPathExists(p *defaultRemoteProvider) bool { return false } +// searchMap recursively searches for a value for path in source map. +// Returns nil if not found. func (v *Viper) searchMap(source map[string]interface{}, path []string) interface{} { - if len(path) == 0 { return source } @@ -430,9 +431,127 @@ func (v *Viper) searchMap(source map[string]interface{}, path []string) interfac // got a value but nested key expected, return "nil" for not found return nil } - } else { - return nil } + return nil +} + +// searchMapWithPathPrefixes recursively searches for a value for path in source map. +// +// While searchMap() considers each path element as a single map key, this +// function searches for, and prioritizes, merged path elements. +// e.g., if in the source, "foo" is defined with a sub-key "bar", and "foo.bar" +// is also defined, this latter value is returned for path ["foo", "bar"]. +// +// This should be useful only at config level (other maps may not contain dots +// in their keys). +func (v *Viper) searchMapWithPathPrefixes(source map[string]interface{}, path []string) interface{} { + if len(path) == 0 { + return source + } + + // search for path prefixes, starting from the longest one + for i := len(path); i > 0; i-- { + prefixKey := strings.ToLower(strings.Join(path[0:i], v.keyDelim)) + + var ok bool + var next interface{} + for k, v := range source { + if strings.ToLower(k) == prefixKey { + ok = true + next = v + break + } + } + + if ok { + var val interface{} + switch next.(type) { + case map[interface{}]interface{}: + val = v.searchMapWithPathPrefixes(cast.ToStringMap(next), path[i:]) + case map[string]interface{}: + // Type assertion is safe here since it is only reached + // if the type of `next` is the same as the type being asserted + val = v.searchMapWithPathPrefixes(next.(map[string]interface{}), path[i:]) + default: + if len(path) == i { + val = next + } + // got a value but nested key expected, do nothing and look for next prefix + } + if val != nil { + return val + } + } + } + + // not found + return nil +} + +// isPathShadowedInDeepMap makes sure the given path is not shadowed somewhere +// on its path in the map. +// e.g., if "foo.bar" has a value in the given map, it “shadows” +// "foo.bar.baz" in a lower-priority map +func (v *Viper) isPathShadowedInDeepMap(path []string, m map[string]interface{}) string { + var parentVal interface{} + for i := 1; i < len(path); i++ { + parentVal = v.searchMap(m, path[0:i]) + if parentVal == nil { + // not found, no need to add more path elements + return "" + } + switch parentVal.(type) { + case map[interface{}]interface{}: + continue + case map[string]interface{}: + continue + default: + // parentVal is a regular value which shadows "path" + return strings.Join(path[0:i], v.keyDelim) + } + } + return "" +} + +// isPathShadowedInFlatMap makes sure the given path is not shadowed somewhere +// in a sub-path of the map. +// e.g., if "foo.bar" has a value in the given map, it “shadows” +// "foo.bar.baz" in a lower-priority map +func (v *Viper) isPathShadowedInFlatMap(path []string, mi interface{}) string { + // unify input map + var m map[string]interface{} + switch mi.(type) { + case map[string]string, map[string]FlagValue: + m = cast.ToStringMap(mi) + default: + return "" + } + + // scan paths + var parentKey string + for i := 1; i < len(path); i++ { + parentKey = strings.Join(path[0:i], v.keyDelim) + if _, ok := m[parentKey]; ok { + return parentKey + } + } + return "" +} + +// isPathShadowedInAutoEnv makes sure the given path is not shadowed somewhere +// in the environment, when automatic env is on. +// e.g., if "foo.bar" has a value in the environment, it “shadows” +// "foo.bar.baz" in a lower-priority map +func (v *Viper) isPathShadowedInAutoEnv(path []string) string { + var parentKey string + var val string + for i := 1; i < len(path); i++ { + parentKey = strings.Join(path[0:i], v.keyDelim) + if val = v.getEnv(v.mergeWithEnvPrefix(parentKey)); val != "" { + return parentKey + } + } + return "" } // SetTypeByDefaultValue enables or disables the inference of a key value's @@ -469,46 +588,16 @@ func Get(key string) interface{} { return v.Get(key) } func (v *Viper) Get(key string) interface{} { lcaseKey := strings.ToLower(key) val := v.find(lcaseKey) - - if val == nil { - path := strings.Split(key, v.keyDelim) - source := v.find(strings.ToLower(path[0])) - if source != nil { - if reflect.TypeOf(source).Kind() == reflect.Map { - val = v.searchMap(cast.ToStringMap(source), path[1:]) - } - } - } - - // if no other value is returned and a flag does exist for the value, - // get the flag's value even if the flag's value has not changed - if val == nil { - if flag, exists := v.pflags[lcaseKey]; exists { - jww.TRACE.Println(key, "get pflag default", val) - switch flag.ValueType() { - case "int", "int8", "int16", "int32", "int64": - val = cast.ToInt(flag.ValueString()) - case "bool": - val = cast.ToBool(flag.ValueString()) - default: - val = flag.ValueString() - } - } - } - if val == nil { return nil } - var valType interface{} - if !v.typeByDefValue { - valType = val - } else { - defVal, defExists := v.defaults[lcaseKey] - if defExists { + valType := val + if v.typeByDefValue { + path := strings.Split(lcaseKey, v.keyDelim) + defVal := v.searchMap(v.defaults, path) + if defVal != nil { valType = defVal - } else { - valType = val } } @@ -752,14 +841,25 @@ func (v *Viper) find(key string) interface{} { var val interface{} var exists bool + // compute the path through the nested maps to the nested value + path := strings.Split(key, v.keyDelim) + if shadow := v.isPathShadowedInDeepMap(path, castMapStringToMapInterface(v.aliases)); shadow != "" { + return nil + } + // if the requested key is an alias, then return the proper key key = v.realKey(key) + // re-compute the path + path = strings.Split(key, v.keyDelim) // Set() override first - val, exists = v.override[key] - if exists { + val = v.searchMap(v.override, path) + if val != nil { return val } + if shadow := v.isPathShadowedInDeepMap(path, v.override); shadow != "" { + return nil + } // PFlag override next flag, exists := v.pflags[key] @@ -776,18 +876,20 @@ func (v *Viper) find(key string) interface{} { return flag.ValueString() } } - - val, exists = v.override[key] - if exists { - return val + if shadow := v.isPathShadowedInFlatMap(path, v.pflags); shadow != "" { + return nil } + // Env override next if v.automaticEnvApplied { // even if it hasn't been registered, if automaticEnv is used, // check any Get request if val = v.getEnv(v.mergeWithEnvPrefix(key)); val != "" { return val } + if shadow := v.isPathShadowedInAutoEnv(path); shadow != "" { + return nil + } } envkey, exists := v.env[key] if exists { @@ -795,39 +897,53 @@ func (v *Viper) find(key string) interface{} { return val } } + if shadow := v.isPathShadowedInFlatMap(path, v.env); shadow != "" { + return nil + } // Config file next - val, exists = v.config[key] - if exists { + val = v.searchMapWithPathPrefixes(v.config, path) + if val != nil { return val } - - // test for nested config parameter - if strings.Contains(key, v.keyDelim) { - path := strings.Split(key, v.keyDelim) - - source := v.find(path[0]) - if source != nil { - if reflect.TypeOf(source).Kind() == reflect.Map { - val := v.searchMap(cast.ToStringMap(source), path[1:]) - if val != nil { - return val - } - } - } + if shadow := v.isPathShadowedInDeepMap(path, v.config); shadow != "" { + return nil } // K/V store next - val, exists = v.kvstore[key] - if exists { + val = v.searchMap(v.kvstore, path) + if val != nil { return val } + if shadow := v.isPathShadowedInDeepMap(path, v.kvstore); shadow != "" { + return nil + } - // Default as last chance - val, exists = v.defaults[key] - if exists { + // Default next + val = v.searchMap(v.defaults, path) + if val != nil { return val } + if shadow := v.isPathShadowedInDeepMap(path, v.defaults); shadow != "" { + return nil + } + + // last chance: if no other value is returned and a flag does exist for the value, + // get the flag's value even if the flag's value has not changed + if flag, exists := v.pflags[key]; exists { + switch flag.ValueType() { + case "int", "int8", "int16", "int32", "int64": + return cast.ToInt(flag.ValueString()) + case "bool": + return cast.ToBool(flag.ValueString()) + case "stringSlice": + s := strings.TrimPrefix(flag.ValueString(), "[") + return strings.TrimSuffix(s, "]") + default: + return flag.ValueString() + } + } + // last item, no need to check shadowing return nil } @@ -835,20 +951,8 @@ func (v *Viper) find(key string) interface{} { // IsSet checks to see if the key has been set in any of the data locations. func IsSet(key string) bool { return v.IsSet(key) } func (v *Viper) IsSet(key string) bool { - path := strings.Split(key, v.keyDelim) - lcaseKey := strings.ToLower(key) val := v.find(lcaseKey) - - if val == nil { - source := v.find(strings.ToLower(path[0])) - if source != nil { - if reflect.TypeOf(source).Kind() == reflect.Map { - val = v.searchMap(cast.ToStringMap(source), path[1:]) - } - } - } - return val != nil } @@ -1033,6 +1137,14 @@ func castToMapStringInterface( return tgt } +func castMapStringToMapInterface(src map[string]string) map[string]interface{} { + tgt := map[string]interface{}{} + for k, v := range src { + tgt[k] = v + } + return tgt +} + // mergeMaps merges two maps. The `itgt` parameter is for handling go-yaml's // insistence on parsing nested structures as `map[interface{}]interface{}` // instead of using a `string` as the key for nest structures beyond one level diff --git a/viper_test.go b/viper_test.go index a66048d721..a46dd48df0 100644 --- a/viper_test.go +++ b/viper_test.go @@ -261,7 +261,7 @@ func TestUnmarshalling(t *testing.T) { assert.False(t, InConfig("state")) assert.Equal(t, "steve", Get("name")) assert.Equal(t, []interface{}{"skateboarding", "snowboarding", "go"}, Get("hobbies")) - assert.Equal(t, map[interface{}]interface{}{"jacket": "leather", "trousers": "denim", "pants": map[interface{}]interface{}{"size": "large"}}, Get("clothing")) + assert.Equal(t, map[string]interface{}{"jacket": "leather", "trousers": "denim", "pants": map[interface{}]interface{}{"size": "large"}}, Get("clothing")) assert.Equal(t, 35, Get("age")) } @@ -424,7 +424,7 @@ func TestAllKeys(t *testing.T) { ks := sort.StringSlice{"title", "newkey", "owner", "name", "beard", "ppu", "batters", "hobbies", "clothing", "age", "hacker", "id", "type", "eyes", "p_id", "p_ppu", "p_batters", "p_type", "p_name", "foos"} dob, _ := time.Parse(time.RFC3339, "1979-05-27T07:32:00Z") - all := map[string]interface{}{"owner": map[string]interface{}{"organization": "MongoDB", "Bio": "MongoDB Chief Developer Advocate & Hacker at Large", "dob": dob}, "title": "TOML Example", "ppu": 0.55, "eyes": "brown", "clothing": map[string]interface{}{"trousers": "denim", "jacket": "leather", "pants": map[string]interface{}{"size": "large"}}, "id": "0001", "batters": map[string]interface{}{"batter": []interface{}{map[string]interface{}{"type": "Regular"}, map[string]interface{}{"type": "Chocolate"}, map[string]interface{}{"type": "Blueberry"}, map[string]interface{}{"type": "Devil's Food"}}}, "hacker": true, "beard": true, "hobbies": []interface{}{"skateboarding", "snowboarding", "go"}, "age": 35, "type": "donut", "newkey": "remote", "name": "Cake", "p_id": "0001", "p_ppu": "0.55", "p_name": "Cake", "p_batters": map[string]interface{}{"batter": map[string]interface{}{"type": "Regular"}}, "p_type": "donut", "foos": []map[string]interface{}{map[string]interface{}{"foo": []map[string]interface{}{map[string]interface{}{"key": 1}, map[string]interface{}{"key": 2}, map[string]interface{}{"key": 3}, map[string]interface{}{"key": 4}}}}} + all := map[string]interface{}{"owner": map[string]interface{}{"organization": "MongoDB", "Bio": "MongoDB Chief Developer Advocate & Hacker at Large", "dob": dob}, "title": "TOML Example", "ppu": 0.55, "eyes": "brown", "clothing": map[string]interface{}{"trousers": "denim", "jacket": "leather", "pants": map[interface{}]interface{}{"size": "large"}}, "id": "0001", "batters": map[string]interface{}{"batter": []interface{}{map[string]interface{}{"type": "Regular"}, map[string]interface{}{"type": "Chocolate"}, map[string]interface{}{"type": "Blueberry"}, map[string]interface{}{"type": "Devil's Food"}}}, "hacker": true, "beard": true, "hobbies": []interface{}{"skateboarding", "snowboarding", "go"}, "age": 35, "type": "donut", "newkey": "remote", "name": "Cake", "p_id": "0001", "p_ppu": "0.55", "p_name": "Cake", "p_batters": map[string]interface{}{"batter": map[string]interface{}{"type": "Regular"}}, "p_type": "donut", "foos": []map[string]interface{}{map[string]interface{}{"foo": []map[string]interface{}{map[string]interface{}{"key": 1}, map[string]interface{}{"key": 2}, map[string]interface{}{"key": 3}, map[string]interface{}{"key": 4}}}}} var allkeys sort.StringSlice allkeys = AllKeys() @@ -644,7 +644,7 @@ func TestFindsNestedKeys(t *testing.T) { "name": "Cake", "hacker": true, "ppu": 0.55, - "clothing": map[interface{}]interface{}{ + "clothing": map[string]interface{}{ "jacket": "leather", "trousers": "denim", "pants": map[interface{}]interface{}{ @@ -693,7 +693,7 @@ func TestReadBufConfig(t *testing.T) { assert.False(t, v.InConfig("state")) assert.Equal(t, "steve", v.Get("name")) assert.Equal(t, []interface{}{"skateboarding", "snowboarding", "go"}, v.Get("hobbies")) - assert.Equal(t, map[interface{}]interface{}{"jacket": "leather", "trousers": "denim", "pants": map[interface{}]interface{}{"size": "large"}}, v.Get("clothing")) + assert.Equal(t, map[string]interface{}{"jacket": "leather", "trousers": "denim", "pants": map[interface{}]interface{}{"size": "large"}}, v.Get("clothing")) assert.Equal(t, 35, v.Get("age")) }