From e1caf0b181594848f19c18388cedac99fa7c6cc5 Mon Sep 17 00:00:00 2001 From: Alex Date: Mon, 31 Jul 2017 11:39:48 -0700 Subject: [PATCH] Merge values for provider group in the constructor (#48) ProviderGroup can and should merge values on construction, instead of on value retrieval. To do that we need need apply expand to all nested YAML nodes, instead of only top level ones. --- CHANGELOG.md | 3 +- config.go | 122 ----------------------------------------- config_test.go | 13 ++--- example_merge_test.go | 5 +- provider_group.go | 24 +++----- provider_group_test.go | 29 ++++------ value.go | 6 +- yaml.go | 43 ++++++++++++--- yaml_test.go | 4 +- 9 files changed, 75 insertions(+), 174 deletions(-) delete mode 100644 config.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 78b1ce8..bdcb511 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,8 @@ other cast libraries. - **[Breaking]** Removed `Value.IsDefault` method. - **[Breaking]** Removed Load* functions. -- **[Breaking]** Removed NewYAMLProviderFromReader* functions. +- **[Breaking]** Unexport NewYAMLProviderFromReader* functions. +- **[Breaking]** `NewProviderGroup` returns an error. ## v1.0.0-rc1 (26 Jun 2017) diff --git a/config.go b/config.go deleted file mode 100644 index f77cef4..0000000 --- a/config.go +++ /dev/null @@ -1,122 +0,0 @@ -// Copyright (c) 2017 Uber Technologies, Inc. -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in -// all copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN -// THE SOFTWARE. - -package config - -import ( - "errors" - "fmt" - "os" - "path/filepath" -) - -// LookUpFunc is a type alias for a function to look for environment variables. -type LookUpFunc func(string) (string, bool) - -// FileInfo represents a file to load by LoadFromFiles function. -type FileInfo struct { - Name string // Name of the file. - Interpolate bool // Interpolate contents? -} - -// LoadDefaults returns a default provider from a set of configuration -// files: -// ./config/base.yaml -// ./config/${ENVIRONMENT}.yaml -// ./config/secrets.yaml -func LoadDefaults() (Provider, error) { - env := "development" - if val, ok := os.LookupEnv("ENVIRONMENT"); ok { - env = val - } - - return LoadFromFiles( - []string{"config"}, - []FileInfo{ - {Name: "base.yaml", Interpolate: true}, - {Name: fmt.Sprintf("%s.yaml", env), Interpolate: true}, - {Name: "secrets.yaml"}, - }, - os.LookupEnv) -} - -// LoadTestProvider returns a default set of test configuration files: -// ./config/base.yaml -// ./config/test.yaml -func LoadTestProvider() (Provider, error) { - return LoadFromFiles( - []string{"config"}, - []FileInfo{ - {Name: "base.yaml", Interpolate: true}, - {Name: "test.yaml", Interpolate: true}, - }, - os.LookupEnv) -} - -// LoadFromFiles reads configuration files from dirs using lookUp function -// for interpolation. First both slices are interpolated with the provided -// lookUp function. Then all the files are loaded from the all dirs. -// For example: -// -// LoadFromFiles([]string{"dir1", "dir2"},[]FileInfo{{"base.yaml"},{"test.yaml"}}, nil) -// -// will try to load files in this order: -// 1. dir1/base.yaml -// 2. dir2/base.yaml -// 3. dir1/test.yaml -// 4. dir2/test.yaml -// The function will return an error, if there are no providers to load. -func LoadFromFiles(dirs []string, files []FileInfo, lookUp LookUpFunc) (Provider, error) { - var providers []Provider - for _, info := range files { - for _, dir := range dirs { - name := filepath.Join(dir, info.Name) - f, err := os.Open(name) - if os.IsNotExist(err) { - continue - } - if err != nil { - return nil, err - } - - if info.Interpolate { - p, err := newYAMLProviderFromReaderWithExpand(lookUp, f) - if err != nil { - return nil, err - } - - providers = append(providers, p) - } else { - p, err := newYAMLProviderFromReader(f) - if err != nil { - return nil, err - } - - providers = append(providers, p) - } - } - } - - if len(providers) == 0 { - return nil, errors.New("no providers were loaded") - } - - return NewProviderGroup("files", providers...), nil -} diff --git a/config_test.go b/config_test.go index 8513252..41bdc60 100644 --- a/config_test.go +++ b/config_test.go @@ -104,7 +104,7 @@ func TestDirectAccess(t *testing.T) { p, err := NewYAMLProviderFromBytes(nestedYaml) require.NoError(t, err, "Can't create a YAML provider") - provider := NewProviderGroup("test", p) + provider, err := NewProviderGroup("test", p) require.NoError(t, err) v, err := provider.Get("n1.id1").WithDefault("xxx") @@ -119,8 +119,7 @@ func TestScopedAccess(t *testing.T) { p, err := NewYAMLProviderFromBytes(nestedYaml) require.NoError(t, err, "Can't create a YAML provider") - - provider := NewProviderGroup("test", p) + provider, err := NewProviderGroup("test", p) require.NoError(t, err) p1 := provider.Get("n1") @@ -141,7 +140,7 @@ func TestSimpleConfigValues(t *testing.T) { p, err := NewYAMLProviderFromBytes(yamlConfig3) require.NoError(t, err, "Can't create a YAML provider") - provider := NewProviderGroup("test", p) + provider, err := NewProviderGroup("test", p) require.NoError(t, err) assert.Equal(t, 123, provider.Get("int").Value()) @@ -165,7 +164,7 @@ func TestNestedStructs(t *testing.T) { p, err := NewYAMLProviderFromBytes(nestedYaml) require.NoError(t, err, "Can't create a YAML provider") - provider := NewProviderGroup("test", p) + provider, err := NewProviderGroup("test", p) require.NoError(t, err) str := &root{} @@ -189,7 +188,7 @@ func TestArrayOfStructs(t *testing.T) { p, err := NewYAMLProviderFromBytes(structArrayYaml) require.NoError(t, err, "Can't create a YAML provider") - provider := NewProviderGroup("test", p) + provider, err := NewProviderGroup("test", p) require.NoError(t, err) target := &arrayOfStructs{} @@ -208,7 +207,7 @@ func TestDefault(t *testing.T) { p, err := NewYAMLProviderFromBytes(nest1) require.NoError(t, err, "Can't create a YAML provider") - provider := NewProviderGroup("test", p) + provider, err := NewProviderGroup("test", p) require.NoError(t, err) target := &nested{} diff --git a/example_merge_test.go b/example_merge_test.go index bbfea54..710dea5 100644 --- a/example_merge_test.go +++ b/example_merge_test.go @@ -54,7 +54,10 @@ config: // Provider is going to keep name from the base provider, // but ports and pool values will be overridden by prod. - p := config.NewProviderGroup("merge", base, prod) + p, err := config.NewProviderGroup("merge", base, prod) + if err != nil { + log.Fatal(err) + } var c struct { Name string diff --git a/provider_group.go b/provider_group.go index b5d1b15..014e472 100644 --- a/provider_group.go +++ b/provider_group.go @@ -21,35 +21,29 @@ package config type providerGroup struct { - name string providers []Provider + name string } // NewProviderGroup creates a configuration provider from a group of providers. // The highest priority provider is the last. -func NewProviderGroup(name string, providers ...Provider) Provider { - return providerGroup{ - name: name, - providers: providers, - } -} - -// Get iterates through the providers and return the value merged from -// underlying of providers. -// // The merge strategy for two objects // from the first provider(A) and the last provider(B) is following: -// * if A and B are maps, A and B will form a new map with keys from +// If A and B are maps, A and B will form a new map with keys from // A and B and values from B will overwrite values of A. e.g. // A: B: merge(A, B): // keep:A new:B keep:A // update:fromA update:fromB update:fromB // new:B // -// * if A is a map and B is not, this function will panic, -// e.g. key:value and -slice +// If A is a map and B is not, this function will return a Value with +// an error inside. // -// * in all the remaining cases B will overwrite A. +// In all the remaining cases B will overwrite A. +func NewProviderGroup(name string, providers ...Provider) (Provider, error) { + return providerGroup{providers: providers, name: name}, nil +} + func (p providerGroup) Get(key string) Value { var res interface{} found := false diff --git a/provider_group_test.go b/provider_group_test.go index 995edac..dd32c42 100644 --- a/provider_group_test.go +++ b/provider_group_test.go @@ -33,7 +33,9 @@ func TestProviderGroup(t *testing.T) { p, err := NewYAMLProviderFromBytes([]byte(`id: test`)) require.NoError(t, err, "Can't create a YAML provider") - pg := NewProviderGroup("test-group", p) + pg, err := NewProviderGroup("test-group", p) + require.NoError(t, err) + assert.Equal(t, "test-group", pg.Name()) assert.Equal(t, "test", pg.Get("id").String()) } @@ -44,7 +46,9 @@ func TestProviderGroupScope(t *testing.T) { p, err := NewStaticProvider(map[string]interface{}{"hello": map[string]int{"world": 42}}) require.NoError(t, err, "Can't create a static provider") - pg := NewProviderGroup("test-group", p) + pg, err := NewProviderGroup("test-group", p) + require.NoError(t, err) + assert.Equal(t, 42, pg.Get("hello").Get("world").Value()) } @@ -87,7 +91,9 @@ logging: f, err := NewYAMLProviderFromBytes(fst) require.NoError(t, err, "Can't create a YAML provider") - pg := NewProviderGroup("group", s, f) + pg, err := NewProviderGroup("group", s, f) + require.NoError(t, err) + assert.True(t, pg.Get("logging").Get("enabled").Value().(bool)) } @@ -100,23 +106,10 @@ func TestProviderGroup_GetChecksAllProviders(t *testing.T) { s, err := NewStaticProvider(map[string]string{"owner": "tst@example.com", "name": "fx"}) require.NoError(t, err, "Can't create the second provider") - pg := NewProviderGroup("test-group", f, s) - require.NotNil(t, pg) + pg, err := NewProviderGroup("test-group", f, s) + require.NoError(t, err) var svc map[string]string require.NoError(t, pg.Get(Root).Populate(&svc)) assert.Equal(t, map[string]string{"name": "fx", "owner": "tst@example.com", "desc": "test"}, svc) } - -func TestProviderGroupMergeFail(t *testing.T) { - t.Parallel() - - m, err := NewStaticProvider(map[string]interface{}{"a": map[string]string{"b": "c"}}) - require.NoError(t, err) - s, err := NewStaticProvider(map[string]interface{}{"a": []string{"b"}}) - require.NoError(t, err) - - g := NewProviderGroup("group", s, m) - v := g.Get("a") - assert.False(t, v.HasValue()) -} diff --git a/value.go b/value.go index 8a9c575..4737776 100644 --- a/value.go +++ b/value.go @@ -71,7 +71,11 @@ func (cv Value) WithDefault(value interface{}) (Value, error) { return cv, err } - g := NewProviderGroup("withDefault", p, cv.provider) + g, err := NewProviderGroup("withDefault", p, cv.provider) + if err != nil { + return cv, err + } + return g.Get(cv.key), nil } diff --git a/yaml.go b/yaml.go index 2a2a723..8b5e3ea 100644 --- a/yaml.go +++ b/yaml.go @@ -73,18 +73,23 @@ func newYAMLProviderCore(files ...io.ReadCloser) (*yamlConfigProvider, error) { }, nil } -// We need to have a custom merge map because yamlV2 doesn't unmarshal `map[interface{}]map[interface{}]interface{}` -// as we expect: it will replace second level maps with new maps on each unmarshal call, instead of merging them. +// We need to have a custom merge map because yamlV2 doesn't unmarshal +// `map[interface{}]map[interface{}]interface{}` as we expect: it will +// replace second level maps with new maps on each unmarshal call, +// instead of merging them. +// // The merge strategy for two objects A and B is following: -// * if A and B are maps, A and B will form a new map with keys from A and B and values from B will overwrite values of A. e.g. +// If A and B are maps, A and B will form a new map with keys from A and B and +// values from B will overwrite values of A. e.g.: // A: B: merge(A, B): // keep:A new:B keep:A // update:fromA update:fromB update:fromB // new:B // -// * if A is a map and B is not, this function will panic, e.g. key:value and -slice +// If A is a map and B is not, this function will return an error, +// e.g. key:value and -slice. // -// * in all the remaining cases B will overwrite A. +// In all the remaining cases B will overwrite A. func mergeMaps(dst interface{}, src interface{}) (interface{}, error) { if dst == nil { return src, nil @@ -321,6 +326,30 @@ func (n *yamlNode) Children() []*yamlNode { return nodes } +// Apply expand to all nested elements of a node. +// There is no need to use reflection, because YAML unmarshaler is using +// map[interface{}]interface{} to store objects and []interface{} +// to store collections. +func recursiveApply(node interface{}, expand func(string) string) interface{} { + if node == nil { + return nil + } + switch t := node.(type) { + case map[interface{}]interface{}: + for k := range t { + t[k] = recursiveApply(t[k], expand) + } + return t + case []interface{}: + for i := range t { + t[i] = recursiveApply(t[i], expand) + } + return t + } + + return os.Expand(fmt.Sprint(node), expand) +} + func (n *yamlNode) applyOnAllNodes(expand func(string) string) (err error) { defer func() { @@ -333,9 +362,7 @@ func (n *yamlNode) applyOnAllNodes(expand func(string) string) (err error) { } }() - if n.nodeType == valueNode && n.value != nil { - n.value = os.Expand(fmt.Sprint(n.value), expand) - } + n.value = recursiveApply(n.value, expand) for _, c := range n.Children() { if err := c.applyOnAllNodes(expand); err != nil { diff --git a/yaml_test.go b/yaml_test.go index a4cfc24..c47effa 100644 --- a/yaml_test.go +++ b/yaml_test.go @@ -254,7 +254,9 @@ func withYamlBytes(yamlBytes []byte, f func(Provider), t *testing.T) { y, err := NewYAMLProviderFromBytes(yamlBytes) require.NoError(t, err, "Can't create a YAML provider") - provider := NewProviderGroup("global", y) + provider, err := NewProviderGroup("global", y) + require.NoError(t, err) + f(provider) }