Golang Api Only Matches Last Route

Question

I have a golang api application. I've defined a set of routes and handlers. However, the mux router only ever returns the last route.

When I request /api/info I get this in my logging:

9:0:38 app | 2018/02/05 09:00:38 GET /api/info Users Create 308.132µs

Why is that routing to the wrong route?

routing package:

// NewRouter establishes the root application router
func NewRouter(context *config.ApplicationContext, routes Routes, notFoundHandler http.HandlerFunc) *mux.Router {
    router := mux.NewRouter()

    router.NotFoundHandler = notFoundHandler

    for _, route := range routes {
        router.
            PathPrefix("/api").
            Methods(route.Method).
            Path(route.Pattern).
            Name(route.Name).
            // TODO: fix HandlerFunc. Right now, it is overriding previous routes and setting a single handler for all
            // this means that the last route is the only router with a handler
            HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
                logRoute(setJSONHeader(route.HandlerFunc), route.Name)(context, w, r)
            })

    }

    return router
}

func logRoute(inner ContextHandlerFunc, name string) ContextHandlerFunc {
    return func(c *config.ApplicationContext, w http.ResponseWriter, r *http.Request) {
        start := time.Now()

        inner(c, w, r)

        log.Printf(
            "%s\t%s\t%s\t%s",
            r.Method,
            r.RequestURI,
            name,
            time.Since(start),
        )
    }
}

func setJSONHeader(inner ContextHandlerFunc) ContextHandlerFunc {
    return func(c *config.ApplicationContext, w http.ResponseWriter, r *http.Request) {
        w.Header().Set("Content-Type", "application/json")
        inner(c, w, r)
    }
}

main package:

var context = config.ApplicationContext{
    Database: database.NewDatabase().Store,
}

var routes = router.Routes{
    router.Route{"Info", "GET", "/info", handlers.InfoShow},
    router.Route{"Users Create", "POST", "/users/create", handlers.UsersCreate},
}

func main() {    
    notFoundHandler := handlers.Errors404
    router := router.NewRouter(&context, routes, notFoundHandler)

    port := os.Getenv("PORT")

    log.Fatal(http.ListenAndServe(":"+port, router))
}

If I visit /api/info it will attempt to call a POST to /users/create. However, if I remove the second route, it will correctly route to the InfoShow handler.

Why is mux overriding the first route? I'm fairly certain there's something wrong with

HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
    logRoute(setJSONHeader(route.HandlerFunc), route.Name)(context, w, r)
}) 

but I'm not sure why that would cause it to map over the first route.

Ideas?

Answer 1

Reading through your code and gorilla/mux, I think I know the issue. You're using the for loop variable route, and specifically its field HanderFunc, in the function literal, but because of how function literals work, the value of that field is not evaluated until the that function literal is called. In Go, the second variable in a range loop is reused on each iteration, rather than created anew, and so after the for loop, if it's still in scope of anything (like your function literal), it will contain the value of the last loop iteration. Here's an example of what I mean:

https://play.golang.org/p/Xx62tuwhtgG

package main

import (
    "fmt"
)

func main() {
    var funcs []func()
    ints := []int{1, 2, 3, 4, 5}

    // How you're doing it
    for i, a := range ints {
        fmt.Printf("Loop i: %v, a: %v\n", i, a)
        funcs = append(funcs, func() {
            fmt.Printf("Lambda i: %v, a: %v\n", i, a)
        })
    }

    for _, f := range funcs {
        f()
    }

    fmt.Println("-------------")

    // How you *should* do it
    funcs = nil
    for i, a := range ints {
        i := i
        a := a
        fmt.Printf("Loop i: %v, a: %v\n", i, a)
        funcs = append(funcs, func() {
            fmt.Printf("Lambda i: %v, a: %v\n", i, a)
        })
    }

    for _, f := range funcs {
        f()
    }
}

In the first example, i and a are being reused on each loop iteration, and aren't evaluated for their values in the lambda (function literal) until that lambda is actually called (by the funcs loop). To fix that, you can shadow a and i by redeclaring them inside the scope of the loop iteration (but outside the lambda's scope). This makes a separate copy for each iteration, to avoid issues with reuse of the same variable.

Specifically for your code, if you change your code to the following, it should work:

for _, route := range routes {
    route := route // make a copy of the route for use in the lambda
    // or alternatively, make scoped vars for the name and handler func

    router.
        PathPrefix("/api").
        Methods(route.Method).
        Path(route.Pattern).
        Name(route.Name).
        // TODO: fix HandlerFunc. Right now, it is overriding previous routes and setting a single handler for all
        // this means that the last route is the only router with a handler
        HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
            logRoute(setJSONHeader(route.HandlerFunc), route.Name)(context, w, r)
        })
}