cockroachdb / errors

Go error library with error portability over the network
Apache License 2.0
2.07k stars 66 forks source link

Implementing Is method for custom errors is not called #58

Closed rlopzc closed 3 years ago

rlopzc commented 3 years ago

When using errors.Is(..) the Is method implementation is not called.

Code example

type WalletError struct {
    prefix string
    cause  error
    Type   WalletErrorType
}

func (e WalletError) Error() string {
    if e.prefix == "" {
        return e.cause.Error()
    }
    return fmt.Sprintf("%s: %v", e.prefix, e.cause)
}

func (e WalletError) Is(target error) bool {
    t, ok := target.(WalletError)
    if !ok {
        return false
    }

    return (e.prefix == t.prefix) &&
        (e.cause.Error() == t.cause.Error()) &&
        (e.Type == t.Type)
}

Using cockroach errors.Is(..) doesn't call the Is method from above. If I instead use the "errors" package then it does get called and my assertion passes.

Am I missing something?

Thank you

knz commented 3 years ago

Hi Romario, this errors library does not yet recognize Is() methods on the error object themselves. This seems like a reasonable feature request so we can probably accommodate it.

In the meantime, you can work around this by implementing a method ErrorKeyMarker() string which returns the Type field of your error payload, and then errors.Is() should work ok for your type.

knz commented 3 years ago

@romariolopezc something to keep in mind though -- this library's errors.Is() function uses an "identity marker" which is composed of message + type.

So in your example, even if there is a Is() method, when this method returns false two errors can still be considered equal as per errors.Is() if they have the same message. So in your case you cannot use a custom Is() method to "make two errors different" based on the separate Type field.

Said in a different way, the rule (as per the go doc) is "if a.Is(b) is true, then errors.Is(a, b) is true",

not "if a.Is(b) is false, then errors.Is(a, b) is false" .

To obtain this last behavior, you'll need a ErrorKeyMarker() method as per my message above, or use errors.Mark().

rlopzc commented 3 years ago

@knz Hey! Thank you for the explanation about the errors.Is usage. It makes sense what you said about the ErrorKeyMarker()

Btw thank you for the PR and prompt response!