Extending web.C with typed response data?

[cbroglie]

Often in middleware, typically for the sake of logging, access to the http status code and content length of the response are required. And the way this is always done is to wrap the ResponseWriter that gets passed to ServeHTTP.

This is wasteful both from the developer time creating wrappers, and at runtime processing multiple wrappers (since each middleware that needs it implements it). In my project I've made a slight improvement which has a single middleware which wraps the ResponseWriter and stores the data in the Env map, making it available to other middleware.

But this is still not shared with the goji default logger middleware. And it's untyped. What do you think about extending web.C to include typed fields (I'm thinking about http status and content length but potentially others), and a new default middleware which wraps ResponseWriter to populate those values?

[zenazn]

I think the web.C structure is set in stone at this point. Which is not to say it's perfect—it isn't—but API stability seems far more important than most other things.

If you haven't seen it, I've written a "standard" http.ResponseWriter proxy object that you may find useful in preventing duplicated effort.

But to address your point, I really don't expect many "production" apps to use the logger middleware that ships with Goji—it's deliberately a barebones, works-well-enough-that-you-can-prototype-with-it solution. The reason for this is that writing a logger that is configurable enough to suit everyone's needs is a poor use of everyone's time, and writing new request loggers simply isn't that hard. If you're looking to standardize around a single http.ResponseWriter proxy that caches information about the request in the web.C environment, I'd encourage you to do so by writing your own logger, not by modifying Goji. (And since you're not the first person to suggest this, I've added a note to the documentation).

As for why the "standard" middleware don't follow this pattern, I think it creates weird spooky action at a distance—the data one middleware layer uses would be created by another, perhaps-not-present layer, and things could fail in weird and unpredictable ways when middleware were rearranged. I think for most people, making middleware understandable is far more important than the handful of nanoseconds you save by cutting one ResponseWriter proxy layer.

Hope this all makes sense!

[cbroglie]

Yes, I saw WriterProxy, and I agree about middleware not depending on other middleware that may or may not be present. I was thinking in terms of an internal middleware which goji inserts automatically to populate the values in web.C. Since it's purely additive, I'm not sure how this hurts API stability.

I was also asking to gauge the feelings about extending web.C in general. Another addition I think would be valuable would be to expose the route that was matched. Being able to log something like /app/:appId/user/:userid is preferable to /app/1111/user/2222, for the sake of log aggregation using tools like Splunk. Since the router is already setting c.URLParams, this seems like a natural addition to me.

p.s. I'm happy to submit PRs for my proposed changes - just wanted to confirm/deny whether they'd be accepted before doing the work.

[zenazn]

The other addition feels like #76 to me, which doesn't require modifying web.C. I hope to get to that some time in the coming weeks, but like all time estimates from engineers, this one too is probably a lie :)

Maybe API stability is the wrong way of putting it, but I think the conceptual simplicity of web.C is part of the API it exposes, and I think the bar for adding new fields to web.C is going to be extraordinarily high.

I guess a good question for you is why you think extending web.C is necessary. The reason web.C.Env exists is to allow any person to attach any data they like to any request. While there's a small list of things which do need router support (exposing the matched route, for one, or returning a list of acceptable HTTP methods), I'd like it to be the case that most things people do with Goji don't have to go through me, and don't require modifying Goji core. If this isn't the case, and if modifying either the router or web.C is necessary to do things with Goji, that's obviously a failing on my part that I'd like to know about.

I do think it's often easier to discuss technical topics by talking about specific pieces of code, so I'd be delighted if you had PRs you'd be willing to share with me. I'll warn you ahead of time that changes that significantly change Goji's scope are unlikely to be merged, but it might nonetheless be useful to help frame a discussion or help me understand what frustrates you about Goji.

[cbroglie]

I certainly understand and respect your desire to limit additions to web.C. And I agree users can fully leverage web.C.Env to attach any data they'd like. But I think my preference would be to leave that map to users, and have Goji publish data from its internals to separate, typed fields.

You made the decision to create a separate web.C.Params map; going to the other extreme, you could argue that you should remove that and just make it another named key in web.C.Env. But having separate fields makes things clearer, adds compile time type checks, and autocompletion for folks using editors. In my opinion, the params, matched route, and list of acceptable HTTP methods should all be separate fields, and not be part of web.C.Env.

The above comments are specific to functionality that Goji needs to expose, since it's logic handled in the router. My original post was about exposing the HTTP status and bytes written, which, admittedly, middleware can get access to using WriterProxy and a couple lines of code. The argument for Goji providing those values is certainly less compelling than it is for routing data, but there is still a cost associated with users having to implement such a solution (it's mostly around discovering the solution, as users coming from other frameworks - go-restful in my case - may be used to those values being available). So definitely more of a gray area, but if everyone needs to solve a problem themselves, there is still some value in solving it once for everyone.

[zenazn]

This issue hasn't seen any activity in a few months, so I'm going to close it out (if that's okay).

While I still think it's extremely unlikely I'll change web.C, a good takeaway for me is that a lot of Goji's capabilities aren't very discoverable. I'm working on revamping Goji's website, which will hopefully make this a lot better in the future.

You mentioned that the chosen route was an example of something that you might want exposed in web.C, and I've recently added a mechanism to expose that, albeit in web.C.Env (https://github.com/zenazn/goji/commit/707cf7e127e738386a38fa9fda3834315892b434). Let me know if there's any other router internal state that you're interested in seeing!

[cbroglie]

Thanks! My opinion is still that it would be better accessed as a standalone field, but exposing the state in web.C.Env certainly addresses my use cases.

Could you share a short code example for making use of mux.Router? I tried adding it as a middleware and am getting a stack overflow.

[cbroglie]

I see TestRouterMiddleware now - will look at what I'm doing wrong.

[mredivo]

@cbroglie I'm interested in what you come up with, too.

I have a workaround, but it has a theoretical failure mode I'm not happy about.

[cbroglie]

It looks like this causes a stack overflow if you use it with nested routers. Here is my existing test code which demonstrates the issue (GetRoutePath was my workaround logic):

func TestRoutePathWithWildcard(t *testing.T) {
    id := ""
    routePath := ""
    middleware := func(c *web.C, h http.Handler) http.Handler {
        fn := func(w http.ResponseWriter, r *http.Request) {
            h.ServeHTTP(w, r)
            id = c.URLParams["id"]
            //routePath = GetRoutePath(*c, r)
            routePath = "implement me"
        }
        return http.HandlerFunc(fn)
    }
    handler := func(w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Content-Type", "text/plain; charset=utf-8")
        w.WriteHeader(http.StatusOK)
        fmt.Fprintln(w, "OK")
    }
    root, m := web.New(), web.New()
    root.Use(root.Router)
    root.Handle("/prefix/*", m)
    m.Use(middleware)
    m.Handle("/prefix/id/:id", handler)
    ts := httptest.NewServer(root)
    defer ts.Close()
    _, err := http.Get(ts.URL + "/prefix/id/1")
    if err != nil {
        t.Fatal(err)
    }

    expectedRoutePath := "/prefix/id/:id"
    if routePath != expectedRoutePath {
        t.Fatalf("Expected route path `%s`, got `%s`\n", expectedRoutePath, routePath)
    }
    expectedId := "1"
    if id != expectedId {
        t.Fatalf("Expected id %s, got %s\n", expectedId, id)
    }
}

Stack will look something like this:

goroutine 17 [stack growth]:
runtime.convT2E(0x261e60, 0xc2281001f8, 0x0, 0x0)
    /usr/local/go/src/runtime/iface.go:134 fp=0xc228100198 sp=0xc228100190
github.com/zenazn/goji/web.GetMatch(0xc20803b950, 0xc20803b890, 0x0, 0x0, 0x0, 0x0)
    /src/github.com/zenazn/goji/web/match.go:30 +0xa4 fp=0xc228100250 sp=0xc228100198
github.com/zenazn/goji/web.(*router).route(0xc208010658, 0xc212b05ce0, 0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/router.go:115 +0x44 fp=0xc2281002f0 sp=0xc228100250
github.com/zenazn/goji/web.func·002(0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/middleware.go:88 +0x60 fp=0xc228100330 sp=0xc2281002f0
net/http.HandlerFunc.ServeHTTP(0xc212b06520, 0x596530, 0xc2080643c0, 0xc208032820)
    /usr/local/go/src/net/http/server.go:1265 +0x41 fp=0xc228100350 sp=0xc228100330
mypackage/go-zsr.func·004(0x596530, 0xc2080643c0, 0xc208032820)
    /src/mypackage/go-zsr/util_test.go:56 +0x78 fp=0xc2281003c8 sp=0xc228100350
net/http.HandlerFunc.ServeHTTP(0xc212b05d10, 0x596530, 0xc2080643c0, 0xc208032820)
    /usr/local/go/src/net/http/server.go:1265 +0x41 fp=0xc2281003e8 sp=0xc2281003c8
github.com/zenazn/goji/web.(*cStack).ServeHTTPC(0xc212b05ce0, 0xc20803b950, 0xc20803b890, 0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/middleware.go:50 +0x91 fp=0xc228100420 sp=0xc2281003e8
github.com/zenazn/goji/web.(*Mux).ServeHTTPC(0xc208010620, 0xc20803b950, 0xc20803b890, 0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/mux.go:53 +0x74 fp=0xc228100460 sp=0xc228100420
github.com/zenazn/goji/web.(*router).route(0xc208010658, 0xc212b05c80, 0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/router.go:119 +0x144 fp=0xc228100500 sp=0xc228100460
github.com/zenazn/goji/web.func·002(0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/middleware.go:88 +0x60 fp=0xc228100540 sp=0xc228100500
net/http.HandlerFunc.ServeHTTP(0xc212b06500, 0x596530, 0xc2080643c0, 0xc208032820)
    /usr/local/go/src/net/http/server.go:1265 +0x41 fp=0xc228100560 sp=0xc228100540
mypackage/go-zsr.func·004(0x596530, 0xc2080643c0, 0xc208032820)
    /src/mypackage/go-zsr/util_test.go:56 +0x78 fp=0xc2281005d8 sp=0xc228100560
net/http.HandlerFunc.ServeHTTP(0xc212b05cb0, 0x596530, 0xc2080643c0, 0xc208032820)
    /usr/local/go/src/net/http/server.go:1265 +0x41 fp=0xc2281005f8 sp=0xc2281005d8
github.com/zenazn/goji/web.(*cStack).ServeHTTPC(0xc212b05c80, 0xc20803b950, 0xc20803b890, 0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/middleware.go:50 +0x91 fp=0xc228100630 sp=0xc2281005f8
github.com/zenazn/goji/web.(*Mux).ServeHTTPC(0xc208010620, 0xc20803b950, 0xc20803b890, 0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/mux.go:53 +0x74 fp=0xc228100670 sp=0xc228100630
github.com/zenazn/goji/web.(*router).route(0xc208010658, 0xc212b05c20, 0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/router.go:119 +0x144 fp=0xc228100710 sp=0xc228100670
github.com/zenazn/goji/web.func·002(0x596530, 0xc2080643c0, 0xc208032820)
    /src/github.com/zenazn/goji/web/middleware.go:88 +0x60 fp=0xc228100750 sp=0xc228100710
net/http.HandlerFunc.ServeHTTP(0xc212b064e0, 0x596530, 0xc2080643c0, 0xc208032820)
    /usr/local/go/src/net/http/server.go:1265 +0x41 fp=0xc228100770 sp=0xc228100750
mypackage/go-zsr.func·004(0x596530, 0xc2080643c0, 0xc208032820)

[zenazn]

Ugh—sorry about that. @cbroglie could you open a new issue to track that bug?

[cbroglie]

Sure - filed #120

[cbroglie]

@mredivo I was able to leverage this to access the matched route, as desired. I believe I was using the same workaround as you.

Old method:

func GetRoutePath(c *web.C, r *http.Request) string {
    // Replace the resource IDs in the path with their variable names. This
    // solution is a hack, and will fail if a value occurs multiple times in the
    // url. But it's the best we can do for now.
    //
    // See https://github.com/zenazn/goji/pull/70 for some additional background
    // and a proposed solution.
    //
    path := r.URL.Path
    for k, v := range c.URLParams {
        // If the route contains a wildcard - say "/prefix/*" - and the request
        // is for "/prefix/test", then Goji will add a key called "*" with the
        // value "/test" to URLParams. Need to ignore it here.
        if k != "*" {
            path = strings.Replace(path, "/"+v, "/:"+k, 1)
        }
    }
    return path
}

New method:

func GetRoutePath(c web.C, r *http.Request) string {
    return fmt.Sprintf("%s", web.GetMatch(c).RawPattern())
}

FYI, there is a gotcha described in #120 about needing sub routers to use the Mux.Router middleware as well as the root router.