Issue a warning when GET route declared before HEAD

[peter-batley-depop]

I have spent several hours debugging a HEAD route and eventually found that I had fallen foul of the note here: https://expressjs.com/en/4x/api.html#router.METHOD

A warning may not be the right approach, but it would be nice to be notified on a HEAD route declared after a GET that the GET logic would be called. I suspect I will be neither the first nor the last to have bashed their head against a wall over this one!

[manojmula]

Hi @peter-batley-depop ,

Can you share your code so it will be more clear on your concern.

[simcar]

I ask to take charge of the issue. The issue occurs when the head method is declared after the get method for a certain path. For example, the code:

const express = require('express')
const app = express()

const router = express.Router()

router.get('/', function (req, res) {
  console.log('Get Called')
  res.send('Hello World')
})

router.head('/', function (req, res, next) {
  console.log('Head Called')
  res.send()
})

app.use(router)

app.listen(3000)

An head request to localhost:3000 will print 'Get Called', so the get method is called instead of the head. The reason is that the layers stack is created in the order in which the methods are defined in the code. When the HEAD method is called, the stack is scrolled until it finds a match with the specified path and method. The _handles_method function in the route.js file checks whether a certain route can handle the method, and returns true if the first route checked is not of type HEAD but the route can handle a GET method. So, when the GET method is defined before HEAD, it is executed.

My proposal is to check all routes in the case of HEAD method, instead of stopping at the first one. For this I propose a change to the code of the next function defined at this line.

The change is to modify the stop condition of the while loop in the case of method HEAD to allow scanning of all defined routes and the definition of temporary variables where the best match is stored. If the method HEAD is found, the loop is stopped. In the case of other methods, the loop works as usual.

The code defined in these lines become something like:

// find next matching layer
var layer;
var match;
var route;

var head_route;
var head_layer;

// scan all routes if method = HEAD
while ((match !== true || req.method === 'HEAD') && idx < stack.length) {
  layer = stack[idx++];
  match = matchLayer(layer, path);
  route = layer.route;

  if (typeof match !== 'boolean') {
    // hold on to layerError
    layerError = layerError || match;
  }

  if (match !== true) {
    continue;
  }

  if (!route) {
    // process non-route handlers normally
    continue;
  }

  if (layerError) {
    // routes do not match with a pending error
    match = false;
    continue;
  }

  var method = req.method;
  var has_method = route._handles_method(method);

  // build up automatic options response
  if (!has_method && method === 'OPTIONS') {
    appendMethods(options, route._options());
  }

  // don't even bother matching route
  if (!has_method && method !== 'HEAD') {
    match = false;
  }

  // best match for head request
  if (has_method && method === 'HEAD') {
    head_route = route;
    head_layer = layer;
    if (Boolean(route.methods['head'])) {
      break;
    }
  }

}

// no match
if (match !== true && !head_route) {
  return done(layerError);
}

// set best head route and layer
if (head_route) {
  route = head_route;
  layer = head_layer;
}