ewilliams28/releases
1
0
Fork 0
mirror of https://github.com/open-policy-agent/opa.git synced 2026-08-13 03:42:35 -06:00
Code Issues Packages Projects Releases Wiki Activity

Improve file loader error handling

Browse Source 
The file loader now handles empty modules properly (previously, it would
include an empty/nil module in the results which resulted in a panic
later on).

Also, the file loader now accumulates errors instead of bailing on the
first one. This makes it easier to find and fix errors when loading
files into OPA.
This commit is contained in:
Torin Sandall
2017-03-26 16:37:14 -07:00
parent e32c5cc36c
commit ed23bfee70
4 changed files with 147 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files
runtime/loader.go
+80 -38
View File
@@ -18,6 +18,26 @@ import (
"github.com/pkg/errors"
)
type loaderErrors []error
func (e loaderErrors) Error() string {
if len(e) == 0 {
return "no error(s)"
}
if len(e) == 1 {
return "1 error occurred during loading: " + e[0].Error()
}
buf := make([]string, len(e))
for i := range buf {
buf[i] = e[i].Error()
}
return fmt.Sprintf("%v errors occured during loading:\n", len(e)) + strings.Join(buf, "\n")
}
func (e *loaderErrors) Add(err error) {
*e = append(*e, err)
}
type loaded struct {
Documents map[string]interface{}
Modules map[string]*loadedModule
@@ -47,14 +67,14 @@ func (l *loaded) WithParent(p string) *loaded {
type unsupportedDocumentType string
func (u unsupportedDocumentType) Error() string {
return "unsupported document type: " + string(u)
func (path unsupportedDocumentType) Error() string {
return string(path) + ": bad document type"
}
type unrecognizedFile string
func (u unrecognizedFile) Error() string {
return "unrecognized file: " + string(u)
func (path unrecognizedFile) Error() string {
return string(path) + ": can't recognize file type"
}
func isUnrecognizedFile(err error) bool {
@@ -62,18 +82,30 @@ func isUnrecognizedFile(err error) bool {
return ok
}
type mergeError string
func (e mergeError) Error() string {
return string(e) + ": merge error"
}
type emptyModuleError string
func (e emptyModuleError) Error() string {
return string(e) + ": empty policy"
}
func (l *loaded) Merge(path string, result interface{}) error {
switch result := result.(type) {
case *loadedModule:
l.Modules[normalizeModuleID(path)] = result
default:
obj, err := makeDir(l.path, result)
if err != nil {
return err
obj, ok := makeDir(l.path, result)
if !ok {
return unsupportedDocumentType(path)
}
merged, err := mergeDocs(l.Documents, obj)
if err != nil {
return err
merged, ok := mergeDocs(l.Documents, obj)
if !ok {
return mergeError(path)
}
for k := range merged {
l.Documents[k] = merged[k]
@@ -85,6 +117,7 @@ func (l *loaded) Merge(path string, result interface{}) error {
func loadAllPaths(paths []string) (*loaded, error) {
root := newLoaded()
errors := loaderErrors{}
for _, path := range paths {
@@ -98,56 +131,59 @@ func loadAllPaths(paths []string) (*loaded, error) {
info, err := os.Stat(path)
if err != nil {
return nil, err
errors.Add(err)
continue
}
if info.IsDir() {
if err := loadDirRecursive(path, loaded.WithParent(info.Name())); err != nil {
return nil, err
}
loadDirRecursive(&errors, path, loaded.WithParent(info.Name()))
} else {
result, err := loadFile(path)
if err != nil {
return nil, err
}
if err := loaded.Merge(path, result); err != nil {
return nil, err
errors.Add(err)
} else {
if err := loaded.Merge(path, result); err != nil {
errors.Add(err)
}
}
}
}
if len(errors) > 0 {
return nil, errors
}
return root, nil
}
func loadDirRecursive(dirPath string, loaded *loaded) error {
func loadDirRecursive(errors *loaderErrors, dirPath string, loaded *loaded) {
files, err := ioutil.ReadDir(dirPath)
if err != nil {
return err
errors.Add(err)
return
}
for _, file := range files {
filePath := filepath.Join(dirPath, file.Name())
info, err := os.Stat(filePath)
if err != nil {
return err
}
if info.IsDir() {
if err := loadDirRecursive(filePath, loaded.WithParent(info.Name())); err != nil {
return err
}
errors.Add(err)
} else {
result, err := loadFileForKnownTypes(filePath)
if err != nil {
if _, ok := err.(unrecognizedFile); !ok {
return err
}
if info.IsDir() {
loadDirRecursive(errors, filePath, loaded.WithParent(info.Name()))
} else {
if err := loaded.Merge(filePath, result); err != nil {
return err
result, err := loadFileForKnownTypes(filePath)
if err != nil {
if _, ok := err.(unrecognizedFile); !ok {
errors.Add(err)
}
} else {
if err := loaded.Merge(filePath, result); err != nil {
errors.Add(err)
}
}
}
}
}
return nil
}
func loadFileForKnownTypes(path string) (interface{}, error) {
@@ -197,7 +233,10 @@ func jsonLoad(path string) (interface{}, error) {
defer f.Close()
decoder := util.NewJSONDecoder(f)
var x interface{}
return x, decoder.Decode(&x)
if err = decoder.Decode(&x); err != nil {
return nil, errors.Wrapf(err, path)
}
return x, nil
}
func regoLoad(path string) (interface{}, error) {
@@ -209,6 +248,9 @@ func regoLoad(path string) (interface{}, error) {
if err != nil {
return nil, err
}
if module == nil {
return nil, emptyModuleError(path)
}
result := &loadedModule{
Parsed: module,
Raw: bs,
@@ -228,13 +270,13 @@ func yamlLoad(path string) (interface{}, error) {
return x, nil
}
func makeDir(path []string, x interface{}) (map[string]interface{}, error) {
func makeDir(path []string, x interface{}) (map[string]interface{}, bool) {
if len(path) == 0 {
obj, ok := x.(map[string]interface{})
if !ok {
return nil, unsupportedDocumentType(fmt.Sprintf("%T", x))
return nil, false
}
return obj, nil
return obj, true
}
return makeDir(path[:len(path)-1], map[string]interface{}{path[len(path)-1]: x})
}
runtime/loader_test.go
+37
View File
@@ -10,6 +10,7 @@ import (
"path/filepath"
"reflect"
"sort"
"strings"
"testing"
"github.com/ghodss/yaml"
@@ -177,6 +178,42 @@ func TestLoadRooted(t *testing.T) {
})
}
func TestLoadErrors(t *testing.T) {
files := map[string]string{
"/x1.json": `{"x": [1,2,3]}`,
"/x2.json": `{"x": {"y": 1}}`,
"/empty.rego": ` `,
"/dir/a.json": ``,
"/dir/b.yaml": `
foo:
- bar:
`,
"/bad_doc.json": "[1,2,3]",
}
withTempFS(files, func(rootDir string) {
paths := mustListPaths(rootDir, false)[1:]
sort.Strings(paths)
_, err := loadAllPaths(paths)
if err == nil {
t.Fatalf("Expected failure")
}
expected := []string{
"bad_doc.json: bad document type",
"a.json: EOF",
"b.yaml: error converting YAML to JSON",
"empty.rego: empty policy",
"x2.json: merge error",
}
for _, s := range expected {
if !strings.Contains(err.Error(), s) {
t.Fatalf("Expected error to contain %v but got:\n%v", s, err)
}
}
})
}
func withTempFS(files map[string]string, f func(string)) {
rootDir, cleanup, err := makeTempFS(files)
if err != nil {
runtime/merge.go
+11 -19
View File
@@ -4,43 +4,35 @@
package runtime
import (
"fmt"
"github.com/pkg/errors"
)
// mergeDocs returns the result of merging a and b. If a and b cannot be merged
// because of conflicting key-value pairs, an error is returned.
func mergeDocs(a map[string]interface{}, b map[string]interface{}) (map[string]interface{}, error) {
// because of conflicting key-value pairs, ok is false.
func mergeDocs(a map[string]interface{}, b map[string]interface{}) (c map[string]interface{}, ok bool) {
merged := map[string]interface{}{}
c = map[string]interface{}{}
for k := range a {
merged[k] = a[k]
c[k] = a[k]
}
for k := range b {
add := b[k]
exist, ok := merged[k]
exist, ok := c[k]
if !ok {
merged[k] = add
c[k] = add
continue
}
existObj, existOk := exist.(map[string]interface{})
addObj, addOk := add.(map[string]interface{})
if !existOk || !addOk {
return nil, fmt.Errorf("%v: merge error: %T cannot merge into %T", k, add, exist)
return nil, false
}
mergedObj, err := mergeDocs(existObj, addObj)
if err != nil {
return nil, errors.Wrapf(err, k)
c[k], ok = mergeDocs(existObj, addObj)
if !ok {
return nil, false
}
merged[k] = mergedObj
}
return merged, nil
return c, true
}
runtime/merge_test.go
+19 -23
View File
@@ -5,7 +5,6 @@
package runtime
import (
"fmt"
"reflect"
"testing"
@@ -15,14 +14,15 @@ import (
func TestMergeDocs(t *testing.T) {
tests := []struct {
a string
b string
c interface{}
a string
b string
c string
ok bool
}{
{`{"x": 1, "y": 2}`, `{"z": 3}`, `{"x": 1, "y": 2, "z": 3}`},
{`{"x": {"y": 2}}`, `{"z": 3, "x": {"q": 4}}`, `{"x": {"y": 2, "q": 4}, "z": 3}`},
{`{"x": 1}`, `{"x": 1}`, fmt.Errorf("x: merge error: json.Number cannot merge into json.Number")},
{`{"x": {"y": [{"z": 2}]}}`, `{"x": {"y": [{"z": 3}]}}`, fmt.Errorf("x: y: merge error: []interface {} cannot merge into []interface {}")},
{`{"x": 1, "y": 2}`, `{"z": 3}`, `{"x": 1, "y": 2, "z": 3}`, true},
{`{"x": {"y": 2}}`, `{"z": 3, "x": {"q": 4}}`, `{"x": {"y": 2, "q": 4}, "z": 3}`, true},
{`{"x": 1}`, `{"x": 1}`, "", false},
{`{"x": {"y": [{"z": 2}]}}`, `{"x": {"y": [{"z": 3}]}}`, "", false},
}
for _, tc := range tests {
@@ -36,27 +36,23 @@ func TestMergeDocs(t *testing.T) {
panic(err)
}
switch c := tc.c.(type) {
case error:
_, err := mergeDocs(a, b)
if !reflect.DeepEqual(err.Error(), c.Error()) {
t.Errorf("Expected error to be exactly %v but got: %v", c, err)
if len(tc.c) == 0 {
c, ok := mergeDocs(a, b)
if ok {
t.Errorf("Expected merge(%v,%v) == false but got: %v", a, b, c)
}
case string:
} else {
expected := map[string]interface{}{}
if err := util.UnmarshalJSON([]byte(c), &expected); err != nil {
if err := util.UnmarshalJSON([]byte(tc.c), &expected); err != nil {
panic(err)
}
result, err := mergeDocs(a, b)
if err != nil {
t.Errorf("Unexpected error on merge(%v, %v): %v", a, b, err)
continue
}
if !reflect.DeepEqual(result, expected) {
t.Errorf("Expected merge(%v, %v) to be %v but got: %v", a, b, expected, result)
c, ok := mergeDocs(a, b)
if !ok || !reflect.DeepEqual(c, expected) {
t.Errorf("Expected merge(%v, %v) == %v but got: %v (ok: %v)", a, b, expected, c, ok)
}
}
}