Merge pull request #48 from knz/20200901-redact

Author: knz

errbase/format_error_internal_test.go

@@ -87,13 +87,13 @@ func TestPrintEntryRedactable(t *testing.T) {
 		// characters; they get enclosed in redaction markers in the final output.
 		{formatEntry{}, ""},
 		{formatEntry{head: b("abc")}, " " + q("abc")},
-		{formatEntry{head: b("abc\nxyz")}, " " + q("abc\nxyz")},
+		{formatEntry{head: b("abc\nxyz")}, " " + q("abc") + "\n" + q("xyz")},
 		{formatEntry{details: b("def")}, " " + q("def")},
-		{formatEntry{details: b("def\nxyz")}, " " + q("def\nxyz")},
+		{formatEntry{details: b("def\nxyz")}, " " + q("def") + "\n" + q("xyz")},
 		{formatEntry{head: b("abc"), details: b("def")}, " " + q("abc") + q("def")},
-		{formatEntry{head: b("abc\nxyz"), details: b("def")}, " " + q("abc\nxyz") + q("def")},
-		{formatEntry{head: b("abc"), details: b("def\n  | xyz")}, " " + q("abc") + q("def\n  | xyz")},
-		{formatEntry{head: b("abc\nxyz"), details: b("def\n  | xyz")}, " " + q("abc\nxyz") + q("def\n  | xyz")},
+		{formatEntry{head: b("abc\nxyz"), details: b("def")}, " " + q("abc") + "\n" + q("xyz") + q("def")},
+		{formatEntry{head: b("abc"), details: b("def\n  | xyz")}, " " + q("abc") + q("def") + "\n" + q("  | xyz")},
+		{formatEntry{head: b("abc\nxyz"), details: b("def\n  | xyz")}, " " + q("abc") + "\n" + q("xyz") + q("def") + "\n" + q("  | xyz")},
 		// If there were markers in the entry, they get escaped in the output.
 		{formatEntry{head: b("abc" + em + sm), details: b("def" + em + sm)}, " " + q("abc"+esc+esc) + q("def"+esc+esc)},
 
@@ -141,7 +141,7 @@ func TestFormatSingleLineOutputRedactable(t *testing.T) {
 		{[]formatEntry{{head: b(`a`)}, {}}, q(`a`)},
 		{[]formatEntry{{head: b(`a`)}, {}, {head: b(`c`)}}, q(`c`) + ": " + q(`a`)},
 		{[]formatEntry{{head: b(`a`), elideShort: true}, {head: b(`b`)}}, q(`b`)},
-		{[]formatEntry{{head: b("abc\ndef")}, {head: b("ghi\nklm")}}, q("ghi\nklm") + ": " + q("abc\ndef")},
+		{[]formatEntry{{head: b("abc\ndef")}, {head: b("ghi\nklm")}}, q("ghi") + "\n" + q("klm") + ": " + q("abc") + "\n" + q("def")},
 
 		// If some entries are redactable but not others, then
 		// only those that are redactable are passed through.
@@ -157,8 +157,8 @@ func TestFormatSingleLineOutputRedactable(t *testing.T) {
 		{[]formatEntry{{head: b(`a`)}, {}, {head: b(`c`)}}, q(`c`) + `: ` + q(`a`)},
 		{[]formatEntry{{head: b(`a`)}, {redactable: true}, {head: b(`c`)}}, q(`c`) + `: ` + q(`a`)},
 		{[]formatEntry{{head: b(`a`), elideShort: true, redactable: true}, {head: b(`b`)}}, q(`b`)},
-		{[]formatEntry{{redactable: true, head: b("abc\ndef")}, {head: b("ghi\nklm")}}, q("ghi\nklm") + ": abc\ndef"},
-		{[]formatEntry{{head: b("abc\ndef")}, {redactable: true, head: b("ghi\nklm")}}, "ghi\nklm: " + q("abc\ndef")},
+		{[]formatEntry{{redactable: true, head: b("abc\ndef")}, {head: b("ghi\nklm")}}, q("ghi") + "\n" + q("klm") + ": abc\ndef"},
+		{[]formatEntry{{head: b("abc\ndef")}, {redactable: true, head: b("ghi\nklm")}}, "ghi\nklm: " + q("abc") + "\n" + q("def")},
 		// Entry already contains some markers.
 		{[]formatEntry{{redactable: true, head: b(`a` + q(" b"))}, {redactable: true, head: b(`c ` + q("d"))}}, `c ` + q(`d`) + `: a` + q(` b`)},
 	}

errutil/message_test.go

@@ -55,8 +55,8 @@ Error types: (1) *withstack.withStack (2) *errutil.leafError`,
 
 		{"newf-empty-arg",
 			errutil.Newf(emptyString, 123),
-			`%!(EXTRA *redact.escapeArg=123)`, `
-%!(EXTRA *redact.escapeArg=123)
+			`%!(EXTRA int=123)`, `
+%!(EXTRA int=123)
 (1) attached stack trace
   -- stack trace:
   | github.com/cockroachdb/errors/errutil_test.TestFormat
@@ -65,7 +65,7 @@ Error types: (1) *withstack.withStack (2) *errutil.leafError`,
   | <tab><path>
   | runtime.goexit
   | <tab><path>
-Wraps: (2) %!(EXTRA *redact.escapeArg=123)
+Wraps: (2) %!(EXTRA int=123)
 Error types: (1) *withstack.withStack (2) *errutil.leafError`,
 		},
 
@@ -87,8 +87,8 @@ Error types: (1) *withstack.withStack (2) *errors.errorString`,
 
 		{"wrapf-empty-arg",
 			errutil.Wrapf(goErr.New("woo"), emptyString, 123),
-			`%!(EXTRA *redact.escapeArg=123): woo`, `
-%!(EXTRA *redact.escapeArg=123): woo
+			`%!(EXTRA int=123): woo`, `
+%!(EXTRA int=123): woo
 (1) attached stack trace
   -- stack trace:
   | github.com/cockroachdb/errors/errutil_test.TestFormat
@@ -97,7 +97,7 @@ Error types: (1) *withstack.withStack (2) *errors.errorString`,
   | <tab><path>
   | runtime.goexit
   | <tab><path>
-Wraps: (2) %!(EXTRA *redact.escapeArg=123)
+Wraps: (2) %!(EXTRA int=123)
 Wraps: (3) woo
 Error types: (1) *withstack.withStack (2) *errutil.withPrefix (3) *errors.errorString`,
 		},
@@ -153,8 +153,8 @@ Error types: (1) *assert.withAssertionFailure (2) *withstack.withStack (3) *barr
 
 		{"assert + wrap empty+arg",
 			errutil.NewAssertionErrorWithWrappedErrf(&werrFmt{goErr.New("woo"), "wuu"}, emptyString, 123),
-			`%!(EXTRA *redact.escapeArg=123): wuu: woo`, `
-%!(EXTRA *redact.escapeArg=123): wuu: woo
+			`%!(EXTRA int=123): wuu: woo`, `
+%!(EXTRA int=123): wuu: woo
 (1) assertion failure
 Wraps: (2) attached stack trace
   -- stack trace:
@@ -164,7 +164,7 @@ Wraps: (2) attached stack trace
   | <tab><path>
   | runtime.goexit
   | <tab><path>
-Wraps: (3) %!(EXTRA *redact.escapeArg=123)
+Wraps: (3) %!(EXTRA int=123)
 Wraps: (4) wuu: woo
   | -- cause hidden behind barrier
   | wuu: woo

errutil/redactable.go

@@ -108,3 +108,56 @@ func init() {
 	errbase.RegisterWrapperEncoder(errbase.GetTypeKey((*withPrefix)(nil)), encodeWithPrefix)
 	errbase.RegisterWrapperDecoder(errbase.GetTypeKey((*withPrefix)(nil)), decodeWithPrefix)
 }
+
+// withNewMessage is like withPrefix but the message completely
+// overrides that of the underlying error.
+type withNewMessage struct {
+	cause   error
+	message redact.RedactableString
+}
+
+var _ error = (*withNewMessage)(nil)
+var _ fmt.Formatter = (*withNewMessage)(nil)
+var _ errbase.SafeFormatter = (*withNewMessage)(nil)
+var _ errbase.SafeDetailer = (*withNewMessage)(nil)
+
+func (l *withNewMessage) Error() string {
+	return l.message.StripMarkers()
+}
+
+func (l *withNewMessage) Cause() error  { return l.cause }
+func (l *withNewMessage) Unwrap() error { return l.cause }
+
+func (l *withNewMessage) Format(s fmt.State, verb rune) { errbase.FormatError(l, s, verb) }
+func (l *withNewMessage) SafeFormatError(p errbase.Printer) (next error) {
+	p.Print(l.message)
+	return nil /* nil here overrides the cause's message */
+}
+
+func (l *withNewMessage) SafeDetails() []string {
+	return []string{l.message.Redact().StripMarkers()}
+}
+
+func encodeWithNewMessage(_ context.Context, err error) (string, []string, proto.Message) {
+	l := err.(*withNewMessage)
+	return l.Error(), l.SafeDetails(), &errorspb.StringPayload{Msg: string(l.message)}
+}
+
+func decodeWithNewMessage(
+	_ context.Context, cause error, _ string, _ []string, payload proto.Message,
+) error {
+	m, ok := payload.(*errorspb.StringPayload)
+	if !ok {
+		// If this ever happens, this means some version of the library
+		// (presumably future) changed the payload type, and we're
+		// receiving this here. In this case, give up and let
+		// DecodeError use the opaque type.
+		return nil
+	}
+	return &withNewMessage{cause: cause, message: redact.RedactableString(m.Msg)}
+}
+
+func init() {
+	errbase.RegisterWrapperEncoder(errbase.GetTypeKey((*withNewMessage)(nil)), encodeWithNewMessage)
+	errbase.RegisterWrapperDecoder(errbase.GetTypeKey((*withNewMessage)(nil)), decodeWithNewMessage)
+}

errutil/utilities.go

@@ -15,10 +15,6 @@
 package errutil
 
 import (
-	"fmt"
-	"regexp"
-
-	"github.com/cockroachdb/errors/safedetails"
 	"github.com/cockroachdb/errors/secondary"
 	"github.com/cockroachdb/errors/withstack"
 	"github.com/cockroachdb/redact"
@@ -78,11 +74,11 @@ func NewWithDepthf(depth int, format string, args ...interface{}) error {
 			errRefs = append(errRefs, e)
 		}
 	}
-	if hasVerbW.MatchString(format) {
-		err = fmt.Errorf(format, args...)
-		err = safedetails.WithSafeDetails(err, format, args...)
+	redactable, wrappedErr := redact.HelperForErrorf(format, args...)
+	if wrappedErr != nil {
+		err = &withNewMessage{cause: wrappedErr, message: redactable}
 	} else {
-		err = error(&leafError{redact.Sprintf(format, args...)})
+		err = &leafError{redactable}
 	}
 	for _, e := range errRefs {
 		err = secondary.WithSecondaryError(err, e)
@@ -91,36 +87,6 @@ func NewWithDepthf(depth int, format string, args ...interface{}) error {
 	return err
 }
 
-var hasVerbW = regexp.MustCompile(
-	`^` +
-		// Ignore any non-%w format directives and other characters:
-		`([^%]+|%` +
-		// A format directive starts with some flags.
-		`[-#0+ ]*` +
-		// Followed by an optional argument number [n].
-		`(\[[0-9]+\])?` +
-		// Followed by an optional width.
-		`(\*|[0-9]+)?` +
-		// followed by an optional precision.
-		`(\.(\*|[0-9]))?` +
-		// We're only interested in non-w verbs here.
-		`[^w]` +
-		// zero or more times.
-		`)*` +
-		// Followed with at least one occurrence of a format directive
-		// with the 'w' verb. We replicate the rules above.
-		`%` +
-		// A format directive starts with some flags.
-		`[-#0+ ]*` +
-		// Followed by an optional argument number [n].
-		`(\[[0-9]+\])?` +
-		// Followed by an optional width.
-		`(\*|[0-9]+)?` +
-		// followed by an optional precision.
-		`(\.(\*|[0-9]))?` +
-		// Just the verb 'w'.
-		`w`)
-
 // Wrap wraps an error with a message prefix.
 // A stack trace is retained.
 //

errutil/utilities_test.go

@@ -197,6 +197,9 @@ var wrapCommands = map[string]commandFn{
 	// FormatError() method.
 	"msg": func(err error, args []arg) error { return errutil.WithMessage(err, strfy(args)) },
 
+	// newfw is errors.Newf("%w") which is the fmt-standard way to wrap an error.
+	"newfw": func(err error, args []arg) error { return errutil.Newf("new-style (%s) :: %w ::", strfy(args), err) },
+
 	// errutil.Wrap implements multi-layer wrappers.
 	"wrapf": func(err error, args []arg) error { return errutil.Wrapf(err, "new-stylew %s", strfy(args)) },
 	// assertions mask their cause from the barriers, but otherwise format as-is.

fmttests/datadriven_test.go

@@ -334,6 +334,25 @@ Error types: (1) *fmttests.werrDelegateEmpty (2) *errors.withStack (3) *fmttests
 	}
 }
 
+func TestHelperForErrorf(t *testing.T) {
+	origErr := goErr.New("small\nuniverse")
+	s, e := redact.HelperForErrorf("hello %s", origErr)
+	if actual, expected := string(s), "hello ‹small›\n‹universe›"; actual != expected {
+		t.Errorf("expected %q, got %q", expected, actual)
+	}
+	if e != nil {
+		t.Errorf("expected no error, got %v", e)
+	}
+
+	s, e = redact.HelperForErrorf("hello %w", origErr)
+	if actual, expected := string(s), "hello ‹small›\n‹universe›"; actual != expected {
+		t.Errorf("expected %q, got %q", expected, actual)
+	}
+	if e != origErr {
+		t.Errorf("expected error %v, got %v (%T)", origErr, e, e)
+	}
+}
+
 func fmtClean(spv string) string {
 	spv = fileref.ReplaceAllString(spv, "<path>:<lineno>")
 	spv = libref.ReplaceAllString(spv, "<path>")