zapcore: Support ObjectMarshaler for error types

Previously, implementing `ObjectMarshaler` on an error type was ignored
unless the error was logged with `zap.Any` or `zap.Object`. We should
respect the `ObjectMarshaler` instead as this is more flexible than the
basic `error` or `fmt.Formatter` interfaces.

Resolves uber-go#1365

zapcore/error.go

@@ -27,8 +27,12 @@ import (
 	"go.uber.org/zap/internal/pool"
 )
 
-// Encodes the given error into fields of an object. A field with the given
-// name is added for the error message.
+// Encodes the given error into fields of an object.
+//
+// If the error implements ObjectMarshaler, this takes precedence and a field
+// with the given name is added with the corresponding MarshalLogObject.
+//
+// Otherwise, a field with the given name is added for the error message.
 //
 // If the error implements fmt.Formatter, a field with the name ${key}Verbose
 // is also added with the full verbose error message.
@@ -37,6 +41,16 @@ import (
 // causer (from github.com/pkg/errors), a ${key}Causes field is added with an
 // array of objects containing the errors this error was comprised of.
 //
+// In the case of a ObjectMarshaler,
+//
+//	{
+//	  "error": {
+//	    ...
+//	  }
+//	}
+//
+// Otherwise,
+//
 //	{
 //	  "error": err.Error(),
 //	  "errorVerbose": fmt.Sprintf("%+v", err),
@@ -60,6 +74,9 @@ func encodeError(key string, err error, enc ObjectEncoder) (retErr error) {
 			retErr = fmt.Errorf("PANIC=%v", rerr)
 		}
 	}()
+	if obj, ok := err.(ObjectMarshaler); ok {
+		return enc.AddObject(key, obj)
+	}
 
 	basic := err.Error()
 	enc.AddString(key, basic)
@@ -79,7 +96,7 @@ func encodeError(key string, err error, enc ObjectEncoder) (retErr error) {
 }
 
 type errorGroup interface {
-	// Provides read-only access to the underlying list of errors, preferably
+	// Errors provides read-only access to the underlying list of errors, preferably
 	// without causing any allocs.
 	Errors() []error
 }

zapcore/error_test.go

@@ -43,7 +43,20 @@ func (e errTooManyUsers) Format(s fmt.State, verb rune) {
 	// Implement fmt.Formatter, but don't add any information beyond the basic
 	// Error method.
 	if verb == 'v' && s.Flag('+') {
-		io.WriteString(s, e.Error())
+		_, _ = io.WriteString(s, e.Error())
+	}
+}
+
+type errTooFewUsers int
+
+func (e errTooFewUsers) Error() string {
+	return fmt.Sprintf("%d too few users", int(e))
+}
+
+func (e errTooFewUsers) Format(s fmt.State, verb rune) {
+	if verb == 'v' && s.Flag('+') {
+		_, _ = io.WriteString(s, "verbose: ")
+		_, _ = io.WriteString(s, e.Error())
 	}
 }
 
@@ -64,89 +77,110 @@ func (e customMultierr) Errors() []error {
 	}
 }
 
+type customObjMarshalerErr struct{}
+
+func (e customObjMarshalerErr) Error() string { return "object marshaler string" }
+
+func (e customObjMarshalerErr) MarshalLogObject(enc ObjectEncoder) error {
+	enc.AddString("nested", "object marshaler nested")
+	return nil
+}
+
 func TestErrorEncoding(t *testing.T) {
 	tests := []struct {
-		k     string
-		t     FieldType // defaults to ErrorType
-		iface interface{}
-		want  map[string]interface{}
+		key   string
+		iface any
+		want  map[string]any
 	}{
 		{
-			k:     "k",
+			key:   "k",
+			iface: customObjMarshalerErr{},
+			want: map[string]any{
+				"k": map[string]any{
+					"nested": "object marshaler nested",
+				},
+			},
+		},
+		{
+			key:   "k",
 			iface: errTooManyUsers(2),
-			want: map[string]interface{}{
+			want: map[string]any{
 				"k": "2 too many users",
 			},
 		},
 		{
-			k: "err",
+			key:   "k",
+			iface: errTooFewUsers(2),
+			want: map[string]any{
+				"k":        "2 too few users",
+				"kVerbose": "verbose: 2 too few users",
+			},
+		},
+		{
+			key: "err",
 			iface: multierr.Combine(
 				errors.New("foo"),
 				errors.New("bar"),
 				errors.New("baz"),
 			),
-			want: map[string]interface{}{
+			want: map[string]any{
 				"err": "foo; bar; baz",
-				"errCauses": []interface{}{
-					map[string]interface{}{"error": "foo"},
-					map[string]interface{}{"error": "bar"},
-					map[string]interface{}{"error": "baz"},
+				"errCauses": []any{
+					map[string]any{"error": "foo"},
+					map[string]any{"error": "bar"},
+					map[string]any{"error": "baz"},
 				},
 			},
 		},
 		{
-			k:     "e",
+			key:   "e",
 			iface: customMultierr{},
-			want: map[string]interface{}{
+			want: map[string]any{
 				"e": "great sadness",
-				"eCauses": []interface{}{
-					map[string]interface{}{"error": "foo"},
-					map[string]interface{}{
+				"eCauses": []any{
+					map[string]any{"error": "foo"},
+					map[string]any{
 						"error": "bar; baz",
-						"errorCauses": []interface{}{
-							map[string]interface{}{"error": "bar"},
-							map[string]interface{}{"error": "baz"},
+						"errorCauses": []any{
+							map[string]any{"error": "bar"},
+							map[string]any{"error": "baz"},
 						},
 					},
 				},
 			},
 		},
 		{
-			k:     "k",
+			key:   "k",
 			iface: fmt.Errorf("failed: %w", errors.New("egad")),
-			want: map[string]interface{}{
+			want: map[string]any{
 				"k": "failed: egad",
 			},
 		},
 		{
-			k: "error",
+			key: "error",
 			iface: multierr.Combine(
 				fmt.Errorf("hello: %w",
 					multierr.Combine(errors.New("foo"), errors.New("bar")),
 				),
 				errors.New("baz"),
 				fmt.Errorf("world: %w", errors.New("qux")),
 			),
-			want: map[string]interface{}{
+			want: map[string]any{
 				"error": "hello: foo; bar; baz; world: qux",
-				"errorCauses": []interface{}{
-					map[string]interface{}{
+				"errorCauses": []any{
+					map[string]any{
 						"error": "hello: foo; bar",
 					},
-					map[string]interface{}{"error": "baz"},
-					map[string]interface{}{"error": "world: qux"},
+					map[string]any{"error": "baz"},
+					map[string]any{"error": "world: qux"},
 				},
 			},
 		},
 	}
 
 	for _, tt := range tests {
-		if tt.t == UnknownType {
-			tt.t = ErrorType
-		}
-
 		enc := NewMapObjectEncoder()
-		f := Field{Key: tt.k, Type: tt.t, Interface: tt.iface}
+		f := Field{Key: tt.key, Type: ErrorType, Interface: tt.iface}
 		f.AddTo(enc)
 		assert.Equal(t, tt.want, enc.Fields, "Unexpected output from field %+v.", f)
 	}