SkipTrailingSlash

[lofcek]

In project, that I'm writing for some handlers I need to remove trailing slash (REST server, where I cannot influence form of requests). I spent a few hours yesterday, but I have not find any good looking solution.

Problem, that when I handle something inside r.Group or r.Mount, inside this handler next handle to root does not work.
Example: In your example account code I don't want be able to handle only
curl 127.0.0.1:3333/accounts/123
but also
curl 127.0.0.1:3333/accounts/123/
with the same handler.

Inside accountsRouter there is no good looking solution how to use the same handler for both cases.
r.Get("/", getAccount)
handles only the first form. When I tried to change it into r.Get("//", ...) it does not handle anything.

Possible solution (if I understand well are):
a) Use two handlers for r.Route("/:accountID", ...) and r.Route("/:accountID/", ...) with same "next" function. That looks confusing, and also I can't use anonymous function. Also when some trailing slashes can appears in many handlers, I have to repeat such bad code for each handler.

b) Write middleware SkipTrailingSlash, that checks, whether path ends with /, and if it, just remove it. I made it, it works fine, but I have to modify chi.Context, because routeCtxKey is not visible neither from my application nor chi.middleware. I had to change it, but it is not nice, because main idea was publish internal routing mechanism and make it visible for everyone.

c) Add flag SkipTrailingSlash, into Mux. I thing that this is the best idea yet, but I welcome any better proposals. I could write it, that is not problem, but do you think it is a good idea?

[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 notifications@github.com 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