From 638ea1278fe714491d1597f3212a1b94e327c2ac Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Mon, 26 Jun 2017 18:25:49 -0700 Subject: [PATCH] Support extracting underlying error list This change adds an `Errors(err) []error` function which returns the underlying list of errors. Additionally, it amends the contract for returned errors that they MAY implement a specific interface. This is needed for Zap integration as per discussion in #6. Resolves #10. --- error.go | 83 +++++++++++++++++++++++++++++++++++ error_test.go | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 202 insertions(+) diff --git a/error.go b/error.go index a16cbb6..6bdd747 100644 --- a/error.go +++ b/error.go @@ -20,6 +20,8 @@ // Package multierr allows combining one or more errors together. // +// Overview +// // Errors can be combined with the use of the Combine function. // // multierr.Combine( @@ -46,6 +48,38 @@ // }() // // ... // } +// +// The underlying list of errors for a returned error object may be retrieved +// with the Errors function. +// +// errors := multierr.Errors(err) +// if len(errors) > 0 { +// fmt.Println("The following errors occurred:") +// } +// +// Advanced Usage +// +// Errors returned by Combine and Append MAY implement the following +// interface. +// +// type errorGroup interface { +// // Returns a slice containing the underlying list of errors. +// // +// // This slice MUST NOT be modified by the caller. +// Errors() []error +// } +// +// Note that the errors MAY implement this interface. If access to the +// underlying error list is required, users should attempt to cast error +// objects to this interface using the two-value assigment form and handle the +// failure case gracefully. +// +// group, ok := err.(errorGroup) +// if ok { +// errors = group.Errors() +// } else { +// errors = []error{err} +// } package multierr // import "go.uber.org/multierr" import ( @@ -90,6 +124,43 @@ var _bufferPool = sync.Pool{ }, } +type errorGroup interface { + Errors() []error +} + +// Errors returns a slice containing zero or more errors that the supplied +// error is composed of. If the error is nil, the returned slice is empty. +// +// err := multierr.Append(r.Close(), w.Close()) +// errors := multierr.Errors(err) +// +// If the error is not composed of other errors, the returned slice contains +// just the error that was passed in. +// +// Callers of this function are free to modify the returned slice. +func Errors(err error) []error { + if err == nil { + return nil + } + + // Note that we're casting to multiError, not errorGroup. Our contract is + // that returned errors MAY implement errorGroup. Errors, however, only + // has special behavior for multierr-specific error objects. + // + // This behavior can be expanded in the future but I think it's prudent to + // start with as little as possible in terms of contract and possibility + // of misuse. + eg, ok := err.(*multiError) + if !ok { + return []error{err} + } + + errors := eg.Errors() + result := make([]error, len(errors)) + copy(result, errors) + return result +} + // multiError is an error that holds one or more errors. // // An instance of this is guaranteed to be non-empty and flattened. That is, @@ -102,6 +173,18 @@ type multiError struct { errors []error } +var _ errorGroup = (*multiError)(nil) + +// Errors returns the list of underlying errors. +// +// This slice MUST NOT be modified. +func (merr *multiError) Errors() []error { + if merr == nil { + return nil + } + return merr.errors +} + func (merr *multiError) Error() string { if merr == nil { return "" diff --git a/error_test.go b/error_test.go index 053859b..9384ff5 100644 --- a/error_test.go +++ b/error_test.go @@ -345,6 +345,109 @@ func TestAppend(t *testing.T) { } } +type notMultiErr struct{} + +var _ errorGroup = notMultiErr{} + +func (notMultiErr) Error() string { + return "great sadness" +} + +func (notMultiErr) Errors() []error { + return []error{errors.New("great sadness")} +} + +func TestErrors(t *testing.T) { + tests := []struct { + give error + want []error + + // Don't attempt to cast to errorGroup or *multiError + dontCast bool + }{ + {dontCast: true}, // nil + { + give: errors.New("hi"), + want: []error{errors.New("hi")}, + dontCast: true, + }, + { + // We don't yet support non-multierr errors. + give: notMultiErr{}, + want: []error{notMultiErr{}}, + dontCast: true, + }, + { + give: Combine( + errors.New("foo"), + errors.New("bar"), + ), + want: []error{ + errors.New("foo"), + errors.New("bar"), + }, + }, + { + give: Append( + errors.New("foo"), + errors.New("bar"), + ), + want: []error{ + errors.New("foo"), + errors.New("bar"), + }, + }, + { + give: Append( + errors.New("foo"), + Combine( + errors.New("bar"), + ), + ), + want: []error{ + errors.New("foo"), + errors.New("bar"), + }, + }, + { + give: Combine( + errors.New("foo"), + Append( + errors.New("bar"), + errors.New("baz"), + ), + errors.New("qux"), + ), + want: []error{ + errors.New("foo"), + errors.New("bar"), + errors.New("baz"), + errors.New("qux"), + }, + }, + } + + for i, tt := range tests { + t.Run(fmt.Sprint(i), func(t *testing.T) { + t.Run("Errors()", func(t *testing.T) { + require.Equal(t, tt.want, Errors(tt.give)) + }) + + if tt.dontCast { + return + } + + t.Run("multiError", func(t *testing.T) { + require.Equal(t, tt.want, tt.give.(*multiError).Errors()) + }) + + t.Run("errorGroup", func(t *testing.T) { + require.Equal(t, tt.want, tt.give.(errorGroup).Errors()) + }) + }) + } +} + func createMultiErrWithCapacity() error { // Create a multiError that has capacity for more errors so Append will // modify the underlying array that may be shared. @@ -383,10 +486,26 @@ func TestAppendRace(t *testing.T) { wg.Wait() } +func TestErrorsSliceIsImmutable(t *testing.T) { + err1 := errors.New("err1") + err2 := errors.New("err2") + + err := Append(err1, err2) + gotErrors := Errors(err) + require.Equal(t, []error{err1, err2}, gotErrors, "errors must match") + + gotErrors[0] = nil + gotErrors[1] = errors.New("err3") + + require.Equal(t, []error{err1, err2}, Errors(err), + "errors must match after modification") +} + func TestNilMultierror(t *testing.T) { // For safety, all operations on multiError should be safe even if it is // nil. var err *multiError require.Empty(t, err.Error()) + require.Empty(t, err.Errors()) }