Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Set maxParam with SetParamNames #1535

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Set maxParam with SetParamNames #1535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf1d6cd
Set maxParam with SetParamNames
178inaba Mar 20, 2020
f93ba1f
Revert go.mod
178inaba Mar 25, 2020
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
2 changes: 0 additions & 2 deletions bind_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,6 @@ func TestBindbindData(t *testing.T) {

func TestBindParam(t *testing.T) {
e := New()
*e.maxParam = 2
req := httptest.NewRequest(GET, "/", nil)
rec := httptest.NewRecorder()
c := e.NewContext(req, rec)
Expand Down Expand Up @@ -363,7 +362,6 @@ func TestBindParam(t *testing.T) {
// Bind something with param and post data payload
body := bytes.NewBufferString(`{ "name": "Jon Snow" }`)
e2 := New()
*e2.maxParam = 2
req2 := httptest.NewRequest(POST, "/", body)
req2.Header.Set(HeaderContentType, MIMEApplicationJSON)

Expand Down
6 changes: 2 additions & 4 deletions context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,17 +310,15 @@ func (c *context) ParamNames() []string {

func (c *context) SetParamNames(names ...string) {
c.pnames = names
*c.echo.maxParam = len(names)
}

func (c *context) ParamValues() []string {
return c.pvalues[:len(c.pnames)]
}

func (c *context) SetParamValues(values ...string) {
// NOTE: Don't just set c.pvalues = values, because it has to have length c.echo.maxParam at all times
for i, val := range values {
c.pvalues[i] = val
}
c.pvalues = values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to set values without checking for the length of pnames here?

Copy link
Contributor Author

@178inaba 178inaba Mar 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Param checks the length, so it's OK.

echo/context.go

Line 298 in f93ba1f

if i < len(c.pvalues) {

ParamValues does not check the length, so setting a slice shorter than pnames will panic.

echo/context.go

Line 317 in f93ba1f

return c.pvalues[:len(c.pnames)]

However, in the documentation it is written to set the same number of values, so it seems to be understood as a specification.
https://echo.labstack.com/guide/testing#setting-path-params
SetParamValues does not return an error, so if you want to fix it, you should check the length with ParamValues.

}

func (c *context) QueryParam(name string) string {
Expand Down
5 changes: 2 additions & 3 deletions context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ func (responseWriterErr) WriteHeader(statusCode int) {

func TestContext(t *testing.T) {
e := New()
*e.maxParam = 1
req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(userJSON))
rec := httptest.NewRecorder()
c := e.NewContext(req, rec).(*context)
Expand Down Expand Up @@ -472,7 +471,6 @@ func TestContextPath(t *testing.T) {

func TestContextPathParam(t *testing.T) {
e := New()
*e.maxParam = 2
req := httptest.NewRequest(http.MethodGet, "/", nil)
c := e.NewContext(req, nil)

Expand All @@ -491,7 +489,8 @@ func TestContextPathParam(t *testing.T) {

func TestContextGetAndSetParam(t *testing.T) {
e := New()
*e.maxParam = 2
r := e.Router()
r.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
2 changes: 0 additions & 2 deletions middleware/jwt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ func TestJWTRace(t *testing.T) {

func TestJWT(t *testing.T) {
e := echo.New()
r := e.Router()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be only cleanup and unrelated to the PR.
Please remove the unrelated fixes, better open a cleanup PR for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix is the code added in #1463 which triggered #1492.
https://github.com/labstack/echo/pull/1463/files#diff-8c1ede4eff7529ee5a22ed18da7944df

Removed because it is no longer needed in this PR.

r.Add("GET", "/:jwt", func(echo.Context) error { return nil })
handler := func(c echo.Context) error {
return c.String(http.StatusOK, "test")
}
Expand Down