common: update Errors type (#1129)

* common: adjust common error slice to allow multi errors.Is matching and conform to interface better

* zb: forgot to save?

* linties: fixies

* linties: word change as well.

* nitters: glorious

* buts

* nitters: fix glorious bug

* Update common/common.go

Co-authored-by: Scott <gloriousCode@users.noreply.github.com>

* nitters: shifty

---------

Co-authored-by: Ryan O'Hara-Reid <ryan.oharareid@thrasher.io>
Co-authored-by: Scott <gloriousCode@users.noreply.github.com>

common/common.go

@@ -433,28 +433,78 @@ func InArray(val, array interface{}) (exists bool, index int) {
 	return
 }
 
-// Errors defines multiple errors
-type Errors []error
-
-// Error implements error interface
-func (e Errors) Error() string {
-	if len(e) == 0 {
-		return ""
-	}
-	var r string
-	for i := range e {
-		r += e[i].Error() + ", "
-	}
-	return r[:len(r)-2]
+// multiError holds all the errors as a slice, this is unexported, so it forces
+// inbuilt error handling.
+type multiError struct {
+	loadedErrors []error
+	offset       *int
 }
 
-// Unwrap implements interface behaviour for errors.Is() matching NOTE: only
-// returns first element.
-func (e Errors) Unwrap() error {
-	if len(e) == 0 {
-		return nil
+// AppendError appends error in a more idiomatic way. This can start out as a
+// standard error e.g. err := errors.New("random error")
+// err = AppendError(err, errors.New("another random error"))
+func AppendError(original, incoming error) error {
+	errSliceP, ok := original.(*multiError)
+	if ok {
+		errSliceP.offset = nil
 	}
-	return e[0]
+	if incoming == nil {
+		return original // Skip append - continue as normal.
+	}
+	if !ok {
+		// This assumes that a standard error is passed in and we can want to
+		// track it and add additional errors.
+		errSliceP = &multiError{}
+		if original != nil {
+			errSliceP.loadedErrors = append(errSliceP.loadedErrors, original)
+		}
+	}
+	if incomingSlice, ok := incoming.(*multiError); ok {
+		// Join slices if needed.
+		errSliceP.loadedErrors = append(errSliceP.loadedErrors, incomingSlice.loadedErrors...)
+	} else {
+		errSliceP.loadedErrors = append(errSliceP.loadedErrors, incoming)
+	}
+	return errSliceP
+}
+
+// Error displays all errors comma separated, if unwrapped has been called and
+// has not been reset will display the individual error
+func (e *multiError) Error() string {
+	if e.offset != nil {
+		return e.loadedErrors[*e.offset].Error()
+	}
+	allErrors := make([]string, len(e.loadedErrors))
+	for x := range e.loadedErrors {
+		allErrors[x] = e.loadedErrors[x].Error()
+	}
+	return strings.Join(allErrors, ", ")
+}
+
+// Unwrap increments the offset so errors.Is() can be called to its individual
+// error for correct matching.
+func (e *multiError) Unwrap() error {
+	if e.offset == nil {
+		e.offset = new(int)
+	} else {
+		*e.offset++
+	}
+	if *e.offset == len(e.loadedErrors) {
+		e.offset = nil
+		return nil // Force errors.Is package to return false.
+	}
+	return e
+}
+
+// Is checks to see if the errors match. It calls package errors.Is() so that
+// we can keep fmt.Errorf() trimmings. This is called in errors package at
+// interface assertion err.(interface{ Is(error) bool }).
+func (e *multiError) Is(incoming error) bool {
+	if e.offset != nil && errors.Is(e.loadedErrors[*e.offset], incoming) {
+		e.offset = nil
+		return true
+	}
+	return false
 }
 
 // StartEndTimeCheck provides some basic checks which occur

common/common_test.go

@@ -3,6 +3,7 @@ package common
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 	"net/url"
 	"os"
@@ -623,19 +624,106 @@ func TestInArray(t *testing.T) {
 
 func TestErrors(t *testing.T) {
 	t.Parallel()
-	var test Errors
-	if test.Error() != "" {
-		t.Fatal("string should be nil")
-	}
-	errTestOne := errors.New("test1")
-	test = append(test, errTestOne)
+
+	var errTestOne = errors.New("test1")
+	var test error
+	test = AppendError(test, errTestOne)
 	if !errors.Is(test, errTestOne) {
 		t.Fatal("does not match error")
 	}
-	test = append(test, errors.New("test2"))
+
+	var errTestTwo = errors.New("test2")
+	test = AppendError(test, errTestTwo)
+	if !errors.Is(test, errTestTwo) {
+		t.Fatal("does not match error")
+	}
+
+	if !errors.Is(test, errTestTwo) {
+		t.Fatal("does not match error")
+	}
+
+	// Append nil should log
+	test = AppendError(test, nil)
+
 	if test.Error() != "test1, test2" {
 		t.Fatal("does not match error")
 	}
+
+	// Join slices for whatever reason
+	test = AppendError(test, test)
+
+	if test.Error() != "test1, test2, test1, test2" {
+		t.Fatal("does not match error")
+	}
+
+	var errTestThree = errors.New("test3")
+	if errors.Is(test, errTestThree) {
+		t.Fatal("expected errors.Is() should not match")
+	}
+
+	if errors.Is(test, errTestThree) {
+		t.Fatal("expected errors.Is() should not match")
+	}
+
+	strangeError := errors.New("this is a strange error")
+
+	strangeError = AppendError(strangeError, errTestOne)
+	if strangeError.Error() != "this is a strange error, test1" {
+		t.Fatal("does not match error")
+	}
+
+	// Add trimmings
+	strangeError = AppendError(strangeError, fmt.Errorf("TRIMMINGS: %w", errTestTwo))
+	if strangeError.Error() != "this is a strange error, test1, TRIMMINGS: test2" {
+		t.Fatal("does not match error")
+	}
+
+	if !errors.Is(strangeError, errTestTwo) {
+		t.Fatal("does not match error")
+	}
+
+	if errors.Is(strangeError, errTestThree) {
+		t.Fatal("should not match")
+	}
+
+	// Test again because unwrap was called multiple times.
+	if strangeError.Error() != "this is a strange error, test1, TRIMMINGS: test2" {
+		t.Fatalf("received: '%v' but expected: '%v'", strangeError.Error(), "this is a strange error, test1, TRIMMINGS: test2")
+	}
+
+	strangeError = AppendError(strangeError, errors.New("even more error"))
+
+	strangeError = AppendError(strangeError, nil) // Skip this nasty thing.
+
+	// Test for individual display of errors
+	target := 0
+	for indv := errors.Unwrap(strangeError); indv != nil; indv = errors.Unwrap(indv) {
+		switch target {
+		case 0:
+			if indv.Error() != "this is a strange error" {
+				t.Fatalf("received: '%v' but expected: '%v'", indv.Error(), "this is a strange error")
+			}
+		case 1:
+			if indv.Error() != "test1" {
+				t.Fatalf("received: '%v' but expected: '%v'", indv.Error(), "test1")
+			}
+
+		case 2:
+			if indv.Error() != "TRIMMINGS: test2" {
+				t.Fatalf("received: '%v' but expected: '%v'", indv.Error(), "TRIMMINGS: test2")
+			}
+		case 3:
+			if indv.Error() != "even more error" {
+				t.Fatalf("received: '%v' but expected: '%v'", indv.Error(), "even more error")
+			}
+		default:
+			t.Fatal("unhandled case")
+		}
+		target++
+	}
+	if target != 4 {
+		t.Fatal("targets not achieved")
+	}
 }
 
 func TestParseStartEndDate(t *testing.T) {

common/timeperiods/timeperiods.go

@@ -13,17 +13,17 @@ import (
 // eg if no comparisonTimes match, you will receive 1 TimeRange of Start End with dataInRange = false
 // eg2 if 1 comparisonTime matches in the middle of start and end, you will receive three ranges
 func FindTimeRangesContainingData(start, end time.Time, period time.Duration, comparisonTimes []time.Time) ([]TimeRange, error) {
-	var errs common.Errors
+	var errs error
 	if start.IsZero() {
-		errs = append(errs, errors.New("invalid start time"))
+		errs = common.AppendError(errs, errors.New("invalid start time"))
 	}
 	if end.IsZero() {
-		errs = append(errs, errors.New("invalid end time"))
+		errs = common.AppendError(errs, errors.New("invalid end time"))
 	}
 	if err := validatePeriod(period); err != nil {
-		errs = append(errs, err)
+		errs = common.AppendError(errs, err)
 	}
-	if len(errs) > 0 {
+	if errs != nil {
 		return nil, errs
 	}
 	var t TimePeriodCalculator
@@ -52,17 +52,17 @@ func validatePeriod(period time.Duration) error {
 // CalculateTimePeriodsInRange can break down start and end times into time periods
 // eg 1 hourly intervals
 func CalculateTimePeriodsInRange(start, end time.Time, period time.Duration) ([]TimePeriod, error) {
-	var errs common.Errors
+	var errs error
 	if start.IsZero() {
-		errs = append(errs, errors.New("invalid start time"))
+		errs = common.AppendError(errs, errors.New("invalid start time"))
 	}
 	if end.IsZero() {
-		errs = append(errs, errors.New("invalid end time"))
+		errs = common.AppendError(errs, errors.New("invalid end time"))
 	}
 	if err := validatePeriod(period); err != nil {
-		errs = append(errs, err)
+		errs = common.AppendError(errs, err)
 	}
-	if len(errs) > 0 {
+	if errs != nil {
 		return nil, errs
 	}