add solution for handling 405 response code for wrong http methods

[mregydev]

According to the issue number 2414 in express which indicates sending 405 status code in case of not allowed http method for existing resource and this issue is found in the router module.

Made an update to handle this issue

[edevil]

@mregydev why are you opening and closing PRs? Isn’t it the same change?

[mregydev]

I am sorry this was a mistake because I am new to PRs and I need your advice about the workflow
1- I have forked the project 2- Cloned it 3- Created a branch 3- Added an enhancement to support 405 response code 4- Ran tests 5- I committed code to the branch 6- Made pull request

Is this the right way of doing things or is there a better method ?

I really appreciate your feedback

[edevil]

Seems correct to me. Now you need to wait for feedback from the project maintainers.

[wesleytodd]

Hey @mregydev, before anyone takes a look at this, can you remove all the unrelated formatting changes? That will give us a better view of what your change actually does, and would be required before any possibility of merging. Thanks!

[mregydev]

Hey @mregydev, before anyone takes a look at this, can you remove all the unrelated formatting changes? That will give us a better view of what your change actually does, and would be required before any possibility of merging. Thanks!

You are right , I have removed unrelated formatting waiting for review

[mregydev]

@dougwilson please any new regarding this merge thanks in advance

[mregydev]

@wesleytodd a great thank for your answer but I think that 405 isn't considered as a special case of 404 as 404 means resource doesn't exist at all while 405 means existence and resource url is identified but method not supported and in most restful framework they are sending 405 in case resource exist but method not supported

Regarding formatting issues I have run lint and it didn't gave me any error

Also other comments are made and pushed in to the branch

Thanks in advance

[wesleytodd]

Yeah, thinking about it now I was trying to be reductionist and so got it wrong. I really meant "it is not an error". This differentiation is important and IMO should be handled differently by the api we expose. And this implementation doesn't really solve the "resource exists but not with that method because the router knows nothing about your resources.

Regarding formatting issues I have run lint and it didn't gave me any error

Yeah sorry about that. We don't currently have good linting because we are slowly converting to standard formatting, but only on the lines which are touched in PR's.

[dougwilson]

Hi All, I'm sorry for being away, and thanks to @wesleytodd for helping push this forward. I think having 405 support is worth while, as it has come up multiple times in the past, though it has never quite been strong enough to really get much PR attention, so thank you to @mregydev for putting together a pull request!

That said, my main concern with this pull request is that, for a feature that does not have everyone begging for, it seems strange to me that adding it will require everyone to restructure their apps. I think that kind of trade off is a non-starter here, as I don't feel like this feature is worth the churn it will force on apps.

As an example, this pull request breaks the ubiquitous code like the following:

router.get('/user/:id', getUserById)
router.put('/user/:id', putUserById)
router.delete('/user/:id', deleteUserById)

The obvious issue here is that this PR will take that working app and make PUT and DELETE 405 now.

Now, this is just the start. It can be restructured into a single Route declaration pretty easily, but larger apps are non-trivial and may not even be possible to get into a single route.

I would have two thoughts on this:

1. Is it possible make this opt in and/or opt out at all? This would allow the best of all the worlds and the flexibility. For example only those who want the 405 support would need to restructure their app while others can just leave it as-is. And if it is, do we think this should be router-wide or route-specific?
2. If it is not possible, can we discuss why it's not and probably need to agree on some sort of deprecation cycle of the above pattern prior to introducing this change. If possible, we may want to help some larger (open source) apps make the move, but that is optional, just would be a nice thing (plus we can make sure we understand the types of issues others will encounter trying to do this to better answer folk's questions about the process).

[mregydev]

@dougwilson A great thank for your reply. I tested what have you said and the it handled in the new implementation for example I tested this code whcih access same resource with different methods and it worked ( I mean that in case resource donot support passed verb it will send 405 status code)

// import our modules
var http         = require('http')
var Router       = require('router')
var finalhandler = require('finalhandler')

 
// store our message to display
var message = "Hello World!"
 
// initialize the router & server and add a final callback.
var router = Router()
var server = http.createServer(function onRequest(req, res) {
  router(req, res, finalhandler(req, res))
})

router.post('/message', function (req, res) {
  res.statusCode = 200
  res.setHeader('Content-Type', 'text/plain; charset=utf-8')
  res.end(message + '\n')
})


// // handle `GET` requests to `/message`
router.get('/message', function (req, res) {
  res.statusCode = 200
  res.setHeader('Content-Type', 'text/plain; charset=utf-8')
  res.end(message + '\n')
})

router.delete('/message', function (req, res) {
  res.statusCode = 200
  res.setHeader('Content-Type', 'text/plain; charset=utf-8')
  res.end("deleted" + '\n')
})


// make our http server listen to connections
server.listen(8080)

Thanks in advance

[mregydev]

Yeah, thinking about it now I was trying to be reductionist and so got it wrong. I really meant "it is not an error". This differentiation is important and IMO should be handled differently by the api we expose. And this implementation doesn't really solve the "resource exists but not with that method because the router knows nothing about your resources.

Regarding formatting issues I have run lint and it didn't gave me any error

Yeah sorry about that. We don't currently have good linting because we are slowly converting to standard formatting, but only on the lines which are touched in PR's.

@wesleytodd a great thank for your reply but do I initialize url resources with router module so how does router know nothing about them .. can you explain in more detail if possible ?

Thanks in advance

[wesleytodd]

adding it will require everyone to restructure their apps

This is probably more important than the point I was trying to make. This introduces a breaking change. I think adding a req.unhandledMethod or something similar would also avoid the breaking change here.

With that addition we could make finalhandler have a major version where it looks at this property and returns the 405. Just one idea, I would be interested in hearing if anyone else had a better non-breaking proposal.

so how does router know nothing about them

I don't want to rabbit hole on these edge issues, I was just intending to point out the difference between a route being matched and a resource existing. Either way, the above point is more important.

[mregydev]

@wesleytodd a great thank for your answer but why everyone will have to restrcture their app I think nothing will change as the module it self will send 405 in case http method not supported for it and also put in consideration that same resource can has multiple http methods

Regarding req.unhandledMethod I think it is a great solution but it will require change in finalhandler module too as you said

[dougwilson]

Hi @mregydev I apologize, the testing I did was not using the right code. I refreshed everything on a new machine and am now testing against your exact PR and the exact example you gave above, and it does work. I realize what you're doing is actually not using the Route object at all to encapsulate this, instead keeping a running tab of all matched methods instead.

I did find an issue, I think, though. It seems like if there is a .use after some routes it will 404 instead of the 405 I would expect. I'm not sure if my expectation is valid, though, without a concise description of what to expect from this change, so it was just from playing around from it a bit to form my expectation. It may be helpful to add some documentation around the behavior to the README as I'm sure when this lands, others will need to understand the behavior as well 👍

I'm going to continue to play around with the PR to see what other changes I can discover.

Here is the weird 404 behavior I'm referring to:

var finalhandler = require('finalhandler')
var http = require('http')
var Router = require('router')

var router = Router()
var server = http.createServer(function onRequest (req, res) {
  router(req, res, finalhandler(req, res))
})

router.post('/message', (req, res) => res.end('POST'))
router.get('/message', (req, res) => res.end('GET'))

router.use((req, res, next) => next())

server.listen(8080)

I'm expecting the following PUT request to 405 but instead of 404s.

$ curl -i -XPUT http://127.0.0.1:8080/message
HTTP/1.1 404 Not Found
Content-Security-Policy: default-src 'self'
X-Content-Type-Options: nosniff
Content-Type: text/html; charset=utf-8
Content-Length: 146
Date: Wed, 12 Dec 2018 03:35:23 GMT
Connection: keep-alive

<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
<body>
<pre>Cannot PUT /message</pre>
</body>
</html>

[mregydev]

@dougwilson thanks for you comment and also this issue and it is solved on this commit and regarding documentation will be added but prefer after confirming this change

[mregydev]

@dougwilson any updates , thanks in advance

[JaneJeon]

any updates?