support wrapError encode and decode

Previously, support for the `wrapError` type generated by
`fmt.Errorf("%w xxxx")` was missing an understanding of the fact
that the format string wasn't limited to just prefixes. Hence the
`opaqueWrapper` struct assumed that all wrappers had prefix strings
that would precede their causal chains.

This change adds a flag to the `EncodedWrapper` which tracks whether
the wrapper "owns" the error string. In the case of `fmt.Errorf`
the `wrapError` struct created within the stdlib contains a fully
interpolated string as its message, not the format string, or a
prefix. This means that when we encode it into a `EncodedWrapper`
for transfer over the network, we need to remember to just print the
wrapper's string when `.Error()` is called on the decoded object on
the other end.

Paired with this change the `opaqueWrapper` that is generated on the
decode side now contains this field as well which is referenced when
calling `.Error()` on the wrapper, ensuring we don't re-concatenate
the causes. This field is also referenced when formatting the error
in `state.formatSingleLineOutput` where we use it to simply output
the final entry's message, instead of concatenating all the entries
together.

This change is backwards compatible since older versions will omit the
new field in the proto, defaulting it to `false` in newer versions,
which will preserve existing behavior when printing out errors. New
code will set the field when necessary and interpret it appropriately.
If an error encoded with a new version is decoded by an older version,
the information about the error string will be discarded, which may
lead to repeated portions of the error message when printed out, if
the wrapper contains a copy of the message of its cause.

Author: dhartunian

README.md

@@ -573,9 +573,11 @@ func RegisterLeafDecoder(typeName TypeKey, decoder LeafDecoder)
 func RegisterLeafEncoder(typeName TypeKey, encoder LeafEncoder)
 func RegisterWrapperDecoder(typeName TypeKey, decoder WrapperDecoder)
 func RegisterWrapperEncoder(typeName TypeKey, encoder WrapperEncoder)
+func RegisterWrapperEncoderWithMessageOverride (typeName TypeKey, encoder WrapperEncoderWithMessageOverride)
 type LeafEncoder = func(ctx context.Context, err error) (msg string, safeDetails []string, payload proto.Message)
 type LeafDecoder = func(ctx context.Context, msg string, safeDetails []string, payload proto.Message) error
 type WrapperEncoder = func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message)
+type WrapperEncoderWithMessageOverride = func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message, overrideError bool)
 type WrapperDecoder = func(ctx context.Context, cause error, msgPrefix string, safeDetails []string, payload proto.Message) error
 
 // Registering package renames for custom error types.

errbase/adapters.go

@@ -212,3 +212,9 @@ func init() {
 
 	RegisterLeafEncoder(GetTypeKey(&OpaqueErrno{}), encodeOpaqueErrno)
 }
+
+func encodeWrapError(_ context.Context, err error) (
+	msgPrefix string, safeDetails []string, payload proto.Message, ownError bool,
+) {
+	return err.Error(), nil, nil, true
+}

errbase/adapters_test.go

@@ -55,6 +55,18 @@ func TestAdaptBaseGoErr(t *testing.T) {
 	tt.CheckDeepEqual(newErr, origErr)
 }
 
+func TestAdaptGoSingleWrapErr(t *testing.T) {
+	origErr := fmt.Errorf("an error %w", goErr.New("hello"))
+	t.Logf("start err: %# v", pretty.Formatter(origErr))
+
+	newErr := network(t, origErr)
+
+	tt := testutils.T{T: t}
+	// The library preserves the cause. It's not possible to preserve the fmt
+	// string.
+	tt.CheckContains(newErr.Error(), "hello")
+}
+
 func TestAdaptPkgWithMessage(t *testing.T) {
 	// Simple message wrappers from github.com/pkg/errors are preserved
 	// completely.

errbase/decode.go

@@ -107,9 +107,10 @@ func decodeWrapper(ctx context.Context, enc *errorspb.EncodedWrapper) error {
 
 	// Otherwise, preserve all details about the original object.
 	return &opaqueWrapper{
-		cause:   cause,
-		prefix:  enc.MessagePrefix,
-		details: enc.Details,
+		cause:           cause,
+		prefix:          enc.MessagePrefix,
+		details:         enc.Details,
+		ownsErrorString: enc.WrapperOwnsErrorString,
 	}
 }
 

errbase/encode.go

@@ -115,10 +115,18 @@ func encodeAsAny(ctx context.Context, err error, payload proto.Message) *types.A
 func encodeWrapper(ctx context.Context, err, cause error) EncodedError {
 	var msg string
 	var details errorspb.EncodedErrorDetails
+	var ownError bool
 
 	if e, ok := err.(*opaqueWrapper); ok {
+		// We delegate all knowledge of the error string
+		// to the original encoder and do not try to re-engineer
+		// the prefix out of the error. This helps maintain
+		// backward compatibility with earlier versions of the
+		// encoder which don't have any understanding of
+		// error string ownership by the wrapper.
 		msg = e.prefix
 		details = e.details
+		ownError = e.ownsErrorString
 	} else {
 		details.OriginalTypeName, details.ErrorTypeMark.FamilyName, details.ErrorTypeMark.Extension = getTypeDetails(err, false /*onlyFamily*/)
 
@@ -127,12 +135,12 @@ func encodeWrapper(ctx context.Context, err, cause error) EncodedError {
 		// If we have a manually registered encoder, use that.
 		typeKey := TypeKey(details.ErrorTypeMark.FamilyName)
 		if enc, ok := encoders[typeKey]; ok {
-			msg, details.ReportablePayload, payload = enc(ctx, err)
+			msg, details.ReportablePayload, payload, ownError = enc(ctx, err)
 		} else {
 			// No encoder.
 			// In that case, we'll try to compute a message prefix
 			// manually.
-			msg = extractPrefix(err, cause)
+			msg, ownError = extractPrefix(err, cause)
 
 			// If there are known safe details, use them.
 			if s, ok := err.(SafeDetailer); ok {
@@ -148,31 +156,47 @@ func encodeWrapper(ctx context.Context, err, cause error) EncodedError {
 	return EncodedError{
 		Error: &errorspb.EncodedError_Wrapper{
 			Wrapper: &errorspb.EncodedWrapper{
-				Cause:         EncodeError(ctx, cause),
-				MessagePrefix: msg,
-				Details:       details,
+				Cause:                  EncodeError(ctx, cause),
+				MessagePrefix:          msg,
+				Details:                details,
+				WrapperOwnsErrorString: ownError,
 			},
 		},
 	}
 }
 
 // extractPrefix extracts the prefix from a wrapper's error message.
 // For example,
-//    err := errors.New("bar")
-//    err = errors.Wrap(err, "foo")
-//    extractPrefix(err)
+//
+//	err := errors.New("bar")
+//	err = errors.Wrap(err, "foo")
+//	extractPrefix(err)
+//
 // returns "foo".
-func extractPrefix(err, cause error) string {
+//
+// If a presumed wrapper does not have a message prefix, it is assumed
+// to override the entire error message and `extractPrefix` returns
+// the entire message and the boolean `true` to signify that the causes
+// should not be appended to it.
+func extractPrefix(err, cause error) (string, bool) {
 	causeSuffix := cause.Error()
 	errMsg := err.Error()
 
 	if strings.HasSuffix(errMsg, causeSuffix) {
 		prefix := errMsg[:len(errMsg)-len(causeSuffix)]
+		// If error msg matches exactly then this is a wrapper
+		// with no message of its own.
+		if len(prefix) == 0 {
+			return "", false
+		}
 		if strings.HasSuffix(prefix, ": ") {
-			return prefix[:len(prefix)-2]
+			return prefix[:len(prefix)-2], false
 		}
 	}
-	return ""
+	// If we don't have the cause as a suffix, then we have
+	// some other string as our error msg, preserve that and
+	// mark as override
+	return errMsg, true
 }
 
 func getTypeDetails(
@@ -295,6 +319,26 @@ var leafEncoders = map[TypeKey]LeafEncoder{}
 // or a different type, ensure that RegisterTypeMigration() was called
 // prior to RegisterWrapperEncoder().
 func RegisterWrapperEncoder(theType TypeKey, encoder WrapperEncoder) {
+	RegisterWrapperEncoderWithMessageOverride(theType, func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message, overrideError bool) {
+		prefix, details, payload := encoder(ctx, err)
+		return prefix, details, payload, false
+	})
+}
+
+// RegisterWrapperEncoderWithMessageOverride can be used to register
+// new wrapper types to the library. Registered wrappers will be
+// encoded using their own Go type when an error is encoded. Wrappers
+// that have not been registered will be encoded using the
+// opaqueWrapper type.
+//
+// This function differs from RegisterWrapperEncoder by allowing the
+// caller to explicitly decide whether the wrapper owns the entire
+// error message or not. Otherwise, the relationship is inferred.
+//
+// Note: if the error type has been migrated from a previous location
+// or a different type, ensure that RegisterTypeMigration() was called
+// prior to RegisterWrapperEncoder().
+func RegisterWrapperEncoderWithMessageOverride(theType TypeKey, encoder WrapperEncoderWithMessageOverride) {
 	if encoder == nil {
 		delete(encoders, theType)
 	} else {
@@ -306,5 +350,12 @@ func RegisterWrapperEncoder(theType TypeKey, encoder WrapperEncoder) {
 // by additional wrapper types not yet known to this library.
 type WrapperEncoder = func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message)
 
+// WrapperEncoderWithMessageOverride is to be provided (via RegisterWrapperEncoderWithMessageOverride above)
+// by additional wrapper types not yet known to this library.
+// This encoder accepts an additional boolean flag which determines whether the
+// wrapper owns the error message completely instead of simply being a prefix
+// with the error message of its causes appended to it.
+type WrapperEncoderWithMessageOverride = func(ctx context.Context, err error) (msgPrefix string, safeDetails []string, payload proto.Message, overrideError bool)
+
 // registry for RegisterWrapperType.
-var encoders = map[TypeKey]WrapperEncoder{}
+var encoders = map[TypeKey]WrapperEncoderWithMessageOverride{}

errbase/err_string_ownership_test.go

@@ -0,0 +1,91 @@
+package errbase_test
+
+import (
+	"context"
+	goErr "errors"
+	"fmt"
+	"testing"
+
+	"github.com/cockroachdb/errors/errbase"
+	"github.com/cockroachdb/errors/errorspb"
+	"github.com/cockroachdb/errors/testutils"
+)
+
+func genEncoded(ownsErrorString bool) errorspb.EncodedError {
+	return errorspb.EncodedError{
+		Error: &errorspb.EncodedError_Wrapper{
+			Wrapper: &errorspb.EncodedWrapper{
+				Cause: errorspb.EncodedError{
+					Error: &errorspb.EncodedError_Leaf{
+						Leaf: &errorspb.EncodedErrorLeaf{
+							Message: "leaf-error-msg",
+						},
+					},
+				},
+				MessagePrefix:          "wrapper-error-msg: leaf-error-msg: extra info",
+				Details:                errorspb.EncodedErrorDetails{},
+				WrapperOwnsErrorString: ownsErrorString,
+			},
+		},
+	}
+}
+
+func TestDecodeOldVersion(t *testing.T) {
+	tt := testutils.T{T: t}
+
+	errOldEncoded := genEncoded(false)
+	errOldDecoded := errbase.DecodeError(context.Background(), errOldEncoded)
+	// Ensure that we will continue to just concat leaf after wrapper
+	// with older errors for backward compatibility.
+	tt.CheckEqual(errOldDecoded.Error(), "wrapper-error-msg: leaf-error-msg: extra info: leaf-error-msg")
+
+	// Check to ensure that when flag is present, we interpret things correctly.
+	errNewEncoded := genEncoded(true)
+	errNewDecoded := errbase.DecodeError(context.Background(), errNewEncoded)
+	tt.CheckEqual(errNewDecoded.Error(), "wrapper-error-msg: leaf-error-msg: extra info")
+}
+
+func TestEncodeDecodeNewVersion(t *testing.T) {
+	tt := testutils.T{T: t}
+	errNewEncoded := errbase.EncodeError(
+		context.Background(),
+		fmt.Errorf(
+			"wrapper-error-msg: %w: extra info",
+			goErr.New("leaf-error-msg"),
+		),
+	)
+
+	errNew := errorspb.EncodedError{
+		Error: &errorspb.EncodedError_Wrapper{
+			Wrapper: &errorspb.EncodedWrapper{
+				Cause: errorspb.EncodedError{
+					Error: &errorspb.EncodedError_Leaf{
+						Leaf: &errorspb.EncodedErrorLeaf{
+							Message: "leaf-error-msg",
+							Details: errorspb.EncodedErrorDetails{
+								OriginalTypeName:  "errors/*errors.errorString",
+								ErrorTypeMark:     errorspb.ErrorTypeMark{FamilyName: "errors/*errors.errorString", Extension: ""},
+								ReportablePayload: nil,
+								FullDetails:       nil,
+							},
+						},
+					},
+				},
+				MessagePrefix: "wrapper-error-msg: leaf-error-msg: extra info",
+				Details: errorspb.EncodedErrorDetails{
+					OriginalTypeName:  "fmt/*fmt.wrapError",
+					ErrorTypeMark:     errorspb.ErrorTypeMark{FamilyName: "fmt/*fmt.wrapError", Extension: ""},
+					ReportablePayload: nil,
+					FullDetails:       nil,
+				},
+				WrapperOwnsErrorString: true,
+			},
+		},
+	}
+
+	tt.CheckDeepEqual(errNewEncoded, errNew)
+	newErr := errbase.DecodeError(context.Background(), errNew)
+
+	// New version correctly decodes error
+	tt.CheckEqual(newErr.Error(), "wrapper-error-msg: leaf-error-msg: extra info")
+}

errbase/format_error.go

@@ -278,8 +278,9 @@ func (s *state) printEntry(entry formatEntry) {
 //
 // This function is used both when FormatError() is called indirectly
 // from .Error(), e.g. in:
-//      (e *myType) Error() { return fmt.Sprintf("%v", e) }
-//      (e *myType) Format(s fmt.State, verb rune) { errors.FormatError(s, verb, e) }
+//
+//	(e *myType) Error() { return fmt.Sprintf("%v", e) }
+//	(e *myType) Format(s fmt.State, verb rune) { errors.FormatError(s, verb, e) }
 //
 // and also to print the first line in the output of a %+v format.
 //
@@ -356,19 +357,15 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
 		if desiredShortening == nil {
 			// The error wants to elide the short messages from inner
 			// causes. Do it.
-			for i := range s.entries {
-				s.entries[i].elideShort = true
-			}
+			s.elideFurtherCauseMsgs()
 		}
 
 	case Formatter:
 		desiredShortening := v.FormatError((*printer)(s))
 		if desiredShortening == nil {
 			// The error wants to elide the short messages from inner
 			// causes. Do it.
-			for i := range s.entries {
-				s.entries[i].elideShort = true
-			}
+			s.elideFurtherCauseMsgs()
 		}
 
 	case fmt.Formatter:
@@ -389,7 +386,11 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
 				s.lastStack = st.StackTrace()
 			}
 		} else {
-			s.formatSimple(err, cause)
+			if elideCauseMsg := s.formatSimple(err, cause); elideCauseMsg {
+				// The error wants to elide the short messages from inner
+				// causes. Do it.
+				s.elideFurtherCauseMsgs()
+			}
 		}
 
 	default:
@@ -411,9 +412,7 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
 				if desiredShortening == nil {
 					// The error wants to elide the short messages from inner
 					// causes. Do it.
-					for i := range s.entries {
-						s.entries[i].elideShort = true
-					}
+					s.elideFurtherCauseMsgs()
 				}
 				break
 			}
@@ -422,7 +421,11 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
 			// If the error did not implement errors.Formatter nor
 			// fmt.Formatter, but it is a wrapper, still attempt best effort:
 			// print what we can at this level.
-			s.formatSimple(err, cause)
+			if elideCauseMsg := s.formatSimple(err, cause); elideCauseMsg {
+				// The error wants to elide the short messages from inner
+				// causes. Do it.
+				s.elideFurtherCauseMsgs()
+			}
 		}
 	}
 
@@ -443,6 +446,18 @@ func (s *state) formatRecursive(err error, isOutermost, withDetail bool) {
 	s.buf = bytes.Buffer{}
 }
 
+// elideFurtherCauseMsgs sets the `elideShort` field
+// on all entries added so far to `true`. Because these
+// entries are added recursively from the innermost
+// cause outward, we can iterate through all entries
+// without bound because the caller is guaranteed not
+// to see entries that it is the causer of.
+func (s *state) elideFurtherCauseMsgs() {
+	for i := range s.entries {
+		s.entries[i].elideShort = true
+	}
+}
+
 func (s *state) collectEntry(err error, bufIsRedactable bool) formatEntry {
 	entry := formatEntry{err: err}
 	if s.wantDetail {
@@ -500,16 +515,19 @@ func RegisterSpecialCasePrinter(fn safeErrorPrinterFn) {
 // formatSimple performs a best effort at extracting the details at a
 // given level of wrapping when the error object does not implement
 // the Formatter interface.
-func (s *state) formatSimple(err, cause error) {
+// Returns true if we want to elide errors from causal chain.
+func (s *state) formatSimple(err, cause error) bool {
 	var pref string
+	elideCauses := false
 	if cause != nil {
-		pref = extractPrefix(err, cause)
+		pref, elideCauses = extractPrefix(err, cause)
 	} else {
 		pref = err.Error()
 	}
 	if len(pref) > 0 {
 		s.Write([]byte(pref))
 	}
+	return elideCauses
 }
 
 // finishDisplay renders s.finalBuf into s.State.