feat(app): warn when HEAD route is declared after GET route on the same path

lib/application.js

@@ -21,6 +21,7 @@ var methods = require('./utils').methods;
 var compileETag = require('./utils').compileETag;
 var compileQueryParser = require('./utils').compileQueryParser;
 var compileTrust = require('./utils').compileTrust;
+var pathsOverlap = require('./utils').pathsOverlap;
 var resolve = require('node:path').resolve;
 var once = require('once')
 var Router = require('router');
@@ -475,6 +476,18 @@ methods.forEach(function (method) {
       return this.set(path);
     }
 
+    if (method === 'head' && this.router && Array.isArray(this.router.stack)) {
+      var hasPrecedingGet = this.router.stack.some(function (layer) {
+        return layer.route && pathsOverlap(layer.route.path, path) && layer.route.methods && layer.route.methods.get;
+      });
+      if (hasPrecedingGet) {
+        process.emitWarning(
+          'HEAD route for "' + path + '" declared after GET route will be shadowed. Declare HEAD routes before GET routes to ensure they execute.',
+          'ExpressWarning'
+        );
+      }
+    }
+
     var route = this.route(path);
     route[method].apply(route, slice.call(arguments, 1));
     return this;

lib/express.js

@@ -63,12 +63,47 @@ exports.application = proto;
 exports.request = req;
 exports.response = res;
 
+var utils = require('./utils');
+
+/**
+ * Express router factory wrapping Router to detect and warn on shadowed HEAD routes.
+ *
+ * @param {Object} [options]
+ * @return {Function} router
+ * @public
+ */
+
+function expressRouter(options) {
+  var router = new Router(options);
+  var origHead = router.head;
+
+  router.head = function head(path) {
+    if (Array.isArray(this.stack)) {
+      var hasPrecedingGet = this.stack.some(function (layer) {
+        return layer.route && utils.pathsOverlap(layer.route.path, path) && layer.route.methods && layer.route.methods.get;
+      });
+      if (hasPrecedingGet) {
+        process.emitWarning(
+          'HEAD route for "' + path + '" declared after GET route will be shadowed. Declare HEAD routes before GET routes to ensure they execute.',
+          'ExpressWarning'
+        );
+      }
+    }
+    return origHead.apply(this, arguments);
+  };
+
+  return router;
+}
+
+expressRouter.Route = Router.Route;
+expressRouter.prototype = Router.prototype;
+
 /**
  * Expose constructors.
  */
 
 exports.Route = Router.Route;
-exports.Router = Router;
+exports.Router = expressRouter;
 
 /**
  * Expose middleware

lib/utils.js

@@ -269,3 +269,27 @@ function parseExtendedQueryString(str) {
     allowPrototypes: true
   });
 }
+
+/**
+ * Check if two route paths overlap (supporting string and array paths).
+ *
+ * @param {String|Array} existingPath
+ * @param {String|Array} newPath
+ * @return {Boolean}
+ * @api private
+ */
+
+exports.pathsOverlap = function pathsOverlap(existingPath, newPath) {
+  if (Array.isArray(existingPath) && Array.isArray(newPath)) {
+    return existingPath.some(function (p) {
+      return newPath.indexOf(p) !== -1;
+    });
+  }
+  if (Array.isArray(existingPath)) {
+    return existingPath.indexOf(newPath) !== -1;
+  }
+  if (Array.isArray(newPath)) {
+    return newPath.indexOf(existingPath) !== -1;
+  }
+  return existingPath === newPath;
+};

test/Router.js

@@ -633,4 +633,107 @@ describe('Router', function () {
       });
     });
   });
+
+  describe('.head()', function () {
+    it('should emit a warning when HEAD is declared after GET on the same path', function (done) {
+      var router = new Router()
+      var warnings = []
+
+      function onWarning(warning) {
+        warnings.push(warning)
+      }
+
+      process.on('warning', onWarning)
+
+      router.get('/tobi', function (req, res) {
+        res.end('tobi')
+      })
+
+      router.head('/tobi', function (req, res) {
+        res.end()
+      })
+
+      process.nextTick(function () {
+        process.removeListener('warning', onWarning)
+        assert.strictEqual(warnings.length, 1)
+        assert.strictEqual(warnings[0].name, 'ExpressWarning')
+        assert.ok(warnings[0].message.includes('HEAD route for "/tobi" declared after GET route will be shadowed'))
+        done()
+      })
+    })
+
+    it('should not emit a warning when HEAD is declared before GET on the same path', function (done) {
+      var router = new Router()
+      var warnings = []
+
+      function onWarning(warning) {
+        warnings.push(warning)
+      }
+
+      process.on('warning', onWarning)
+
+      router.head('/tobi', function (req, res) {
+        res.end()
+      })
+
+      router.get('/tobi', function (req, res) {
+        res.end('tobi')
+      })
+
+      process.nextTick(function () {
+        process.removeListener('warning', onWarning)
+        assert.strictEqual(warnings.length, 0)
+        done()
+      })
+    })
+
+    it('should emit a warning when GET is declared with an array containing the HEAD path', function (done) {
+      var router = new Router()
+      var warnings = []
+
+      function onWarning(warning) {
+        warnings.push(warning)
+      }
+
+      process.on('warning', onWarning)
+
+      router.get(['/a', '/b'], function (req, res) {
+        res.end('get')
+      })
+
+      router.head('/a', function (req, res) {
+        res.end()
+      })
+
+      process.nextTick(function () {
+        process.removeListener('warning', onWarning)
+        assert.strictEqual(warnings.length, 1)
+        assert.strictEqual(warnings[0].name, 'ExpressWarning')
+        assert.ok(warnings[0].message.includes('HEAD route for "/a" declared after GET route will be shadowed'))
+        done()
+      })
+    })
+
+    it('should not mutate the underlying router package prototype', function () {
+      var StandaloneRouter = require('router')
+      var standalone = new StandaloneRouter()
+
+      var warnings = []
+      function onWarning(warning) {
+        warnings.push(warning)
+      }
+
+      process.on('warning', onWarning)
+
+      standalone.get('/standalone', function (req, res) {
+        res.end()
+      })
+      standalone.head('/standalone', function (req, res) {
+        res.end()
+      })
+
+      process.removeListener('warning', onWarning)
+      assert.strictEqual(warnings.length, 0)
+    })
+  })
 })

test/app.head.js

@@ -63,4 +63,108 @@ describe('app.head()', function(){
       .expect('x-method', 'head')
       .expect(200, done)
   })
+
+  it('should emit a warning when HEAD is declared after GET on the same path', function (done) {
+    var app = express()
+    var warnings = []
+
+    function onWarning(warning) {
+      warnings.push(warning)
+    }
+
+    process.on('warning', onWarning)
+
+    app.get('/tobi', function (req, res) {
+      res.send('tobi')
+    })
+
+    app.head('/tobi', function (req, res) {
+      res.end()
+    })
+
+    process.nextTick(function () {
+      process.removeListener('warning', onWarning)
+      assert.strictEqual(warnings.length, 1)
+      assert.strictEqual(warnings[0].name, 'ExpressWarning')
+      assert.ok(warnings[0].message.includes('HEAD route for "/tobi" declared after GET route will be shadowed'))
+      done()
+    })
+  })
+
+  it('should not emit a warning when HEAD is declared before GET on the same path', function (done) {
+    var app = express()
+    var warnings = []
+
+    function onWarning(warning) {
+      warnings.push(warning)
+    }
+
+    process.on('warning', onWarning)
+
+    app.head('/tobi', function (req, res) {
+      res.end()
+    })
+
+    app.get('/tobi', function (req, res) {
+      res.send('tobi')
+    })
+
+    process.nextTick(function () {
+      process.removeListener('warning', onWarning)
+      assert.strictEqual(warnings.length, 0)
+      done()
+    })
+  })
+
+  it('should not emit a warning when HEAD and GET are declared on different paths', function (done) {
+    var app = express()
+    var warnings = []
+
+    function onWarning(warning) {
+      warnings.push(warning)
+    }
+
+    process.on('warning', onWarning)
+
+    app.get('/foo', function (req, res) {
+      res.send('foo')
+    })
+
+    app.head('/bar', function (req, res) {
+      res.end()
+    })
+
+    process.nextTick(function () {
+      process.removeListener('warning', onWarning)
+      assert.strictEqual(warnings.length, 0)
+      done()
+    })
+  })
+
+  it('should emit a warning when GET is declared with an array containing the HEAD path', function (done) {
+    var app = express()
+    var warnings = []
+
+    function onWarning(warning) {
+      warnings.push(warning)
+    }
+
+    process.on('warning', onWarning)
+
+    app.get(['/a', '/b'], function (req, res) {
+      res.send('get')
+    })
+
+    app.head('/a', function (req, res) {
+      res.end()
+    })
+
+    process.nextTick(function () {
+      process.removeListener('warning', onWarning)
+      assert.strictEqual(warnings.length, 1)
+      assert.strictEqual(warnings[0].name, 'ExpressWarning')
+      assert.ok(warnings[0].message.includes('HEAD route for "/a" declared after GET route will be shadowed'))
+      done()
+    })
+  })
 })