route() should handle 405 Method not allowed

[nnarhinen]

I know this has been discussed in numerous issues here, but not about this specific case.

IMHO the following code

app.route('/user/:id').get(oneUser).delete(delUser).put(updateUser);

should send 405 Method Not Allowed for a POST request to /user/123.

I can of course workaround this easily by adding a .all(send405) to all of my route() calls but I couldn't find any variable in req that would expose what methods the route does have (for Allow header).

I know there are a ton of ways how POST /user/123 could still be defined, but if you do call .route() then it would be IMO reasonable to believe that it is the only way of defining methods for that one route.

So I suggest three alternative ways how I'd like this to be fixed (in the order of preference):

1. Make it the default behavior to send a correct 405 response if route() does not have that method.
2. Make it configurable (per router or something) to automatically send 405 responses.
3. Expose configured methods to be self used in all().

This most likely will raise the question about 'How about when GET returns 404, then POST should also return 404 even though with a different id it should send 405.'

I kinda agree, and this is why I add first in chain a method:

var withUser = function(req, res, next) {
  fetchUser(req.params.id, function(err, user) {
    if (err) return next(err);
    if (!user) return res.status(404).end();
    req.user = user;
    next();
  });
};

router.route('/users/:id').all(withUser).get(sendUser).put(updateUser).delete(deleteUser).all(send405);

[dougwilson]

#1 is not backwards-compatible, so just won't happen in 4.x and the others are really just cumbersome or something. I would envision this as the API, what do you think?

// route-specific
app.route(path, { automatic405: true })
.get(handler)

// router-wide default
var router = express.Router({ automatic405: true })

// can be overridden
router.route(path, { automatic405: false })
.get(handler)

This would go along with how other things like the automatic HEAD and OPTIONS settings would be handled. What do you think?

The remaining open question is how would the following work? Would it be a compile-time error? Would the post just never be called and lead to much confusion among users? Something else?

var router = express.Router({ automatic405: true })
router.route(path)
.get(handler)
.put(handler)
router.post(path, handler)

[nnarhinen]

IMHO the .post() could add a debug log (like the deprecation warnings from res.send(201, {}) etc) saying something like POST handler for <path> will never be called, check your routes.

[nnarhinen]

Otherwise I like the API, something I'd use with pleasure.

[dozoisch]

@dougwilson any update on that issue? It really is something that would be nice to have! :+1:

I guess a compile-time error would be perfect. It wouldn't break existing code because it would only send the error if a route is added on a path when automatic405 is true. I guess that makes sense!

[dougwilson]

For some reason this issue slipped by me this last release, otherwise I soul have added it. I'm sorry!

[dozoisch]

Awesome! Thanks for the update and nice work on last release!

[codenamezjames]

+1 In the meantime is there a way to "Hack" around this?

[dougwilson]

In the meantime is there a way to "Hack" around this?

There may be a clever way, but the simplest way is to add a .all on your route:

router.route('/some/path')
.get(getHandler)
.post(postHandler)
.all(methodNotAllowedHandler)

function methodNotAllowedHandler(req, res) {
  res.sendStatus(405)
}

[codenamezjames]

@dougwilson Thanks for the quick response, that doesn't seem like a bad idea. Thanks

[dozoisch]

@dougwilson any update on that!? Thanks a lot!

[dougwilson]

PRs are welcome!

[moar55]

Hi, I noticed there are PR's for this. So what is the state of this, how can I contribute to it?

[wesleytodd]

Hey @moar55, this example was merged on the router package which might help. https://github.com/pillarjs/router/pull/63

I think the current answer is that we will not be adding a feature to directly support this, but there is a decent work around if you want your api to support it. I think if there was sufficient demand, and a really good PR showed up then we would reconsider, but that is not where it is at now. If you want to generate that PR and demonstrate the demand I am happy to comment/help more.

[mregydev]

Please any new regarding this issue as still not working in express 5.x

[srkyaganti]

Hi, What is the current state of this issue? If it is still open, can I contribute to it?

Thanks, Srikar

[wesleytodd]

Hey @srkyaganti, you can start by looking at these related issues and PRs:

https://github.com/pillarjs/router/pull/63 https://github.com/pillarjs/router/pull/70 https://github.com/expressjs/express/issues/3790

But I think where this was left is that it is possible today, and all the proposed solutions are breaking changes, so probably not going to land.

[Lordfirespeed]

@dougwilson are you still open to an API similar to the one you envisioned in this comment?

It could be helpful to also add opt-in flags for OPTIONS request handling at the same priority as the route, as it is currently difficult to use the built-in OPTIONS request handling when using any catch-all middleware.

var router = express.Router({ automaticOptions: true })

Another nice feature would be the option to pass a middleware function to the flag:

function handleMethodNotAllowed(req, res) {
  res.status(405).send("no!");
}
var router = express.Router({ automatic405: handleMethodNotAllowed })

In terms of your concern:

var router = express.Router({ automatic405: true })
router.route(path)
  .get(handler)
  .put(handler)
 router.post(path, handler)

I think it would be most user-friendly if this threw a compile-time error providing useful info, for example that next('route') would allow the POST middleware to be reached, etc.

The problem with that, then, is detecting 'equivalent' routes:

var topLevelRouter = express.Router();
var childRouter = express.Router();

childRouter.route("/bar")
  .get(...);

topLevelRouter.use("/foo", childRouter);

topLevelRouter.route("/foo/bar")
  .get(...)

and then the implementation starts to get complicated.

Maybe a more elegant solution would be to expose (in a library) the OPTIONS and 405-handling middleware, so that similarly to this comment, users could

import { optionsHandler, methodNotAllowedCatcher } from "express";

router.route(path)
  .get(handler)
  .options(optionsHandler())
  .all(methodNotAllowedCatcher())

And in that case, why not place those as instance methods onto the Route:

router.route(path)
  .get(handler)
  .automaticOptions()
  .automatic405()