Would a single ctx.Error be better than ctx.Errors?

[diankong]

Hi, I see you add Errors to ctx, but I'm thinking what gin does may not a good idea.

Basically, when we get an error, we just want to return from route handler. Collecting errors like gin seems doesn't make sense

Golang's team offered a best practice on their blog about handling errors: Errors are values

Their solution is: define a single error object in a top level struct ( which should be in ctx in our cases )

So we can do something like this:

ctx.Error := SomeFunc()
// we don't need to deal with error here, a middleware will handle it automatically
if ctx.Error != nil { return 0, nil}

But with ctx.Errors, I have to write more code:

err := SomeFunc()
if err != nil {
    ctx.Error(err)
    return 0, nil
}

If we handle it with ctx.Errors[0]:

ctx.Errors := make([]error, 1)
ctx.Errors[0] := SomeFunc()

In this way, ctx.HasErrors() will always be true even if ctx.Errors[0] is nil

Following golang team's strategy, with a single ctx.Error, we can even do something like this:

func SomeFunc(param1, param2, err) error {
    //check error first
    if err != nil {return}
    //do something
}

func SomeFunc2(param1, param2, err) error {
    if err != nil {return}
    //do something else
}

ctx.Error := SomeFunc(param1, param2, ctx.Error)
ctx.Error := SomeFunc2(param1, param2, ctx.Error)
ctx.Error := SomeFunc3(param1, param2, ctx.Error)
return 200, ctx.Text("done")
//then, middleware will handle ctx.Error, if there is one.

In this way, we don't need to check ctx.Error at all, since every function has checked that error at beginning, if there is an error, all those functions will just return.

I think golang team's solution is much better than gin's, a single ctx.Error may truely better than collecting all errors.

[ivpusic]

Hey...

Yes, I was thinking about the same thing too. Let me explain why I decided to do it like this, not just because gin is doing it as well.

So my first idea was not to use ctx.Errors or ctx.Error at all. So what I wanted to achieve. Currently your define middleware like this:

app.Use(func(ctx *neo.Ctx, next neo.Next) {
    // middleware implementation
    // call next middleware
    next()
})

I wanted to change this in order that middleware returns error, so next middleware can check for it, and automatically return if there is an error. So something like this:

app.Use(func(ctx *neo.Ctx, next neo.Next) error {
    // middleware implementation
    // call next middleware
    err := next()
    if err != nil { return err; }
})

But there is a problem with this. You cannot be sure that all middlewares are following this convention. If your middleware is called you cannot be sure that there is no error in previous middleware. What this means for you. It means that you maybe started transaction somewhere, and if error happens in any part of the code your want to do rollback. As you can see then we have a problem.

Then I started thinking about ctx.Error. In this case middleware can overwrite error from previous middleware, so we loose potentially useful information what is happening in our app. Also idea of ctx.Error in case of neo is not to be used from your services, DB functions, etc., but just from middlewares and from route handler.

Then my third and current idea was to introduce ctx.Errors, so we can collect all errors happened in middlewares and route handler.

I have to think again about this probably. What do you think about my first idea? Changing middleware signature?

[diankong]

Hi, I agree that we don't need to change middleware signature, just put error into ctx will be fine.

But there is one thing I don't understand. You said:

Then I started thinking about ctx.Error. In this case middleware can overwrite error from previous middleware

If there is an error from previous middleware, why this middleware doesn't return immediately?

It's true that we can't 100% sure if there is any middleware don't follow this rule. But I think if we point it out in document, everyone who writes middleware for neo will follow it. Even they don't, we can just create an issue to their github repositories.