Skip to content

Set maxParam with SetParamNames #1535

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

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Set maxParam with SetParamNames #1535

Show file tree
Hide file tree
Changes from 1 commit
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

@lammel lammel Mar 25, 2020

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
3 changes: 2 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ module github.com/labstack/echo/v4
go 1.14

require (
github.com/dgrijalva/jwt-go v3.2.0+incompatible
Copy link
Contributor

@lammel lammel Mar 25, 2020

Choose a reason for hiding this comment

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

The go.mod updates are very likely not required.
Please cleanup your PR to not modify unrelated files.

Copy link
Contributor Author

@178inaba 178inaba Mar 25, 2020

Choose a reason for hiding this comment

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

Fixed!

github.com/labstack/gommon v0.3.0
github.com/mattn/go-colorable v0.1.6 // indirect
github.com/stretchr/testify v1.4.0
github.com/valyala/fasttemplate v1.1.0 // indirect
github.com/valyala/fasttemplate v1.1.0
golang.org/x/crypto v0.0.0-20200221231518-2aa609cf4a9d
golang.org/x/net v0.0.0-20200226121028-0de0cce0169b
golang.org/x/text v0.3.2 // indirect
Expand Down
3 changes: 3 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/dgrijalva/jwt-go v3.2.0+incompatible h1:7qlOGliEKZXTDg6OTjfoBKDXWrumCAMpl/TFQ4/5kLM=
github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZmtrrCbhqsmaPHjLKYnJCaQ=
github.com/labstack/gommon v0.3.0 h1:JEeO0bvc78PKdyHxloTKiF8BD5iGrH8T6MSeGvSgob0=
github.com/labstack/gommon v0.3.0/go.mod h1:MULnywXg0yavhxWKc+lOruYdAhDwPK9wf0OL7NoOu+k=
github.com/mattn/go-colorable v0.1.2 h1:/bC9yWikZXAL9uJdulbSfyVNIR3n3trXl+v8+1sx8mU=
Expand Down Expand Up @@ -41,6 +43,7 @@ golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.2 h1:tW2bmiBqwgJj/UpqtC8EpXEZVYOwU0yG4iWbprSVAcs=
golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=
gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
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

@lammel lammel Mar 25, 2020

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

@178inaba 178inaba Mar 25, 2020

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