Fix YAMLRemoteRefParser with empty input

Do not treat empty input as an invalid remote reference, since apps may
use an empty file as an on/off switch. Also add some tests for parsing,
since there are several edge cases now.

appconfig/yaml.go

@@ -15,25 +15,47 @@
 package appconfig
 
 import (
-	"errors"
+	"fmt"
 
 	"gopkg.in/yaml.v2"
 )
 
 // YAMLRemoteRefParser parses b as a YAML-encoded RemoteRef. It assumes all
 // parsing errors mean the content is not a RemoteRef.
 func YAMLRemoteRefParser(path string, b []byte) (*RemoteRef, error) {
-	var ref RemoteRef
-	if err := yaml.UnmarshalStrict(b, &ref); err != nil {
+	var maybeRef struct {
+		Remote *string `yaml:"remote"`
+		Path   *string `yaml:"path"`
+		Ref    *string `yaml:"ref"`
+	}
+
+	if err := yaml.UnmarshalStrict(b, &maybeRef); err != nil {
 		// assume errors mean this isn't a remote config
 		return nil, nil
 	}
+	if maybeRef.Remote == nil && maybeRef.Path == nil && maybeRef.Ref == nil {
+		return nil, nil
+	}
 
-	if ref.Remote == "" {
-		return nil, errors.New("invalid remote reference: empty \"remote\" field")
+	ref := RemoteRef{}
+	if err := copyField(&ref.Remote, maybeRef.Remote, "remote"); err != nil {
+		return nil, err
+	}
+	if err := copyField(&ref.Path, maybeRef.Path, "path"); err != nil {
+		return nil, err
 	}
-	if ref.Path == "" {
-		return nil, errors.New("invalid remote references: empty \"path\" field")
+	if err := copyField(&ref.Ref, maybeRef.Ref, "ref"); err != nil {
+		// do nothing, ref is not a required field
 	}
 	return &ref, nil
 }
+
+func copyField(dst, src *string, name string) error {
+	if src != nil {
+		*dst = *src
+	}
+	if *dst == "" {
+		return fmt.Errorf("invalid remote reference: empty %q field", name)
+	}
+	return nil
+}

appconfig/yaml_test.go

@@ -0,0 +1,89 @@
+// Copyright 2021 Palantir Technologies, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package appconfig
+
+import (
+	"testing"
+)
+
+func TestYAMLRemoteRefParser(t *testing.T) {
+	tests := map[string]struct {
+		Input  string
+		Output *RemoteRef
+		Error  bool
+	}{
+		"complete": {
+			Input: "{remote: test/test, path: test.yaml, ref: main}",
+			Output: &RemoteRef{
+				Remote: "test/test",
+				Path:   "test.yaml",
+				Ref:    "main",
+			},
+		},
+		"empty": {
+			Input:  "",
+			Output: nil,
+		},
+		"missingRemote": {
+			Input: "{path: test.yaml, ref: main}",
+			Error: true,
+		},
+		"missingPath": {
+			Input: "{remote: test/test, ref: main}",
+			Error: true,
+		},
+		"missingRef": {
+			Input: "{remote: test/test, path: test.yaml}",
+			Output: &RemoteRef{
+				Remote: "test/test",
+				Path:   "test.yaml",
+			},
+		},
+		"extraFields": {
+			Input:  "{remote: test/test, path: test.yaml, key: value}",
+			Output: nil,
+		},
+		"unknownFields": {
+			Input:  "{key: value}",
+			Output: nil,
+		},
+	}
+
+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			ref, err := YAMLRemoteRefParser("test.yml", []byte(test.Input))
+			if test.Error {
+				if err == nil {
+					t.Fatal("expected error parsing ref, but got nil")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error parsing ref: %v", err)
+			}
+
+			switch {
+			case test.Output == nil && ref != nil:
+				t.Errorf("expected nil ref, but got: %+v", *ref)
+			case test.Output != nil && ref == nil:
+				t.Errorf("expected %+v, but got nil", *test.Output)
+			case test.Output != nil && ref != nil:
+				if *test.Output != *ref {
+					t.Errorf("expected %+v, but got %+v", *test.Output, *ref)
+				}
+			}
+		})
+	}
+}