Vapor Abort doesn't work as expected.

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:

vapor-routing/Sources/VaporRouting/VaporRouting.swift

Line 45 in 36fb225

    
           return Response(status: .notFound, body: .init(string: "Routing \(routingError)"))

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

    
           guard request.application.environment == .development 
        
           else { throw error } 
        
           return Response(status: .notFound, body: .init(string: "Routing \(routingError)"))

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 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.

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

vapor-routing/Sources/VaporRouting/VaporRouting.swift

Line 40 in 36fb225

request.logger.info("\(routingError)")

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?

	guard request.application.environment == .development
	else { throw error }

	return Response(status: .notFound, body: .init(string: "Routing \(routingError)"))