From 8a7d32ac92aa05963280b6f62de8b56cd9ffde14 Mon Sep 17 00:00:00 2001 From: Carl Jackson Date: Sun, 30 Mar 2014 19:36:12 -0700 Subject: [PATCH] Get rid of name parameter in middleware The "name" parameter was originally a workaround for the fact that function pointers in Go never compare equal to each other (unless they are both nil). Unfortunately, this presents a pretty terrible interface to the end programmer, since they'll probably end up stuttering something like: mux.Use("MyMiddleware", MyMiddleware) Luckily, I found a workaround for doing function pointer equality (it doesn't even require "unsafe"!), so we can get rid of the name parameter for good. --- default.go | 26 ++++++--------- example/main.go | 4 +-- web/middleware.go | 46 ++++++++++---------------- web/middleware_test.go | 75 +++++++++++++++++------------------------- 4 files changed, 60 insertions(+), 91 deletions(-) diff --git a/default.go b/default.go index 70b8ed3..a3d3911 100644 --- a/default.go +++ b/default.go @@ -11,34 +11,28 @@ var DefaultMux *web.Mux func init() { DefaultMux = web.New() - DefaultMux.Use("RequestId", middleware.RequestId) - DefaultMux.Use("Logger", middleware.Logger) - DefaultMux.Use("Recoverer", middleware.Recoverer) - DefaultMux.Use("AutomaticOptions", middleware.AutomaticOptions) + DefaultMux.Use(middleware.RequestId) + DefaultMux.Use(middleware.Logger) + DefaultMux.Use(middleware.Recoverer) + DefaultMux.Use(middleware.AutomaticOptions) } // Append the given middleware to the default Mux's middleware stack. See the // documentation for web.Mux.Use for more informatino. -func Use(name string, middleware interface{}) { - DefaultMux.Use(name, middleware) +func Use(middleware interface{}) { + DefaultMux.Use(middleware) } // Insert the given middleware into the default Mux's middleware stack. See the // documentation for web.Mux.Insert for more informatino. -func Insert(name string, middleware interface{}, before string) error { - return DefaultMux.Insert(name, middleware, before) +func Insert(middleware, before interface{}) error { + return DefaultMux.Insert(middleware, before) } // Remove the given middleware from the default Mux's middleware stack. See the // documentation for web.Mux.Abandon for more informatino. -func Abandon(name string) error { - return DefaultMux.Abandon(name) -} - -// Retrieve the list of middleware from the default Mux's middleware stack. See -// the documentation for web.Mux.Middleware() for more information. -func Middleware() []string { - return DefaultMux.Middleware() +func Abandon(middleware interface{}) error { + return DefaultMux.Abandon(middleware) } // Add a route to the default Mux. See the documentation for web.Mux for more diff --git a/example/main.go b/example/main.go index 3499aa2..076948b 100644 --- a/example/main.go +++ b/example/main.go @@ -36,7 +36,7 @@ func main() { // Middleware can be used to inject behavior into your app. The // middleware for this application are defined in middleware.go, but you // can put them wherever you like. - goji.Use("PlainText", PlainText) + goji.Use(PlainText) // If the last character of a pattern is an asterisk, the path is // treated as a prefix, and can be used to implement sub-routes. @@ -44,7 +44,7 @@ func main() { // Goji's interfaces are completely composable. admin := web.New() goji.Handle("/admin/*", admin) - admin.Use("SuperSecure", SuperSecure) + admin.Use(SuperSecure) // Set up admin routes. Note that sub-routes do *not* mutate the path in // any way, so we need to supply full ("/admin" prefixed) paths. diff --git a/web/middleware.go b/web/middleware.go index b62c67a..12c6eae 100644 --- a/web/middleware.go +++ b/web/middleware.go @@ -12,7 +12,7 @@ const mPoolSize = 32 type mLayer struct { fn func(*C, http.Handler) http.Handler - name string + orig interface{} } type mStack struct { @@ -41,9 +41,8 @@ func (s *cStack) ServeHTTPC(c C, w http.ResponseWriter, r *http.Request) { s.m.ServeHTTP(w, r) } -func (m *mStack) appendLayer(name string, fn interface{}) { - var ml mLayer - ml.name = name +func (m *mStack) appendLayer(fn interface{}) { + ml := mLayer{orig: fn} switch fn.(type) { case func(http.Handler) http.Handler: unwrapped := fn.(func(http.Handler) http.Handler) @@ -60,9 +59,9 @@ func (m *mStack) appendLayer(name string, fn interface{}) { m.stack = append(m.stack, ml) } -func (m *mStack) findLayer(name string) int { +func (m *mStack) findLayer(l interface{}) int { for i, middleware := range m.stack { - if middleware.name == name { + if funcEqual(l, middleware.orig) { return i } } @@ -138,11 +137,11 @@ func (m *mStack) release(cs *cStack) { // Append the given middleware to the middleware stack. See the documentation // for type Mux for a list of valid middleware types. // -// No attempt is made to enforce the uniqueness of middleware names. -func (m *mStack) Use(name string, middleware interface{}) { +// No attempt is made to enforce the uniqueness of middlewares. +func (m *mStack) Use(middleware interface{}) { m.lock.Lock() defer m.lock.Unlock() - m.appendLayer(name, middleware) + m.appendLayer(middleware) m.invalidate() } @@ -150,9 +149,9 @@ func (m *mStack) Use(name string, middleware interface{}) { // the stack. See the documentation for type Mux for a list of valid middleware // types. Returns an error if no middleware has the name given by "before." // -// No attempt is made to enforce the uniqueness of names. If the insertion point -// is ambiguous, the first (outermost) one is chosen. -func (m *mStack) Insert(name string, middleware interface{}, before string) error { +// No attempt is made to enforce the uniqueness of middlewares. If the insertion +// point is ambiguous, the first (outermost) one is chosen. +func (m *mStack) Insert(middleware, before interface{}) error { m.lock.Lock() defer m.lock.Unlock() i := m.findLayer(before) @@ -160,7 +159,7 @@ func (m *mStack) Insert(name string, middleware interface{}, before string) erro return fmt.Errorf("web: unknown middleware %v", before) } - m.appendLayer(name, middleware) + m.appendLayer(middleware) inserted := m.stack[len(m.stack)-1] copy(m.stack[i+1:], m.stack[i:]) m.stack[i] = inserted @@ -169,17 +168,17 @@ func (m *mStack) Insert(name string, middleware interface{}, before string) erro return nil } -// Remove the given named middleware from the middleware stack. Returns an error -// if there is no middleware by that name. +// Remove the given middleware from the middleware stack. Returns an error if +// no such middleware can be found. // // If the name of the middleware to delete is ambiguous, the first (outermost) // one is chosen. -func (m *mStack) Abandon(name string) error { +func (m *mStack) Abandon(middleware interface{}) error { m.lock.Lock() defer m.lock.Unlock() - i := m.findLayer(name) + i := m.findLayer(middleware) if i < 0 { - return fmt.Errorf("web: unknown middleware %v", name) + return fmt.Errorf("web: unknown middleware %v", middleware) } copy(m.stack[i:], m.stack[i+1:]) @@ -188,14 +187,3 @@ func (m *mStack) Abandon(name string) error { m.invalidate() return nil } - -// Returns a list of middleware currently in use. -func (m *mStack) Middleware() []string { - m.lock.Lock() - defer m.lock.Unlock() - stack := make([]string, len(m.stack)) - for i, ml := range m.stack { - stack[i] = ml.name - } - return stack -} diff --git a/web/middleware_test.go b/web/middleware_test.go index 747b541..548e9b9 100644 --- a/web/middleware_test.go +++ b/web/middleware_test.go @@ -3,7 +3,6 @@ package web import ( "net/http" "net/http/httptest" - "reflect" "testing" "time" ) @@ -61,8 +60,8 @@ func TestSimple(t *testing.T) { ch := make(chan string) st := makeStack(ch) - st.Use("one", chanWare(ch, "one")) - st.Use("two", chanWare(ch, "two")) + st.Use(chanWare(ch, "one")) + st.Use(chanWare(ch, "two")) go simpleRequest(ch, st) assertOrder(t, ch, "one", "two", "router", "end") } @@ -72,10 +71,10 @@ func TestTypes(t *testing.T) { ch := make(chan string) st := makeStack(ch) - st.Use("one", func(h http.Handler) http.Handler { + st.Use(func(h http.Handler) http.Handler { return h }) - st.Use("two", func(c *C, h http.Handler) http.Handler { + st.Use(func(c *C, h http.Handler) http.Handler { return h }) } @@ -85,16 +84,16 @@ func TestAddMore(t *testing.T) { ch := make(chan string) st := makeStack(ch) - st.Use("one", chanWare(ch, "one")) + st.Use(chanWare(ch, "one")) go simpleRequest(ch, st) assertOrder(t, ch, "one", "router", "end") - st.Use("two", chanWare(ch, "two")) + st.Use(chanWare(ch, "two")) go simpleRequest(ch, st) assertOrder(t, ch, "one", "two", "router", "end") - st.Use("three", chanWare(ch, "three")) - st.Use("four", chanWare(ch, "four")) + st.Use(chanWare(ch, "three")) + st.Use(chanWare(ch, "four")) go simpleRequest(ch, st) assertOrder(t, ch, "one", "two", "three", "four", "router", "end") } @@ -104,18 +103,23 @@ func TestInsert(t *testing.T) { ch := make(chan string) st := makeStack(ch) - st.Use("one", chanWare(ch, "one")) - st.Use("two", chanWare(ch, "two")) + one := chanWare(ch, "one") + two := chanWare(ch, "two") + st.Use(one) + st.Use(two) go simpleRequest(ch, st) assertOrder(t, ch, "one", "two", "router", "end") - err := st.Insert("sloth", chanWare(ch, "sloth"), "squirrel") + err := st.Insert(chanWare(ch, "sloth"), chanWare(ch, "squirrel")) if err == nil { t.Error("Expected error when referencing unknown middleware") } - st.Insert("middle", chanWare(ch, "middle"), "two") - st.Insert("start", chanWare(ch, "start"), "one") + st.Insert(chanWare(ch, "middle"), two) + err = st.Insert(chanWare(ch, "start"), one) + if err != nil { + t.Fatal(err) + } go simpleRequest(ch, st) assertOrder(t, ch, "start", "one", "middle", "two", "router", "end") } @@ -125,51 +129,34 @@ func TestAbandon(t *testing.T) { ch := make(chan string) st := makeStack(ch) - st.Use("one", chanWare(ch, "one")) - st.Use("two", chanWare(ch, "two")) - st.Use("three", chanWare(ch, "three")) + one := chanWare(ch, "one") + two := chanWare(ch, "two") + three := chanWare(ch, "three") + st.Use(one) + st.Use(two) + st.Use(three) go simpleRequest(ch, st) assertOrder(t, ch, "one", "two", "three", "router", "end") - st.Abandon("two") + st.Abandon(two) go simpleRequest(ch, st) assertOrder(t, ch, "one", "three", "router", "end") - err := st.Abandon("panda") + err := st.Abandon(chanWare(ch, "panda")) if err == nil { t.Error("Expected error when deleting unknown middleware") } - st.Abandon("one") - st.Abandon("three") + st.Abandon(one) + st.Abandon(three) go simpleRequest(ch, st) assertOrder(t, ch, "router", "end") - st.Use("one", chanWare(ch, "one")) + st.Use(one) go simpleRequest(ch, st) assertOrder(t, ch, "one", "router", "end") } -func TestMiddlewareList(t *testing.T) { - t.Parallel() - - ch := make(chan string) - st := makeStack(ch) - st.Use("one", chanWare(ch, "one")) - st.Use("two", chanWare(ch, "two")) - st.Insert("mid", chanWare(ch, "mid"), "two") - st.Insert("before", chanWare(ch, "before"), "mid") - st.Abandon("one") - - m := st.Middleware() - if !reflect.DeepEqual(m, []string{"before", "mid", "two"}) { - t.Error("Middleware list was not as expected") - } - - go simpleRequest(ch, st) - assertOrder(t, ch, "before", "mid", "two", "router", "end") -} - // This is a pretty sketchtacular test func TestCaching(t *testing.T) { ch := make(chan string) @@ -225,7 +212,7 @@ func TestContext(t *testing.T) { pool: make(chan *cStack, mPoolSize), router: HandlerFunc(router), } - st.Use("one", func(c *C, h http.Handler) http.Handler { + st.Use(func(c *C, h http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { if c.Env != nil || c.UrlParams != nil { t.Error("Expected a clean context") @@ -238,7 +225,7 @@ func TestContext(t *testing.T) { return http.HandlerFunc(fn) }) - st.Use("two", func(c *C, h http.Handler) http.Handler { + st.Use(func(c *C, h http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { if c.Env == nil { t.Error("Expected env from last middleware")