fix(restify): support an array of handlers (#709)

If an array of handlers is given as an argument to `server.use` instead
of just a single function, the agent would cause an exception to be
raised.

This commit changes the Restify instrumentation logic so that the
handlers no longer needs to be wrapped, and so that the arguments to the
`server.use` function no longer needs to be parsed by the agent.

Instead the agent now patches a central (private) error handling
function to be notified when ever an error is passed to the `next`
function.

The route name is instead gathered by the same logic that also gathers
the route name for Express - from `req.route.path` - as Restify just so
happens to use the same API for storing the route name on the request
object as Express in this case.

This way of getting the route name is actually more stable as the
previous implementation didn't work if the request never made it to the
actual route handler (e.g. if an error occurred in a middleware
handler). In that case the transaction would have been named `use` (the
function name used to register the middleware handler).

Fixes: #706

Author: watson

.tav.yml

@@ -210,15 +210,8 @@ cassandra-driver:
   commands: node test/instrumentation/modules/cassandra/cassandra.js
 
 restify:
-  # All versions before 4.4 are unsupported. Additionally,
-  # there are several broken releases that need to be skipped:
-  # - 6.2.0
-  # - 6.0.1
-  # - 6.0.0
-  # - 5.0.0
-  versions: '>=4.4 <5 || >5 <6 || >6.0.1 <6.2 || >6.2'
-  commands:
-    - node test/instrumentation/modules/restify.js
+  versions: '>=5.2.0'
+  commands: node test/instrumentation/modules/restify.js
 
 finalhandler:
   versions: '*'

docs/supported-technologies.asciidoc

@@ -49,7 +49,7 @@ These are the frameworks that we officially support:
 |<<koa,Koa>> via koa-router |>=5.2.0 <8.0.0 |Koa doesn't have a built in router,
 so we can't support Koa directly since we rely on router information for full support.
 We currently support the most popular Koa router called https://github.com/alexmingoia/koa-router[koa-router]
-|<<restify,Restify>> |>=4.4 <8 |
+|<<restify,Restify>> |>=5.2.0 |
 |=======================================================================
 
 [float]

lib/instrumentation/express-utils.js

@@ -2,6 +2,8 @@
 
 var symbols = require('../symbols')
 
+// This function is also able to extract the path from a Restify request as
+// it's storing the route name on req.route.path as well
 exports.getPathFromRequest = function (req) {
   var path
 

lib/instrumentation/modules/restify.js

@@ -1,58 +1,30 @@
 'use strict'
 
-const isError = require('core-util-is').isError
+const semver = require('semver')
 
 const shimmer = require('../shimmer')
 
-const httpMethods = [
-  'del',
-  'get',
-  'head',
-  'opts',
-  'post',
-  'put',
-  'patch'
-]
-
 module.exports = function (restify, agent, version, enabled) {
   if (!enabled) return restify
   if (!agent._conf.frameworkName) agent._conf.frameworkName = 'restify'
   if (!agent._conf.frameworkVersion) agent._conf.frameworkVersion = version
 
-  const reportedSymbol = Symbol('reported')
-
-  function makeWrapHandler (name) {
-    return function wrapHandler (handler) {
-      return function wrappedHandler (request) {
-        agent._instrumentation.setDefaultTransactionName(name)
-        shimmer.wrap(arguments, 2, function wrapNext (next) {
-          return function wrappedNext (err) {
-            if (isError(err) && !err[reportedSymbol]) {
-              err[reportedSymbol] = true
-              agent.captureError(err, { request })
-            }
-            return next.apply(this, arguments)
-          }
-        })
-        return handler.apply(this, arguments)
-      }
-    }
-  }
-
   function patchServer (server) {
-    shimmer.massWrap(server, httpMethods, function wrapMethod (fn, method) {
-      return function wrappedMethod (path) {
-        const name = `${method.toUpperCase()} ${path}`
-        const fns = Array.prototype.slice.call(arguments, 1).map(makeWrapHandler(name))
-        return fn.apply(this, [path].concat(fns))
-      }
-    })
-
-    shimmer.massWrap(server, [ 'use', 'pre' ], function wrapMiddleware (fn, method) {
-      return function wrappedMiddleware () {
-        return fn.apply(this, Array.prototype.slice.call(arguments).map(makeWrapHandler(method)))
-      }
-    })
+    if (semver.gte(version, '7.0.0')) {
+      shimmer.wrap(server, '_onHandlerError', function (orig) {
+        return function _wrappedOnHandlerError (err, req, res, isUncaught) {
+          if (err) agent.captureError(err, { request: req, handled: !isUncaught })
+          return orig.apply(this, arguments)
+        }
+      })
+    } else {
+      shimmer.wrap(server, '_emitErrorEvents', function (orig) {
+        return function _wrappedOnHandlerError (req, res, route, err, cb) {
+          if (err) agent.captureError(err, { request: req })
+          return orig.apply(this, arguments)
+        }
+      })
+    }
   }
 
   shimmer.wrap(restify, 'createServer', function (fn) {

package.json

@@ -138,7 +138,7 @@
     "standard": "^12.0.1",
     "tape": "^4.9.1",
     "tedious": "^2.6.1",
-    "test-all-versions": "^3.3.3",
+    "test-all-versions": "^4.0.0",
     "thunky": "^1.0.3",
     "untildify": "^3.0.3",
     "util.promisify": "^1.0.0",

test/instrumentation/modules/restify.js

@@ -41,6 +41,7 @@ test('transaction name', function (t) {
   // otherwise this will use IPv6, which fails on Travis CI.
   server.listen(null, '0.0.0.0', function () {
     const req = http.get(`${server.url}/hello/world`, res => {
+      t.equal(res.statusCode, 200, 'server should respond with status code 200')
       const chunks = []
       res.on('data', chunks.push.bind(chunks))
       res.on('end', () => {
@@ -97,6 +98,7 @@ test('error reporting', function (t) {
   // otherwise this will use IPv6, which fails on Travis CI.
   server.listen(null, '0.0.0.0', function () {
     const req = http.get(`${server.url}/hello/world`, res => {
+      t.equal(res.statusCode, 500, 'server should respond with status code 500')
       res.resume()
       res.on('end', () => {
         agent.flush()
@@ -149,6 +151,69 @@ test('error reporting from chained handler', function (t) {
   // otherwise this will use IPv6, which fails on Travis CI.
   server.listen(null, '0.0.0.0', function () {
     const req = http.get(`${server.url}/hello/world`, res => {
+      t.equal(res.statusCode, 500, 'server should respond with status code 500')
+      res.resume()
+      res.on('end', () => {
+        agent.flush()
+        done()
+      })
+    })
+    req.end()
+  })
+})
+
+test('error reporting from chained handler given as array', function (t) {
+  resetAgent((data) => {
+    t.ok(errored, 'reported an error')
+    t.equal(data.transactions.length, 1, 'has a transaction')
+
+    const trans = data.transactions[0]
+    t.equal(trans.name, 'GET /hello/:name', 'transaction name is GET /hello/:name')
+    t.equal(trans.type, 'request', 'transaction type is request')
+    t.end()
+  })
+
+  let request
+  let errored = false
+  const error = new Error('wat')
+  const captureError = agent.captureError
+  agent.captureError = function (err, data) {
+    t.equal(err, error, 'has the expected error')
+    t.ok(data, 'captured data with error')
+    t.equal(data.request, request, 'captured data has the request object')
+    errored = true
+  }
+  t.on('end', function () {
+    agent.captureError = captureError
+  })
+
+  const server = restify.createServer()
+  const done = once(() => {
+    server.close()
+  })
+  t.on('end', done)
+
+  server.use([
+    function (req, res, next) {
+      next()
+    },
+    function (req, res, next) {
+      request = req
+      next(error)
+    }
+  ])
+
+  // It's important to have the route registered. Otherwise the request will
+  // not reach the middleware above and will just be ended with a 404
+  server.get('/hello/:name', (req, res, next) => {
+    t.fail('should never call route handler')
+  })
+
+  // NOTE: Hostname must be supplied to force IPv4 mode,
+  // otherwise this will use IPv6, which fails on Travis CI.
+  server.listen(null, '0.0.0.0', function () {
+    const req = http.get(`${server.url}/hello/world`, res => {
+      t.equal(res.statusCode, 500, 'server should respond with status code 500')
       res.resume()
       res.on('end', () => {
         agent.flush()