uber-go/fx

Errors should be wrapped not simple passed as value

tymonx opened this issue · 5 comments

tymonx commented

Describe the bug
The fx project use fmt.Errorf("%v", err) instead of fmt.Errorf("%w", err) like here. This prevent from using errors.Unwrap(), errors.Is() and errors.As() functions in custom logger. Also this hide true errors and make error message unclear.

To Reproduce

type Error struct {
    Message string
    Err error
}

func (e *Error) Error() string {
    return e.Message 
}

func (e *Error) Unwrap() error {
    return e.Err
}

fx.New(
    fx.Invoke(
        func() error {
            return &Error{
                 Message: "Error A",
                 Err: &Error{
                     Message: "Error B wrapped in error A",
                 },
            },
        },
    )
).Run()
  • Error value provided by the fx in logger will contain only fx bla bla: ble ble: Error A without containing the message Error B wrapped in error A
  • Error value provided by the fx in logger will contain only fx bla bla: ble ble: Error A (that is OK) but cannot get wrapped error with the errors.Unwrap() function and the wrapped error message is lost Error B wrapped in error A and their corresponding chain of wrapped errors
  • It always returns single error value without any wrapped errors. Missing error context
  • Using errors.Unwrap(), errors.Is() or errors.As() is worthless

Expected behavior

Error provided by the fx should support wrapped errors without loosing error context.

Error value provided by the fx in logger will contain only fx bla bla: ble ble: Error A without containing the message Error B wrapped in error A

I think this is due to a bug in your example: func (e *Error) Error() string does not return the inner error message.

I think if you want to see the inner error message, your code should be something like

func (e *Error) Error() string {
    if e == nil {
        return ""
    }
    if e.Err == nil {
        return e.Message
    }
    return e.Message + ": " + e.Err.Error()
}

A longer comparison: https://go.dev/play/p/_rCdOroAHBk

tymonx commented

@jkanywhere No, it is not a bug and it is also not related with the opened issue.

Yes, it is generally a common approach in Go to return concatenated string from parent and child error using the <parent>: <child> format. But is not mandatory and it depends on a library owner(s). Someone can go with this or return plain parent sting message error or use any kind of format style in the Error() method like for example <file>:<line>:<number>: it is error <message> ## <wrapped_message>!!!. There is no strict rule here.

For example you can return only plain error message from the Error() method without concatenated error message from wrapped error in your library/API because it is easier to generate JSON document with list of errors or like printing tree of error relationships without doing complicate de-formatting from the Error() method that is not standardized to only for example extract parent error message.

Anyway, it is not important like missing information of wrapped type error used in the errors.As(), errors.Is() functions, if or switch-case error type checking.

I agree it would be helpful for Fx to wrap errors, for example using %w, so that errors.As() and errors.Is() work. This is the second two bullets in the issue.

I agree there are many ways to handle error messages, as you say, and that it should be unrelated to the open issue, even though it is listed as the first of three bullet points in the open issue.

tymonx commented

@jkanywhere Yes, the bug was in the wrong statement in the first bullet not in the example. I have already fixed that in the description of the issue. Thanks for comment!