Skip to content

Panic when a route is added with different path params #1762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -462,17 +462,20 @@ func TestContextCookie(t *testing.T) {

func TestContextPath(t *testing.T) {
e := New()
r := e.Router()
b := NewRouter(e)

b.Add(http.MethodGet, "/users/:id", "", nil)
b.Add(http.MethodGet, "/users/:uid/files/:fid", "", nil)

r, _ := b.Build()

r.Add(http.MethodGet, "/users/:id", nil)
c := e.NewContext(nil, nil)
r.Find(http.MethodGet, "/users/1", c)

assert := testify.New(t)

assert.Equal("/users/:id", c.Path())

r.Add(http.MethodGet, "/users/:uid/files/:fid", nil)
c = e.NewContext(nil, nil)
r.Find(http.MethodGet, "/users/1/files/1", c)
assert.Equal("/users/:uid/files/:fid", c.Path())
Expand All @@ -498,8 +501,7 @@ func TestContextPathParam(t *testing.T) {

func TestContextGetAndSetParam(t *testing.T) {
e := New()
r := e.Router()
r.Add(http.MethodGet, "/:foo", func(Context) error { return nil })
e.Add(http.MethodGet, "/:foo", func(Context) error { return nil })
req := httptest.NewRequest(http.MethodGet, "/:foo", nil)
c := e.NewContext(req, nil)
c.SetParamNames("foo")
Expand Down Expand Up @@ -672,17 +674,20 @@ func BenchmarkContext_Store(b *testing.B) {

func TestContextHandler(t *testing.T) {
e := New()
r := e.Router()
b := new(bytes.Buffer)
b := NewRouter(e)
buff := new(bytes.Buffer)

r.Add(http.MethodGet, "/handler", func(Context) error {
_, err := b.Write([]byte("handler"))
b.Add(http.MethodGet, "/handler", "", func(Context) error {
_, err := buff.Write([]byte("handler"))
return err
})

r, _ := b.Build()

c := e.NewContext(nil, nil)
r.Find(http.MethodGet, "/handler", c)
err := c.Handler()(c)
testify.Equal(t, "handler", b.String())
testify.Equal(t, "handler", buff.String())
testify.NoError(t, err)
}

Expand Down
94 changes: 46 additions & 48 deletions echo.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ Learn more at https://echo.labstack.com
package echo

import (
"bytes"
stdContext "context"
"crypto/tls"
"errors"
Expand Down Expand Up @@ -75,8 +74,10 @@ type (
premiddleware []MiddlewareFunc
middleware []MiddlewareFunc
maxParam *int
router *Router
routers map[string]*Router
routerBuilder RouteBuilder
routersBuilder map[string]RouteBuilder
router Router
routers map[string]Router
notFoundHandler HandlerFunc
pool sync.Pool
Server *http.Server
Expand All @@ -97,13 +98,6 @@ type (
ListenerNetwork string
}

// Route contains a handler and information for matching against requests.
Route struct {
Method string `json:"method"`
Path string `json:"path"`
Name string `json:"name"`
}

// HTTPError represents an error that occurred while handling a request.
HTTPError struct {
Code int `json:"-"`
Expand Down Expand Up @@ -320,8 +314,8 @@ func New() (e *Echo) {
e.pool.New = func() interface{} {
return e.NewContext(nil, nil)
}
e.router = NewRouter(e)
e.routers = map[string]*Router{}
e.routerBuilder = NewRouter(e)
e.routersBuilder = map[string]RouteBuilder{}
return
}

Expand All @@ -338,15 +332,33 @@ func (e *Echo) NewContext(r *http.Request, w http.ResponseWriter) Context {
}

// Router returns the default router.
func (e *Echo) Router() *Router {
func (e *Echo) Router() Router {
return e.router
}

// Routers returns the map of host => router.
func (e *Echo) Routers() map[string]*Router {
func (e *Echo) Routers() map[string]Router {
return e.routers
}

//BuildRouters builds the internal Routers
func (e *Echo) BuildRouters() error {
var err error
if e.router, err = e.routerBuilder.Build(); err != nil {
return err
}
e.routers = make(map[string]Router)
for host, routeBuilder := range e.routersBuilder {
router, err := routeBuilder.Build()
if err == nil {
e.routers[host] = router
} else {
return err
}
}
return nil
}

// DefaultHTTPErrorHandler is the default HTTP error handler. It sends a JSON response
// with status code.
func (e *Echo) DefaultHTTPErrorHandler(err error, c Context) {
Expand Down Expand Up @@ -530,17 +542,11 @@ func (e *Echo) File(path, file string, m ...MiddlewareFunc) *Route {

func (e *Echo) add(host, method, path string, handler HandlerFunc, middleware ...MiddlewareFunc) *Route {
name := handlerName(handler)
router := e.findRouter(host)
router.Add(method, path, func(c Context) error {
router := e.findRouterBuilder(host)
r, _ := router.Add(method, path, name, func(c Context) error {
h := applyMiddleware(handler, middleware...)
return h(c)
})
r := &Route{
Method: method,
Path: path,
Name: name,
}
e.router.routes[method+path] = r
return r
}

Expand All @@ -552,7 +558,7 @@ func (e *Echo) Add(method, path string, handler HandlerFunc, middleware ...Middl

// Host creates a new router group for the provided host and optional host-level middleware.
func (e *Echo) Host(name string, m ...MiddlewareFunc) (g *Group) {
e.routers[name] = NewRouter(e)
e.routersBuilder[name] = NewRouter(e)
g = &Group{host: name, echo: e}
g.Use(m...)
return
Expand All @@ -578,34 +584,12 @@ func (e *Echo) URL(h HandlerFunc, params ...interface{}) string {

// Reverse generates an URL from route name and provided parameters.
func (e *Echo) Reverse(name string, params ...interface{}) string {
uri := new(bytes.Buffer)
ln := len(params)
n := 0
for _, r := range e.router.routes {
if r.Name == name {
for i, l := 0, len(r.Path); i < l; i++ {
if (r.Path[i] == ':' || r.Path[i] == '*') && n < ln {
for ; i < l && r.Path[i] != '/'; i++ {
}
uri.WriteString(fmt.Sprintf("%v", params[n]))
n++
}
if i < l {
uri.WriteByte(r.Path[i])
}
}
break
}
}
return uri.String()
return e.routerBuilder.Reverse(name, params...)
}

// Routes returns the registered routes.
func (e *Echo) Routes() []*Route {
routes := make([]*Route, 0, len(e.router.routes))
for _, v := range e.router.routes {
routes = append(routes, v)
}
routes, _ := e.routerBuilder.Routes()
return routes
}

Expand Down Expand Up @@ -757,6 +741,11 @@ func (e *Echo) configureServer(s *http.Server) (err error) {
e.colorer.Printf(banner, e.colorer.Red("v"+Version), e.colorer.Blue(website))
}

//Build Routers
if err := e.BuildRouters(); err != nil {
return err
}

if s.TLSConfig == nil {
if e.Listener == nil {
e.Listener, err = newListener(s.Addr, e.ListenerNetwork)
Expand Down Expand Up @@ -912,7 +901,7 @@ func WrapMiddleware(m func(http.Handler) http.Handler) MiddlewareFunc {
}
}

func (e *Echo) findRouter(host string) *Router {
func (e *Echo) findRouter(host string) Router {
if len(e.routers) > 0 {
if r, ok := e.routers[host]; ok {
return r
Expand All @@ -921,6 +910,15 @@ func (e *Echo) findRouter(host string) *Router {
return e.router
}

func (e *Echo) findRouterBuilder(host string) RouteBuilder {
if len(e.routersBuilder) > 0 {
if r, ok := e.routersBuilder[host]; ok {
return r
}
}
return e.routerBuilder
}

func handlerName(h HandlerFunc) string {
t := reflect.ValueOf(h).Type()
if t.Kind() == reflect.Func {
Expand Down
15 changes: 15 additions & 0 deletions echo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const userXMLPretty = `<user>

func TestEcho(t *testing.T) {
e := New()
e.BuildRouters()
req := httptest.NewRequest(http.MethodGet, "/", nil)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)
Expand Down Expand Up @@ -185,6 +186,7 @@ func TestEchoStatic(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
e := New()
e.Static(tc.givenPrefix, tc.givenRoot)
e.BuildRouters()
req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
rec := httptest.NewRecorder()
e.ServeHTTP(rec, req)
Expand Down Expand Up @@ -243,6 +245,7 @@ func TestEchoStaticRedirectIndex(t *testing.T) {
func TestEchoFile(t *testing.T) {
e := New()
e.File("/walle", "_fixture/images/walle.png")
e.BuildRouters()
c, b := request(http.MethodGet, "/walle", e)
assert.Equal(t, http.StatusOK, c)
assert.NotEmpty(t, b)
Expand Down Expand Up @@ -286,6 +289,8 @@ func TestEchoMiddleware(t *testing.T) {
return c.String(http.StatusOK, "OK")
})

e.BuildRouters()

c, b := request(http.MethodGet, "/", e)
assert.Equal(t, "-1123", buf.String())
assert.Equal(t, http.StatusOK, c)
Expand All @@ -300,6 +305,7 @@ func TestEchoMiddlewareError(t *testing.T) {
}
})
e.GET("/", NotFoundHandler)
e.BuildRouters()
c, _ := request(http.MethodGet, "/", e)
assert.Equal(t, http.StatusInternalServerError, c)
}
Expand All @@ -312,6 +318,8 @@ func TestEchoHandler(t *testing.T) {
return c.String(http.StatusOK, "OK")
})

e.BuildRouters()

c, b := request(http.MethodGet, "/ok", e)
assert.Equal(t, http.StatusOK, c)
assert.Equal(t, "OK", b)
Expand Down Expand Up @@ -473,6 +481,7 @@ func TestEchoEncodedPath(t *testing.T) {
e.GET("/:id", func(c Context) error {
return c.NoContent(http.StatusOK)
})
e.BuildRouters()
req := httptest.NewRequest(http.MethodGet, "/with%2Fslash", nil)
rec := httptest.NewRecorder()
e.ServeHTTP(rec, req)
Expand Down Expand Up @@ -525,6 +534,8 @@ func TestEchoGroup(t *testing.T) {
})
g3.GET("", h)

e.BuildRouters()

request(http.MethodGet, "/users", e)
assert.Equal(t, "0", buf.String())

Expand All @@ -539,6 +550,7 @@ func TestEchoGroup(t *testing.T) {

func TestEchoNotFound(t *testing.T) {
e := New()
e.BuildRouters()
req := httptest.NewRequest(http.MethodGet, "/files", nil)
rec := httptest.NewRecorder()
e.ServeHTTP(rec, req)
Expand All @@ -550,6 +562,7 @@ func TestEchoMethodNotAllowed(t *testing.T) {
e.GET("/", func(c Context) error {
return c.String(http.StatusOK, "Echo!")
})
e.BuildRouters()
req := httptest.NewRequest(http.MethodPost, "/", nil)
rec := httptest.NewRecorder()
e.ServeHTTP(rec, req)
Expand Down Expand Up @@ -840,6 +853,7 @@ func testMethod(t *testing.T, method, path string, e *Echo) {
})
i := interface{}(e)
reflect.ValueOf(i).MethodByName(method).Call([]reflect.Value{p, h})
e.BuildRouters()
_, body := request(method, path, e)
assert.Equal(t, method, body)
}
Expand Down Expand Up @@ -884,6 +898,7 @@ func TestDefaultHTTPErrorHandler(t *testing.T) {
"error": "stackinfo",
})
})
e.BuildRouters()
// With Debug=true plain response contains error message
c, b := request(http.MethodGet, "/plain", e)
assert.Equal(t, http.StatusInternalServerError, c)
Expand Down
5 changes: 5 additions & 0 deletions group_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ func TestGroupFile(t *testing.T) {
e := New()
g := e.Group("/group")
g.File("/walle", "_fixture/images/walle.png")
e.BuildRouters()
expectedData, err := ioutil.ReadFile("_fixture/images/walle.png")
assert.Nil(t, err)
req := httptest.NewRequest(http.MethodGet, "/group/walle", nil)
Expand Down Expand Up @@ -75,6 +76,8 @@ func TestGroupRouteMiddleware(t *testing.T) {
g.GET("/404", h, m4)
g.GET("/405", h, m5)

e.BuildRouters()

c, _ := request(http.MethodGet, "/group/404", e)
assert.Equal(t, 404, c)
c, _ = request(http.MethodGet, "/group/405", e)
Expand Down Expand Up @@ -105,6 +108,8 @@ func TestGroupRouteMiddlewareWithMatchAny(t *testing.T) {
e.GET("unrelated", h, m2)
e.GET("*", h, m2)

e.BuildRouters()

_, m := request(http.MethodGet, "/group/help", e)
assert.Equal(t, "/group/help", m)
_, m = request(http.MethodGet, "/group/help/other", e)
Expand Down
3 changes: 3 additions & 0 deletions middleware/compress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ func TestGzipErrorReturned(t *testing.T) {
e.GET("/", func(c echo.Context) error {
return echo.ErrNotFound
})
e.BuildRouters()
req := httptest.NewRequest(http.MethodGet, "/", nil)
req.Header.Set(echo.HeaderAcceptEncoding, gzipScheme)
rec := httptest.NewRecorder()
Expand All @@ -128,6 +129,7 @@ func TestGzipErrorReturnedInvalidConfig(t *testing.T) {
c.Response().Write([]byte("test"))
return nil
})
e.BuildRouters()
req := httptest.NewRequest(http.MethodGet, "/", nil)
req.Header.Set(echo.HeaderAcceptEncoding, gzipScheme)
rec := httptest.NewRecorder()
Expand All @@ -141,6 +143,7 @@ func TestGzipWithStatic(t *testing.T) {
e := echo.New()
e.Use(Gzip())
e.Static("/test", "../_fixture/images")
e.BuildRouters()
req := httptest.NewRequest(http.MethodGet, "/test/walle.png", nil)
req.Header.Set(echo.HeaderAcceptEncoding, gzipScheme)
rec := httptest.NewRecorder()
Expand Down
Loading