RFC 2616 10.4.6 Compliance - Method Not Allowed On Resources Method isn't Specified

[husobee]

I noticed this issue when trying to access methods of a resource that are not allowed. I have drastically simplified the router code to accommodate RFC compliance by removing the individual Method based tree structures in the router. At most the gains IMO are negligible in splitting up the resource url route trees, and the complexity causes confusion. Moreover, if you are looking to implement new methods, this structure will allow that more easily. In this commit I did the following:

Added methodNotAllowedHandler helper, altered router to comply with RFC2616 10.4.6, Method Not Allowed, when trying to access a method on a Resource, where that method is not specified by the application. Altered router to comply with RFC2626 14.7 Allow Header on 405 Status Responses.

Issue Submitted for this Pull Request: #204

I look forward to hearing from you about this suggestion!

[husobee]

this newer version goes faster than the original PR,
before:

BenchmarkEcho_GithubAll    30000             51171 ns/op               0 B/op          0 allocs/op

after:

BenchmarkEcho_GithubAll    30000             45594 ns/op               0 B/op          0 allocs/op
BenchmarkGin_GithubAll     30000             44502 ns/op               0 B/op          0 allocs/op

Looking into the test failures.

[husobee]

Will continue to investigate ways to lower this back down. Have a few more ideas.

[axdg]

This looks promising. I haven't run the benchmarks myself yet (but will do).

@vishr, we could set the EnableMethodNotAllowed option, this is what the other tree routers have been doing, although this really seems pretty hacky to me. It will degrade performance in every case where an incorrect (or a non-sensical method) is sent. My personal opinion is that I would rather have no option at all than setting a flag.

The more I think about it, the more I think it's a good idea to arrange the router this way (as long as the perf cost is minimal, which it appears to be). This structure would not just allow for a 405 response, It would also mean that the proper response could be sent to an OPTIONS request, and would allow for a very minimal implementation of #74.

[axdg]

@husobee, I just swapped out labstack for your fork in the benchmarks and got some very different results?

[husobee]

let me try to replicate. Those are wildly different that what I am seeing. Will use your benchmark repo to try to duplicate @axdg

[husobee]

I think some of my changes might have affected what you were seeing, maybe. I have altered my fork to be more clear, (and correct, were some issues with the route tree I introduced) Here is what I am seeing now on my machine:

Master labstack/echo:

BenchmarkEcho_GithubAll    30000             40348 ns/op               0 B/op          0 allocs/op

Gin:

BenchmarkGin_GithubAll     50000             39550 ns/op               0 B/op          0 allocs/op

husobee/echo:

BenchmarkEcho_GithubAll    30000             48702 ns/op               0 B/op          0 allocs/op

Let me know if you are still seeing differences. I would like to continue trying to optimize, I have one idea for creation of a full radix tree (like how it is being done in this fork), then making the individual method based trees, pulling the handler struct i introduced in router.go from the full tree into the individual trees. I feel like that extra overhead on creation of the method based trees would be worth it. Seems to me if we went that direction, it would be very clearly keep the same physical properties labstack/echo currently has, but allow us to validate the HTTP method is valid, and insert other logic for performing OPTIONS on routes for CORS. Will be in touch.

[axdg]

@husobee Yep, I'm actually seeing slightly better results than you are now.