refactor: http error serialization matches the new error schema (infl…

…uxdata#15196)

The http error schema has been changed to simplify the outward facing
API. The `op` and `error` attributes have been dropped because they
confused people. The `error` attribute will likely be readded in some
form in the future, but only as additional context and will not be
required or even suggested for the UI to use.

Errors are now output differently both when they are serialized to JSON
and when they are output as strings. The `op` is no longer used if it is
present. It will only appear as an optional attribute if at all. The
`message` attribute for an error is always output and it will be the
prefix for any nested error. When this is serialized to JSON, the
message is automatically flattened so a nested error such as:

    influxdb.Error{
        Msg: errors.New("something bad happened"),
        Err: io.EOF,
    }

This would be written to the message as:

    something bad happened: EOF

This matches a developers expectations much more easily as most
programmers assume that wrapping an error will act as a prefix for the
inner error.

This is flattened when written out to HTTP in order to make this logic
immaterial to a frontend developer.

The code is still present and plays an important role in categorizing
the error type. On the other hand, the code will not be output as part
of the message as it commonly plays a redundant and confusing role when
humans read it. The human readable message usually gives more context
and a message like with the code acting as a prefix is generally not
desired. But, the code plays a very important role in helping to
identify categories of errors and so it is very important as part of the
return response.

authorizer/task_test.go

@@ -285,7 +285,7 @@ from(bucket:"bad") |> range(start:-5m) |> to(bucket:"bad", org:"thing")`,
 					return fmt.Errorf(errfmt, "platform.Error.Err to be present", perr.Err)
 				}
 
-				if !strings.Contains(perr.Err.Error(), "<not found> bucket \"bad\" not found") {
+				if !strings.Contains(perr.Err.Error(), "bucket \"bad\" not found") {
 					return fmt.Errorf(errfmt, "to container bucket not found", perr.Err)
 				}
 

bolt/passwords.go

@@ -16,13 +16,15 @@ var (
 	// EIncorrectPassword is returned when any password operation fails in which
 	// we do not want to leak information.
 	EIncorrectPassword = &platform.Error{
-		Msg: "<forbidden> your username or password is incorrect",
+		Code: platform.EForbidden,
+		Msg:  "your username or password is incorrect",
 	}
 
 	// EShortPassword is used when a password is less than the minimum
 	// acceptable password length.
 	EShortPassword = &platform.Error{
-		Msg: "<invalid> passwords must be at least 8 characters long",
+		Code: platform.EInvalid,
+		Msg:  "passwords must be at least 8 characters long",
 	}
 )
 

errors.go

@@ -100,29 +100,20 @@ func WithErrorOp(op string) func(*Error) {
 	}
 }
 
-// Error implement the error interface by outputing the Code and Err.
+// Error implements the error interface by writing out the recursive messages.
 func (e *Error) Error() string {
-	var b strings.Builder
-
-	// Print the current operation in our stack, if any.
-	if e.Op != "" {
-		fmt.Fprintf(&b, "%s: ", e.Op)
-	}
-
-	// If wrapping an error, print its Error() message.
-	// Otherwise print the error code & message.
-	if e.Err != nil {
-		b.WriteString(e.Err.Error())
-	} else {
-		if e.Code != "" {
-			fmt.Fprintf(&b, "<%s>", e.Code)
-			if e.Msg != "" {
-				b.WriteString(" ")
-			}
-		}
+	if e.Msg != "" && e.Err != nil {
+		var b strings.Builder
 		b.WriteString(e.Msg)
+		b.WriteString(": ")
+		b.WriteString(e.Err.Error())
+		return b.String()
+	} else if e.Msg != "" {
+		return e.Msg
+	} else if e.Err != nil {
+		return e.Err.Error()
 	}
-	return b.String()
+	return fmt.Sprintf("<%s>", e.Code)
 }
 
 // ErrorCode returns the code of the root error, if available; otherwise returns EINTERNAL.

errors_test.go

@@ -23,39 +23,52 @@ func TestErrorMsg(t *testing.T) {
 			msg:  "<not found>",
 		},
 		{
-			name: "with op",
+			name: "with message",
 			err: &platform.Error{
 				Code: platform.ENotFound,
-				Op:   "bolt.FindAuthorizationByID",
+				Msg:  "bucket not found",
 			},
-			msg: "bolt.FindAuthorizationByID: <not found>",
+			msg: "bucket not found",
 		},
 		{
-			name: "with op and value",
+			name: "with a third party error and no message",
 			err: &platform.Error{
-				Code: platform.ENotFound,
-				Op:   "bolt.FindAuthorizationByID",
-				Msg:  fmt.Sprintf("with ID %d", 323),
+				Code: EFailedToGetStorageHost,
+				Err:  errors.New("empty value"),
 			},
-			msg: "bolt.FindAuthorizationByID: <not found> with ID 323",
+			msg: "empty value",
 		},
 		{
-			name: "with a third party error",
+			name: "with a third party error and a message",
 			err: &platform.Error{
 				Code: EFailedToGetStorageHost,
-				Op:   "cmd/fluxd.injectDeps",
+				Msg:  "failed to get storage hosts",
 				Err:  errors.New("empty value"),
 			},
-			msg: "cmd/fluxd.injectDeps: empty value",
+			msg: "failed to get storage hosts: empty value",
 		},
 		{
-			name: "with a internal error",
+			name: "with an internal error and no message",
 			err: &platform.Error{
 				Code: EFailedToGetStorageHost,
-				Op:   "cmd/fluxd.injectDeps",
-				Err:  &platform.Error{Code: platform.EEmptyValue, Op: "cmd/fluxd.getStrList"},
+				Err: &platform.Error{
+					Code: platform.EEmptyValue,
+					Msg:  "empty value",
+				},
+			},
+			msg: "empty value",
+		},
+		{
+			name: "with an internal error and a message",
+			err: &platform.Error{
+				Code: EFailedToGetStorageHost,
+				Msg:  "failed to get storage hosts",
+				Err: &platform.Error{
+					Code: platform.EEmptyValue,
+					Msg:  "empty value",
+				},
 			},
-			msg: "cmd/fluxd.injectDeps: cmd/fluxd.getStrList: <empty value>",
+			msg: "failed to get storage hosts: empty value",
 		},
 	}
 	for _, c := range cases {

http/dashboard_test.go

@@ -991,7 +991,7 @@ func TestService_handlePostDashboardCell(t *testing.T) {
 			wants: wants{
 				statusCode:  http.StatusBadRequest,
 				contentType: "application/json; charset=utf-8",
-				body:        `{"code":"invalid","message":"bad request json body","error":"EOF"}`,
+				body:        `{"code":"invalid","message":"bad request json body: EOF"}`,
 			},
 		},
 		{
@@ -1117,7 +1117,7 @@ func TestService_handlePostDashboardCell(t *testing.T) {
 				t.Errorf("%q. handlePostDashboardCell() = %v, want %v", tt.name, content, tt.wants.contentType)
 			}
 			if tt.wants.body != "" {
-				if eq, diff, err := jsonEqual(string(body), tt.wants.body); err != nil {
+				if eq, diff, err := jsonEqual(tt.wants.body, string(body)); err != nil {
 					t.Errorf("%q, handlePostDashboardCell(). error unmarshaling json %v", tt.name, err)
 				} else if !eq {
 					t.Errorf("%q. handlePostDashboardCell() = ***%s***", tt.name, diff)

http/document_test.go

@@ -786,7 +786,7 @@ func TestService_handlePostDocuments(t *testing.T) {
 			wants: wants{
 				statusCode:  http.StatusBadRequest,
 				contentType: "application/json; charset=utf-8",
-				body:        `{"code":"invalid","error":"EOF","message": "document body error"}`,
+				body:        `{"code":"invalid","message": "document body error: EOF"}`,
 			},
 		},
 		{
@@ -933,7 +933,7 @@ func TestService_handlePostDocuments(t *testing.T) {
 				t.Errorf("%q. handlePostDocument() = %v, want %v", tt.name, content, tt.wants.contentType)
 			}
 			if tt.wants.body != "" {
-				if eq, diff, err := jsonEqual(string(body), tt.wants.body); err != nil {
+				if eq, diff, err := jsonEqual(tt.wants.body, string(body)); err != nil {
 					t.Errorf("%q, handlePostDocument(). error unmarshaling json %v", tt.name, err)
 				} else if !eq {
 					t.Errorf("%q. handlePostDocument() = ***%s***", tt.name, diff)

http/errors.go

@@ -114,19 +114,15 @@ func (h ErrorHandler) HandleHTTPError(ctx context.Context, err error, w http.Res
 	w.Header().Set(PlatformErrorCodeHeader, code)
 	w.Header().Set("Content-Type", "application/json; charset=utf-8")
 	w.WriteHeader(httpCode)
-	var e error
-	if pe, ok := err.(*platform.Error); ok {
-		e = &platform.Error{
-			Code: code,
-			Op:   platform.ErrorOp(err),
-			Msg:  platform.ErrorMessage(err),
-			Err:  pe.Err,
-		}
+	var e struct {
+		Code    string `json:"code"`
+		Message string `json:"message"`
+	}
+	e.Code = platform.ErrorCode(err)
+	if err, ok := err.(*platform.Error); ok {
+		e.Message = err.Error()
 	} else {
-		e = &platform.Error{
-			Code: platform.EInternal,
-			Err:  err,
-		}
+		e.Message = "An internal error has occurred"
 	}
 	b, _ := json.Marshal(e)
 	_, _ = w.Write(b)

http/errors_test.go

@@ -29,7 +29,11 @@ func TestEncodeError(t *testing.T) {
 
 func TestEncodeErrorWithError(t *testing.T) {
 	ctx := context.TODO()
-	err := fmt.Errorf("there's an error here, be aware")
+	err := &influxdb.Error{
+		Code: influxdb.EInternal,
+		Msg:  "an error occurred",
+		Err:  fmt.Errorf("there's an error here, be aware"),
+	}
 
 	w := httptest.NewRecorder()
 
@@ -44,14 +48,15 @@ func TestEncodeErrorWithError(t *testing.T) {
 		t.Errorf("expected X-Platform-Error-Code: %s, got: %s", influxdb.EInternal, errHeader)
 	}
 
-	expected := &influxdb.Error{
-		Code: influxdb.EInternal,
-		Err:  err,
+	// The http handler will flatten the message and it will not
+	// have an error property, so reading the serialization results
+	// in a different error.
+	pe := http.CheckError(w.Result()).(*influxdb.Error)
+	if want, got := influxdb.EInternal, pe.Code; want != got {
+		t.Errorf("unexpected code -want/+got:\n\t- %q\n\t+ %q", want, got)
 	}
-
-	pe := http.CheckError(w.Result())
-	if pe.(*influxdb.Error).Err.Error() != expected.Err.Error() {
-		t.Errorf("errors encode err: got %s", w.Body.String())
+	if want, got := "an error occurred: there's an error here, be aware", pe.Msg; want != got {
+		t.Errorf("unexpected message -want/+got:\n\t- %q\n\t+ %q", want, got)
 	}
 }
 

http/query_handler_test.go

@@ -250,7 +250,7 @@ func TestFluxHandler_postFluxAST(t *testing.T) {
 			name:   "error from bad json",
 			w:      httptest.NewRecorder(),
 			r:      httptest.NewRequest("POST", "/api/v2/query/ast", bytes.NewBufferString(`error!`)),
-			want:   `{"code":"invalid","message":"invalid json","error":"invalid character 'e' looking for beginning of value"}`,
+			want:   `{"code":"invalid","message":"invalid json: invalid character 'e' looking for beginning of value"}`,
 			status: http.StatusBadRequest,
 		},
 	}

http/router_test.go

@@ -173,8 +173,7 @@ func TestRouter_Panic(t *testing.T) {
 				body: `
 {
   "code": "internal error",
-  "message": "a panic has occurred",
-  "error": "not implemented"
+  "message": "a panic has occurred: not implemented"
 }`,
 			},
 		},
@@ -207,7 +206,7 @@ func TestRouter_Panic(t *testing.T) {
 			if tt.wants.contentType != "" && content != tt.wants.contentType {
 				t.Errorf("%q. get %v, want %v", tt.name, content, tt.wants.contentType)
 			}
-			if eq, diff, _ := jsonEqual(string(body), tt.wants.body); tt.wants.body != "" && !eq {
+			if eq, diff, _ := jsonEqual(tt.wants.body, string(body)); tt.wants.body != "" && !eq {
 				t.Errorf("%q. get ***%s***", tt.name, diff)
 			}
 			if tt.wants.logged != tw.Logged() {

http/task_service_test.go

@@ -380,12 +380,7 @@ func TestTaskHandler_handleGetTasks(t *testing.T) {
 				contentType: "application/json; charset=utf-8",
 				body: `{
 "code": "invalid",
-"error": {
-"code": "not found",
-"error": "org not found or unauthorized",
-"message": "org non-existent-org not found or unauthorized"
-},
-"message": "failed to decode request"
+"message": "failed to decode request: org non-existent-org not found or unauthorized: org not found or unauthorized"
 }`,
 			},
 		},
@@ -414,7 +409,7 @@ func TestTaskHandler_handleGetTasks(t *testing.T) {
 				t.Errorf("%q. handleGetTasks() = %v, want %v", tt.name, content, tt.wants.contentType)
 			}
 			if tt.wants.body != "" {
-				if eq, diff, err := jsonEqual(string(body), tt.wants.body); err != nil {
+				if eq, diff, err := jsonEqual(tt.wants.body, string(body)); err != nil {
 					t.Errorf("%q, handleGetTasks(). error unmarshaling json %v", tt.name, err)
 				} else if !eq {
 					t.Errorf("%q. handleGetTasks() = ***%s***", tt.name, diff)
@@ -517,8 +512,7 @@ func TestTaskHandler_handlePostTasks(t *testing.T) {
 				body: `
 {
     "code": "invalid",
-    "message": "something really went wrong",
-    "error": "something went wrong"
+    "message": "something really went wrong: something went wrong"
 }
 `,
 			},
@@ -544,8 +538,7 @@ func TestTaskHandler_handlePostTasks(t *testing.T) {
 				body: `
 {
     "code": "internal error",
-    "message": "failed to create task",
-    "error": "something bad happened"
+    "message": "failed to create task: something bad happened"
 }
 `,
 			},
@@ -582,7 +575,7 @@ func TestTaskHandler_handlePostTasks(t *testing.T) {
 				t.Errorf("%q. handlePostTask() = %v, want %v", tt.name, content, tt.wants.contentType)
 			}
 			if tt.wants.body != "" {
-				if eq, diff, err := jsonEqual(string(body), tt.wants.body); err != nil {
+				if eq, diff, err := jsonEqual(tt.wants.body, string(body)); err != nil {
 					t.Errorf("%q, handlePostTask(). error unmarshaling json %v", tt.name, err)
 				} else if !eq {
 					t.Errorf("%q. handlePostTask() = ***%s***", tt.name, diff)

http/write_handler_test.go

@@ -137,7 +137,7 @@ func TestWriteHandler_handleWrite(t *testing.T) {
 			},
 			wants: wants{
 				code: 500,
-				body: `{"code":"internal error","message":"unexpected error writing points to database","op":"http/handleWrite","error":"error"}`,
+				body: `{"code":"internal error","message":"unexpected error writing points to database: error"}`,
 			},
 		},
 		{
@@ -153,7 +153,7 @@ func TestWriteHandler_handleWrite(t *testing.T) {
 			},
 			wants: wants{
 				code: 400,
-				body: `{"code":"invalid","message":"writing requires points","op":"http/handleWrite"}`,
+				body: `{"code":"invalid","message":"writing requires points"}`,
 			},
 		},
 		{
@@ -236,7 +236,7 @@ func TestWriteHandler_handleWrite(t *testing.T) {
 			},
 			wants: wants{
 				code: 403,
-				body: `{"code":"forbidden","message":"cannot write to internal bucket ","op":"http/handleWrite"}`,
+				body: `{"code":"forbidden","message":"cannot write to internal bucket "}`,
 			},
 		},
 		{
@@ -253,7 +253,7 @@ func TestWriteHandler_handleWrite(t *testing.T) {
 			},
 			wants: wants{
 				code: 403,
-				body: `{"code":"forbidden","message":"insufficient permissions for write","op":"http/handleWrite"}`,
+				body: `{"code":"forbidden","message":"insufficient permissions for write"}`,
 			},
 		},
 		{

inmem/passwords.go

@@ -2,7 +2,6 @@ package inmem
 
 import (
 	"context"
-	"fmt"
 
 	platform "github.com/influxdata/influxdb"
 	"golang.org/x/crypto/bcrypt"
@@ -15,13 +14,15 @@ var (
 	// EIncorrectPassword is returned when any password operation fails in which
 	// we do not want to leak information.
 	EIncorrectPassword = &platform.Error{
-		Msg: "<forbidden> your username or password is incorrect",
+		Code: platform.EForbidden,
+		Msg:  "your username or password is incorrect",
 	}
 
 	// EShortPassword is used when a password is less than the minimum
 	// acceptable password length.
 	EShortPassword = &platform.Error{
-		Msg: "<invalid> passwords must be at least 8 characters long",
+		Code: platform.EInvalid,
+		Msg:  "passwords must be at least 8 characters long",
 	}
 )
 
@@ -62,7 +63,7 @@ func (s *Service) ComparePassword(ctx context.Context, name string, password str
 	}
 
 	if err := bcrypt.CompareHashAndPassword(hash.([]byte), []byte(password)); err != nil {
-		return fmt.Errorf("<forbidden> your username or password is incorrect")
+		return EIncorrectPassword
 	}
 	return nil
 }