NewProviderGroup: Handle empty sources correctly (#95)

We were encountering a bug where `NewProviderGroup` was interpreting
empty files correctly despite the previous fix. Specifically, the
following works with `NewYAML`,

    # foo.yaml contains:
    #
    #   foo:
    #     bar: 42

    p := NewYAML(File("foo.yaml"), File("empty.yaml")))
    fmt.Println(p.Get("foo.bar").Value())
    // Output: 42

But the following does not,

    p1 := NewYAML(File("foo.yaml"))
    p2 := NewYAML(File("empty.yaml"))
    p  := NewProviderGroup(p1, p2)
    fmt.Println(p.Get("foo.bar").Value())
    // Output: nil

It doesn't work because `NewProviderGroup` works by calling `Get(Root)`
on the providers and building a new `Provider` with the merged contents.
`p2` doesn't know the difference between empty file and `nil` so it
returns `nil` for its contents, which the merging logic interprets as
unsetting everything at that level.

Unfortunately, switching this call site from `NewProviderGroup` to
`NewYAML` isn't possible because `NewYAML` applies options like
`Permissive` and `Expand` to all files and we don't want to `Expand` on
one of the files.

To fix this, I made two changes:

- YAML providers now have a record of whether they were empty or not.
  When a `Get` is performed on a`YAML` provider, the resulting `Value`
  now accurately knows whether it has a value.
- `NewProviderGroup` includes the contents of a provider only if its
  `Value.HasValue` returns true.

I added failing test cases for this case. I replicated that test for the
null sources case to avoid accidentally breaking it in the future.

CHANGELOG.md

@@ -4,6 +4,13 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+### Changed
+- Undo deprecation of `NewProviderGroup`.
+
+### Fixed
+- Handle empty sources correctly in `NewProviderGroup`.
+
 ## 1.2.1 - 2018-07-06
 ### Fixed
 - Handle empty sources and top-level nulls correctly.

config.go

@@ -52,6 +52,7 @@ type YAML struct {
 	raw      [][]byte
 	contents interface{}
 	strict   bool
+	empty    bool
 }
 
 // NewYAML constructs a YAML provider. See the various YAMLOptions for
@@ -86,19 +87,22 @@ func NewYAML(options ...YAMLOption) (*YAML, error) {
 		merged = bytes.NewBuffer(exp)
 	}
 
-	var contents interface{}
+	y := &YAML{
+		name:   cfg.name,
+		raw:    cfg.sources,
+		strict: cfg.strict,
+	}
+
 	dec := yaml.NewDecoder(merged)
 	dec.SetStrict(cfg.strict)
-	if err := dec.Decode(&contents); err != nil && err != io.EOF {
-		return nil, fmt.Errorf("couldn't decode merged YAML: %v", err)
+	if err := dec.Decode(&y.contents); err != nil {
+		if err != io.EOF {
+			return nil, fmt.Errorf("couldn't decode merged YAML: %v", err)
+		}
+		y.empty = true
 	}
 
-	return &YAML{
-		name:     cfg.name,
-		raw:      cfg.sources,
-		contents: contents,
-		strict:   cfg.strict,
-	}, nil
+	return y, nil
 }
 
 // Name returns the name of the provider. It defaults to "YAML".
@@ -137,6 +141,10 @@ func (y *YAML) get(path []string) Value {
 // YAML mappings are unmarshalled as map[interface{}]interface{}, sequences as
 // []interface{}, and scalars as interface{}.
 func (y *YAML) at(path []string) (interface{}, bool) {
+	if y.empty {
+		return nil, false
+	}
+
 	cur := y.contents
 	for _, segment := range path {
 		m, ok := cur.(map[interface{}]interface{})
@@ -197,18 +205,22 @@ func (y *YAML) withDefault(d interface{}) (*YAML, error) {
 		return nil, fmt.Errorf("merging default and existing YAML failed: %v", err)
 	}
 
-	var contents interface{}
+	newY := &YAML{
+		name:   y.name,
+		raw:    sources,
+		strict: y.strict,
+	}
+
 	dec := yaml.NewDecoder(merged)
 	dec.SetStrict(y.strict)
-	if err := dec.Decode(&contents); err != nil && err != io.EOF {
-		return nil, fmt.Errorf("unmarshaling merged YAML failed: %v", err)
+	if err := dec.Decode(&newY.contents); err != nil {
+		if err != io.EOF {
+			return nil, fmt.Errorf("unmarshaling merged YAML failed: %v", err)
+		}
+		newY.empty = true
 	}
-	return &YAML{
-		name:     y.name,
-		raw:      sources,
-		contents: contents,
-		strict:   y.strict,
-	}, nil
+
+	return newY, nil
 }
 
 // A Value is a subset of a provider's configuration.

config_ext_test.go

@@ -649,36 +649,59 @@ func TestEmptySources(t *testing.T) {
 	empty := ""
 	comment := "# just a comment"
 
-	tests := []struct {
-		desc    string
-		sources []string
-		expect  interface{}
-	}{
-		{"no sources", []string{}, nil},
-		{"empty base", []string{empty, full}, val},
-		{"empty override", []string{full, empty}, val},
-		{"empty base and override", []string{empty, empty}, nil},
-		{"comment-only base", []string{comment, full}, val},
-		{"comment-only override", []string{full, comment}, val},
-		{"empty base and comment-only override", []string{empty, comment}, nil},
-		{"comment-only base and empty override", []string{comment, empty}, nil},
+	runTests := func(t *testing.T, newProvider func([]string) Provider) {
+		tests := []struct {
+			desc    string
+			sources []string
+			expect  interface{}
+		}{
+			{"no sources", []string{}, nil},
+			{"empty base", []string{empty, full}, val},
+			{"empty override", []string{full, empty}, val},
+			{"empty base and override", []string{empty, empty}, nil},
+			{"comment-only base", []string{comment, full}, val},
+			{"comment-only override", []string{full, comment}, val},
+			{"empty base and comment-only override", []string{empty, comment}, nil},
+			{"comment-only base and empty override", []string{comment, empty}, nil},
+		}
+
+		for _, tt := range tests {
+			t.Run(tt.desc, func(t *testing.T) {
+				p := newProvider(tt.sources)
+				assert.Equal(t, tt.expect, p.Get("foo").Value(), "incorrect merged result")
+
+				d, err := p.Get("not_there").WithDefault(42)
+				require.NoError(t, err, "failed to set default")
+				assert.Equal(t, 42, d.Value(), "incorrect defaulted value")
+			})
+		}
 	}
 
-	for _, tt := range tests {
-		t.Run(tt.desc, func(t *testing.T) {
-			opts := make([]YAMLOption, len(tt.sources))
-			for i := range tt.sources {
-				opts[i] = Source(strings.NewReader(tt.sources[i]))
+	t.Run("NewYAML", func(t *testing.T) {
+		runTests(t, func(sources []string) Provider {
+			opts := make([]YAMLOption, len(sources))
+			for i := range sources {
+				opts[i] = Source(strings.NewReader(sources[i]))
 			}
 			p, err := NewYAML(opts...)
 			require.NoError(t, err, "failed to create provider")
-			assert.Equal(t, tt.expect, p.Get("foo").Value(), "incorrect merged result")
+			return p
+		})
+	})
 
-			d, err := p.Get("not_there").WithDefault(42)
-			require.NoError(t, err, "failed to set default")
-			assert.Equal(t, 42, d.Value(), "incorrect defaulted value")
+	t.Run("NewProviderGroup", func(t *testing.T) {
+		runTests(t, func(sources []string) Provider {
+			providers := make([]Provider, len(sources))
+			for i := range sources {
+				var err error
+				providers[i], err = NewYAML(Source(strings.NewReader(sources[i])))
+				require.NoError(t, err, "failed to create provider")
+			}
+			p, err := NewProviderGroup("foo", providers...)
+			require.NoError(t, err, "failed to create provider")
+			return p
 		})
-	}
+	})
 }
 
 func TestNullSources(t *testing.T) {
@@ -690,30 +713,53 @@ func TestNullSources(t *testing.T) {
 	empty := ""
 	null := "~"
 
-	tests := []struct {
-		desc    string
-		sources []string
-		expect  interface{}
-	}{
-		{"null base", []string{null, empty, full}, val},
-		{"null override", []string{full, empty, null}, nil},
+	runTests := func(t *testing.T, newProvider func([]string) Provider) {
+		tests := []struct {
+			desc    string
+			sources []string
+			expect  interface{}
+		}{
+			{"null base", []string{null, empty, full}, val},
+			{"null override", []string{full, empty, null}, nil},
+		}
+
+		for _, tt := range tests {
+			t.Run(tt.desc, func(t *testing.T) {
+				p := newProvider(tt.sources)
+				assert.Equal(t, tt.expect, p.Get("foo").Value(), "incorrect merged result")
+
+				d, err := p.Get("not_there").WithDefault(42)
+				require.NoError(t, err, "failed to set default")
+				// Since there's an explicit nil in the initial sources, calls to
+				// WithDefault should have no effect.
+				assert.Equal(t, nil, d.Value(), "default should have no effect")
+			})
+		}
 	}
 
-	for _, tt := range tests {
-		t.Run(tt.desc, func(t *testing.T) {
-			opts := make([]YAMLOption, len(tt.sources))
-			for i := range tt.sources {
-				opts[i] = Source(strings.NewReader(tt.sources[i]))
+	t.Run("NewYAML", func(t *testing.T) {
+		runTests(t, func(sources []string) Provider {
+			opts := make([]YAMLOption, len(sources))
+			for i := range sources {
+				opts[i] = Source(strings.NewReader(sources[i]))
 			}
 			p, err := NewYAML(opts...)
 			require.NoError(t, err, "failed to create provider")
-			assert.Equal(t, tt.expect, p.Get("foo").Value(), "incorrect merged result")
+			return p
+		})
+	})
 
-			d, err := p.Get("not_there").WithDefault(42)
-			require.NoError(t, err, "failed to set default")
-			// Since there's an explicit nil in the initial sources, calls to
-			// WithDefault should have no effect.
-			assert.Equal(t, nil, d.Value(), "default should have no effect")
+	t.Run("NewProviderGroup", func(t *testing.T) {
+		runTests(t, func(sources []string) Provider {
+			providers := make([]Provider, len(sources))
+			for i := range sources {
+				var err error
+				providers[i], err = NewYAML(Source(strings.NewReader(sources[i])))
+				require.NoError(t, err, "failed to create provider")
+			}
+			p, err := NewProviderGroup("foo", providers...)
+			require.NoError(t, err, "failed to create provider")
+			return p
 		})
-	}
+	})
 }

constructors.go

@@ -51,14 +51,16 @@ func NewScopedProvider(prefix string, provider Provider) Provider {
 // documentation. To preserve backward compatibility, the resulting provider
 // disables strict unmarshalling.
 //
-// Deprecated: compose multiple sources of configuration using NewYAML and the
-// Static, File, and/or Source options directly. This enables strict
-// unmarshalling by default and allows use of other options at the same time.
+// Prefer using NewYAML instead of this where possible. NewYAML gives you
+// strict unmarshalling by default and allows use of other options at the same
+// time.
 func NewProviderGroup(name string, providers ...Provider) (Provider, error) {
 	opts := make([]YAMLOption, 0, len(providers)+2)
 	opts = append(opts, Name(name), Permissive())
 	for _, p := range providers {
-		opts = append(opts, Static(p.Get(Root).Value()))
+		if v := p.Get(Root); v.HasValue() {
+			opts = append(opts, Static(v.Value()))
+		}
 	}
 	return NewYAML(opts...)
 }