RedactableError support for errors.Is/As/AsType#8182
Conversation
There was a problem hiding this comment.
Pull request overview
Adds standard library error-chain interoperability to base.RedactableError so callers can use errors.Is, errors.As, and errors.AsType against wrapped errors.
Changes:
- Added
(*RedactableError).Unwrapto participate in Go’s error unwrapping chain. - Added unit tests covering
errors.Is/errors.As/errors.AsTypescenarios (including multiple%w).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| base/redactable_error.go | Adds an Unwrap method intended to expose wrapped errors for stdlib error-chain APIs. |
| base/redactable_error_test.go | Adds tests validating errors.Is/As/AsType behavior through RedactableError wrappers. |
| // Unwrap returns the underlying error(s) created by fmt.Errorf. Allows use of errors.As, errors.AsType, errors.Is | ||
| func (re *RedactableError) Unwrap() error { | ||
| return fmt.Errorf(re.fmt, re.args...) | ||
| } |
There was a problem hiding this comment.
Unwrap currently returns a newly constructed fmt.Errorf(...) error, which is not the underlying wrapped error. This breaks errors.Unwrap(re) semantics (it will return the formatted wrapper, not the wrapped error), and can leak unredacted user data if the unwrapped error is logged (since the formatted message uses the original args, not redact(...)). Consider implementing Unwrap to return the actual wrapped error (or nil when there is no %w), e.g. by constructing the fmt.Errorf wrapper and then extracting/returning its Unwrap() result(s) rather than returning the wrapper itself (joining multiple unwraps if needed).
| assert.True(t, errors.As(re, &redactedTarget), "errors.As(re, &redactedTarget) should be true") | ||
| assert.Equal(t, re, redactedTarget) | ||
| }) | ||
|
|
There was a problem hiding this comment.
The new tests cover errors.Is/errors.As/errors.AsType, but they don't assert the behavior of errors.Unwrap on RedactableError. Since there is production code that uses errors.Unwrap(err) (and expects it to return the underlying error), it would be good to add a test ensuring errors.Unwrap(re) returns the wrapped error for a single %w and returns nil when there is no %w.
| t.Run("errors.Unwrap", func(t *testing.T) { | |
| err := &customErr{msg: "unwrap error"} | |
| re := RedactErrorf("wrapped: %w", err) | |
| assert.Equal(t, err, errors.Unwrap(re)) | |
| reNoWrap := RedactErrorf("not wrapped: %s", "value") | |
| assert.Nil(t, errors.Unwrap(reNoWrap)) | |
| }) |
|
|
||
| // Unwrap returns the underlying error(s) created by fmt.Errorf. Allows use of errors.As, errors.AsType, errors.Is | ||
| func (re *RedactableError) Unwrap() error { | ||
| return fmt.Errorf(re.fmt, re.args...) |
There was a problem hiding this comment.
I'm not against this but it does cause all of the redaction/type information to be lost, which seems more prone to accidental use (e.g. returning an unwrapped error to reduce noise/context from a known error-handling codepath after an errors.Is/As)
I think if we want to do this, we should do it in a way that allows Unwrap without building a new error.
bbrks
left a comment
bbrks
left a comment
There was a problem hiding this comment.
I'm OK with this last iteration of Unwrap - we should just get some test coverage of redacting the pre and post-unwrap stages
| var target *customErr | ||
| assert.True(t, errors.As(re2, &target)) | ||
| assert.Equal(t, err1, target) // errors.As returns the first one it finds | ||
| }) |
There was a problem hiding this comment.
Looks pretty good. Maybe just one test case to cover the wrapped redactable error handling?
t.Run("Redactor interface survives Unwrap", func(t *testing.T) {
inner := RedactErrorf("user %s", UD("Bob"))
outer := RedactErrorf("wrapped: %w", inner)
// Outer redacts correctly
assert.Contains(t, outer.Redact(), "user <ud>Bob</ud>")
// errors.As extracts inner, still satisfies Redactor
var target *RedactableError
require.True(t, errors.As(outer, &target))
require.NotSame(t, outer, target)
// Unwrapped inner still redacts
var r Redactor = target
assert.Contains(t, r.Redact(), "user <ud>Bob</ud>")
assert.NotContains(t, r.Redact(), "user Bob")
})
RedactableError support for errors.Is/As/AsType
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests