Vapor Abort doesn't work as expected.
JaapWijnen opened this issue ยท 7 comments
I've replaced a part of the routes in an existing project using VaporRouting. I Replaced a single Vapour Controller by removing it from the registration in routes()
and mounted a siteHandler
and swift-parsing router
that only covers the routes from that controller.
Now within a site handler when throwing for example a .badRequest
error for example by using:
throw Abort(.badRequest, reason: "...")
A response is returned with status .notFound
, I'm expecting this to be the route not being found. However my router did reach the end it's just the siteHandler that failed somewhere down the chain. I would expect my response to return this .badRequest
error
Ah I now see this line:
Does this mean partial implementation using VaporRouting is not possible?
This is behavior we added to development-only builds to better surface errors, but perhaps isn't correct for us to do so. You can see the full scope of behavior here:
vapor-routing/Sources/VaporRouting/VaporRouting.swift
Lines 42 to 45 in 36fb225
Do you have a suggestion for surfacing routing errors in development while still allowing the middleware chain to do its thing? Would logging suffice?
I see the routingError does exist one scope higher.
(How do I reference multiple lines? ๐
)
let route: Router.Output
do {
route = try self.router.parse(requestData)
return try await self.respond(request, route).encodeResponse(for: request)
} catch let routingError {
do {
return try await next.respond(to: request)
} catch {
request.logger.info("\(routingError)")
guard request.application.environment == .development
else { throw error }
return Response(status: .notFound, body: .init(string: "Routing \(routingError)"))
}
}
The routingError does exist (Which I'd expect to be caught and return a response containing that error).
The development build throwing is nice for unexpected throws, but the behaviour on non development builds would also be unexpected in my opinion since it returns a .notFound
error.
So even without the error throwing on development builds I don't think it's currently possible to use both the Vapor router and the vapor-routing package?
Maybe we can check for Abort
errors specifically and we can then assume those to be thrown on purpose and therefore propagate those instead of the errors thrown by try await next.respond(to: request)
(And therefore also not return the Response(status: .notFound, body: .init(string: "Routing \(routingError)"))
)
I think it is currently not possible to make failing requests like bad request
or others and this is used in Vapor quite a lot I think. Good thing is you need them less often of course because of all the added type safety ^^ But still something that will be needed for interacting with databases through for example Fluent, like Fluent's Model.find(..)
method.
So even without the error throwing on development builds I don't think it's currently possible to use both the Vapor router and the vapor-routing package?
I don't think I fully follow. In non-development builds, if our router fails, we call next
and then if it fails, we allow its error to be thrown. In the code you quote:
do {
return try await next.respond(to: request)
} catch {
request.logger.info("\(routingError)")
guard request.application.environment == .development
else { throw error /* ๐ `next`'s error is thrown, not `.notFound` */ }
And wouldn't these include the Abort
errors you mention? I also believe folks have already experimented with this library in existing apps where their existing routes play nicely...but it might help for us to have an example to work with.
Maybe we can check for
Abort
errors specifically and we can then assume those to be thrown on purpose and therefore propagate those instead of the errors thrown bytry await next.respond(to: request)
(And therefore also not return theResponse(status: .notFound, body: .init(string: "Routing \(routingError)"))
)I think it is currently not possible to make failing requests like
bad request
or others and this is used in Vapor quite a lot I think. Good thing is you need them less often of course because of all the added type safety ^^ But still something that will be needed for interacting with databases through for example Fluent, like Fluent'sModel.find(..)
method.
We're definitely down to change the behavior here to something that makes more sense! Do you have time to prep a small demo that shows the problem? And/or would you want to PR a potential fix?
Ah let me clarify! The Abort error that's not propagated in my case is coming from my siteHandler function! So from one of the routes implemented using the vapor-routing package. My "old" routes all work nicely.
Here's an example:
somewhere down the chain of siteHandler functions I have this actionHandler:
func actionHandler(request: Request, route: ActionRoute, actionID: UUID) async throws -> AsyncResponseEncodable {
// first we do some upfront work that's shared for all "action" routes which can fail (throw an Abort) depending on:
// - no Action entry exists for the actionID that is provided.
// - no user is authorized on the request
// - the users's organization doesn't match the one the action is linked to
guard let action = try await Action.find(actionID, on: request.db) else {
throw Abort(.notFound, reason: "Could not find entity of type \(Action.self) for parameter \(Action.parameterName):\(actionID)")
}
// this could throw an Abort(.unauthorized)
let user = try request.auth.require(User.self)
let organization = try await action.getOrganization(on: request)
guard user.$organization.id == organization.id else {
throw Abort(
.forbidden,
reason: "You are not allowed to access other organization's resources"
)
}
// here we go further down the chain of siteHandlers or return a response for some of the route cases
switch route {
case let .owners(route):
return try await ownersHandler(request: request, route: route, model: action, keyPath: \.$owners)
case let .update(updateData):
let updatedAction = try await updateData.update(action, on: request)
try await updatedAction.save(on: request.db)
return try updatedAction.apiModel()
case .delete:
try await action.delete(on: request.db)
return HTTPResponseStatus.noContent
case .fetch:
return try action.apiModel()
}
}
The Abort's mentioned in the top half are logged by
But not returned in the final response that is being sent by the server.
So the old do indeed routes work!
But using Abort within a siteHandler function (vapor-routing package) is not propagated and so the response doesn't contain something like for example a badRequest status when thrown from within a siteHandler function.
So when running the above with an unauthorized user for example a .unauthorized error is logged (routeError in the previously mentioned code) But the response returned contains a .notFound error (either from the vapor router because it doesn't exist and is thrown in development mode or the returned Response
at the end of that same mentioned scope)
Does this clarify my issue?
@stephencelis
This fixes my current issues but does prevent users from doing their own custom error handling.
It catches all "Vapor style" errors before defaulting to the routingError throwing with the catch let error as Abort
line and throw those again to have them propagate up the chain of vapor error handling.
do {
route = try self.router.parse(requestData)
return try await self.respond(request, route).encodeResponse(for: request)
} catch let error as Abort {
throw error
} catch let routingError {
do {
return try await next.respond(to: request)
} catch {
request.logger.info("\(routingError)")
guard request.application.environment == .development
else { throw error }
return Response(status: .notFound, body: .init(string: "Routing \(routingError)"))
}
}
Another solution would be to catch the routing error as the RoutingError
enum and thus propagating other errors further down the vapor chain and only catching "Routing" errors from the routing library. (Currently RoutingError
from swift-url-routing` is not public however)
EDIT:
I just tested the last mentioned solution but ParserErrors are also thrown for unmatched routes so this wouldn't work unless we make sure we catch all swift-url-routing
and swift-parsing
thrown types of errors
@JaapWijnen We think decoupling the router.parse
line from the response line hopefully fixes this behavior for you. Can you check out #8 and confirm?