Merge pull request #113 from dhartunian/fix-wrapError-encode-decode

support wrapError encode and decode

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_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

@@ -97,7 +97,7 @@ func decodeWrapper(ctx context.Context, enc *errorspb.EncodedWrapper) error {
 	typeKey := TypeKey(enc.Details.ErrorTypeMark.FamilyName)
 	if decoder, ok := decoders[typeKey]; ok {
 		// Yes, use it.
-		genErr := decoder(ctx, cause, enc.MessagePrefix, enc.Details.ReportablePayload, payload)
+		genErr := decoder(ctx, cause, enc.Message, enc.Details.ReportablePayload, payload)
 		if genErr != nil {
 			// Decoding succeeded. Use this.
 			return genErr
@@ -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.Message,
+		details:     enc.Details,
+		messageType: MessageType(enc.MessageType),
 	}
 }
 

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
+	messageType := Prefix
 
 	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
+		messageType = e.messageType
 	} 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, messageType = enc(ctx, err)
 		} else {
 			// No encoder.
 			// In that case, we'll try to compute a message prefix
 			// manually.
-			msg = extractPrefix(err, cause)
+			msg, messageType = 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),
+				Message:     msg,
+				Details:     details,
+				MessageType: errorspb.MessageType(messageType),
 			},
 		},
 	}
 }
 
 // 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, MessageType) {
 	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 "", Prefix
+		}
 		if strings.HasSuffix(prefix, ": ") {
-			return prefix[:len(prefix)-2]
+			return prefix[:len(prefix)-2], Prefix
 		}
 	}
-	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, FullMessage
 }
 
 func getTypeDetails(
@@ -295,6 +319,35 @@ var leafEncoders = map[TypeKey]LeafEncoder{}
 // or a different type, ensure that RegisterTypeMigration() was called
 // prior to RegisterWrapperEncoder().
 func RegisterWrapperEncoder(theType TypeKey, encoder WrapperEncoder) {
+	RegisterWrapperEncoderWithMessageType(
+		theType,
+		func(ctx context.Context, err error) (
+			msgPrefix string,
+			safeDetails []string,
+			payload proto.Message,
+			messageType MessageType,
+		) {
+			prefix, details, payload := encoder(ctx, err)
+			return prefix, details, payload, messageType
+		})
+}
+
+// RegisterWrapperEncoderWithMessageType 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 RegisterWrapperEncoderWithMessageType(
+	theType TypeKey, encoder WrapperEncoderWithMessageType,
+) {
 	if encoder == nil {
 		delete(encoders, theType)
 	} else {
@@ -304,7 +357,42 @@ func RegisterWrapperEncoder(theType TypeKey, encoder WrapperEncoder) {
 
 // WrapperEncoder is to be provided (via RegisterWrapperEncoder above)
 // 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)
+type WrapperEncoder func(ctx context.Context, err error) (
+	msgPrefix string,
+	safeDetails []string,
+	payload proto.Message,
+)
+
+// MessageType is used to encode information about an error message
+// within a wrapper error type. This information is used to affect
+// display logic.
+type MessageType errorspb.MessageType
+
+// Values below should match the ones in errorspb.MessageType for
+// direct conversion.
+const (
+	// Prefix denotes an error message that should be prepended to the
+	// message of its cause.
+	Prefix MessageType = MessageType(errorspb.MessageType_PREFIX)
+	// FullMessage denotes an error message that contains the text of its
+	// causes and can be displayed standalone.
+	FullMessage = MessageType(errorspb.MessageType_FULL_MESSAGE)
+)
+
+// WrapperEncoderWithMessageType is to be provided (via
+// RegisterWrapperEncoderWithMessageType above) by additional wrapper
+// types not yet known to this library. This encoder returns an
+// additional enum which indicates whether the wrapper owns the error
+// message completely instead of simply being a prefix with the error
+// message of its causes appended to it. This information is encoded
+// along with the prefix in order to provide context during error
+// display.
+type WrapperEncoderWithMessageType func(ctx context.Context, err error) (
+	msgPrefix string,
+	safeDetails []string,
+	payload proto.Message,
+	messageType MessageType,
+)
 
 // registry for RegisterWrapperType.
-var encoders = map[TypeKey]WrapperEncoder{}
+var encoders = map[TypeKey]WrapperEncoderWithMessageType{}

errbase/err_string_ownership_test.go

@@ -0,0 +1,105 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+// implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+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(mt errorspb.MessageType) 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",
+						},
+					},
+				},
+				Message:     "wrapper-error-msg: leaf-error-msg: extra info",
+				Details:     errorspb.EncodedErrorDetails{},
+				MessageType: mt,
+			},
+		},
+	}
+}
+
+func TestDecodeOldVersion(t *testing.T) {
+	tt := testutils.T{T: t}
+
+	errOldEncoded := genEncoded(errorspb.MessageType_PREFIX)
+	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(errorspb.MessageType_FULL_MESSAGE)
+	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,
+							},
+						},
+					},
+				},
+				Message: "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,
+				},
+				MessageType: errorspb.MessageType_FULL_MESSAGE,
+			},
+		},
+	}
+
+	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")
+}