From a47a395d0a958ca59eb2c65fe4993a8fe261cb08 Mon Sep 17 00:00:00 2001 From: Siemen Sikkema Date: Wed, 31 Jul 2019 15:18:48 +0200 Subject: [PATCH 1/4] Add ReportableError protocol --- Sources/Bugsnag/BugsnagMiddleware.swift | 27 ++++++++++++++++++++----- Sources/Bugsnag/ReportableError.swift | 22 ++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) create mode 100644 Sources/Bugsnag/ReportableError.swift diff --git a/Sources/Bugsnag/BugsnagMiddleware.swift b/Sources/Bugsnag/BugsnagMiddleware.swift index d7321b8..c70f85a 100644 --- a/Sources/Bugsnag/BugsnagMiddleware.swift +++ b/Sources/Bugsnag/BugsnagMiddleware.swift @@ -7,13 +7,30 @@ public struct BugsnagMiddleware { extension BugsnagMiddleware: Middleware { public func respond(to req: Request, chainingTo next: Responder) throws -> Future { - return Future.flatMap(on: req) { + return Future + .flatMap(on: req) { try next.respond(to: req) - }.catchFlatMap { error in - self.reporter - .report(error, on: req) - .map { throw error } } + .catchFlatMap { error in + self.handleError(error, on: req).map { throw error } + } + } + + private func handleError(_ error: Error, on container: Container) -> Future { + if let reportableError = error as? ReportableError { + guard reportableError.shouldReport else { + return container.future() + } + return self.reporter.report( + reportableError, + severity: reportableError.severity, + userId: reportableError.userId, + metadata: reportableError.metadata, + on: container + ) + } else { + return self.reporter.report(error, on: container) + } } } diff --git a/Sources/Bugsnag/ReportableError.swift b/Sources/Bugsnag/ReportableError.swift new file mode 100644 index 0000000..fcdaab1 --- /dev/null +++ b/Sources/Bugsnag/ReportableError.swift @@ -0,0 +1,22 @@ +/// Errors conforming to this protocol have more control about how (or if) they will be reported. +public protocol ReportableError: Error { + + /// Whether to report this error (defaults to `true`) + var shouldReport: Bool { get } + + /// Error severity (defaults to `.error`) + var severity: Severity { get } + + /// The associated user id (if any) for the error (defaults to `nil`) + var userId: CustomStringConvertible? { get } + + /// Any additional metadata (defaults to `[:]`) + var metadata: [String: CustomDebugStringConvertible] { get } +} + +public extension ReportableError { + var shouldReport: Bool { return true } + var severity: Severity { return .error } + var userId: CustomStringConvertible? { return nil } + var metadata: [String: CustomDebugStringConvertible] { return [:] } +} From 346f692c35b34e4c0d525860e0f62fd6b079827e Mon Sep 17 00:00:00 2001 From: Siemen Sikkema Date: Wed, 31 Jul 2019 15:19:21 +0200 Subject: [PATCH 2/4] Loosen restriction on getting reason phrase --- Sources/Bugsnag/Bugsnag.swift | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Sources/Bugsnag/Bugsnag.swift b/Sources/Bugsnag/Bugsnag.swift index b5cc5e1..c0db21a 100644 --- a/Sources/Bugsnag/Bugsnag.swift +++ b/Sources/Bugsnag/Bugsnag.swift @@ -87,11 +87,10 @@ struct BugsnagException: Encodable { let type: String init(error: Error, stacktrace: BugsnagStacktrace) { - let abort = error as? AbortError self.errorClass = error.localizedDescription - self.message = abort?.reason ?? "Something went wrong" + self.message = (error as? Debuggable)?.reason ?? "Something went wrong" self.stacktrace = [stacktrace] - self.type = (abort?.status ?? .internalServerError).reasonPhrase + self.type = ((error as? AbortError)?.status ?? .internalServerError).reasonPhrase } } From c1512ec38e22c8e571ac687a105e808a2fcc0902 Mon Sep 17 00:00:00 2001 From: Siemen Sikkema Date: Wed, 31 Jul 2019 15:33:16 +0200 Subject: [PATCH 3/4] Add explanation in README --- README.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 174ca46..b092691 100644 --- a/README.md +++ b/README.md @@ -52,7 +52,7 @@ public func configure( ``` ### Reporting -Bugsnag offers three different types of reports: info, warning and error. To make a report just instantiate a `ErrorReporter` and use the respective functions. +Bugsnag offers three different types of reports: info, warning and error. To make a report just instantiate an `ErrorReporter` and use the respective functions. ##### Examples ```swift @@ -72,6 +72,15 @@ reporter.report( ) ``` +By conforming to the `ReportableError` protocol you can have full control over how (and if) the BugsnagMiddleware reports your errors. It has the following properties: + +| Name | Type | Function | Default | +|---|---|---|---| +| `shouldReport` | `Bool` | Opt out of error reporting by returning `false` | `true` | +| `severity` | `Severity` | Indicate error severity (`.info`\|`.warning`\|`.error`) | `.error` | +| `userId` | `CustomStringConvertible?` | An optional user id associated with the error | `nil` | +| `metadata` | `[String: CustomDebugStringConvertible]` | Additional metadata to include in the report | `[:]` | + #### Users Conforming your `Authenticatable` model to `BugsnagReportableUser` allows you to easily pair the data to a report. The protocol requires your model to have an `id` field that is `CustomStringConvertible`. From 28f8d2fa0721a9baaf42cc02811dfef4f37d8564 Mon Sep 17 00:00:00 2001 From: Siemen Sikkema Date: Wed, 31 Jul 2019 16:18:10 +0200 Subject: [PATCH 4/4] Add tests --- Tests/BugsnagTests/BugsnagTests.swift | 106 +--------------- Tests/BugsnagTests/MiddlewareTests.swift | 154 +++++++++++++++++++++++ Tests/BugsnagTests/XCTestManifests.swift | 24 +++- 3 files changed, 177 insertions(+), 107 deletions(-) create mode 100644 Tests/BugsnagTests/MiddlewareTests.swift diff --git a/Tests/BugsnagTests/BugsnagTests.swift b/Tests/BugsnagTests/BugsnagTests.swift index 1ce34ba..1b1a891 100644 --- a/Tests/BugsnagTests/BugsnagTests.swift +++ b/Tests/BugsnagTests/BugsnagTests.swift @@ -1,108 +1,8 @@ -import Vapor +import Bugsnag import XCTest -@testable import Bugsnag - -extension Application { - public static func test() throws -> Application { - var services = Services() - try services.register(BugsnagProvider(config: BugsnagConfig( - apiKey: "e9792272fae71a3b869a1152008f7f0f", - releaseStage: "development" - ))) - - var middlewaresConfig = MiddlewareConfig() - middlewaresConfig.use(BugsnagMiddleware.self) - services.register(middlewaresConfig) - - let sharedThreadPool = BlockingIOThreadPool(numberOfThreads: 2) - sharedThreadPool.start() - services.register(sharedThreadPool) - - return try Application(config: Config(), environment: .testing, services: services) - } -} - -private class TestErrorReporter: ErrorReporter { - - var capturedReportParameters: ( - error: Error, - severity: Severity, - userId: CustomStringConvertible?, - metadata: [String: CustomDebugStringConvertible], - file: String, - function: String, - line: Int, - column: Int, - container: Container - )? - func report( - _ error: Error, - severity: Severity, - userId: CustomStringConvertible?, - metadata: [String: CustomDebugStringConvertible], - file: String, - function: String, - line: Int, - column: Int, - on container: Container - ) -> Future { - capturedReportParameters = ( - error, - severity, - userId, - metadata, - file, - function, - line, - column, - container - ) - return container.future() - } -} - -private class TestResponder: Responder { - var mockErrorToThrow: Error? - var mockErrorToReturnInFuture: Error? - func respond(to req: Request) throws -> Future { - if let error = mockErrorToThrow { - throw error - } else if let error = mockErrorToReturnInFuture { - return req.future(error: error) - } else { - return req.future(Response(using: req)) - } - } -} +import Vapor final class BugsnagTests: XCTestCase { - func testMiddleware() throws { - let application = try Application.test() - let request = Request(using: application) - let errorReporter = TestErrorReporter() - let middleware = BugsnagMiddleware(reporter: errorReporter) - let responder = TestResponder() - _ = try middleware.respond(to: request, chainingTo: responder).wait() - - // expect no error reported when response is successful - XCTAssertNil(errorReporter.capturedReportParameters) - - responder.mockErrorToThrow = NotFound() - - _ = try? middleware.respond(to: request, chainingTo: responder).wait() - - // expect an error to be reported when responder throws - XCTAssertNotNil(errorReporter.capturedReportParameters) - - errorReporter.capturedReportParameters = nil - responder.mockErrorToReturnInFuture = NotFound() - - _ = try? middleware.respond(to: request, chainingTo: responder).wait() - - // expect an error to be reported when responder returns an errored future - XCTAssertNotNil(errorReporter.capturedReportParameters) - } - func testSendReport() throws { var capturedSendReportParameters: ( host: String, @@ -116,7 +16,7 @@ final class BugsnagTests: XCTestCase { sendReport: { host, headers, data, container in capturedSendReportParameters = (host, headers, data, container) return container.future(Response(http: HTTPResponse(status: .ok), using: container)) - }) + }) let application = try Application.test() let request = Request(using: application) request.breadcrumb(name: "a", type: .log) diff --git a/Tests/BugsnagTests/MiddlewareTests.swift b/Tests/BugsnagTests/MiddlewareTests.swift new file mode 100644 index 0000000..91fa704 --- /dev/null +++ b/Tests/BugsnagTests/MiddlewareTests.swift @@ -0,0 +1,154 @@ +import Vapor +import XCTest +@testable import Bugsnag + +extension Application { + public static func test() throws -> Application { + var services = Services() + try services.register(BugsnagProvider(config: BugsnagConfig( + apiKey: "e9792272fae71a3b869a1152008f7f0f", + releaseStage: "development" + ))) + + var middlewaresConfig = MiddlewareConfig() + middlewaresConfig.use(BugsnagMiddleware.self) + services.register(middlewaresConfig) + + let sharedThreadPool = BlockingIOThreadPool(numberOfThreads: 2) + sharedThreadPool.start() + services.register(sharedThreadPool) + + return try Application(config: Config(), environment: .testing, services: services) + } +} + +final class TestErrorReporter: ErrorReporter { + + var capturedReportParameters: ( + error: Error, + severity: Severity, + userId: CustomStringConvertible?, + metadata: [String: CustomDebugStringConvertible], + file: String, + function: String, + line: Int, + column: Int, + container: Container + )? + func report( + _ error: Error, + severity: Severity, + userId: CustomStringConvertible?, + metadata: [String: CustomDebugStringConvertible], + file: String, + function: String, + line: Int, + column: Int, + on container: Container + ) -> Future { + capturedReportParameters = ( + error, + severity, + userId, + metadata, + file, + function, + line, + column, + container + ) + return container.future() + } +} + +final class TestResponder: Responder { + var mockErrorToThrow: Error? + var mockErrorToReturnInFuture: Error? + func respond(to req: Request) throws -> Future { + if let error = mockErrorToThrow { + throw error + } else if let error = mockErrorToReturnInFuture { + return req.future(error: error) + } else { + return req.future(Response(using: req)) + } + } +} + +final class MiddlewareTests: XCTestCase { + var application: Application! + var request: Request! + var middleware: BugsnagMiddleware! + + let errorReporter = TestErrorReporter() + let responder = TestResponder() + + override func setUp() { + application = try! Application.test() + request = Request(using: application) + middleware = BugsnagMiddleware(reporter: errorReporter) + } + + func testNoErrorReportedByDefault() throws { + _ = try middleware.respond(to: request, chainingTo: responder).wait() + + // expect no error reported when response is successful + XCTAssertNil(errorReporter.capturedReportParameters) + } + + func testRespondErrorsAreCaptured() throws { + responder.mockErrorToThrow = NotFound() + + _ = try? middleware.respond(to: request, chainingTo: responder).wait() + + // expect an error to be reported when responder throws + XCTAssertNotNil(errorReporter.capturedReportParameters) + } + + func testErrorsInFutureAreCaptured() throws { + errorReporter.capturedReportParameters = nil + responder.mockErrorToReturnInFuture = NotFound() + + _ = try? middleware.respond(to: request, chainingTo: responder).wait() + + // expect an error to be reported when responder returns an errored future + XCTAssertNotNil(errorReporter.capturedReportParameters) + } + + func testReportableErrorPropertiesAreRespected() throws { + struct MyError: ReportableError { + let severity = Severity.info + let userId: CustomStringConvertible? = 123 + let metadata: [String: CustomDebugStringConvertible] = ["meta": "data"] + } + + let error = MyError() + responder.mockErrorToThrow = error + + _ = try? middleware.respond(to: request, chainingTo: responder).wait() + + guard + let params = errorReporter.capturedReportParameters + else { + XCTFail("No error was thrown") + return + } + + XCTAssertNotNil(params.error as? MyError) + XCTAssertEqual(params.metadata as? [String: String], ["meta": "data"]) + XCTAssertEqual(params.severity.value, "info") + XCTAssertEqual(params.userId as? Int, 123) + } + + func testOptOutOfErrorReporting() throws { + struct MyError: ReportableError { + let shouldReport = false + } + + responder.mockErrorToThrow = MyError() + + _ = try? middleware.respond(to: request, chainingTo: responder).wait() + + XCTAssertNil(errorReporter.capturedReportParameters) + } +} diff --git a/Tests/BugsnagTests/XCTestManifests.swift b/Tests/BugsnagTests/XCTestManifests.swift index dbfdc59..06b4115 100644 --- a/Tests/BugsnagTests/XCTestManifests.swift +++ b/Tests/BugsnagTests/XCTestManifests.swift @@ -1,17 +1,33 @@ +#if !canImport(ObjectiveC) import XCTest extension BugsnagTests { - static let __allTests = [ - ("testMiddleware", testMiddleware), + // DO NOT MODIFY: This is autogenerated, use: + // `swift test --generate-linuxmain` + // to regenerate. + static let __allTests__BugsnagTests = [ ("testReportingCanBeDisabled", testReportingCanBeDisabled), ("testSendReport", testSendReport), ] } -#if !os(macOS) +extension MiddlewareTests { + // DO NOT MODIFY: This is autogenerated, use: + // `swift test --generate-linuxmain` + // to regenerate. + static let __allTests__MiddlewareTests = [ + ("testErrorsInFutureAreCaptured", testErrorsInFutureAreCaptured), + ("testNoErrorReportedByDefault", testNoErrorReportedByDefault), + ("testOptOutOfErrorReporting", testOptOutOfErrorReporting), + ("testReportableErrorPropertiesAreRespected", testReportableErrorPropertiesAreRespected), + ("testRespondErrorsAreCaptured", testRespondErrorsAreCaptured), + ] +} + public func __allTests() -> [XCTestCaseEntry] { return [ - testCase(BugsnagTests.__allTests), + testCase(BugsnagTests.__allTests__BugsnagTests), + testCase(MiddlewareTests.__allTests__MiddlewareTests), ] } #endif