gorilla / mux

Package gorilla/mux is a powerful HTTP router and URL matcher for building Go web servers with 🦍
https://gorilla.github.io
BSD 3-Clause "New" or "Revised" License
20.92k stars 1.85k forks source link

Tests for MethodNotAllowed error #774

Open rodionovv opened 3 weeks ago

rodionovv commented 3 weeks ago

What type of PR is this? (check all applicable)

Description

Hello!

This pr adds some tests for checking that MethodNotAllowed error is returned correctly in some cases. All new tests are passing in current main branch state. But some of them are not passing in v1.8.1 (b4617d0).

In released version 404 can be returned instead of 405 when after adding required route, another one with different method and url was added. And also there is a difference in how the route was created. It is also important to make a request to required path with Method used in the second route. Sending request with every new and different method that was added after required route with required method will return 404 instead of 405 . Sending requests with method that hasn't been previously added will work fine and return 405. Example:

func TestMethodNotAllowed_diffMethods_bothHandleFuncs(t *testing.T) {
    handler := func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }
    router := NewRouter()

    router.HandleFunc("/thing", handler).Methods(http.MethodPost)
    router.HandleFunc("/something", handler).Methods(http.MethodGet)

    w := NewRecorder()
    req := newRequest(http.MethodGet, "/thing")

    router.ServeHTTP(w, req)

    if w.Code != http.StatusMethodNotAllowed {
        t.Fatalf("Expected status code 405 (got %d)", w.Code)
    }
}

This test is passing in both versions. But next example is not passing in v1.18.1:

func TestMethodNotAllowed_diffMethods_handleFuncMethods(t *testing.T) {
    handler := func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }
    router := NewRouter()

    router.HandleFunc("/thing", handler).Methods(http.MethodPost)
    router.Methods(http.MethodGet).Path("/something").Handler(http.HandlerFunc(handler))

    w := NewRecorder()
    req := newRequest(http.MethodGet, "/thing")

    router.ServeHTTP(w, req)

    if w.Code != http.StatusMethodNotAllowed {
        t.Fatalf("Expected status code 405 (got %d)", w.Code)
    }
}

The only change here is that HandleFunc().Methods() is replaced with Methods().Path().Handler(). So different way of creation of router leads to an error. The difference in v1.18.1 in produced routes was in order of matchers in inner array. Thats want I've mentioned. Wasn't able to identify how this influenced the result. Also I found different ways of how routes can be produced with this error:

router.Methods(http.MethodPost).Path("/thing").Handler(http.HandlerFunc(handler))
router.Methods(http.MethodGet).Path("/something").Handler(http.HandlerFunc(handler)) 

router.HandleFunc("/thing", handler).Methods(http.MethodPost)
router.Methods(http.MethodGet).Path("/something").Handler(http.HandlerFunc(handler))

router.Path("/thing").Methods(http.MethodPost).Handler(http.HandlerFunc(handler))
router.Methods(http.MethodGet).Path("/something").Handler(http.HandlerFunc(handler)) 

Currently provided tests do not cover cases of route possible varying way of creation except HandleFunc . So i added them, trying to check all possible ways of adding routes.

Despite issue being fixed new tests can ensure that it wont return later.

If I've missed something, let me know please.

Related Tickets & Documents

Added/updated tests?

Run verifications and test