Log source file and line info for errors in ErrorMiddleware when poss…

…ible (#3187)

* Make ErrorMiddleware report source location (file, function, line) if available from the given error (will always be available on Abort errors). Don't hold a lock while doing a JSON encode.
* Improve the correctness of the error messages provided for DecodingErrors. Update tests accordingly.
* Don't use hardcoded strings where not necessary

Sources/Vapor/Error/AbortError.swift

@@ -68,18 +68,17 @@ extension DecodingError: AbortError {
     /// See `AbortError.reason`
     public var reason: String {
         switch self {
-        case .dataCorrupted(let ctx):
-            return "Data corrupted at path '\(ctx.codingPath.dotPath)'\(ctx.debugDescriptionAndUnderlyingError)"
-        case .keyNotFound(let key, let ctx):
-            let path = ctx.codingPath + [key]
-            return "Value required for key at path '\(path.dotPath)'\(ctx.debugDescriptionAndUnderlyingError)"
-        case .typeMismatch(let type, let ctx):
-            return "Value at path '\(ctx.codingPath.dotPath)' was not of type '\(type)'\(ctx.debugDescriptionAndUnderlyingError)"
-        case .valueNotFound(let type, let ctx):
-            return "Value of type '\(type)' was not found at path '\(ctx.codingPath.dotPath)'\(ctx.debugDescriptionAndUnderlyingError)"
-        @unknown default: return "Unknown error."
+        case let .dataCorrupted(ctx):       return "Data corrupted \(self.contextReason(ctx))"
+        case let .keyNotFound(key, ctx):    return "No such key '\(key.stringValue)' \(self.contextReason(ctx))"
+        case let .typeMismatch(type, ctx):  return "Value was not of type '\(type)' \(self.contextReason(ctx))"
+        case let .valueNotFound(type, ctx): return "No value found (expected type '\(type)') \(self.contextReason(ctx))"
+        @unknown default:                   return "Unknown error"
         }
     }
+
+    private func contextReason(_ context: Context) -> String {
+        "at path '\(context.codingPath.dotPath)'\(context.debugDescriptionAndUnderlyingError)"
+    }
 }
 
 private extension DecodingError.Context {
@@ -92,17 +91,13 @@ private extension DecodingError.Context {
         if self.debugDescription.isEmpty {
             return ""
         } else if self.debugDescription.last == "." {
-            return ". \(String(self.debugDescription.dropLast()))"
+            return ". \(self.debugDescription.dropLast())"
         } else {
             return ". \(self.debugDescription)"
         }
     }
 
     private var underlyingErrorDescription: String {
-        if let underlyingError = self.underlyingError {
-            return ". Underlying error: \(underlyingError)"
-        } else {
-            return ""
-        }
+        self.underlyingError.map { ". Underlying error: \(String(describing: $0))" } ?? ""
     }
 }

Sources/Vapor/Middleware/ErrorMiddleware.swift

@@ -20,48 +20,44 @@ public final class ErrorMiddleware: Middleware {
     ///     - environment: The environment to respect when presenting errors.
     public static func `default`(environment: Environment) -> ErrorMiddleware {
         return .init { req, error in
-            // variables to determine
-            let status: HTTPResponseStatus
-            let reason: String
-            let headers: HTTPHeaders
+            let status: HTTPResponseStatus, reason: String, source: ErrorSource
+            var headers: HTTPHeaders
 
-            // inspect the error type
+            // Inspect the error type and extract what data we can.
             switch error {
+            case let debugAbort as (DebuggableError & AbortError):
+                (reason, status, headers, source) = (debugAbort.reason, debugAbort.status, debugAbort.headers, debugAbort.source ?? .capture())
+
             case let abort as AbortError:
-                // this is an abort error, we should use its status, reason, and headers
-                reason = abort.reason
-                status = abort.status
-                headers = abort.headers
+                (reason, status, headers, source) = (abort.reason, abort.status, abort.headers, .capture())
+
+            case let debugErr as DebuggableError:
+                (reason, status, headers, source) = (debugErr.reason, .internalServerError, [:], debugErr.source ?? .capture())
+
             default:
-                // if not release mode, and error is debuggable, provide debug info
-                // otherwise, deliver a generic 500 to avoid exposing any sensitive error info
-                reason = environment.isRelease
-                    ? "Something went wrong."
-                    : String(describing: error)
-                status = .internalServerError
-                headers = [:]
+                // In debug mode, provide the error description; otherwise hide it to avoid sensitive data disclosure.
+                reason = environment.isRelease ? "Something went wrong." : String(describing: error)
+                (status, headers, source) = (.internalServerError, [:], .capture())
             }
 
-            // Report the error to logger.
-            req.logger.report(error: error)
-
-            // create a Response with appropriate status
-            let response = Response(status: status, headers: headers)
+            // Report the error
+            req.logger.report(error: error, file: source.file, function: source.function, line: source.line)
 
             // attempt to serialize the error to json
+            let body: Response.Body
             do {
-                let errorResponse = ErrorResponse(error: true, reason: reason)
-                try response.responseBox.withLockedValue { box in
-                    box.body = try .init(data: JSONEncoder().encode(errorResponse), byteBufferAllocator: req.byteBufferAllocator)
-                    box.headers.replaceOrAdd(name: .contentType, value: "application/json; charset=utf-8")
-                }
+                body = .init(
+                    buffer: try JSONEncoder().encodeAsByteBuffer(ErrorResponse(error: true, reason: reason), allocator: req.byteBufferAllocator),
+                    byteBufferAllocator: req.byteBufferAllocator
+                )
+                headers.contentType = .json
             } catch {
-                response.responseBox.withLockedValue { box in
-                    box.body = .init(string: "Oops: \(error)", byteBufferAllocator: req.byteBufferAllocator)
-                    box.headers.replaceOrAdd(name: .contentType, value: "text/plain; charset=utf-8")
-                }
+                body = .init(string: "Oops: \(String(describing: error))\nWhile encoding error: \(reason)", byteBufferAllocator: req.byteBufferAllocator)
+                headers.contentType = .plainText
             }
-            return response
+
+            // create a Response with appropriate status
+            return Response(status: status, headers: headers, body: body)
         }
     }
 
@@ -77,8 +73,8 @@ public final class ErrorMiddleware: Middleware {
     }
 
     public func respond(to request: Request, chainingTo next: Responder) -> EventLoopFuture<Response> {
-        return next.respond(to: request).flatMapErrorThrowing { error in
-            return self.closure(request, error)
+        next.respond(to: request).flatMapErrorThrowing { error in
+            self.closure(request, error)
         }
     }
 }

Tests/VaporTests/ContentTests.swift

@@ -80,7 +80,7 @@ final class ContentTests: XCTestCase {
 
         try app.testable().test(.GET, "/decode_error") { res in
             XCTAssertEqual(res.status, .badRequest)
-            XCTAssertContains(res.body.string, #"Value at path 'bar' was not of type 'Int'. Expected to decode Int but found a string"#)
+            XCTAssertContains(res.body.string, #"Value was not of type 'Int' at path 'bar'. Expected to decode Int but found a string"#)
         }
     }
 
@@ -564,7 +564,7 @@ final class ContentTests: XCTestCase {
         XCTAssertThrowsError(try req.content.decode(PostInput.self)) { error in
             XCTAssertEqual(
                 (error as? AbortError)?.reason,
-                #"Value required for key at path 'is_free'. No value associated with key CodingKeys(stringValue: "is_free", intValue: nil) ("is_free")."#
+                #"No such key 'is_free' at path ''. No value associated with key CodingKeys(stringValue: "is_free", intValue: nil) ("is_free")."#
             )
         }
     }
@@ -617,7 +617,7 @@ final class ContentTests: XCTestCase {
         XCTAssertThrowsError(try req.content.decode(DecodeModel.self)) { error in
             XCTAssertEqual(
                 (error as? AbortError)?.reason,
-                #"Value of type 'String' was not found at path 'items.Index 1'. Unkeyed container is at end."#
+                #"No value found (expected type 'String') at path 'items.Index 1'. Unkeyed container is at end."#
             )
         }
     }
@@ -642,7 +642,7 @@ final class ContentTests: XCTestCase {
         XCTAssertThrowsError(try req.content.decode(DecodeModel.self)) { error in
             XCTAssertContains(
                 (error as? AbortError)?.reason,
-                #"Value at path 'item.title' was not of type 'Int'. Expected to decode Int but found a string"#
+                #"Value was not of type 'Int' at path 'item.title'. Expected to decode Int but found a string"#
             )
         }
     }