Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: Rethink common.GetErrorMsg approach #3042

Closed
oncilla opened this issue Aug 26, 2019 · 3 comments
Closed

lib: Rethink common.GetErrorMsg approach #3042

oncilla opened this issue Aug 26, 2019 · 3 comments

Comments

@oncilla
Copy link
Contributor

oncilla commented Aug 26, 2019

The current common.GetErrorMsg approach is very brittle. If during a refactor the error is wrapped. or a non-constant string is modified, the check will fail.

We should consider an approach similar to xerrors.Is or just use xerrors.Is and let common.BasicError implement an Is method.

@oncilla
Copy link
Contributor Author

oncilla commented Aug 26, 2019

E.g. the cause of #3037

@lukedirtwalker
Copy link
Collaborator

With Go 1.13 moving towards the current xerrors.Is etc. (See https://tip.golang.org/doc/go1.13#error_wrapping) we should definitely work towards this. Me and @oncilla quickly drafted a PoC how we can integrate this into our existing common errors library:

type SimpleError string

func (e SimpleError) Error() string {
	return string(e)
}

type BasicError struct {
	// Error message
	Msg SimpleError
	// Error context, for logging purposes only
	logCtx []interface{}
	// Nested error, if any.
	Err error
}

func (be BasicError) Is(err error) bool {
	switch other := err.(type) {
	case BasicError:
		return be.Msg == other.Msg
	case SimpleError:
		return be.Msg == other
	default:
		return false
	}
}

func (be BasicError) Unrwap() error {
	return be.GetErr()
}

Note that BasicError is the same existing error with Msg changed from string to SimpleError.

Note that using old string messages would still be allowed (except fmt.Sprintf break but those often contain stuff that should go into context anyway.) But the idea is that we move to typed errors:

const (
    ErrInvariantViolation common.SimpleError = "TRC invariant violation"
)

func foo() error {
     return common.NewBasicError(ErrInvariantViolation, nil, "ctx", 1)
}

func main() {
    err := foo()
    if xerrors.Is(err, ErrInvariantViolation) {
        fmt.Sprintf("Invariant violation\n")
    }
}

Note that with this we can define all errors as constants (no vars for simple text only errors as would be the case with errors.New or common.NewBasicError("text",nil)) so we get additional guarantees that no one can tamper with error message strings.

lukedirtwalker added a commit to lukedirtwalker/scion that referenced this issue Aug 30, 2019
This commit adds Unwrap and Is methods to the BasicError type.
It also adds a SimpleError type that can be used for error string constants.
This will allow us to get rid of brittle error checks with GetErrorMsg=="some string".

Contributes scionproto#3042
lukedirtwalker added a commit to lukedirtwalker/scion that referenced this issue Aug 30, 2019
This commit adds Unwrap and Is methods to the BasicError type.
It also adds a SimpleError type that can be used for error string constants.
This will allow us to get rid of brittle error checks with GetErrorMsg=="some string".

Contributes scionproto#3042
lukedirtwalker added a commit that referenced this issue Aug 30, 2019
This commit adds Unwrap and Is methods to the BasicError type.
It also adds a SimpleError type that can be used for error string constants.
This will allow us to get rid of brittle error checks with GetErrorMsg=="some string".

Contributes #3042
lukedirtwalker added a commit that referenced this issue Sep 20, 2019
This should in the long term replace:
-errors.New
-common.NewBasicError

The package is ready to be used with Go1.13 (or xerrors) Is and As.

Also it provides all the functionality that was provided by common.BasicError/common.MultiError.

By putting it in a serrors package we can make the API slightly nicer to use.

Contributes #3042
lukedirtwalker added a commit to lukedirtwalker/scion that referenced this issue Sep 20, 2019
Those checks are dangerous since a refactor could easily break them.
Using xerrors.Is instead is safer against refactors.

Contributes scionproto#3042
lukedirtwalker added a commit that referenced this issue Sep 20, 2019
Those checks are dangerous since a refactor could easily break them.
Using xerrors.Is instead is safer against refactors.

Contributes #3042
@oncilla
Copy link
Contributor Author

oncilla commented Dec 23, 2019

serrors support is in (including a linter)
What remains to be done is translating the old common.BasicError to serrors.Error

@oncilla oncilla closed this as completed Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants