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

merged 2 commits into from
Mar 30, 2020

Conversation

178inaba
Copy link
Contributor

Fixes #1492

@codecov
Copy link

codecov bot commented Mar 20, 2020 •
edited
Loading

Codecov Report

Merging #1535 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1535   +/-   ##
=======================================
  Coverage   84.84%   84.84%           
=======================================
  Files          28       28           
  Lines        2165     2165           
=======================================
  Hits         1837     1837           
  Misses        213      213           
  Partials      115      115
Impacted Files Coverage Δ
context.go 89.72% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc4b1c0...f93ba1f. Read the comment docs.

@178inaba 178inaba mentioned this pull request Mar 20, 2020
Closed
3 tasks
@vishr
Copy link
Member

vishr commented Mar 21, 2020

@lammel Can you please help review this?

@lammel
Copy link
Contributor

lammel commented Mar 23, 2020

@vishr Yes, OK. I'll review the patch later.

lammel
lammel reviewed Mar 25, 2020
View reviewed changes
middleware/jwt_test.go
@@ -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.

lammel
lammel reviewed Mar 25, 2020
View reviewed changes
go.mod Outdated
@@ -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

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

Choose a reason for hiding this comment

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

Fixed!

lammel
lammel requested changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

Looking good overall.
Please cleanup the unrelated files.
Didn't have time for testing or debugging yet, did you do real world testing with this patches?

context.go
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.

@178inaba
Copy link
Contributor Author

@lammel Thanks!

Didn't have time for testing or debugging yet, did you do real world testing with this patches?

Yes!
I tested this locally on my and found that it worked.

lammel
lammel approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

Reviewed changes and usage of SetParamValues and SetParamNames.
This helpers are used only for testing and this patch fixes the interactions with the internal echo.maxParam.

Tests are green, test coverage is there. Approved

@lammel lammel mentioned this pull request Mar 30, 2020
Closed
3 tasks
@vishr vishr merged commit 269dfcc into labstack:master Mar 30, 2020
@vishr
Copy link
Member

vishr commented Mar 30, 2020

@178inaba @lammel Thanks for your support 👍

@178inaba 178inaba deleted the set_max_param branch March 31, 2020 20:08
@stevenvegt stevenvegt mentioned this pull request May 4, 2020
Closed
@pafuent pafuent mentioned this pull request Nov 22, 2020
Closed
@iaburton iaburton mentioned this pull request Dec 1, 2020
Closed
This was referenced Jun 6, 2021
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishr vishr vishr approved these changes

@lammel lammel lammel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SetParamValues function throwing index out of range [0] with length 0 error
3 participants
@178inaba @vishr @lammel