From 422ebbe80390e219ce9a0469ed0d5a0dcf7ab00f Mon Sep 17 00:00:00 2001
From: Gareth Kirwan <gbjkirwan@gmail.com>
Date: Tue, 20 Feb 2024 00:14:59 +0100
Subject: [PATCH] Common: Fix fmt msg lost in AppendError().Error() (#1484)

Retain the .msg field of a go fmt.Errorf .msg field returned by .Error()
when wrapping multiple errors.
This fixes a situation where a nested stack of errors would lose
formatting information, which is often used to supply identifying
context.
e.g.
```
err = fmt.Errorf("%w `%s`: %w", errParsingField, fieldName,
parsingError)
errs = common.AppendError(errs, err)
```

This isn't really an issue with our implementation; Calling Unwrap() on a
fmt.Errorf() which returns a wrapErrors will lose that formatting.
Our issue was that we were using just Unwrap() to bind together our
chain-of-custody.
---
 common/common.go      | 55 +++++++++++++++++++++++++++++++++----------
 common/common_test.go |  7 ++++++
 2 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/common/common.go b/common/common.go
index 9e0d32db..c4483a2d 100644
--- a/common/common.go
+++ b/common/common.go
@@ -463,11 +463,22 @@ func InArray(val, array interface{}) (exists bool, index int) {
 	return
 }
 
+// fmtError holds a formatted msg and the errors which formatted it
+type fmtError struct {
+	errs []error
+	msg  string
+}
+
 // multiError holds errors as a slice
 type multiError struct {
 	errs []error
 }
 
+type unwrappable interface {
+	Unwrap() []error
+	Error() string
+}
+
 // AppendError appends an error to a list of exesting errors
 // Either argument may be:
 // * A vanilla error
@@ -481,20 +492,35 @@ func AppendError(original, incoming error) error {
 	if original == nil {
 		return incoming
 	}
-	newErrs := []error{incoming}
-	if u, ok := incoming.(interface{ Unwrap() []error }); ok {
-		newErrs = u.Unwrap()
+	if u, ok := incoming.(unwrappable); ok {
+		incoming = &fmtError{
+			errs: u.Unwrap(),
+			msg:  u.Error(),
+		}
 	}
-	if u, ok := original.(interface{ Unwrap() []error }); ok {
-		return &multiError{
-			errs: append(u.Unwrap(), newErrs...),
+	switch v := original.(type) {
+	case *multiError:
+		v.errs = append(v.errs, incoming)
+		return v
+	case unwrappable:
+		original = &fmtError{
+			errs: v.Unwrap(),
+			msg:  v.Error(),
 		}
 	}
 	return &multiError{
-		errs: append([]error{original}, newErrs...),
+		errs: append([]error{original}, incoming),
 	}
 }
 
+func (e *fmtError) Error() string {
+	return e.msg
+}
+
+func (e *fmtError) Unwrap() []error {
+	return e.errs
+}
+
 // Error displays all errors comma separated
 func (e *multiError) Error() string {
 	allErrors := make([]string, len(e.errs))
@@ -506,11 +532,16 @@ func (e *multiError) Error() string {
 
 // Unwrap returns all of the errors in the multiError
 func (e *multiError) Unwrap() []error {
-	return e.errs
-}
-
-type unwrappable interface {
-	Unwrap() []error
+	errs := make([]error, 0, len(e.errs))
+	for _, e := range e.errs {
+		switch v := e.(type) {
+		case unwrappable:
+			errs = append(errs, unwrapDeep(v)...)
+		default:
+			errs = append(errs, v)
+		}
+	}
+	return errs
 }
 
 // unwrapDeep walks down a stack of nested fmt.Errorf("%w: %w") errors
diff --git a/common/common_test.go b/common/common_test.go
index fd6d448d..89403f8d 100644
--- a/common/common_test.go
+++ b/common/common_test.go
@@ -703,6 +703,13 @@ func TestErrors(t *testing.T) {
 	assert.ErrorIs(t, ExcludeError(err, e5), e3, "Excluding e5 should retain e3")
 	assert.ErrorIs(t, ExcludeError(err, e5), e4, "Excluding e5 should retain the vanilla co-wrapped e4")
 	assert.NotErrorIs(t, ExcludeError(err, e5), e5, "e4 should be excluded")
+
+	// Formatting retention
+	err = AppendError(e1, fmt.Errorf("%w: Run out of `%s`: %w", e3, "sausages", e5))
+	assert.ErrorIs(t, err, e1, "Should be an e1")
+	assert.ErrorIs(t, err, e3, "Should be an e3")
+	assert.ErrorIs(t, err, e5, "Should be an e5")
+	assert.ErrorContains(t, err, "sausages", "Should know about secret sausages")
 }
 
 func TestParseStartEndDate(t *testing.T) {