diff --git a/context.go b/context.go index a397fba7..5f4a2010 100644 --- a/context.go +++ b/context.go @@ -55,6 +55,16 @@ type Context interface { // PathParam returns path parameter by name. PathParam(name string) string + // PathParamDefault returns the path parameter or default value for the provided name. + // + // Notes for DefaultRouter implementation: + // Path parameter could be empty for cases like that: + // * route `/release-:version/bin` and request URL is `/release-/bin` + // * route `/api/:version/image.jpg` and request URL is `/api//image.jpg` + // but not when path parameter is last part of route path + // * route `/download/file.:ext` will not match request `/download/file.` + PathParamDefault(name string, defaultValue string) string + // PathParams returns path parameter values. PathParams() PathParams @@ -176,6 +186,9 @@ type Context interface { Redirect(code int, url string) error // Echo returns the `Echo` instance. + // + // WARNING: Remember that Echo public fields and methods are coroutine safe ONLY when you are NOT mutating them + // anywhere in your code after Echo server has started. Echo() *Echo } @@ -379,7 +392,10 @@ func (c *DefaultContext) PathParam(name string) string { return c.pathParams.Get(name, "") } -// PathParamDefault does not exist as expecting empty path param makes no sense +// PathParamDefault returns the path parameter or default value for the provided name. +func (c *DefaultContext) PathParamDefault(name, defaultValue string) string { + return c.pathParams.Get(name, defaultValue) +} // PathParams returns path parameter values. func (c *DefaultContext) PathParams() PathParams { @@ -406,7 +422,8 @@ func (c *DefaultContext) QueryParam(name string) string { } // QueryParamDefault returns the query param or default value for the provided name. -// Note: QueryParamDefault does not distinguish if form had no value by that name or value was empty string +// Note: QueryParamDefault does not distinguish if query had no value by that name or value was empty string +// This means URLs `/test?search=` and `/test` would both return `1` for `c.QueryParamDefault("search", "1")` func (c *DefaultContext) QueryParamDefault(name, defaultValue string) string { value := c.QueryParam(name) if value == "" { diff --git a/context_test.go b/context_test.go index dca680f9..0df10539 100644 --- a/context_test.go +++ b/context_test.go @@ -448,22 +448,142 @@ func TestContextCookie(t *testing.T) { assert.Contains(t, rec.Header().Get(HeaderSetCookie), "HttpOnly") } -func TestContextPathParam(t *testing.T) { - e := New() - req := httptest.NewRequest(http.MethodGet, "/", nil) - c := e.NewContext(req, nil).(*DefaultContext) - - params := &PathParams{ - {Name: "uid", Value: "101"}, - {Name: "fid", Value: "501"}, +func TestContext_PathParams(t *testing.T) { + var testCases = []struct { + name string + given *PathParams + expect PathParams + }{ + { + name: "param exists", + given: &PathParams{ + {Name: "uid", Value: "101"}, + {Name: "fid", Value: "501"}, + }, + expect: PathParams{ + {Name: "uid", Value: "101"}, + {Name: "fid", Value: "501"}, + }, + }, + { + name: "params is empty", + given: &PathParams{}, + expect: PathParams{}, + }, } - // ParamNames - c.pathParams = params - assert.EqualValues(t, *params, c.PathParams()) - // Param - assert.Equal(t, "501", c.PathParam("fid")) - assert.Equal(t, "", c.PathParam("undefined")) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := New() + req := httptest.NewRequest(http.MethodGet, "/", nil) + c := e.NewContext(req, nil) + + c.(RoutableContext).SetRawPathParams(tc.given) + + assert.EqualValues(t, tc.expect, c.PathParams()) + }) + } +} + +func TestContext_PathParam(t *testing.T) { + var testCases = []struct { + name string + given *PathParams + whenParamName string + expect string + }{ + { + name: "param exists", + given: &PathParams{ + {Name: "uid", Value: "101"}, + {Name: "fid", Value: "501"}, + }, + whenParamName: "uid", + expect: "101", + }, + { + name: "multiple same param values exists - return first", + given: &PathParams{ + {Name: "uid", Value: "101"}, + {Name: "uid", Value: "202"}, + {Name: "fid", Value: "501"}, + }, + whenParamName: "uid", + expect: "101", + }, + { + name: "param does not exists", + given: &PathParams{ + {Name: "uid", Value: "101"}, + }, + whenParamName: "nope", + expect: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := New() + req := httptest.NewRequest(http.MethodGet, "/", nil) + c := e.NewContext(req, nil) + + c.(RoutableContext).SetRawPathParams(tc.given) + + assert.EqualValues(t, tc.expect, c.PathParam(tc.whenParamName)) + }) + } +} + +func TestContext_PathParamDefault(t *testing.T) { + var testCases = []struct { + name string + given *PathParams + whenParamName string + whenDefaultValue string + expect string + }{ + { + name: "param exists", + given: &PathParams{ + {Name: "uid", Value: "101"}, + {Name: "fid", Value: "501"}, + }, + whenParamName: "uid", + whenDefaultValue: "999", + expect: "101", + }, + { + name: "param exists and is empty", + given: &PathParams{ + {Name: "uid", Value: ""}, + {Name: "fid", Value: "501"}, + }, + whenParamName: "uid", + whenDefaultValue: "999", + expect: "", // <-- this is different from QueryParamDefault behaviour + }, + { + name: "param does not exists", + given: &PathParams{ + {Name: "uid", Value: "101"}, + }, + whenParamName: "nope", + whenDefaultValue: "999", + expect: "999", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := New() + req := httptest.NewRequest(http.MethodGet, "/", nil) + c := e.NewContext(req, nil) + + c.(RoutableContext).SetRawPathParams(tc.given) + + assert.EqualValues(t, tc.expect, c.PathParamDefault(tc.whenParamName, tc.whenDefaultValue)) + }) + } } func TestContextGetAndSetParam(t *testing.T) { @@ -568,27 +688,129 @@ func TestContextFormValue(t *testing.T) { assert.Error(t, err) } -func TestContextQueryParam(t *testing.T) { - q := make(url.Values) - q.Set("name", "Jon Snow") - q.Set("email", "jon@labstack.com") - req := httptest.NewRequest(http.MethodGet, "/?"+q.Encode(), nil) - e := New() - c := e.NewContext(req, nil) +func TestContext_QueryParams(t *testing.T) { + var testCases = []struct { + name string + givenURL string + expect url.Values + }{ + { + name: "multiple values in url", + givenURL: "/?test=1&test=2&email=jon%40labstack.com", + expect: url.Values{ + "test": []string{"1", "2"}, + "email": []string{"jon@labstack.com"}, + }, + }, + { + name: "single value in url", + givenURL: "/?nope=1", + expect: url.Values{ + "nope": []string{"1"}, + }, + }, + { + name: "no query params in url", + givenURL: "/?", + expect: url.Values{}, + }, + } - // QueryParam - assert.Equal(t, "Jon Snow", c.QueryParam("name")) - assert.Equal(t, "jon@labstack.com", c.QueryParam("email")) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tc.givenURL, nil) + e := New() + c := e.NewContext(req, nil) - // QueryParamDefault - assert.Equal(t, "Jon Snow", c.QueryParamDefault("name", "nope")) - assert.Equal(t, "default", c.QueryParamDefault("missing", "default")) + assert.Equal(t, tc.expect, c.QueryParams()) + }) + } +} - // QueryParams - assert.Equal(t, url.Values{ - "name": []string{"Jon Snow"}, - "email": []string{"jon@labstack.com"}, - }, c.QueryParams()) +func TestContext_QueryParam(t *testing.T) { + var testCases = []struct { + name string + givenURL string + whenParamName string + expect string + }{ + { + name: "value exists in url", + givenURL: "/?test=1", + whenParamName: "test", + expect: "1", + }, + { + name: "multiple values exists in url", + givenURL: "/?test=9&test=8", + whenParamName: "test", + expect: "9", // <-- first value in returned + }, + { + name: "value does not exists in url", + givenURL: "/?nope=1", + whenParamName: "test", + expect: "", + }, + { + name: "value is empty in url", + givenURL: "/?test=", + whenParamName: "test", + expect: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tc.givenURL, nil) + e := New() + c := e.NewContext(req, nil) + + assert.Equal(t, tc.expect, c.QueryParam(tc.whenParamName)) + }) + } +} + +func TestContext_QueryParamDefault(t *testing.T) { + var testCases = []struct { + name string + givenURL string + whenParamName string + whenDefaultValue string + expect string + }{ + { + name: "value exists in url", + givenURL: "/?test=1", + whenParamName: "test", + whenDefaultValue: "999", + expect: "1", + }, + { + name: "value does not exists in url", + givenURL: "/?nope=1", + whenParamName: "test", + whenDefaultValue: "999", + expect: "999", + }, + { + name: "value is empty in url", + givenURL: "/?test=", + whenParamName: "test", + whenDefaultValue: "999", + expect: "999", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tc.givenURL, nil) + e := New() + c := e.NewContext(req, nil) + + assert.Equal(t, tc.expect, c.QueryParamDefault(tc.whenParamName, tc.whenDefaultValue)) + }) + } } func TestContextFormFile(t *testing.T) { diff --git a/echo.go b/echo.go index 61f63c11..ccd83db0 100644 --- a/echo.go +++ b/echo.go @@ -55,7 +55,9 @@ import ( ) // Echo is the top-level framework instance. -// Note: replacing/nilling public fields is not coroutine/thread-safe and can cause data-races/panics. +// +// Note: replacing/nilling public fields is not coroutine/thread-safe and can cause data-races/panics. This is very likely +// to happen when you access Echo instances through Context.Echo() method. type Echo struct { // premiddleware are middlewares that are run for every request before routing is done premiddleware []MiddlewareFunc @@ -67,8 +69,8 @@ type Echo struct { routerCreator func(e *Echo) Router contextPool sync.Pool - // contextPathParamAllocSize holds maximum parameter count for all added routes. This is necessary info for context - // creation time so we can allocate path parameter values slice. + // contextPathParamAllocSize holds maximum parameter count for all added routes. This is necessary info at context + // creation moment so we can allocate path parameter values slice with correct size. contextPathParamAllocSize int // NewContextFunc allows using custom context implementations, instead of default *echo.context diff --git a/router.go b/router.go index d4bb3077..bee6c2bd 100644 --- a/router.go +++ b/router.go @@ -8,6 +8,15 @@ import ( ) // Router is interface for routing request contexts to registered routes. +// +// Contract between Echo/Context instance and the router: +// * all routes must be added through methods on echo.Echo instance. +// Reason: Echo instance uses RouteInfo.Params() length to allocate slice for paths parameters (see `Echo.contextPathParamAllocSize`). +// * Router must populate Context during Router.Route call with: +// * RoutableContext.SetPath +// * RoutableContext.SetRawPathParams (IMPORTANT! with same slice pointer that c.RawPathParams() returns) +// * RoutableContext.SetRouteInfo +// And optionally can set additional information to Context with RoutableContext.Set type Router interface { // Add registers Routable with the Router and returns registered RouteInfo Add(routable Routable) (RouteInfo, error) @@ -19,11 +28,6 @@ type Router interface { // Route searches Router for matching route and applies it to the given context. In case when no matching method // was not found (405) or no matching route exists for path (404), router will return its implementation of 405/404 // handler function. - // When implementing custom Router make sure to populate Context during Router.Route call with: - // * RoutableContext.SetPath - // * RoutableContext.SetRawPathParams (IMPORTANT! with same slice pointer that c.RawPathParams() returns) - // * RoutableContext.SetRouteInfo - // And optionally can set additional information to Context with RoutableContext.Set Route(c RoutableContext) HandlerFunc } diff --git a/router_test.go b/router_test.go index 596e9f4e..eed4016f 100644 --- a/router_test.go +++ b/router_test.go @@ -2242,6 +2242,64 @@ func TestRouter_Match_DifferentParamNamesForSamePlace(t *testing.T) { } } +// Issue #2164 - this test is meant to document path parameter behaviour when request url has empty value in place +// of the path parameter. As tests show the result is different depending on where parameter exists in the route path. +func TestDefaultRouter_PathParamsCanMatchEmptyValues(t *testing.T) { + var testCases = []struct { + name string + whenURL string + expectRoute string + expectParam map[string]string + expectError error + }{ + { + name: "ok, route is matched with even empty param is in the middle and between slashes", + whenURL: "/a//b", + expectRoute: "/a/:id/b", + expectParam: map[string]string{"id": ""}, + }, + { + name: "ok, route is matched with even empty param is in the middle", + whenURL: "/a2/b", + expectRoute: "/a2:id/b", + expectParam: map[string]string{"id": ""}, + }, + { + name: "ok, route is NOT matched with even empty param is at the end", + whenURL: "/a3/", + expectRoute: "", + expectParam: map[string]string{}, + expectError: ErrNotFound, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + e := New() + + e.GET("/a/:id/b", handlerFunc) + e.GET("/a2:id/b", handlerFunc) + e.GET("/a3/:id", handlerFunc) + + req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil) + c := e.NewContext(req, nil).(*DefaultContext) + handler := e.router.Route(c) + + err := handler(c) + if tc.expectError != nil { + assert.Equal(t, tc.expectError, err) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tc.expectRoute, c.Path()) + for param, expectedValue := range tc.expectParam { + assert.Equal(t, expectedValue, c.pathParams.Get(param, "")) + } + checkUnusedParamValues(t, c, tc.expectParam) + }) + } +} + // Issue #729 func TestRouterParamAlias(t *testing.T) { api := []testRoute{