Discussion: 405 Support breaks wildcard routes

[julienschmidt]

As reported by @ydnar in #51, the 405 Support introduced in #51 breaks wildcard routes:

This completely breaks wildcard routes for alternate methods, for example:

r.Handle("GET", "/v1/search", search)
r.Handle("OPTIONS", "/*path", handler)
GET requests for /v1/search will always return a 405 with this change.

[javierprovecho]

Excuse me @julienschmidt , but I can't replicate that bad behaviour, even changing the order of the handlers. Here is my code:

package main

import (
    "fmt"
    "github.com/julienschmidt/httprouter"
    "log"
    "net/http"
)

func Index(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
    fmt.Fprint(w, "Welcome!\n")
}

func Hello(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
    fmt.Fprintf(w, "hello, %s!\n", ps.ByName("name"))
}

func main() {
    router := httprouter.New()
    router.Handle("OPTIONS", "/*name", Hello)
    router.Handle("GET", "/v1/search", Index)
    log.Fatal(http.ListenAndServe(":8080", router))
}

And here the request I made to try it:

$ curl -X OPTIONS localhost:8080/mynameisjavier
hello, /mynameisjavier!

$ curl localhost:8080/mynameisjavier
405 method not allowed

$ curl localhost:8080/v1/search
Welcome!

$ curl -X OPTIONS localhost:8080/v1/search
hello, /v1/search!

$ curl -X POST localhost:8080/v1/search
405 method not allowed

Thank you in advance for your response.

[ydnar]

Morning... I'll send an example (or a test) that illustrates the bug.

Randy

On Jan 6, 2015, at 3:39 AM, Javier Provecho Fernandez [email protected] wrote:

Excuse me @julienschmidt , but I can't replicate that bad behaviour, even changing the order of the handlers. Here is my code:

package main

import (
"fmt"
"github.com/julienschmidt/httprouter"
"log"
"net/http"
)

func Index(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
fmt.Fprint(w, "Welcome!\n")
}

func Hello(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
fmt.Fprintf(w, "hello, %s!\n", ps.ByName("name"))
}

func main() {
router := httprouter.New()
router.Handle("OPTIONS", "/*name", Hello)
router.Handle("GET", "/v1/search", Index)
log.Fatal(http.ListenAndServe(":8080", router))
}
And here the request I made to try it:

$ curl -X OPTIONS localhost:8080/mynameisjavier
hello, /mynameisjavier!

$ curl localhost:8080/mynameisjavier
405 method not allowed

$ curl localhost:8080/v1/search
Welcome!

$ curl -X OPTIONS localhost:8080/v1/search
hello, /v1/search!

$ curl -X POST localhost:8080/v1/search
405 method not allowed
Thank you in advance for your response.

�
Reply to this email directly or view it on GitHub.

[ydnar]

@javierprovecho you have to test against another route (that should 404 or 200). The test below should pass, but fails with the 405 code:

func TestRouterNotAllowed(t *testing.T) {
    handlerFunc := func(_ http.ResponseWriter, _ *http.Request, _ Params) {}

    router := New()
    router.GET("/handler", handlerFunc)
    router.Handle("OPTIONS", "/*path", handlerFunc)
    router.NotFound = func(_ http.ResponseWriter, _ *http.Request) {}

    testRoutes := []struct {
        method string
        route  string
        code   int
    }{
        {"GET", "/handler", 200},
        {"POST", "/handler", 405},
        {"OPTIONS", "/other", 200},
        {"GET", "/", 404}, // This test fails
    }

    for _, tr := range testRoutes {
        r, _ := http.NewRequest(tr.method, tr.route, nil)
        w := httptest.NewRecorder()
        router.ServeHTTP(w, r)
        if !(w.Code == tr.code) {
            t.Errorf("Handling %s %s failed. Want=%d Got=%d", tr.method, tr.route, tr.code, w.Code)
        }
    }
}

[ydnar]

For a bit more context, we chain multiple routers (Hitch instances, actually) together with different middleware. Router A’s NotFound handler is Router B:

apis := hitch.New()
apis.Use(httpgzip.Handler, AuthenticateClient, geoIP)
apis.Get("/v1/search", http.HandlerFunc(V1Search))
apis.Get("/v1/info", http.HandlerFunc(V1Info))

// Base handler
h := hitch.New()
h.Use(logger)
h.Get("/", http.RedirectHandler(apiDocsURL, 301))
h.Get("/favicon.ico", http.NotFoundHandler())
h.Options("/*path", http.HandlerFunc(CORSPreflight))
h.Next(apis.Handler())

return h.Handler()

[javierprovecho]

@ydnar we are probably discovering a new bug, because that test also fails with previous commit.

--- FAIL: TestRouterNotAllowedYDNAR (0.00s)
    ...
    notallowedYDNAR_test.go:33: Handling GET / failed. Want=404 Got=200
    ...

And the router is right, as said in README.md, https://github.com/julienschmidt/httprouter#catch-all-parameters, a route like router.Handle("OPTIONS", "/*path", handlerFunc) will even match, so if you request it with a different method, it should return 405. I would like both yours and @julienschmidt opinion on this.

Thank you @ydnar for both examples.

[ydnar]

I can think of two ways to solve this (there are probably more):

1. Skip wildcard routes when detecting MethodNotAllowed.
2. Treat OPTIONS requests as a special case. They should only exist for existing other routes (GET, POST, etc.). Browsers issue OPTIONS requests during the CORS preflight process.

The wildcard route we used in the example above is symptomatic of our desire to not repeat our routes. We could solve this with an OptionsHandler, similar to NotFound, or a helper func that automatically generates handlers for OPTIONS requests for other methods.

[julienschmidt]

... or just add an option to turn the 405 handling off.

The abuse of the NotFound handler to chain the router is not really intended and probably not a really common use case. But of course it is not my intention to make this use-case impossible.

[julienschmidt]

No comments?
I'd like to reintroduce the 405 support soon again.

[javierprovecho]

No comments from me actually because I had two hard weeks at university.

Your solution of turning on/off 405 method looks nice.

I'll try to push a new pull request soon.

As always, I'm reading your mails, but sometimes I'm not able to respond quickly.

—
Enviado desde Mailbox

On Tue, Jan 13, 2015 at 3:43 PM, Julien Schmidt [email protected]
wrote:

No comments?

I'd like to reintroduce the 405 support soon again.

Reply to this email directly or view it on GitHub:
#52 (comment)