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

Add ability to customize HTTP response error handling #75

Closed
donut opened this issue Apr 5, 2016 · 9 comments
Closed

Add ability to customize HTTP response error handling #75

donut opened this issue Apr 5, 2016 · 9 comments

Comments

@donut
Copy link

donut commented Apr 5, 2016

For example, HTTPFile.FileResponder simply return a 404 response on any file errors, regardless of whether its permission or the file is just a directory.

I opened a pull request regarding this with a very in-elegant solution.

It looks like Zewo/RecoveryMiddleware gets us halfway there.

@Danappelxx
Copy link
Member

I think RecoveryMiddleware would be fine if we made all of our modules throw errors instead of simply returning a response with a bad status.

However, considering how vital error handling is, I think that RecoveryMiddleware should add an extension to RouterBuilder so that one can call the method route.error as I proposed in this comment.

@paulofaria
Copy link
Member

yeah! that's the second part of the solution.
provide a complete list of possible HTTP errors. This should be in HTTP I think.

protocol Error: ErrorProtocol {
    var status: S4.Status { get }
}

enum ClientError: Error {
    case badRequest
    case unauthorized
    case paymentRequired
    case forbidden
    ...

    var status: S4.Status {
        switch self {
        case badRequest: return .badRequest
        case unauthorized: return .unauthorized
        ...
       }
    }
}

enum ServerError: Error {
    case internalServerError
    case notImplemented
    ...
}

Then we throw these errors in our modules instead of returning a response, like @Danappelxx said. And as a safety measure. If a user doesn't catch these in a RecoveryMiddleware and the error gets thrown all the way to the server. We catch errors of type HTTP.Error there and convert them to a regular Response(status: error.status).

@paulofaria
Copy link
Member

Actually, I'd like HTTPServer to not depend on HTTP so maybe a module called HTTPError would be better.

@paulofaria
Copy link
Member

Or we can propose to move this to S4.

@donut
Copy link
Author

donut commented Apr 5, 2016

This all sounds good to me. As @Danappelxx said, RecoverMiddleware with his route.error{} suggestion sounds great. Will this limit the errors that can be returned by modules like HTTPFile? Does it make sense that it would only return errors that fit into HTTP status codes? Should there be flexibility to give more information? I'm saying this, but I'm having trouble coming up with examples of when that might be necessary.

@paulofaria
Copy link
Member

I think we should strive to only return HTTP errors when we're in that scope. maybe add some more information to HTTP. Error or S4.Error. So we can create a nice looking HTML page for the error. But worst case scenario we catch an anonymous error and convert it to internalServerError.

@tanner0101
Copy link

If you want to make some S4 compatible Middleware or Router, etc. You'd have to throw a common error or else something like Error.badRequest would end up going to a catch all in the receiving application and probably cause a 500.

@paulofaria
Copy link
Member

ok I'll create a PR with this in S4. we can continue discussing what properties S4.Error should have so we can create nicer HTML error pages.

@paulofaria
Copy link
Member

This is possible now. We should inspect where we can throw the HTTP errors and add a default catcher to our HTTP servers.

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

4 participants