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

[PLAT-6174] feat: Ensure server plugins honour autoDetectErrors option #1464

Merged
merged 4 commits into from
Jul 8, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12,460 changes: 12,460 additions & 0 deletions examples/reactnative/rn063example/package-lock.json

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions packages/plugin-express/src/express.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ module.exports = {
requestClient.addMetadata('request', metadata)
}, true)

if (!client._config.autoDetectErrors) return next()

// unhandled errors caused by this request
dom.on('error', (err) => {
const event = client.Event.create(err, false, handledState, 'express middleware', 1)
Expand All @@ -48,6 +50,8 @@ module.exports = {
}

const errorHandler = (err, req, res, next) => {
if (!client._config.autoDetectErrors) return next(err)

const event = client.Event.create(err, false, handledState, 'express middleware', 1)

const { metadata, request } = getRequestAndMetadataFromReq(req)
Expand Down
6 changes: 6 additions & 0 deletions packages/plugin-koa/src/koa.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ module.exports = {
event.addMetadata('request', metadata)
}, true)

if (!client._config.autoDetectErrors) return next()

try {
await next()
} catch (err) {
Expand Down Expand Up @@ -59,6 +61,8 @@ module.exports = {
event.request = { ...event.request, ...request }
}, true)

if (!client._config.autoDetectErrors) return next()

try {
yield next
} catch (err) {
Expand All @@ -71,6 +75,8 @@ module.exports = {
}

const errorHandler = (err, ctx) => {
if (!client._config.autoDetectErrors) return

const event = client.Event.create(err, false, handledState, 'koa middleware', 1)

const { metadata, request } = getRequestAndMetadataFromCtx(ctx)
Expand Down
3 changes: 3 additions & 0 deletions packages/plugin-restify/src/restify.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ module.exports = {
event.request = { ...event.request, ...request }
}, true)

if (!client._config.autoDetectErrors) return next()

// unhandled errors caused by this request
dom.on('error', (err) => {
const event = client.Event.create(err, false, handledState, 'restify middleware', 1)
Expand All @@ -51,6 +53,7 @@ module.exports = {
}

const errorHandler = (req, res, err, cb) => {
if (!client._config.autoDetectErrors) return cb()
if (err.statusCode && err.statusCode < 500) return cb()

const event = client.Event.create(err, false, handledState, 'restify middleware', 1)
Expand Down
62 changes: 62 additions & 0 deletions test/node/features/express_disabled.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
Feature: @bugsnag/plugin-express autoDetectErrors=false

Background:
Given I store the api key in the environment variable "BUGSNAG_API_KEY"
And I store the notify endpoint in the environment variable "BUGSNAG_NOTIFY_ENDPOINT"
And I store the sessions endpoint in the environment variable "BUGSNAG_SESSIONS_ENDPOINT"
And I start the service "express-disabled"
And I wait for the host "express-disabled" to open port "80"

Scenario: a synchronous thrown error in a route
Then I open the URL "http://express-disabled/sync"
And I wait for 5 seconds
bengourley marked this conversation as resolved.
Show resolved Hide resolved
And I should receive no errors

Scenario: an asynchronous thrown error in a route
Then I open the URL "http://express-disabled/sync"
bengourley marked this conversation as resolved.
Show resolved Hide resolved
And I wait for 5 seconds
And I should receive no errors

Scenario: an error passed to next(err)
Then I open the URL "http://express-disabled/next"
And I wait for 5 seconds
And I should receive no errors

Scenario: a synchronous promise rejection in a route
Then I open the URL "http://express-disabled/rejection-sync"
And I wait for 5 seconds
And I should receive no errors

Scenario: an asynchronous promise rejection in a route
Then I open the URL "http://express-disabled/rejection-async"
And I wait for 5 seconds
And I should receive no errors

Scenario: a string passed to next(err)
Then I open the URL "http://express-disabled/string-as-error"
And I wait for 5 seconds
And I should receive no errors

Scenario: throwing non-Error error
Then I open the URL "http://express-disabled/throw-non-error"
And I wait for 5 seconds
And I should receive no errors

Scenario: a handled error passed to req.bugsnag.notify()
Then I open the URL "http://express-disabled/handled"
And I wait to receive an error
Then the error is valid for the error reporting API version "4" for the "Bugsnag Node" notifier
And the event "unhandled" is false
And the event "severity" equals "warning"
And the exception "errorClass" equals "Error"
And the exception "message" equals "handled"
And the exception "type" equals "nodejs"
And the "file" of stack frame 0 equals "scenarios/app-disabled.js"
And the event "request.url" equals "http://express-disabled/handled"
And the event "request.httpMethod" equals "GET"
And the event "request.clientIp" is not null

Scenario: adding body to request metadata
When I POST the data "data=in_request_body" to the URL "http://express-disabled/bodytest"
And I wait for 5 seconds
And I should receive no errors
48 changes: 48 additions & 0 deletions test/node/features/fixtures/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,22 @@ services:
- express
restart: "no"

express-disabled:
build:
context: express
args:
- NODE_VERSION
environment:
- BUGSNAG_API_KEY
- BUGSNAG_NOTIFY_ENDPOINT
- BUGSNAG_SESSIONS_ENDPOINT
networks:
default:
aliases:
- express-disabled
restart: "no"
entrypoint: "node scenarios/app-disabled"

restify:
build:
context: restify
Expand All @@ -106,6 +122,22 @@ services:
- restify
restart: "no"

restify-disabled:
build:
context: restify
args:
- NODE_VERSION
environment:
- BUGSNAG_API_KEY
- BUGSNAG_NOTIFY_ENDPOINT
- BUGSNAG_SESSIONS_ENDPOINT
networks:
default:
aliases:
- restify-disabled
restart: "no"
entrypoint: "node scenarios/app-disabled"

koa:
build:
context: koa
Expand All @@ -121,6 +153,22 @@ services:
- koa
restart: "no"

koa-disabled:
build:
context: koa
args:
- NODE_VERSION
environment:
- BUGSNAG_API_KEY
- BUGSNAG_NOTIFY_ENDPOINT
- BUGSNAG_SESSIONS_ENDPOINT
networks:
default:
aliases:
- koa-disabled
restart: "no"
entrypoint: "node scenarios/app-disabled"

koa-1x:
build:
context: koa-1x
Expand Down
81 changes: 81 additions & 0 deletions test/node/features/fixtures/express/scenarios/app-disabled.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
var Bugsnag = require('@bugsnag/node')
var bugsnagExpress = require('@bugsnag/plugin-express')
var express = require('express')
var bodyParser = require('body-parser')

Bugsnag.start({
apiKey: process.env.BUGSNAG_API_KEY,
endpoints: {
notify: process.env.BUGSNAG_NOTIFY_ENDPOINT,
sessions: process.env.BUGSNAG_SESSIONS_ENDPOINT
},
autoTrackSessions: false,
autoDetectErrors: false,
plugins: [bugsnagExpress]
})


var middleware = Bugsnag.getPlugin('express')

var app = express()

app.use(middleware.requestHandler)

// If the server hasn't started sending something within 2 seconds
// it probably won't. So end the request and hurry the failing test
// along.
app.use(function (req, res, next) {
setTimeout(function () {
if (!res.headersSent) return res.sendStatus(500)
}, 2000)
next()
})

app.get('/', function (req, res) {
res.end('ok')
})

app.get('/sync', function (req, res) {
throw new Error('sync')
})

app.get('/async', function (req, res) {
setTimeout(function () {
throw new Error('async')
}, 100)
})

app.get('/next', function (req, res, next) {
next(new Error('next'))
})

app.get('/rejection-sync', function (req, res, next) {
Promise.reject(new Error('reject sync')).catch(next)
})

app.get('/rejection-async', function (req, res, next) {
setTimeout(function () {
Promise.reject(new Error('reject async')).catch(next)
}, 100)
})

app.get('/string-as-error', function (req, res, next) {
next('errrr')
})

app.get('/throw-non-error', function (req, res, next) {
throw 1 // eslint-disable-line
})

app.get('/handled', function (req, res, next) {
req.bugsnag.notify(new Error('handled'))
res.end('OK')
})

app.post('/bodytest', bodyParser.urlencoded(), function (req, res, next) {
throw new Error('request body')
})

app.use(middleware.errorHandler)

app.listen(80)
70 changes: 70 additions & 0 deletions test/node/features/fixtures/koa/scenarios/app-disabled.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
const Bugsnag = require('@bugsnag/node')
const bugsnagKoa = require('@bugsnag/plugin-koa')
const Koa = require('koa')
const bodyParser = require('koa-bodyparser');

Bugsnag.start({
apiKey: process.env.BUGSNAG_API_KEY,
endpoints: {
notify: process.env.BUGSNAG_NOTIFY_ENDPOINT,
sessions: process.env.BUGSNAG_SESSIONS_ENDPOINT
},
autoDetectErrors: false,
plugins: [bugsnagKoa]
})


const middleware = Bugsnag.getPlugin('koa')

const app = new Koa()

// If the server hasn't started sending something within 2 seconds
// it probably won't. So end the request and hurry the failing test
// along.
app.use(async (ctx, next) => {
setTimeout(function () {
if (!ctx.response.headerSent) ctx.response.status = 500
}, 2000)
await next()
})

app.use(async (ctx, next) => {
if (ctx.path === '/error-before-handler') {
throw new Error('nope')
} else {
await next()
}
})

app.use(middleware.requestHandler)

app.use(bodyParser());

const erroneous = () => new Promise((resolve, reject) => reject(new Error('async noooop')))

app.use(async (ctx, next) => {
if (ctx.path === '/') {
ctx.body = 'ok'
} else if (ctx.path === '/err') {
throw new Error('noooop')
} else if (ctx.path === '/async-err') {
await erroneous()
} else if (ctx.path === '/ctx-throw') {
ctx.throw(500, 'thrown')
} else if (ctx.path === '/ctx-throw-400') {
ctx.throw(400, 'thrown')
} else if (ctx.path === '/throw-non-error') {
throw 'error' // eslint-disable-line
} else if (ctx.path === '/handled') {
ctx.bugsnag.notify(new Error('handled'))
await next()
} else if (ctx.path === '/bodytest') {
throw new Error('request body')
} else {
await next()
}
})

app.on('error', middleware.errorHandler)

app.listen(80)
Loading