southclaws/fault

Go errors but structured and composable. Fault provides an extensible yet ergonomic mechanism for wrapping errors.

Hi, I'm Barnaby Keene, also known as Southclaws.

Experienced with a wide variety of creative tools and technical skills, I like to solve design, storytelling and technical problems in digital media.

fault's Issues

Play nicely with multierror libraries

I'm honestly not super sure how this will work...

There was a case where a function iterated through a list of items and performed work that errored. The output error only contained one set of fctx metadata but ideally you'd have access to all error metadata.

Same goes for human-readable messages.

Redesign fmsg to be less verbose and more useful.

A common use-case for Fault is to wrap user-facing errors with fmsg.WithDesc where the second argument is a well-formed sentence of copy intended to be displayed to non-technical end-users.

One issue with this API is that it conflates internal error message wrapping with end-user copy.

It has two APIs:

• fmsg.With for serving the same purpose as errors.Wrap(err, "message")
• fmsg.WithDesc which does the same as above but with one extra argument: a message for the end-user.

This frequently results in code that looks like this:

return nil, fault.Wrap(fault.New("user account not found"),
	fmsg.WithDesc("not found",
		"No account could be found with the specified email address."),

And when the error is logged, the flattened form is:

not found: user account not found

Which is useless duplication of concepts, adding noise to the logs and making tracking issues more annoying.

There are two problems here:

• When declaring a new error, you do not need to immediately provide a "contextual" internal error message
• When you want to decorate an error chain with a message intended for the end-user, you're forced to provide a "contextual" internal error message

So I think it would be best if either fmsg was split into two packages or it provided two separate APIs for internal and external error messages. Something like:

return nil, fault.Wrap(fault.New("user account not found"),
	fmsg.WithExternal("No account could be found with the specified email address."),

For when you're creating a brand new error that already has an internal message (user account not found)

Or, for when you're wrapping an existing error and wish to provide some additional internal context as well as an external end-user message:

return nil, fault.Wrap(err,
	fmsg.WithInternal("failed to verify signature"),
	fmsg.WithExternal("Your signature could not be verified, please try again."),

I'm not set on WithInternal and WithExternal though, I'd prefer to keep short concise names that don't add too much noise to error decoration sites.

Error prints do not include external errors in some cases

in this error for some reason the chain ended at "failed to migrate schema" and did not include the rest.

expected:

failed to migrate schema: sql/schema: modify \"accounts\" ...

Make fault less opinionated on displaying "stack" trace and error messages together

It is great that the final err.Error() returns the "stack" lines and the error messages intermingled. It is quite opinionated though. Users might want them separately, so adding some helper functions to Chain could be good, like chain.Stack() to get the "stack" trace (separate from the errors) and chain.Error() to get just the error messages without the "stack" trace.

It might actually make more sense to just make err.Error() just return the error messages as per normal, like it would with normal golang error wrapping. Then chain.String() returns the combined format of "stack" lines plus error messages and then add a helper function like chain.Stack() to return just the "stack" lines.

Enable locations to be generated for the caller in libraries

If a library uses this fault package but the caller does not (yet) then the reported error line is within the library which is not so useful to the caller. It makes sense to me that libraries should report the location of the caller to make it easier for non fault code to get an accurate location.
I however think that In user code this should stay as the line where the wrap is generated to enable the user to identify the relevant return statement.
To handle these 2 situations I separated out location generation in my fork and ensure that only 1 location is created per fault.Wrap
I have some additional changes on my branch relating to nil handling that I can separate out if this issue is accepted but my nil handling suggestion not.

Add fault for sql queries and parameters

I think there is an advantage to have common error types directly in the fault library so that other libraries can throw a common fault that can be extracted from the error chain consistently.
I created such a wrapper that captures the SQL statement and parameters.

https://github.com/spearson78/fsql/blob/main/fsql.go

I propose that that withSql struct and the corresponding wrap and with methods be added to fault

This way other code that execute SQL can capture them and they can be logged consistently.

More cleverly split up glued error messages

Currently, if you use a library that outputs wrapped errors, you end up with duplicates.

For example:

Results in:

errorf wrapped: stdlib sentinel error: stdlib sentinel error

The stdlib sentinel error part is duplicated, which is just noise when trying to read errors.

Solution? Not sure what the best course of action is, but it seems fairly simple to just use the next error message to de-duplicate/split the current.

For example, given:

[1] errorf wrapped: stdlib sentinel error
[2] stdlib sentinel error

When processing [1], look ahead to [2] and remove [2]'s nested message from [1], so we're left with:

[1] errorf wrapped
[2] stdlib sentinel error

Refactor Flatten to first flatten the tree and then filter the results

Currently, the implementation of Flatten is a bit more complex than it needs to be. This is partly due to the way the loop mutates next in a couple of places and also doubly calls Unwrap and re-assigns err twice.

This could be simplified by first running the Unwrap loop to product a simple slice of errors. Then, once there's a slice, it would be easier to do the filtering with lookahead/lookbehind instead of (what is essentially) double step/next iterations etc.

De-duplication logic causes some sentinel error patterns to result in error chain losses

fault/flatten.go

Lines 73 to 79 in 84c038e

	// de-duplicate nested error messages
	if next != nil {
	if idx := strings.Index(message, next.Error()); idx != -1 {
	// cut off the duplicate message and remove the separator.
	message = strings.Trim(message[:idx], ": ")
	}
	}

Repro:

ErrPost = errors.New("post")

ErrExampleOne = fmt.Errorf("%w: %s", ErrPost, "example one")
ErrExampleTwo = fmt.Errorf("%w: %s", ErrExampleOne, "example two")

Expected chain:

post: example one: example two

What actually happens:

post

This one-word error message pattern seems to pop up sometimes, it's even in the standard library so it's definitely worth supporting.

Essentially what happens, I think, is that fmt.Errorf when used this way results in a single error with two messages nested within it:

ErrPost = "post"
ErrExampleOne = "post: example one"
ErrExampleTwo = "post: example one: example two"

So when you chain these together, you get a lot of duplication:

post: post: example one: post: example one: example two

Or, flattened:

"post"
"post: example one"
"post: example one: example two"

So when the de-duplication logic runs over this, it strips the 2 succeeding error messages because they begin with the first 4 bytes as the what the current iteration points to.

Which I guess is fine, as the actual chained error is mostly useless anyway, the Flatten output is what matters.

What I'd ideally want out of this is:

"post"
"example one"
"example two"

But, rolling all of these into one error will probably be simpler for now instead of trying to unwrap error messages into error chains where errors never existed. Otherwise you end up with:

"post"
"post"
"example one"
"post"
"example one"
"example two"

Make fault.Wrap nil tolerant

I found it counter intuitive that fault.Wrap panicked when I passed a nil error.
I tried to upgrade my code by simply replacing

return x,err

with

return x,fault.Wrap(x,fctx.With(ctx))

However sometimes the err in my code is nil and fault.Wrap panicked.

For an error handling library it makes sense to me to not introduce panics into code.

I have these changes on my optional location branch (https://github.com/spearson78/fault/tree/floc). I can separate out the changes if needed.

Error chains are not aware of "non message" error wrappers

Currently, Fault has this concept of a wrapper that has a message and a wrapper that is just a wrapper with metadata.

The wrappers that only provide metadata and do not provide a useful string message will show up in error chains like this:

<fctx>: <fctx>: account not found: <kind>: <fctx>: sql: no rows in result set

That's a lot of useless noise and is a tough sell despite the benefits of Fault decorators.

What's happening here is each decorator that only provides metadata has no information to return when Error() string is called. So they simply return their name in angle brackets.

So when a top-level Error method is called, and it joins together the entire error chain separated by : (as with most error libraries) you end up with a bunch of wrapper names in between the useful internal error messages.

Avoid dependencies

So there's nothing wrong with using testify/assert library if it floats your boat- though I feel there is so little use of it that the whole dependency could be ditched. Like ol' Go proverbs saying goes: a little copying is better than a little dependency :)

I can contribute this feature as I feel fault might have a big future ahead of itself. Awesome library by the way!

v1.0 Roadmap - consolidate built-in wrappers and implement API and DX improvements.

It's been about a year that this library has been in production use in multiple real-world revenue-generating products and I've got a good handle on the improvements I want to make and the direction this library is taking.

So, to mark this, I'd like to bundle the existing wrappers into a single package for a few reasons:

• remove the individual packages: fctx, fmsg, ftag and combine them into one
• introduce all the improvements listed in
  • #38
    • you'll be able to pass anything as a value, like stdlib slog.
  • #9
    • multi-error libraries will no longer cause confusion when unwrapping trees of errors
  • #25
    • add support for the new Go standard Unwrap() []error interface
  • #32
    • the "fmsg" (Fault message) library will only be concerned with end-user messages, not fmt.Errorf style internal messages
  • #31
    • No more Wrap(New(...), wrappers...) just New("msg", wrappers...) 🥳
• settle on the API designs for v1 and commit to compatibility going forward

I'd like to use a single nice short package name for all the built-in wrappers, I'm thinking of just fault/f or fault/w. Mostly just for aesthetics, similar to how Zap and other log aggregators work:

Settled on wrap for context, tags and user-friendly error message reference wrappers

return fault.Wrap(err,
        wrap.Ctx(ctx),
        wrap.Tag(fault.TagNotFound),
        wrap.Msg("There was a technical error while retrieving your account"),
)

Previous ideas:

return fault.Wrap(err,
        f.WithCtx(ctx), // decorate the error with key-value metadata from context
        f.WithTag(ftag.NotFound), // categorise the error as a "not found"
        f.WithMsg("There was a technical error while retrieving your account"), // provide an end-user message
)

Or with:

return fault.Wrap(err,
        with.Ctx(ctx), // decorate the error with key-value metadata from context
        with.Tag(ftag.NotFound), // categorise the error as a "not found"
        with.Msg("There was a technical error while retrieving your account"), // provide an end-user message
)

Or just w

toqueteos pointed out this would be a bad idea as single letter symbols are often used for variable and parameter names.

return fault.Wrap(err,
        w.Ctx(ctx), // decorate the error with key-value metadata from context
        w.Tag(ftag.NotFound), // categorise the error as a "not found"
        w.Msg("There was a technical error while retrieving your account"), // provide an end-user message
)

Any ideas for this are welcome though! cc @matdurand @the-goodies @semihbkgr @toqueteos (just tagging folks who have contributed in case you have ideas or reservations around this!)

Support Unwrap for multiple errors

For Go 1.20

Wrap should fail if err is nil

Pretty standard behaviour, most error wrapping libraries do this. An error being nil is usually the result of accidentally using an err value that's in scope due to an earlier error check in a context that's non-error-like:

err := nothing()
if err != nil {
  return err
}

if someCondition {
  // err is nil here due to the earlier check being false resulting in a nil err remaining in scope
  return errctx.Wrap(err, ctx)
}

fault/errctx/errctx.go

Line 93 in 427aba6

func Wrap(err error, ctx context.Context, kv ...string) error {

Support arbitrary value type in fctx metadata

Hi, thanks for the library.

While testing the library, I see that fctx only supports string values. If I want to add a struct to the metadata, I'll have to either stringify it (but the log will be hard to read when logged using a structured logger) or add each field manually (which is painful if the struct has a lot of fields).

So I want to ask if there is any reason why fctx only supports string values, and if it is possible to make it support arbitrary-type (interface{}) values instead.

Add snippets

Once API is v1, provide some snippets since I write errctx wrapping constantly:

{
  "if err != nil": {
    "prefix": "iferr",
    "body": [
      "if err != nil {",
      "\treturn nil, fault.Wrap(err, fctx.With(ctx))",
      "}"
    ],
    "description": "Snippet for if err != nil"
  }
}

Pass ...Wrapper list to New constructor

A pattern I've seen pop up in usage of Fault is constructing a new error then immediately wrapping it.

For example, this is from a real codebase, we mostly use sentinel errors for fault.News but it still pops up from time to time.

return nil, fault.Wrap(fault.New("user account not found"),
	fctx.With(ctx),
	ftag.With(ftag.NotFound),
	fmsg.WithDesc("not found",
		"No account could be found with the specified email address."),

Two problems:

• fault.Wrap(fault.New is noisy and verbose
• "user account not found" + "not found" is pointless duplication
  • results in three container{} wraps
  • more writing
  • that issue is covered here: #32

Ideal interface:

return nil, fault.New("user account not found",
	fctx.With(ctx),
	ftag.With(ftag.NotFound),
	fmsg.With("No account could be found with the specified email address."),

Mainly what I'd like to tackle is giving fault.New a new signature:

func New(message string, w ...Wrapper) error

And immediately perform the wrapping internally if w contains any elements. This would allow you to create new errors and immediately pass them through your wrappers of choice without the need to first create the error then pass it to Wrap().

It would also make the wrapped sentinel pattern a bit less verbose too (a pattern for declaring sentinel errors with pre-determined wrappers such as error tags)

// Before
var (
    ErrEmptyPassword     = fault.Wrap(fault.New("password parameter is empty"), ftag.With(ftag.InvalidArgument))
    ErrUserNotFound      = fault.Wrap(fault.New("user account not found"), ftag.With(ftag.NotFound))
)

// After
var (
    ErrEmptyPassword     = fault.New("password parameter is empty", ftag.With(ftag.InvalidArgument))
    ErrUserNotFound      = fault.New("user account not found", ftag.With(ftag.NotFound))
)

errctx.Wrap does not store additional kv data if internal ctx typecast fails

Support multiple ftag

Hey,

While using the lib, I discoved that it would be useful to support multiple ftags. Fault already allows me to set multiple ftags (Security being a custom one in this example)

err = fault.Wrap(err,
		fmsg.WithDesc("User not found", "Cannot find the requested user"),
		ftag.With(ftag.NotFound),
		ftag.With(Security))

but the API to get them only return the first tag. This is confusing since the code above is valid.

tag := ftag.Get(err) // will return Security in this case

If you define the solution, I can create a pull request.

Underlying map is modified globally in fctx

A confusing bug just occurred which was the result (I think, not fully investigated yet) of the map in a context chain being modified from deep in the call tree.

I assume this is because new contexts actually just store a pointer to the map, not a copy. So when fctx.WithMeta creates a new context, it's still pointing at the hash table from the parent context. Meaning when you add or change a field, you're actually just mutating a single map instance shared between all contexts.

Now this is kinda useful but it's too complex to deal with. The assumption should always be that contexts grow with the call tree as new children are added and are cleaned up as stack frames return. There should be no shared state and each ctx = somelib.WithXyz(ctx) should always create a copy of the data.

Message missing due to incomplete Format implementation

In some cases, due to my lazy format implementation, you get no useful strings out of a Fault:

<fctx>
	.../db.go:419
<fctx>
	.../db.go:227

This I noticed when using assert.NoError(t, err) where err is a Fault chain.

Should be a simple fix, it seems NoError uses %+v when printing the error, same as panic does and others.

Not getting the expected output

Hi. I love the lib idea and would love to use this, but I'm not getting the expected output.

The first thing I'm not seeing would be multiple stacktrace lines. In the readme, you give this example

stdlib sentinel error
    /Users/southclaws/Work/fault/fault_test.go:34
failed to call function
    /Users/southclaws/Work/fault/fault_test.go:43
    /Users/southclaws/Work/fault/fault_test.go:52

where we see 2 lines for the "failed to call function" error stack (43 and 52), which leads me to believe this is possible.

The second thing I'm not seeing is the right line for my "fault root cause" error.

So what I'm seeing is

fault root cause error
        test/cmd/main.go:38 (this is a call to fault.Wrap, not where the error is created)
failed to call function
        test/cmd/main.go:29

what I would love to see

fault root cause error
        test/cmd/main.go:10 (this line is the line where we create the error using fault.New)
        test/cmd/main.go:14
        (maybe additional stacks here ... not sure how we would configure the depth)
??? (not sure what it should output since this is simple a call to fault.Wrap without additional context)
        test/cmd/main.go:34
failed to call function
        test/cmd/main.go:25

Am I using it wrong?

Here is the example program for reference

package main

import (
	"fmt"
	"github.com/Southclaws/fault"
	"github.com/Southclaws/fault/fmsg"
)

func rootCause1() error {
	return fault.New("fault root cause error")
}

func rootCause2() error {
	return rootCause1()
}

func ErrorCaller() error {
	err := errorCallerFromMiddleOfChain()
	if err != nil {
		return fault.Wrap(err)
	}

	return nil
}

func errorCallerFromMiddleOfChain() error {
	err := errorProducerFromRootCause()
	if err != nil {
		return fault.Wrap(err, fmsg.With("failed to call function"))
	}

	return nil
}

func errorProducerFromRootCause() error {
	err := rootCause2()
	if err != nil {
		return fault.Wrap(err)
	}

	return nil
}

func main() {
	err := ErrorCaller()
	fmt.Printf("%+v", err)
}

Thank you