-
Notifications
You must be signed in to change notification settings - Fork 43
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
Would a single ctx.Error be better than ctx.Errors? #34
Comments
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.
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:
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? |
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:
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. |
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:
But with ctx.Errors, I have to write more code:
If we handle it with ctx.Errors[0]:
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:
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.
The text was updated successfully, but these errors were encountered: