Question on appropriate ways to use errors pkg with additional data in the error

[pkg]

Question on appropriate ways to use errors pkg with additional data in the error

[davecheney]

@solher opaque error handling means the caller of a function that returns an error does not customise it's behaviour depending on the value or type of the error returned; it only reacts to the presence of the error, err != nil.

If you can write your code in such a way, then you can use opaque errors and the other techniques I described in my talk. If you can not, then the caller and the code you are calling are tightly coupled because the type or value of the error returned is known to both pieces of code, is part of the API, and affects the behaviour of the caller.

With that said, you can still use error values and types with this package, you just have to call errors.Cause before inspecting the error value.

    if err := Parse(); err != nil {
        switch t := errors.Cause(err).(type) {
            case errs.ErrParsing:
                GeneratePreciseAPIError("PARSING_ERROR", t.Line, t.Field)
            default:
                GenerateGenericAPIError(err.Error())
        }
    }

[cep21]

It may be worth reading #34

[davecheney]

Thank you for raising this issue. You've written a lot, and I apologise up front that I have not been able to distil this into a single request. If my answer is not correct, please reply and correct me.

What I believe you are trying to do is what I described in my GopherCon talk as an error type. The alternative is to return an error which implements something like an ErrorCode() int method.

wrt. to you questions about the Temporary interface, please open a new issue. One issue per topic please.

[veqryn]

@cep21 Thanks, I did read the whole discussion. I think everyone can agree that additional context probably doesn't belong in this library at this point, and that it is up to people to add context or additional error handling behavior by themselves. That is sort of why I made this issue: to see how to do that while still using this library.

@davecheney Thanks for your reply. I've remove the question about Temporary in the original post above.

So, here would be my example above, if we added an ErrorCode() int interface:

type MyError struct {
    // embedded builtin types are not exported, so alias to Err
    Err error
    Code  int
}

type ErrorCoder interface {
    ErrorCode() int
}

func (e MyError) ErrorCode() int {
    return e.Code
}

func (e MyError) Cause() error {
    return errors.Cause(e.Err)
}

const NilErrorCode = 0
const UnknownErrorCode = -1

func ErrorCode(err error) int {
    if err == nil {
        return NilErrorCode
    }
    if ec, ok := err.(ErrorCoder); ok {
        return ec.ErrorCode()
    }
    return UnknownErrorCode
}

func (e MyError) Format(s fmt.State, verb rune) {
    switch verb {
    case 'v':
        if s.Flag('+') {
            _, _ = fmt.Fprintf(s, "%+v\n(Code:%d)", e.Err, e.Code)
            return
        }
        fallthrough
    case 's':
        _, _ = io.WriteString(s, e.Error())
    case 'q':
        _, _ = fmt.Fprintf(s, "%q", e.Error())
    }
}

func (e MyError) Error() string {
    return fmt.Sprintf("%s; (Code:%d)", e.Err, e.Code)
}

// Implement MarshalJSON for writing to json logs, and WriteTo for writing to non-json logs...

Here is an example of its usage:

// New Error
var errNew error = MyError{errors.New("new error"), 1}

// Wrapping:
var errWrapped error = MyError{errors.Wrap(innerErr, "outer error"), 255}

if errWrapped != nil {
    if ErrorCode(errWrapped) == 255 {
        // handle 255
    }
    // handle other stuff
}

Questions:

1. In your original example, you made the interface itself non-exported, and had the helper method be exported. Should the ErrorCoder interface be exported, and why or why not?
2. Would you have the helper method func ErrorCode(err error) int also return an error if the error argument did not implement ErrorCoder interface? Your original example with temporary just returned false if the error did not implement the interface, but there was no way to tell with the helper method the difference between an error that didn't implement temporary, and an error that was tempoary=false.
3. Would you recommend this or something similar to people who need to handle errors' behavior when that behavior can get more complex than your temporary interface example? (In my particular example, a vendor's api returns a lot of different error states and reasons, which need to be handled in different ways.)

[davecheney]

1. Interfaces should be defined by the caller, not the implementer. If you
   need an interface e decl, decl it in your own package to reduce coupling.
2. Similarly. If you need this helper, declare it in your own package, it
   doesn't need to go in the errors package.
3. It sounds like you want an error type (like os.PathError). To clarify
   what I said in my presentation, I didn't say you must not use error types
   or values, just recognise the they are less flexible than treating the
   error as an opaque value that is either in error or not.

On Fri, 29 Jul 2016, 03:41 Chris Duncan [email protected] wrote:

@cep21 https://github.com/cep21 Thanks, I did read the whole
discussion. I think everyone can agree that additional context probably
doesn't belong in this library at this point, and that it is up to people
to add context or additional error handling behavior by themselves. That is
sort of why I made this issue: to see how to do that while still using this
library.

@davecheney https://github.com/davecheney Thanks for your reply. I've
remove the question about Temporary in the original post above.

So, here would be my example above, if we added an ErrorCode() int
interface:

type MyError struct {
// embedded builtin types are not exported, so alias to Err
Err error
Code int
}
type ErrorCoder interface {
ErrorCode() int
}
func (e MyError) ErrorCode() int {
return e.Code

}
func (e MyError) Cause() error

{
return errors.Cause(e.Err)
}
const NilErrorCode = 0const UnknownErrorCode = -1

func ErrorCode(err error) int {
if err == nil

{
return NilErrorCode
}
if ec, ok := err.(ErrorCoder); ok {
return ec.ErrorCode()
}
return UnknownErrorCode
}

func (e MyError) Format(s fmt.State, verb rune) {
switch verb {
case 'v':
if s.Flag('+'

) {
_, _ = fmt.Fprintf(s, "%+v\n(Code:%d)", e.Err, e.Code)

return
}
fallthrough
case 's':
_, _ = io.WriteString(s, e.Error())
case 'q':
_, _ = fmt.Fprintf(s, "%q", e.Error())
}
}
func (e MyError) Error() string

{
return fmt.Sprintf("%s; (Code:%d)", e.Err, e.Code)
}
// Implement MarshalJSON for writing to json logs, and WriteTo for writing to non-json logs...

Here is an example of its usage:

// New Errorvar errNew error = MyError{errors.New("new error"), 1}
// Wrapping:var errWrapped error = MyError{errors.Wrap(innerErr, "outer error"), 255}
if errWrapped != nil {
if ErrorCode(errWrapped) == 255 {

// handle 255
}
// handle other stuff
}

Questions:

In your original example, you made the interface itself non-exported,
and had the helper method be exported. Should the ErrorCoder interface
be exported, and why or why not?
2.

Would you have the helper method func ErrorCode(err error) int also
return an error if the error argument did not implement ErrorCoder
interface? Your original example with temporary just returned false if
the error did not implement the interface, but there was no way to tell
with the helper method the difference between an error that didn't
implement temporary, and an error that was tempoary=false.
3.

Would you recommend this or something similar to people who need to
handle errors' behavior when that behavior can get more complex than your
temporary interface example? (In my particular example, a vendor's api
returns a lot of different error states and reasons, which need to be
handled in different ways.)

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#73 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAcA0poqCP1Kvyo4FSjT9ts1ofNUe3wks5qaOm6gaJpZM4JRVpH
.

[veqryn]

Re:

1&2: I'd never considered that, I suppose because I see so many interfaces defined close to where they are implemented. I'm not sure yet what I think about making all client libraries implement their own helpers over and over again, possibly with mistakes, when the most typical use case can be implemented beside the error.

3: Just trying to figure out what the best way is, among many possible solutions, but also while using this library (which is why the error implements Cause and Format).

thanks, and I'm OK to close this if no further advice/discussion

[davecheney]

1&2: I'd never considered that, I suppose because I see so many interfaces defined close to where they are implemented. I'm not sure yet what I think about making all client libraries implement their own helpers over and over again, possibly with mistakes, when the most typical use case can be implemented beside the error.

Interfaces and helpers don't need to be implemented over and over again, but they also don't need to be in this package. The goal of this package is to have the smallest possible surface area with a hope to having something included in Go 1.8.

3: Just trying to figure out what the best way is, among many possible solutions, but also while using this library (which is why the error implements Cause and Format).

The specific case of overloading the error return to carry http status code has been mentioned many times in conversation. This isn't a useful statement as it's unlikely that the http interface will be changed, but IMO http status codes are not errors (even if some http status codes in the 5xx block indicate errors), so smuggling them into the error path is a kludge.

[veqryn]

I may have caused a misunderstanding, but I wasn't advocating any of the above code be added to this github.com/pkg/errors package.
I was actually just asking about the best way to use this github.com/pkg/errors package, when you want to be able to handle errors in different ways. All of my code above would be in my own package, I was just wondering how best to make use of your package.

[davecheney]

@veqryn i'm sorry, that was my misunderstanding.

I think what you are looking for is something like a type switch

switch err := errors.Cause(err).(type) {
case *MyError:
   // handle err which is of type *MyError
default:
   // unknown type
}

[solher]

I have to admit that my current questioning about error handling is almost exactly similar to @veqryn.

I totally got from your (amazing as always) GopherCon talk the difference between the three categories of error handling but I didn't really get if the use of opaque errors had to be systematic (or at least preferred) or if it was only the solution to a very precise problem.

In other words and using the example of a parsing error, should we do:

// ----------------------------------------------
// In a separate "errs" package for example

type parsingError interface {
    Line() int
    Field() string
}
// Just an example. I guess we could also split this method in three.
func IsParsingError(err error) (int, string, bool) {
    e, ok := err.(parsingError)
    if != ok {
        return 0, "", false
    }
    return e.Line(), e.Field(), true
}

// ----------------------------------------------
// In the main package for example

type errParsing struct {
    err error
    line  int
    field string
}
func (e errParsing) Line() int {
    return e.line
}
func (e errParsing) Field() string {
    return e.field
}
func (e errParsing) Error() string {
    return e.err
}

func Parse() error {
    return errParsing{
        err: errors.New("invalid value: cannot be blank"),
        line: 3,
        field: "firstName",
    }
}

func main() {
    if err := Parse(); err != nil {
        if line, field, ok := errs.IsParsingError(err); ok {
            GeneratePreciseAPIError("PARSING_ERROR", line, field)
            return
        }
        GenerateGenericAPIError(err.Error())
    }
}

Or should we simply use a type switch and rather do:

// ----------------------------------------------
// In a separate "errs" package for example

type ErrParsing struct {
    Err error
    Line  int
    Field string
}

// ----------------------------------------------
// In the main package for example

func Parse() error {
    return errs.ErrParsing{
        err: errors.New("invalid value: cannot be blank"),
        line: 3,
        field: "firstName",
    }
}

func main() {
    if err := Parse(); err != nil {
        switch t := err.(type) {
            case errs.ErrParsing:
                GeneratePreciseAPIError("PARSING_ERROR", t.Line, t.Field)
            default:
                GenerateGenericAPIError(err.Error())
        }
    }
}

Or I may not have understood the talk after all :)

[solher]

Alright it makes sense.
So from what I understand, the best solution to get values from errors is still to use a good old type switch.
Thanks a lot!

[davecheney]

If you want the caller to change its behaviour on what is returned in the error value, yes.

If you can avoid altering your behaviour, using opaque errors, this is better IMO.

On 5 Sep 2016, at 18:25, Fabien Herfray [email protected] wrote:

Alright it makes sense.
So from what I understand, the best solution to get values from errors is still to use a good old type switch.
Thanks a lot!

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.

[solher]

Yeah totally. Basically I'm only using values/type switches in the transport domain when I want to render the error.

[matejb]

Hi, I'm having an interesting requirement in application I'm working on regarding errors and it's related to this topic.

Requirement is to log errors to multiple loggers. Some loggers are meant to be used by end-user, stderr, and should contain error. Other logger are meant to be used by system administrator and should contain same errors with additional data for example args used to start application.

I solved this requirement with proxy package that proxies all of this package interface and adds following functional addition:

// WithData adds hidden data to err that can 
// be accessed explicitly by using Data function 
// of this package.
// If err is nil, WithData returns nil.
func WithData(err error, data string) error {
    if err == nil {
        return nil
    }

    return &withDetail{
        cause: err,
        data:  data,
    }
}

type withDetail struct {
    cause error
    data  string
}

// Error will NOT output hidden data
func (w *withDetail) Error() string {
    return fmt.Sprintf("%s", w.cause.Error())
}
func (w *withDetail) Cause() error { return w.cause }
func (w *withDetail) Data() string { return w.data }

// Format will NOT output hidden data
func (w *withDetail) Format(s fmt.State, verb rune) {
    switch verb {
    case 'v':
        if s.Flag('+') {
            fmt.Fprintf(s, "%+v\n", w.Cause())
            return
        }
        fallthrough
    case 's', 'q':
        io.WriteString(s, w.Error())
    }
}

// Data returns the underlying data of 
// all wrapped errors.
// An error value has a data if it implements 
// the following interface:
//
//     type dater interface {
//            Cause() error
//            Data() string
//     }
//
// If the error does not implement dater, 
// the empty []string will be returned.
// If the error is nil, empty []string will be returned.
func Data(err error) (data []string) {
    type dater interface {
        Cause() error
        Data() string
    }

    for err != nil {
        d, ok := err.(dater)
        if !ok {
            break
        }
        data = append(data, d.Data())
        err = d.Cause()
    }
    return
}

Then custom logger uses errors.Data(err) to acquire hidden error data for system administrators.
If any other part of the application logs or prints error special data will not be outputted.

I like the small and concise usage of your's error package, great job with that :-)
Not sure if my addition is appropriate to be in package but if you like I could add it and make a pull req.

If you have any other other suggestion how to implement my application requirement in a better way pls tell :-)

[ChrisHines]

@matejb

Requirement is to log errors to multiple loggers. Some loggers are meant to be used by end-user, stderr, and should contain error. Other logger are meant to be used by system administrator and should contain same errors with additional data for example args used to start application.

Given this requirement I would be more inclined to associate the administrator data with the logger, rather than with the errors. I am sure that you haven't described all of the details involved, so forgive me if I lack understanding of your requirements. I am curious what kind of "secret" data would you associate with individual errors that isn't "global" in nature?

[matejb]

@ChrisHines data is related to error, eq. value of some local variable in time error occured. It is not so much requirement of secrecy as of usability. End-user does not need that much information about the error but administrator looking through logs later would like to have that extra information. Error is logical carrier of that extra information, any other mechanism that I can think of could be awkwardly to use in code, eq. fill logger directly would mean that logger would have to pair information with error that comes later to be logged it also couples code that produces error with specific logger.

[ChrisHines]

@matejb OK, so you do have data related to the error. I also understand the desire to keep user facing output simple and to the point. This discussion is now beginning to duplicate the discussion in #34.