diff --git a/ast/parser_ext.go b/ast/parser_ext.go index 7693d62036..d20bb8a51c 100644 --- a/ast/parser_ext.go +++ b/ast/parser_ext.go @@ -763,12 +763,13 @@ func (vt *varToRefTransformer) Transform(x interface{}) (interface{}, error) { return x, nil } -type parserErrorDetail struct { - line string - idx int +// ParserErrorDetail holds additional details for parser errors. +type ParserErrorDetail struct { + Line string `json:"line"` + Idx int `json:"idx"` } -func newParserErrorDetail(bs []byte, pos position) *parserErrorDetail { +func newParserErrorDetail(bs []byte, pos position) *ParserErrorDetail { offset := pos.offset @@ -809,16 +810,17 @@ func newParserErrorDetail(bs []byte, pos position) *parserErrorDetail { line := bs[begin:end] index := offset - begin - return &parserErrorDetail{ - line: string(line), - idx: index, + return &ParserErrorDetail{ + Line: string(line), + Idx: index, } } -func (d parserErrorDetail) Lines() []string { - line := strings.TrimLeft(d.line, "\t") // remove leading tabs - tabCount := len(d.line) - len(line) - return []string{line, strings.Repeat(" ", d.idx-tabCount) + "^"} +// Lines returns the pretty formatted line output for the error details. +func (d ParserErrorDetail) Lines() []string { + line := strings.TrimLeft(d.Line, "\t") // remove leading tabs + tabCount := len(d.Line) - len(line) + return []string{line, strings.Repeat(" ", d.Idx-tabCount) + "^"} } func isNewLineChar(b byte) bool { diff --git a/ast/parser_test.go b/ast/parser_test.go index 8459035c0c..c7b6cb526e 100644 --- a/ast/parser_test.go +++ b/ast/parser_test.go @@ -1542,15 +1542,15 @@ func TestParseErrorDetails(t *testing.T) { tests := []struct { note string - exp *parserErrorDetail + exp *ParserErrorDetail err string input string }{ { note: "no match: bad rule name", - exp: &parserErrorDetail{ - line: ".", - idx: 0, + exp: &ParserErrorDetail{ + Line: ".", + Idx: 0, }, input: ` package test @@ -1558,54 +1558,54 @@ package test }, { note: "no match: bad termination for comprehension", - exp: &parserErrorDetail{ - line: "p = [true | true}", - idx: 16, + exp: &ParserErrorDetail{ + Line: "p = [true | true}", + Idx: 16, }, input: ` package test p = [true | true}`}, { note: "no match: non-terminated comprehension", - exp: &parserErrorDetail{ - line: "p = [true | true", - idx: 15, + exp: &ParserErrorDetail{ + Line: "p = [true | true", + Idx: 15, }, input: ` package test p = [true | true`}, { note: "no match: expected expression", - exp: &parserErrorDetail{ - line: "p { true; }", - idx: 10, + exp: &ParserErrorDetail{ + Line: "p { true; }", + Idx: 10, }, input: ` package test p { true; }`}, { note: "empty body", - exp: &parserErrorDetail{ - line: "p { }", - idx: 2, + exp: &ParserErrorDetail{ + Line: "p { }", + Idx: 2, }, input: ` package test p { }`}, { note: "non-terminated string", - exp: &parserErrorDetail{ - line: `p = "foo`, - idx: 4, + exp: &ParserErrorDetail{ + Line: `p = "foo`, + Idx: 4, }, input: ` package test p = "foo`}, { note: "rule with error begins with one tab", - exp: &parserErrorDetail{ - line: "\tas", - idx: 2, + exp: &ParserErrorDetail{ + Line: "\tas", + Idx: 2, }, input: ` package test @@ -1615,9 +1615,9 @@ package test ^`}, { note: "rule term with error begins with two tabs", - exp: &parserErrorDetail{ - line: "\t\tas", - idx: 3, + exp: &ParserErrorDetail{ + Line: "\t\tas", + Idx: 3, }, input: ` package test diff --git a/cmd/check.go b/cmd/check.go index 394667889d..091154ea00 100644 --- a/cmd/check.go +++ b/cmd/check.go @@ -5,13 +5,14 @@ package cmd import ( - "encoding/json" "fmt" + "io" "os" "github.com/spf13/cobra" "github.com/open-policy-agent/opa/ast" + pr "github.com/open-policy-agent/opa/internal/presentation" "github.com/open-policy-agent/opa/loader" "github.com/open-policy-agent/opa/util" ) @@ -99,14 +100,18 @@ func checkModules(args []string) int { func outputErrors(err error) { switch checkParams.format.String() { case checkFormatJSON: - result := map[string]error{ - "errors": err, + result := pr.Output{ + Errors: pr.NewOutputErrors(err), } - bs, err := json.MarshalIndent(result, "", " ") + var out io.Writer if err != nil { - fmt.Fprintln(os.Stderr, err) + out = os.Stderr } else { - fmt.Fprintln(os.Stdout, string(bs)) + out = os.Stdout + } + err := pr.JSON(out, result) + if err != nil { + fmt.Fprintln(os.Stderr, err.Error()) } default: fmt.Fprintln(os.Stdout, err) diff --git a/cmd/eval.go b/cmd/eval.go index 978852db72..ee316818c4 100644 --- a/cmd/eval.go +++ b/cmd/eval.go @@ -330,25 +330,28 @@ func eval(args []string, params evalCommandParams, w io.Writer) (bool, error) { eval := rego.New(regoArgs...) var result pr.Output + var resultErr error var parsedModules map[string]*ast.Module if !params.partial { var pq rego.PreparedEvalQuery - pq, result.Error = eval.PrepareForEval(ctx) - if result.Error == nil { + pq, resultErr = eval.PrepareForEval(ctx) + if resultErr == nil { parsedModules = pq.Modules() - result.Result, result.Error = pq.Eval(ctx) + result.Result, resultErr = pq.Eval(ctx) } } else { var pq rego.PreparedPartialQuery - pq, result.Error = eval.PrepareForPartial(ctx) - if result.Error == nil { + pq, resultErr = eval.PrepareForPartial(ctx) + if resultErr == nil { parsedModules = pq.Modules() - result.Partial, result.Error = eval.Partial(ctx) + result.Partial, resultErr = eval.Partial(ctx) } } + result.Errors = pr.NewOutputErrors(resultErr) + switch params.explain.String() { case explainModeFull: result.Explanation = *tracer @@ -390,7 +393,7 @@ func eval(args []string, params evalCommandParams, w io.Writer) (bool, error) { if err != nil { return false, err - } else if result.Error != nil { + } else if len(result.Errors) > 0 { // If the rego package returned an error, return a special error here so // that the command doesn't print the same error twice. The error will // have been printed above by the presentation package. diff --git a/cmd/eval_test.go b/cmd/eval_test.go index 5ef1e34d1a..c04a4fc6ad 100755 --- a/cmd/eval_test.go +++ b/cmd/eval_test.go @@ -288,7 +288,7 @@ func TestEvalErrorJSONOutput(t *testing.T) { t.Fatal(err) } - if output["error"] == nil { + if output["errors"] == nil { t.Fatalf("Expected error to be non-nil") } diff --git a/cmd/parse.go b/cmd/parse.go index 054421f47d..7eddeb3de2 100644 --- a/cmd/parse.go +++ b/cmd/parse.go @@ -12,6 +12,7 @@ import ( "github.com/spf13/cobra" "github.com/open-policy-agent/opa/ast" + pr "github.com/open-policy-agent/opa/internal/presentation" "github.com/open-policy-agent/opa/loader" "github.com/open-policy-agent/opa/util" ) @@ -49,13 +50,14 @@ func parse(args []string) int { } result, err := loader.Rego(args[0]) - if err != nil { - fmt.Fprintln(os.Stderr, err) - return 1 - } switch parseParams.format.String() { case parseFormatJSON: + if err != nil { + pr.JSON(os.Stderr, pr.Output{Errors: pr.NewOutputErrors(err)}) + return 1 + } + bs, err := json.MarshalIndent(result.Parsed, "", " ") if err != nil { fmt.Fprintln(os.Stderr, err) diff --git a/internal/presentation/presentation.go b/internal/presentation/presentation.go index ba719295ca..9ec89a5ed0 100644 --- a/internal/presentation/presentation.go +++ b/internal/presentation/presentation.go @@ -21,9 +21,11 @@ import ( "github.com/open-policy-agent/opa/ast" "github.com/open-policy-agent/opa/cover" "github.com/open-policy-agent/opa/format" + "github.com/open-policy-agent/opa/loader" "github.com/open-policy-agent/opa/metrics" "github.com/open-policy-agent/opa/profiler" "github.com/open-policy-agent/opa/rego" + "github.com/open-policy-agent/opa/storage" "github.com/open-policy-agent/opa/topdown" ) @@ -107,7 +109,7 @@ func (o DepAnalysisOutput) sort() { // Output contains the result of evaluation to be presented. type Output struct { - Error error `json:"error,omitempty"` + Errors OutputErrors `json:"errors,omitempty"` Result rego.ResultSet `json:"result,omitempty"` Partial *rego.PartialQueries `json:"partial,omitempty"` Metrics metrics.Metrics `json:"metrics,omitempty"` @@ -127,6 +129,114 @@ func (e Output) undefined() bool { return len(e.Result) == 0 && (e.Partial == nil || len(e.Partial.Queries) == 0) } +// NewOutputErrors creates a new slice of OutputError's based +// on the type of error passed in. Known structured types will +// be translated as appropriate, while unknown errors are +// placed into a structured format with their string value. +func NewOutputErrors(err error) []OutputError { + var errs []OutputError + if err != nil { + // Handle known structured errors + + switch typedErr := err.(type) { + case *ast.Error: + oe := OutputError{ + Code: typedErr.Code, + Message: typedErr.Message, + Details: typedErr.Details, + err: typedErr, + } + + // TODO(patrick-east): Why does the JSON marshaller marshal + // location as `null` when err.location == nil?! + if typedErr.Location != nil { + oe.Location = typedErr.Location + } + errs = []OutputError{oe} + case *topdown.Error: + errs = []OutputError{{ + Code: typedErr.Code, + Message: typedErr.Message, + Location: typedErr.Location, + err: typedErr, + }} + case *storage.Error: + errs = []OutputError{{ + Code: typedErr.Code, + Message: typedErr.Message, + err: typedErr, + }} + + // The cases below are wrappers for other errors, format errors + // recursively on them. + case ast.Errors: + for _, e := range typedErr { + if e != nil { + errs = append(errs, NewOutputErrors(e)...) + } + } + case rego.Errors: + for _, e := range typedErr { + if e != nil { + errs = append(errs, NewOutputErrors(e)...) + } + } + case loader.Errors: + { + for _, e := range typedErr { + if e != nil { + errs = append(errs, NewOutputErrors(e)...) + } + } + } + default: + // Any errors which don't have a structure we know about + // are converted to their string representation only. + errs = []OutputError{{ + Message: err.Error(), + err: typedErr, + }} + } + } + return errs +} + +// OutputErrors is a list of errors encountered +// which are to presented. +type OutputErrors []OutputError + +func (e OutputErrors) Error() string { + if len(e) == 0 { + return "no error(s)" + } + + if len(e) == 1 { + return fmt.Sprintf("1 error occurred: %v", e[0].Error()) + } + + var s []string + for _, err := range e { + s = append(s, err.Error()) + } + + return fmt.Sprintf("%d errors occurred:\n%s", len(e), strings.Join(s, "\n")) +} + +// OutputError provides a common structure for all OPA +// library errors so that the JSON output given by the +// presentation package is consistent and parsable. +type OutputError struct { + Message string `json:"message"` + Code string `json:"code,omitempty"` + Location interface{} `json:"location,omitempty"` + Details interface{} `json:"details,omitempty"` + err error +} + +func (j OutputError) Error() string { + return j.err.Error() +} + // JSON writes x to w with indentation. func JSON(w io.Writer, x interface{}) error { encoder := json.NewEncoder(w) @@ -136,8 +246,8 @@ func JSON(w io.Writer, x interface{}) error { // Bindings prints the bindings from r to w. func Bindings(w io.Writer, r Output) error { - if r.Error != nil { - return prettyError(w, r.Error) + if r.Errors != nil { + return prettyError(w, r.Errors) } for _, rs := range r.Result { if err := JSON(w, rs.Bindings); err != nil { @@ -149,8 +259,8 @@ func Bindings(w io.Writer, r Output) error { // Values prints the values from r to w. func Values(w io.Writer, r Output) error { - if r.Error != nil { - return prettyError(w, r.Error) + if r.Errors != nil { + return prettyError(w, r.Errors) } for _, rs := range r.Result { line := make([]interface{}, len(rs.Expressions)) @@ -171,8 +281,8 @@ func Pretty(w io.Writer, r Output) error { return err } } - if r.Error != nil { - if err := prettyError(w, r.Error); err != nil { + if r.Errors != nil { + if err := prettyError(w, r.Errors); err != nil { return err } } else if r.undefined() { diff --git a/internal/presentation/presentation_test.go b/internal/presentation/presentation_test.go new file mode 100644 index 0000000000..0f12c2cd56 --- /dev/null +++ b/internal/presentation/presentation_test.go @@ -0,0 +1,360 @@ +// Copyright 2019 The OPA Authors. All rights reserved. +// Use of this source code is governed by an Apache2 +// license that can be found in the LICENSE file. + +package presentation + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "testing" + + "github.com/open-policy-agent/opa/ast" + "github.com/open-policy-agent/opa/loader" + "github.com/open-policy-agent/opa/rego" + "github.com/open-policy-agent/opa/storage" + "github.com/open-policy-agent/opa/storage/inmem" + "github.com/open-policy-agent/opa/util/test" +) + +type testErrorWithMarshaller struct { + msg string +} + +func (t *testErrorWithMarshaller) Error() string { + return t.msg +} + +func (t *testErrorWithMarshaller) MarshalJSON() ([]byte, error) { + return json.Marshal(struct { + Text string `json:"text"` + }{ + Text: t.msg, + }) +} + +func validateJSONOutput(t *testing.T, testErr error, expected string) { + t.Helper() + output := Output{Errors: NewOutputErrors(testErr)} + var buf bytes.Buffer + err := JSON(&buf, output) + if err != nil { + t.Fatalf("Unexpected error: %s", err) + } + + if buf.String() != expected { + t.Fatalf("Unexpected marshalled error value.\n Expected (len=%d):\n>>>\n%s\n<<<\n\nActual (len=%d):\n>>>>\n%s\n<<<<\n", + len(expected), expected, len(buf.String()), buf.String()) + } +} + +func TestOutputJSONErrorUnstructured(t *testing.T) { + err := errors.New("some text") + expected := `{ + "errors": [ + { + "message": "some text" + } + ] +} +` + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorCustomMarshaller(t *testing.T) { + err := &testErrorWithMarshaller{ + msg: "custom message", + } + expected := `{ + "errors": [ + { + "message": "custom message" + } + ] +} +` + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorStructuredASTErr(t *testing.T) { + err := &ast.Error{ + Code: "1", + Message: "error message", + } + expected := `{ + "errors": [ + { + "message": "error message", + "code": "1" + } + ] +} +` + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorStructuredStorageErr(t *testing.T) { + store := inmem.New() + txn := storage.NewTransactionOrDie(context.Background(), store) + err := store.Write(context.Background(), txn, storage.AddOp, storage.Path{}, map[string]interface{}{"foo": 1}) + expected := `{ + "errors": [ + { + "message": "data write during read transaction", + "code": "storage_invalid_txn_error" + } + ] +} +` + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorStructuredTopdownErr(t *testing.T) { + mod := ` + package test + + p(x) = y { + y = x[_] + } + + z := p([1, 2, 3]) + ` + + _, err := rego.New( + rego.Module("test.rego", mod), + rego.Query("data.test.z"), + ).Eval(context.Background()) + + expected := `{ + "errors": [ + { + "message": "functions must not produce multiple outputs for same inputs", + "code": "eval_conflict_error", + "location": { + "file": "test.rego", + "row": 4, + "col": 3 + } + } + ] +} +` + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorStructuredAstErr(t *testing.T) { + _, err := rego.New(rego.Query("count(0)")).Eval(context.Background()) + expected := `{ + "errors": [ + { + "message": "count: invalid argument(s)", + "code": "rego_type_error", + "location": { + "file": "", + "row": 1, + "col": 1 + }, + "details": { + "have": [ + { + "type": "number" + }, + null + ], + "want": [ + { + "of": [ + { + "of": { + "of": [], + "type": "any" + }, + "type": "set" + }, + { + "dynamic": { + "of": [], + "type": "any" + }, + "type": "array" + }, + { + "dynamic": { + "key": { + "of": [], + "type": "any" + }, + "value": { + "of": [], + "type": "any" + } + }, + "type": "object" + }, + { + "type": "string" + } + ], + "type": "any" + }, + { + "type": "number" + } + ] + } + } + ] +} +` + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorStructuredAstParseErr(t *testing.T) { + _, err := rego.New( + rego.Module("parse-err.rego", "!!!"), + rego.Query("!!!"), + ).Eval(context.Background()) + + expected := `{ + "errors": [ + { + "message": "no match found", + "code": "rego_parse_error", + "location": { + "file": "parse-err.rego", + "row": 1, + "col": 1 + }, + "details": { + "line": "!!!", + "idx": 0 + } + } + ] +} +` + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorStructuredASTErrList(t *testing.T) { + c := ast.NewCompiler() + c.Compile(map[string]*ast.Module{ + "error.rego": ast.MustParseModule(` +package test + +q { + bad[reference] +} +`)}) + c.Errors.Sort() + err := c.Errors + + expected := `{ + "errors": [ + { + "message": "var bad is unsafe", + "code": "rego_unsafe_var_error", + "location": { + "file": "", + "row": 5, + "col": 2 + } + }, + { + "message": "var reference is unsafe", + "code": "rego_unsafe_var_error", + "location": { + "file": "", + "row": 5, + "col": 2 + } + } + ] +} +` + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorStructuredLoaderErrList(t *testing.T) { + files := map[string]string{ + // bundle a + "a/data.json": "{{{", + "b/data.json": "...", + } + + var err error + var tmpPath string + test.WithTempFS(files, func(path string) { + tmpPath = path + _, err = loader.All([]string{path}) + }) + + expected := fmt.Sprintf(`{ + "errors": [ + { + "message": "%s/a/data.json: invalid character '{' looking for beginning of object key string" + }, + { + "message": "%s/b/data.json: invalid character '.' looking for beginning of value" + } + ] +} +`, tmpPath, tmpPath) + + validateJSONOutput(t, err, expected) +} + +func TestOutputJSONErrorStructuredRegoErrList(t *testing.T) { + mod := ` +package test + +p { + bad_func1() +} + +q { + bad_func2() +} +` + _, err := rego.New( + rego.Module("error.rego", mod), + rego.Query("data"), + ).PrepareForEval(context.Background()) + + expected := `{ + "errors": [ + { + "message": "undefined function bad_func1", + "code": "rego_type_error", + "location": { + "file": "error.rego", + "row": 5, + "col": 2 + } + }, + { + "message": "undefined function bad_func2", + "code": "rego_type_error", + "location": { + "file": "error.rego", + "row": 9, + "col": 2 + } + } + ] +} +` + + validateJSONOutput(t, err, expected) +} diff --git a/loader/errors.go b/loader/errors.go index 44098d3b0d..95cebe0a6d 100644 --- a/loader/errors.go +++ b/loader/errors.go @@ -11,9 +11,10 @@ import ( "github.com/open-policy-agent/opa/ast" ) -type loaderErrors []error +// Errors is a wrapper for multiple loader errors. +type Errors []error -func (e loaderErrors) Error() string { +func (e Errors) Error() string { if len(e) == 0 { return "no error(s)" } @@ -27,7 +28,7 @@ func (e loaderErrors) Error() string { return fmt.Sprintf("%v errors occurred during loading:\n", len(e)) + strings.Join(buf, "\n") } -func (e *loaderErrors) Add(err error) { +func (e *Errors) add(err error) { if errs, ok := err.(ast.Errors); ok { for i := range errs { *e = append(*e, errs[i]) diff --git a/loader/loader.go b/loader/loader.go index d41d4b567a..14d1661c07 100644 --- a/loader/loader.go +++ b/loader/loader.go @@ -248,7 +248,7 @@ func (l *Result) withParent(p string) *Result { } func all(paths []string, filter Filter, f func(*Result, string, int) error) (*Result, error) { - errors := loaderErrors{} + errors := Errors{} root := newResult() for _, path := range paths { @@ -274,17 +274,17 @@ func all(paths []string, filter Filter, f func(*Result, string, int) error) (*Re return root, nil } -func allRec(path string, filter Filter, errors *loaderErrors, loaded *Result, depth int, f func(*Result, string, int) error) { +func allRec(path string, filter Filter, errors *Errors, loaded *Result, depth int, f func(*Result, string, int) error) { path, err := fileurl.Clean(path) if err != nil { - errors.Add(err) + errors.add(err) return } info, err := os.Stat(path) if err != nil { - errors.Add(err) + errors.add(err) return } @@ -294,7 +294,7 @@ func allRec(path string, filter Filter, errors *loaderErrors, loaded *Result, de if !info.IsDir() { if err := f(loaded, path, depth); err != nil { - errors.Add(err) + errors.add(err) } return } @@ -307,7 +307,7 @@ func allRec(path string, filter Filter, errors *loaderErrors, loaded *Result, de files, err := ioutil.ReadDir(path) if err != nil { - errors.Add(err) + errors.add(err) return } diff --git a/rego/rego.go b/rego/rego.go index d3293adc7d..c9d6de343f 100644 --- a/rego/rego.go +++ b/rego/rego.go @@ -1254,7 +1254,7 @@ func (r *Rego) parseModules(ctx context.Context, txn storage.Transaction, m metr } if len(errs) > 0 { - return errors.New(errs.Error()) + return errs } return nil @@ -1266,7 +1266,7 @@ func (r *Rego) loadFiles(ctx context.Context, txn storage.Transaction, m metrics result, err := loader.Filtered(r.loadPaths.paths, r.loadPaths.filter) if err != nil { - return fmt.Errorf("error loading paths: %s", err) + return err } for name, mod := range result.Modules { r.parsedModules[name] = mod.Parsed @@ -1275,7 +1275,7 @@ func (r *Rego) loadFiles(ctx context.Context, txn storage.Transaction, m metrics if len(result.Documents) > 0 { err = r.store.Write(ctx, txn, storage.AddOp, storage.Path{}, result.Documents) if err != nil { - return fmt.Errorf("error writing loaded documents to store: %s", err) + return err } } return nil diff --git a/repl/repl.go b/repl/repl.go index 50c3734fc1..89eb63c4bf 100644 --- a/repl/repl.go +++ b/repl/repl.go @@ -864,7 +864,7 @@ func (r *REPL) evalBody(ctx context.Context, compiler *ast.Compiler, input ast.V rs, err := eval.Eval(ctx) output := pr.Output{ - Error: err, + Errors: pr.NewOutputErrors(err), Result: rs, Metrics: r.metrics, } @@ -920,7 +920,7 @@ func (r *REPL) evalPartial(ctx context.Context, compiler *ast.Compiler, input as output := pr.Output{ Metrics: r.metrics, Partial: pq, - Error: err, + Errors: pr.NewOutputErrors(err), } switch r.explain { diff --git a/server/server_test.go b/server/server_test.go index e9c95060ce..17de642306 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -3027,7 +3027,10 @@ func TestBadQueryV1(t *testing.T) { "row": 1, "col": 1 }, - "details": {} + "details": { + "line": "^ -i", + "idx": 0 + } } ] }`