SkipTrailingSlash

[go-chi]

SkipTrailingSlash

[pkieltyka]

hey @lofcek, it's a good point and the trailing slash has been a bit of a pain. I added a test case 53adbc4 that can show you how to do it now. The reason this works is because routes are layered on top of each other when build the routing tree.

I do agree I should build something standard into the mux. However, there are usually two options, many people will want to redirect /accounts/123/ to /accounts/123 and others will want the trailing slash to be treated to the same handler.

one idea is instead of changing the Mux{} object, because, its more of an underlying struct and its best to use the Router interface, and we don't really support configuration options (not that we couldn't), but it limits extensibility..

I think instead... I could update the test code to support something like..

func TestMuxTrailingSlash(t *testing.T) {
    r := NewRouter()
    r.NotFound(func(ctx context.Context, w http.ResponseWriter, r *http.Request) {
        w.WriteHeader(404)
        w.Write([]byte("nothing here"))
    })

    subRoutes := NewRouter()
    indexHandler := func(ctx context.Context, w http.ResponseWriter, r *http.Request) {
        accountID := URLParam(ctx, "accountID")
        w.Write([]byte(accountID))
    }
    subRoutes.Get("/", indexHandler)

    // ***NOTE*** the middleware before the mount
    r.Mount("/accounts/:accountID", middleware.StripSlashes, subRoutes)

    // or..
    // r.Mount("/accounts/:accountID", middleware.RedirectSlashes, subRoutes)

    ts := httptest.NewServer(r)
    defer ts.Close()

    if resp := testRequest(t, ts, "GET", "/accounts/admin", nil); resp != "admin" {
        t.Fatalf(resp)
    }
    if resp := testRequest(t, ts, "GET", "/accounts/admin/", nil); resp != "admin" {
        t.Fatalf(resp)
    }
    if resp := testRequest(t, ts, "GET", "/nothing-here", nil); resp != "nothing here" {
        t.Fatalf(resp)
    }
}

so, the introduction is to add middleware.StripSlashes and middleware.RedirectSlashes

...alternatively, there could be an option on the chi routing Context, and a middleware could be used to flip the flags on the routing context, and pass along the request

[pkieltyka]

btw.. this works..

func StripSlashes(next Handler) Handler {
    fn := func(ctx context.Context, w http.ResponseWriter, r *http.Request) {
        path := r.URL.Path
        if path[len(path)-1] == '/' {
            rctx := RouteContext(ctx)
            rctx.RoutePath = path[:len(path)-1]
        }
        next.ServeHTTPC(ctx, w, r)
    }
    return HandlerFunc(fn)
}

I'll be packaging this into middleware pkg soon enuf once I write the RedirectSlashes middleware, but I gotta head out for now

[lofcek]

That is nearly what I had, with few notes.

• Correct name should be StripTrailingSlash, because is it strips only one slash and only at the of the path.
• You should not simply use rctx.RoutePath = path[:len(path)-1]. That work correctly on root path, but not if RouteContext was already used (in subrouter after Mount).
• in if statement you forgot ctx = rctx
• I'm not sure, but is it correct if path refers to root (path = "/")

[lofcek]

And I also didn't realize it immediately- I think change data inside Context during execution is a bit confusing. I think it should be better to create a new chi.Context, don't change previous one.

[pkieltyka]

Thanks for the thoughts but I don't fully agree. You're right about the root path, I'll make a check but if you have other opinions, please submit a PR or a breaking test case. Ie. rctx is a pointer and doesn't need to set to ctx again, since it already on the context chain, and we don't need a new chi.Context

On Jun 20, 2016, at 7:30 AM, lofcek [email protected] wrote:

And I also didn't realize it immediately- I think change data inside Context during execution is a bit confusing. I think it should be better to create a new chi.Context, don't change previous one.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[pkieltyka]

thanks for the feedback. I've added support with b74aedb