From ea4172290221b67c1f41b3fa8c6a10c3fbc44c0f Mon Sep 17 00:00:00 2001
From: Milos Gajdos <milosthegajdos@gmail.com>
Date: Wed, 18 Oct 2023 10:02:21 +0100
Subject: [PATCH] refactor: Storage driver errors

Small refactoring of storagedriver errors.
We change the Enclosed field to Detail and make sure
Errors get properly serialized to JSON.
We also add tests.

Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>
---
 registry/storage/driver/base/base.go     |  6 +-
 registry/storage/driver/errors_test.go   | 80 ++++++++++++++++++++++++
 registry/storage/driver/storagedriver.go | 36 ++++++++---
 3 files changed, 111 insertions(+), 11 deletions(-)
 create mode 100644 registry/storage/driver/errors_test.go

diff --git a/registry/storage/driver/base/base.go b/registry/storage/driver/base/base.go
index 4ea3394a..69371712 100644
--- a/registry/storage/driver/base/base.go
+++ b/registry/storage/driver/base/base.go
@@ -79,12 +79,10 @@ func (base *Base) setDriverName(e error) error {
 		actual.DriverName = base.StorageDriver.Name()
 		return actual
 	default:
-		storageError := storagedriver.Error{
+		return storagedriver.Error{
 			DriverName: base.StorageDriver.Name(),
-			Enclosed:   e,
+			Detail:     e,
 		}
-
-		return storageError
 	}
 }
 
diff --git a/registry/storage/driver/errors_test.go b/registry/storage/driver/errors_test.go
new file mode 100644
index 00000000..f692466f
--- /dev/null
+++ b/registry/storage/driver/errors_test.go
@@ -0,0 +1,80 @@
+package driver
+
+import (
+	"encoding/json"
+	"errors"
+	"fmt"
+	"testing"
+)
+
+func TestErrorFormat(t *testing.T) {
+	drvName := "foo"
+	errMsg := "unexpected error"
+
+	e := Error{
+		DriverName: drvName,
+		Detail:     errors.New(errMsg),
+	}
+
+	exp := fmt.Sprintf("%s: %s", drvName, errMsg)
+
+	if e.Error() != exp {
+		t.Errorf("expected: %s, got: %s", exp, e.Error())
+	}
+
+	b, err := json.Marshal(&e)
+	if err != nil {
+		t.Fatal(err)
+	}
+	expJson := `{"driver":"foo","detail":"unexpected error"}`
+	if gotJson := string(b); gotJson != expJson {
+		t.Fatalf("expected JSON: %s,\n got: %s", expJson, gotJson)
+	}
+}
+
+func TestErrors(t *testing.T) {
+	drvName := "foo"
+
+	testCases := []struct {
+		name    string
+		errs    Errors
+		exp     string
+		expJson string
+	}{
+		{
+			name:    "no details",
+			errs:    Errors{DriverName: drvName},
+			exp:     fmt.Sprintf("%s: <nil>", drvName),
+			expJson: `{"driver":"foo","details":[]}`,
+		},
+		{
+			name:    "single detail",
+			errs:    Errors{DriverName: drvName, Errs: []error{errors.New("err msg")}},
+			exp:     fmt.Sprintf("%s: err msg", drvName),
+			expJson: `{"driver":"foo","details":["err msg"]}`,
+		},
+		{
+			name:    "multiple details",
+			errs:    Errors{DriverName: drvName, Errs: []error{errors.New("err msg1"), errors.New("err msg2")}},
+			exp:     fmt.Sprintf("%s: errors:\nerr msg1\nerr msg2\n", drvName),
+			expJson: `{"driver":"foo","details":["err msg1","err msg2"]}`,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			if got := tc.errs.Error(); got != tc.exp {
+				t.Errorf("got error: %s, expected: %s", got, tc.exp)
+			}
+			b, err := json.Marshal(&tc.errs)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if gotJson := string(b); gotJson != tc.expJson {
+				t.Errorf("expected JSON: %s,\n got: %s", tc.expJson, gotJson)
+			}
+		})
+	}
+}
diff --git a/registry/storage/driver/storagedriver.go b/registry/storage/driver/storagedriver.go
index ef81a60e..a5ff6852 100644
--- a/registry/storage/driver/storagedriver.go
+++ b/registry/storage/driver/storagedriver.go
@@ -178,20 +178,20 @@ func (err InvalidOffsetError) Error() string {
 // the driver type on which it occurred.
 type Error struct {
 	DriverName string
-	Enclosed   error
+	Detail     error
 }
 
 func (err Error) Error() string {
-	return fmt.Sprintf("%s: %s", err.DriverName, err.Enclosed)
+	return fmt.Sprintf("%s: %s", err.DriverName, err.Detail)
 }
 
 func (err Error) MarshalJSON() ([]byte, error) {
 	return json.Marshal(struct {
 		DriverName string `json:"driver"`
-		Enclosed   string `json:"enclosed"`
+		Detail     string `json:"detail"`
 	}{
 		DriverName: err.DriverName,
-		Enclosed:   err.Enclosed.Error(),
+		Detail:     err.Detail.Error(),
 	})
 }
 
@@ -207,14 +207,36 @@ var _ error = Errors{}
 func (e Errors) Error() string {
 	switch len(e.Errs) {
 	case 0:
-		return "<nil>"
+		return fmt.Sprintf("%s: <nil>", e.DriverName)
 	case 1:
-		return e.Errs[0].Error()
+		return fmt.Sprintf("%s: %s", e.DriverName, e.Errs[0].Error())
 	default:
 		msg := "errors:\n"
 		for _, err := range e.Errs {
 			msg += err.Error() + "\n"
 		}
-		return msg
+		return fmt.Sprintf("%s: %s", e.DriverName, msg)
 	}
 }
+
+// MarshalJSON converts slice of errors into the format
+// that is serializable by JSON.
+func (e Errors) MarshalJSON() ([]byte, error) {
+	tmpErrs := struct {
+		DriverName string   `json:"driver"`
+		Details    []string `json:"details"`
+	}{
+		DriverName: e.DriverName,
+	}
+
+	if len(e.Errs) == 0 {
+		tmpErrs.Details = make([]string, 0)
+		return json.Marshal(tmpErrs)
+	}
+
+	for _, err := range e.Errs {
+		tmpErrs.Details = append(tmpErrs.Details, err.Error())
+	}
+
+	return json.Marshal(tmpErrs)
+}