From 23c75ef7023b44372f75bfd9ddc4f36b610569f4 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Fri, 30 Jul 2021 21:23:04 +0000 Subject: [PATCH 1/3] 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 | 36 +++++++++++++---- appconfig/yaml_test.go | 89 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 7 deletions(-) create mode 100644 appconfig/yaml_test.go diff --git a/appconfig/yaml.go b/appconfig/yaml.go index f9d784766..29ac84520 100644 --- a/appconfig/yaml.go +++ b/appconfig/yaml.go @@ -15,7 +15,7 @@ package appconfig import ( - "errors" + "fmt" "gopkg.in/yaml.v2" ) @@ -23,17 +23,39 @@ import ( // 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 +} diff --git a/appconfig/yaml_test.go b/appconfig/yaml_test.go new file mode 100644 index 000000000..ee3b4ef2f --- /dev/null +++ b/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) + } + } + }) + } +} From 27ab024fce3be114261f1bd018f7ffbf77afae43 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Fri, 30 Jul 2021 22:56:57 +0000 Subject: [PATCH 2/3] Add test with comments --- appconfig/yaml_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/appconfig/yaml_test.go b/appconfig/yaml_test.go index ee3b4ef2f..3994cc526 100644 --- a/appconfig/yaml_test.go +++ b/appconfig/yaml_test.go @@ -36,6 +36,10 @@ func TestYAMLRemoteRefParser(t *testing.T) { Input: "", Output: nil, }, + "comments": { + Input: "# the existence of this file enables the app\n", + Output: nil, + }, "missingRemote": { Input: "{path: test.yaml, ref: main}", Error: true, From 0aba29f5017ebc3f60ba6b75e439af4680dec597 Mon Sep 17 00:00:00 2001 From: Billy Keyes Date: Fri, 30 Jul 2021 23:45:27 +0000 Subject: [PATCH 3/3] Make path optional, simplify parsing --- appconfig/yaml.go | 32 ++++++++++---------------------- appconfig/yaml_test.go | 33 ++++++++++++++++++++------------- 2 files changed, 30 insertions(+), 35 deletions(-) diff --git a/appconfig/yaml.go b/appconfig/yaml.go index 29ac84520..95ca50b20 100644 --- a/appconfig/yaml.go +++ b/appconfig/yaml.go @@ -15,7 +15,7 @@ package appconfig import ( - "fmt" + "errors" "gopkg.in/yaml.v2" ) @@ -25,37 +25,25 @@ import ( func YAMLRemoteRefParser(path string, b []byte) (*RemoteRef, error) { var maybeRef struct { Remote *string `yaml:"remote"` - Path *string `yaml:"path"` - Ref *string `yaml:"ref"` + 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 { + if maybeRef.Remote == nil { return nil, nil } - ref := RemoteRef{} - if err := copyField(&ref.Remote, maybeRef.Remote, "remote"); err != nil { - return nil, err + ref := RemoteRef{ + Remote: *maybeRef.Remote, + Path: maybeRef.Path, + Ref: maybeRef.Ref, } - if err := copyField(&ref.Path, maybeRef.Path, "path"); err != nil { - return nil, err - } - if err := copyField(&ref.Ref, maybeRef.Ref, "ref"); err != nil { - // do nothing, ref is not a required field + if ref.Remote == "" { + return nil, errors.New("invalid remote reference: empty \"remote\" 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 -} diff --git a/appconfig/yaml_test.go b/appconfig/yaml_test.go index 3994cc526..765d4167b 100644 --- a/appconfig/yaml_test.go +++ b/appconfig/yaml_test.go @@ -32,21 +32,16 @@ func TestYAMLRemoteRefParser(t *testing.T) { Ref: "main", }, }, - "empty": { - Input: "", - Output: nil, - }, - "comments": { - Input: "# the existence of this file enables the app\n", - Output: nil, - }, "missingRemote": { - Input: "{path: test.yaml, ref: main}", - Error: true, + Input: "{path: test.yaml, ref: main}", + Output: nil, }, "missingPath": { Input: "{remote: test/test, ref: main}", - Error: true, + Output: &RemoteRef{ + Remote: "test/test", + Ref: "main", + }, }, "missingRef": { Input: "{remote: test/test, path: test.yaml}", @@ -55,11 +50,23 @@ func TestYAMLRemoteRefParser(t *testing.T) { Path: "test.yaml", }, }, + "emptyRemote": { + Input: "{remote: ''}", + Error: true, + }, + "empty": { + Input: "", + Output: nil, + }, + "commentsOnly": { + Input: "# the existence of this file enables the app\n", + Output: nil, + }, "extraFields": { - Input: "{remote: test/test, path: test.yaml, key: value}", + Input: "{remote: test/test, path: test.yaml, ref: main, key: value}", Output: nil, }, - "unknownFields": { + "onlyUnknownFields": { Input: "{key: value}", Output: nil, },