Merge branch 'ps-new-error-helpers' into 'master'

helper: Extra error creation functions

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4975

Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: karthik nayak <knayak@gitlab.com>
Approved-by: Toon Claes <toon@gitlab.com>

3 files changed, 79 insertions, 0 deletions

diff --git a/STYLE.md b/STYLE.md
index cb4e72a71..d2ba78d3b 100644
--- a/STYLE.md
+++ b/STYLE.md
@@ -55,6 +55,52 @@ interpolating strings. The `%q` operator quotes strings and escapes
 spaces and non-printable characters. This can save a lot of debugging
 time.
 
+### Use [helper](internal/helper/error.go) for error creation
+
+[gRPC](https://grpc.io/) is the only transport supported by Gitaly and Praefect.
+
+To return proper error codes, you must use the
+[`status.Error()`](https://pkg.go.dev/google.golang.org/grpc@v1.50.1/status#Error)
+function, otherwise the `Unknown` status code is returned that doesn't provide much
+information about what happened on the server.
+
+However, using `status.Error()` everywhere may be too much of dependency for Gitaly.
+Therefore, you can use a set of helper functions to create statuses with
+proper codes. The benefit of using these is that you can wrap errors
+without losing initial status code that should be returned to the caller.
+
+The example below uses helpers with the standard `fmt.Errorf()` function that
+adds additional context to the error on the intermediate step. The result of the RPC
+call is:
+
+- `codes.InvalidArgument` code.
+- `action crashed: validation: condition` message.
+
+This means it fairly safe to use error wrapping in combination with helper functions.
+That said, pay attention when using a helper wrapper on the transport layer, otherwise
+the `Unknown` code may be returned by the RPC.
+
+```golang
+// We have declaration of some `service`
+type service struct {}
+
+func (s service) Action() error {
+	if err := s.validation(); err != nil {
+        return fmt.Errorf("validation: %w", err)
+    }
+    return nil
+}
+
+func (service) validation() error {
+	return helper.ErrInvalidArgumentf("condition")
+}
+
+// Somewhere at the transport layer:
+if err := srv.Action(); err != nil {
+    return helper.ErrInternalf("action crashed: %w", err)
+}
+```
+
 ## Logging
 
 ### Use context-based logging
diff --git a/internal/helper/error.go b/internal/helper/error.go
index 7a6d16f3f..173f1b023 100644
--- a/internal/helper/error.go
+++ b/internal/helper/error.go
@@ -133,6 +133,24 @@ func ErrAbortedf(format string, a ...interface{}) error {
 	return formatError(codes.Aborted, format, a...)
 }
 
+// ErrDataLossf wraps a formatted error with codes.DataLoss, unless the formatted error is a wrapped
+// gRPC error.
+func ErrDataLossf(format string, a ...interface{}) error {
+	return formatError(codes.DataLoss, format, a...)
+}
+
+// ErrUnknownf wraps a formatted error with codes.Unknown, unless the formatted error is a wrapped
+// gRPC error.
+func ErrUnknownf(format string, a ...interface{}) error {
+	return formatError(codes.Unknown, format, a...)
+}
+
+// ErrUnimplementedf wraps a formatted error with codes.Unimplemented, unless the formatted error is a wrapped
+// gRPC error.
+func ErrUnimplementedf(format string, a ...interface{}) error {
+	return formatError(codes.Unimplemented, format, a...)
+}
+
 // grpcErrorMessageWrapper is used to wrap a gRPC `status.Status`-style error such that it behaves
 // like a `status.Status`, except that it generates a readable error message.
 type grpcErrorMessageWrapper struct {
diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go
index c0e7d6b5d..cb62b7d7e 100644
--- a/internal/helper/error_test.go
+++ b/internal/helper/error_test.go
@@ -166,6 +166,21 @@ func TestErrorf(t *testing.T) {
 			errorf:       ErrAbortedf,
 			expectedCode: codes.Aborted,
 		},
+		{
+			desc:         "ErrDataLossf",
+			errorf:       ErrDataLossf,
+			expectedCode: codes.DataLoss,
+		},
+		{
+			desc:         "ErrUnknownf",
+			errorf:       ErrUnknownf,
+			expectedCode: codes.Unknown,
+		},
+		{
+			desc:         "ErrUnimplementedf",
+			errorf:       ErrUnimplementedf,
+			expectedCode: codes.Unimplemented,
+		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			t.Run("with non-gRPC error", func(t *testing.T) {