Question: Handling errors

[elliotlings]

I'm in the process of writing a REST API which returns JSON. I've found my route handlers can get quite cluttered with error handling. I've been looking at ways I can reduce and simply error handling and would appreciate anyone's opinions or advice on what I've come up with.

I'm following the JSON API standard for error returning, http://jsonapi.org/format/#errors, they contain a bit more than the standard Error in Go, so when I initiate an error it can get quite big.

I have created an APIError struct, some helper functions and some default errors:

type APIErrors struct {
    Errors      []*APIError `json:"errors"`
}

func (errors *APIErrors) Status() int {
    return errors.Errors[0].Status
}

type APIError struct {
    Status      int         `json:"status"`
    Code        string      `json:"code"`
    Title       string      `json:"title"`
    Details     string      `json:"details"`
    Href        string      `json:"href"`
}

func newAPIError(status int, code string, title string, details string, href string) *APIError {
    return &APIError{
        Status:     status,
        Code:       code,
        Title:      title,
        Details:    details,
        Href:       href,
    }
}

var (
    errDatabase         = newAPIError(500, "database_error", "Database Error", "An unknown error occurred.", "")
    errInvalidSet       = newAPIError(404, "invalid_set", "Invalid Set", "The set you requested does not exist.", "")
    errInvalidGroup     = newAPIError(404, "invalid_group", "Invalid Group", "The group you requested does not exist.", "")
)

All of my other funcs in my app return their err but when it get backs to my handler I need to handle how I return it to the client. As far as I'm aware I can't return to middleware so I've added my own Recovery middleware which handles APIError that I panic from my handlers, i.e.

func Recovery() gin.HandlerFunc {
    return func(c *gin.Context) {
        defer func() {
            if err := recover(); err != nil {
                switch err.(type) {
                    case *APIError:
                        apiError := err.(*APIError)
                        apiErrors := &APIErrors{
                            Errors: []*APIError{apiError},
                        }
                        c.JSON(apiError.Status, apiErrors)
                    case *APIErrors:
                        apiErrors := err.(*APIErrors)
                        c.JSON(apiErrors.Status(), apiErrors)
                }
                panic(err)
            }
        }()

        c.Next()
    }
}

Then in my handler I can do something like this:

    // Grab the set from storage
    set, err := storage.GetSet(set_uuid)

    if (err == database.RecordNotFound) {
        panic(errInvalidSet)
    } else {
        panic(errDatabase)
    }

which, in my opinion, keeps things really tidy and reduces repetition. I've seen that people recommend you try to use panic as little as possible, but I'm not sure if it works well in this situation? I wonder what people think of this, whether it's a good way to go about it and, if my method isn't great, whether there's any recommended alternatives?

[techjanitor]

I have something kind of similar. You can handle errors in your own middleware though, after Next() happens. I wouldn't personally use panic like that.

error types to feed into c.JSON:
https://github.com/techjanitor/pram-get/blob/master/errors/errors.go

my typical error handlers in a controller:
https://github.com/techjanitor/pram-get/blob/master/controllers/index.go#L25-L36
i pass any errors to some middleware, which you can see here:
https://github.com/techjanitor/pram-get/blob/master/middleware/cache.go#L35-L42

[elliotlings]

Thanks for you reply. The way you handle your errors looks better than panicing, also I found, that if I panic I loose the reference to the Error which isn't helpful.

I have created an ErrorMessage like yourself:

func ErrorMessage(err error, error interface{}) (int, *APIErrors) {
    var apiErrors *APIErrors

    // This the best way to log?
    trace := make([]byte, 1024)
    runtime.Stack(trace, true)
    log.Printf("ERROR: %s\n%s", err, trace)

    switch error.(type) {
        case *APIError:
            apiError := error.(*APIError)
            apiErrors = &APIErrors{
                Errors: []*APIError{apiError},
            }
        case *APIErrors:
            apiErrors = error.(*APIErrors)
        default:
            apiErrors = &APIErrors{
                Errors: []*APIError{errUnknown},
            }
    }
    return apiErrors.Status(), apiErrors
}

and then call it like such:

    // Grab the set from storage
    set, err := storage.GetSet(set_uuid)

    if (err == database.RecordNotFound) {
        c.JSON(ErrorMessage(err, errInvalidSet))
        return
    }
    if (err != nil) {
        c.JSON(ErrorMessage(err, errDatabase))
        return
    }

This seems much better. Do you think this looks good? Also, what do you think about the way I am logging the error? I want to keep the logging outside of the handler for these kind of errors (to reduce repetition), so I thought logging it when I create ErrorMessage may be the best place, also have the stack trace is helpful for debugging.

[nazwa]

I use a similar, but I suppose more basic approach.

For all user-space errors I simply tell the user what's wrong and forget about it.

    if !c.Bind(&details) {
        c.JSON(http.StatusBadRequest, gin.H{"Error": FormDataError.Error()})
        return
    }

If there is an unexpected situation or server error, I also add it to context:

    if err != nil {
        c.Error(err, err.Error())
        c.JSON(http.StatusInternalServerError, gin.H{"Error": BraintreeError.Error()})
        return
    }

Which is then picked up by my error handling middleware:

func Rollbar() gin.HandlerFunc {
    return func(c *gin.Context) {
        c.Next()

        for _, e := range c.Errors {
            rollbar.RequestError(rollbar.ERR, c.Request, errors.New(e.Err))
        }

    }
}

It does need a bit of repetition but for small projects it's more than enough :)

[elliotlings]

That looks nice, I was unaware c.Errors worked like that.

I am now doing this:

    // Grab the set from storage
    set, err := storage.GetSet(set_uuid)

    if (err == database.RecordNotFound) {
        c.Error(err, err.Error())
        c.JSON(ErrorMessage(errInvalidSet))
        return
    }
    if (err != nil) {
        c.Error(err, err.Error())
        c.JSON(ErrorMessage(errDatabase))
        return
    }

A bit of repetition but feels like I'm handling the errors better.

[nazwa]

Yea, this is pretty much the same concept. I was thinking about wrapping errors into a struct rather that gin.H{"Error": "xxx"} style thing, but I couldn't find a simple way to fit all use cases. I might looking into replicating what you've done to actually provide more detailed errors.

My main issue is how to condense this:

    if (err != nil) {
        c.Error(err, err.Error())
        c.JSON(ErrorMessage(errDatabase))
        return
    }

Into a single line, but I just cant think of a way to set all the variables in one call. Ideal scenario would be something like this:

if( err != nil) {
   return setErrors(c, err, errDatabase)
}


with 
func setErrors(c *gin.Context, internal, public error)  bool {
    if( internal != nil) {
       c.Error(internal, internal.error())
    }
    c.JSON(ErrorMessage(public))
    return true
}

but gin handlers don't accept returns, so it's impossible as far as i know to return a result of subfunction in a handler :( So we would still need 1 line to set errors + one line for return, which sucks ballz.

[elliotlings]

Yeh that is the same issue I have now. You could do something like this:

if (err != nil) {
    setErrors(c, err, errDatabase)
    return
}

but it feels a bit wrong passing the context to that function. Ideally, like you say, it would be nicer if you could return from a handler.

[elliotlings]

It may be helpful if there was an additional JSONError func on the context, e.g.

func (c *Context) JSONError(err error, code int, obj interface{}) {
    if (err != nil) {
       c.Error(err, err.Error())
    }
    c.Render(code, render.JSON, obj)
}

that would allow us to do something like

c.JSONError(ErrorMessage(err, errDatabase))

[manucorporat]

@elliotlings in my opinion, Gin provides a even better solution!

basically c.Error() helps developers to collect errors! it does not abort the request.
it does not print anything either, it just collect errors! then you can do what ever you want.

then, we have c.Fail() just like c.Error(error) but c.Fail(code, error) stops the request. c.Error() can be called many times in the same request. c.Fail() should be called just once.

I handle errors like this:

func main() {
    router := gin.Default()
    router.Use(errorHandler)
    router.GET("/", func(c *gin.Context) {
        c.Fail(500, errors.New("something failed!"))
    })

    router.Run(":8080")
}

func errorHandler(c *gin.Context) {
    c.Next()

    if len(c.Errors) {
        c.JSON(-1, c.Errors) // -1 == not override the current error code
    }
}

[elliotlings]

Thanks @manucorporat, that looks really neat. My only issue is that because I am defining errors as variables, e.g.

var (
    errDatabase         = newAPIError(500, "database_error", "Database Error", "An unknown error occurred.", "")
    errInvalidSet       = newAPIError(404, "invalid_set", "Invalid Set", "The set you requested does not exist.", "")
    errInvalidGroup     = newAPIError(404, "invalid_group", "Invalid Group", "The group you requested does not exist.", "")
)

I have no way of logging the actual error through use of c.Fail.

I could, however, always do something like such:

c.Error(err, err.Error())
c.Fail(errDatabase.Status, errDatabase)

(What would be ideal is a method where I could do something like c.Abort(error, publicError))

[manucorporat]

yes, in fact I will change the API. c.Error will accept an error, not a string any more.

[nguyenthenguyen]

And some response like that?

{
"errors":[{
    "id": "internal_server_error",
    "status": 500,
    "title": "Internal Server Error",
    "detail": "Something went wrong."
}]
}

or

{
    "id": "internal_server_error",
    "status": 500,
    "title": "Internal Server Error",
    "detail": "Something went wrong."
}

[manucorporat]

I will give you more details soon, I am working to improve the error management.

Btw, I posted an issue in Go. golang/go#10748

[nazwa]

It's definitely a step in the right direction. It's a shame go doesn't allow 'return void' style calls, so even with the above, we are back to square one with 3 lines of code for every error in a handler :D (error + fail + return)

[elliotlings]

Yeh, ideally I need a way to pass the error for debugging, but also a public error to pass to the user (e.g. there may be a database error that I need to know about so I can fix it, but at the same time I don't want to expose that to the user).

I don't think it should dictate the response format so should just accept an interface like the others. Like I say, something like this would be ideal:

func (c *Context) ErrorAndAbort(err error, publicError interface{}) {
    c.Error(err, "Operation aborted")
    c.PublicError = publicError
    c.Abort()
}

Then in my application this would allow something like:

func errorHandler(c *gin.Context) {
    c.Next()

    if c.PublicError != nil {
        errorMessage := ErrorMessage(c.PublicError)
        c.JSON(errorMessage.Status(), errorMessage)
    }
}

which can be triggered with:

c.ErrorAndAbort(err, errDatabase)

That way the (type) error gets logged and I can return a nice message to the user, which I can decide the format of.

Just a suggestion, not sure what you think?

Also, thanks for looking into improving the error handling! :)

[nazwa]

I'm with @elliotlings on this. Even to the point where ErrorAndAbort would accept 3 parameters - status code, public error and private error.

None of it can be automated too much, as it needs to be up to the user (via middleware) to decide how is that public error then displayed, but a simple separation of internal and public would be super handy.

[nguyenthenguyen]

And How about a better Recovery()?

type Errors struct {
    Errors []*Error `json:"errors"`
}

func (e *Errors) StatusCode() int {
    if len(e.Errors) > 0 {
        return e.Errors[0].Status
    }
    return 500
}

func (e *Errors) HasError() bool {
    return len(e.Errors) > 0
}

type Error struct {
    Status int    `json:"status"`
    Title  string `json:"title"`
    Detail string `json:"detail"`
}
func (e *Error) IsNil() bool {
    return e.Status == 0 && e.Detail == "" && e.Title == ""
}
var (
    ErrInternalServer = &Error{500, "Internal Server Error", "Something went wrong."}
    ErrBadRequest     = &Error{400, "Bad Request", "The request had bad syntax or was inherently impossible to be satisfied"}
    ErrUnauthorized   = &Error{401, "Unauthorized", "Login required"}
    ErrForbidden      = &Error{403, "Forbidden", "Access deny"}
    ErrNotFound       = &Error{404, "Not Found", "Not found anything matching the URI given"}
)

func Recovery() gin.HandlerFunc {
    return func(c *gin.Context) {
        defer func() {
            if err := recover(); err != nil {
                log.Printf("panic: %+v", err)
                errors := &Errors{Errors: []*Error{ErrInternalServer}}
                c.JSON(errors.StatusCode(), errors)
            }
        }()

        c.Next()
    }
}

And helper function

func SetErrors(c *gin.Context) {
    c.Set("errors", &Errors{})
}

func AddError(c *gin.Context, e Error) {
    errors := c.MustGet("errors").(*Errors)
    errors.Errors = append(errors.Errors, e)
}
func GetErrors(c *gin.Context) *Errors {
    return c.MustGet("errors").(*Errors)
}

Use in gin Context:

if !err.IsNil() {
        h.AddError(c, err)
        return
    }

My errorHander():

func ErrorHandler() gin.HandlerFunc {
    return func(c *gin.Context) {
        h.SetErrors(c)
        c.Next()
        errors := h.GetErrors(c)
        if errors.HasError() {
            c.JSON(errors.StatusCode(), errors)
        }
    }
}

That seem not good idea?

[manucorporat]

@nguyenthenguyen @nazwa @elliotlings @techjanitor ok! I just finished a new API for errors.

c.Abort() // aborts the handlers chain (no error reported)
c.AbortWithStatus(code) // like Abort() but also writes the headers with the specified status code (no error reported)
c.Error(error) // it appends an error in c.Errors
c.AbortWithError(code, error) // (old c.Fail(): c.AbortWithStatus(code)  + c.Error(error)

but there are much more improvements under the hoods!
c.Error(error) as been simplified, now it just takes a error, but it is much more powerful:

c.Error(err) // simple error
c.Error(err).Type(gin.ErrorTypePublic)
c.Error(err).Type(gin.ErrorTypePrivate).Meta("more data")

by default c.Error() will store the error as gin.ErrorTypePrivate

[manucorporat]

Then how to handle the error? easy. you can create a middleware! (Gin will provide a default one)

func main() {
    router := gin.Default()
    router.Use(handleErrors)
    // [...]
    router.Run(":8080")
}

func handleErrors(c gin.Context) {
    c.Next() // execute all the handlers

    // at this point, all the handlers finished. Let's read the errors!
    // in this example we only will use the **last error typed as public**
    // but you could iterate over all them since c.Errors is a slice!
    errorToPrint := c.Errors.ByType(gin.ErrorTypePublic).Last()
    if errorToPrint != nil {
        c.JSON(500, gin.H {
            "status": 500,
            "message": errorToPrint.Error(),
        })
    }
}

[manucorporat]

Or you guys could use the Meta as the public error.

c.Error(err).Meta(gin.H{
   "status": "this is bad",
   "error": "something really bad happened",
})

// [...]

func handleErrors(c gin.Context) {
    c.Next() // execute all the handlers

    // at this point, all the handlers finished. Let's read the errors!
    // in this example we only will use the **last error typed as public**
    // but you could iterate over all them since c.Errors is a slice!
    errorToPrint := c.Errors.ByType(gin.ErrorTypePublic).Last()
    if errorToPrint != nil && errorToPrint.Meta != nil {
        c.JSON(500, errorToPrint.Meta)
    }
}

[elliotlings]

Nice! Thanks for these new methods.

With c.Error will that stop execution or will I need to return void after it?

[manucorporat]

@elliotlings c.Error() does not stop execution, it just append an error to c.Errors.
you can call c.Error as many times as you want.

Also, retuning void! does not stop the execution, calling return only makes your program jump to the end of the current handler.

If you want stop execution (abort). You should explicitly call Abort(), AbortWithStatus(), or AbortWithError().

Abort() = it only stops the execution
AbortWithStatus(code) = Abort() + c.Writer.WriteHeader(code)
AbortWithError(code, error) = AbortWithStatus(code) + Error(error)

If you want to continue the handlers chain. You do not need to call c.Next().
c.Next() is only used when you need to execute the next handlers in chain inside the current handler's scope.

For example, for a latency logger:

func latency(c *gin.Context) {
    after := time.Now()

    c.Next() // the next handlers are execute here

    // at this point all the handlers finished
    now := time.Now()
    latency := now.Sub(after)
    fmt.Println(latency)
}

[elliotlings]

Would this help me simplify this at all?

    if (err != nil) {
        c.Error(err, err.Error())
        c.JSON(ErrorMessage(errDatabase))
        return
    }

err is the internal error which I will log and errDatabase is a public error. ErrorMessage takes an interface and just formats it ready for r.JSON call.

func ErrorMessage(error interface{}) (int, *APIErrors) {
    var apiErrors *APIErrors

    switch error.(type) {
        case *APIError:
            apiError := error.(*APIError)
            apiErrors = &APIErrors{
                Errors: []*APIError{apiError},
            }
        case *APIErrors:
            apiErrors = error.(*APIErrors)
        default:
            apiErrors = &APIErrors{
                Errors: []*APIError{errUnknown},
            }
    }
    return apiErrors.Status(), apiErrors
}

[elliotlings]

It would be cool if there were 2 more additions, maybe:

func (c *Context) ErrorAndAbort(private error, public error) {
    c.Error(private).Type(ErrorTypePrivate)
    c.Error(public).Type(ErrorTypePublic)
    c.Abort()
}

func (c *Context) ErrorAndAbortWithStatus(code int, private error, public error) {
    c.Error(private).Type(ErrorTypePrivate)
    c.Error(public).Type(ErrorTypePublic)
    c.AbortWithStatus(code)
}

Which would allow me to do something like this?

if (err != nil) {
    c.ErrorAndAbort(err, ErrorMessage(errDatabase))
}

[manucorporat]

@elliotlings I have the responsibility to create something that matches many people use cases.
Sometimes less is more, if I add a shortcut for each particular case, the framework would be massive.

Maybe you should rethink your error management.

middleware {
   check error
   if error.type == public {
        render error.code, error

   } error.type == private {
        // https://www.getsentry.com/welcome/
        submit error to sentry
        render 500, message="error ref: "+sentry.id
   }
}

[manucorporat]

@elliotlings

    if (err != nil) {
        c.Error(err, err.Error())
        c.JSON(ErrorMessage(errDatabase))
        return
    }

looks good to me.

c.Error() is a built helper, it is really useful when several things can fail. Remember it is just an array of errors. If you do not need it do not use it.

In my case, several things can fail, so we use c.Error(), then we have a errorMiddleware.
The error middleware takes all the private errors and submit them to sentry + a log.

If there are public errors, we print them. if we only have private errors, we print ref codes:

    if !isPrivate || r.DebugMode {
        return errors.New(report.Message)
    } else {
        // If the debug mode is disabled and the report is private
        // we return error message instead the original one.
        // this is very important since there are internal errors that should not be public because of *security reasons*.
        return errors.New("internal error. Ref " + report.Id)
    }

In my case, the error logic is completely separated:

func User(c *gin.Context) {
    var req models.UserJSON
    if c.Bind(&req) {
        result, err := MODELS.C(c).UsersByIds(req.Users)
        if err == nil {
            c.JSON(200, gin.H{"users": result})
        }
    }
}

[christianhellsten]

I have a struct with validations:

type Transaction struct {
...
    Name        string    `json:"name" binding:"required,max=255"`
...
}

I see this in the log if the Name field fails validation:

[GIN] 2015/06/06 - 11:18:15 | 400 |    1.975987ms | [::1]:61662 |   POST    /transactions
Error #01: Struct:Transaction
Field validation for "Name" failed on the "required" tag

How can I silence these validation errors? It doesn't seem possible at the moment, and I don't want the log to be filled with validation errors.

[manucorporat]

@christianhellsten you are right, validation errors should not be in the logs at all.
let me fix it.

[manucorporat]

@christianhellsten Done. dde06a0

[christianhellsten]

Thank you. That silenced the errors.

[joeyave]

I've got a code where I want to skip error handler with c.Abort but it gets called anyway. Why?

func main() {
    router := gin.Default()
    router.Use(handleErrors)
    router.Post("/test", handler)
    router.Run(":8080")
}

func handler(c *gin.Context) {
    c.Error(errors.New("testerr"))
     // I wan't to skip handleErrors here.
    c.Abort()
    return
}

func handleErrors(c gin.Context) {
    c.Next()

    // This shouldn't be called because I use c.Abort in handler above.
    errorToPrint := c.Errors.ByType(gin.ErrorTypePublic).Last()
    if errorToPrint != nil {
        c.JSON(500, gin.H {
            "status": 500,
            "message": errorToPrint.Error(),
        })
    }
}

[cycloss]

@joeyave I believe you have to call c.IsAborted() after calling c.Next() to check if any of the handlers have called abort on the context.
Edit: Just realised that your comment is over a year old. Didn't read the year.