SetParamValues function throwing index out of range [0] with length 0 error

[yenskillah]

Issue Description

Hi,

I' currently encountering an issue when setting parameter values via SetParamValues function. This is frequently used by my test cases, hence, tests are all failing.

Checklist

• Dependencies installed
• No typos
• Searched existing issues and docs

Expected behaviour

Should be able to set parameter values via SetParamValues function

Actual behaviour

SetParamValues function throws runtime error: index out of range [0] with length 0

Steps to reproduce

1. Create a folder inside your go source path.
2. Copy the codes below into files handler.go and handler_test.go and store them inside that folder.
3. cd yourfolder
4. go test -run ^(TestGetUser)$ -v

It will show this test logs afterwards:

=== RUN TestGetUser
--- FAIL: TestGetUser (0.00s)
panic: runtime error: index out of range [0] with length 0 [recovered]
panic: runtime error: index out of range [0] with length 0

goroutine 20 [running]:
testing.tRunner.func1(0xc00010e100)
/usr/local/Cellar/go/1.13.3/libexec/src/testing/testing.go:874 +0x3a3
panic(0x1387d40, 0xc0000c4380)
/usr/local/Cellar/go/1.13.3/libexec/src/runtime/panic.go:679 +0x1b2
github.com/labstack/echo.(*context).SetParamValues(0xc0000bd180, 0xc00008d2b0, 0x1, 0x1)
/Users/sonnysidramos/go/src/github.com/labstack/echo/context.go:317 +0x93
bitbucket.org/pulseid/echotest.TestGetUser(0xc00010e100)
/Users/sonnysidramos/go/src/bitbucket.org/pulseid/echotest/handler_test.go:27 +0x307
testing.tRunner(0xc00010e100, 0x13ce2a0)
/usr/local/Cellar/go/1.13.3/libexec/src/testing/testing.go:909 +0xc9
created by testing.(*T).Run
/usr/local/Cellar/go/1.13.3/libexec/src/testing/testing.go:960 +0x350
exit status 2

Working code to debug

handler.go

package handler

import (
	"net/http"

	"github.com/labstack/echo"
)

type (
	User struct {
		Name  string `json:"name" form:"name"`
		Email string `json:"email" form:"email"`
	}
	handler struct {
		db map[string]*User
	}
)

func (h *handler) getUser(c echo.Context) error {
	email := c.Param("email")
	user := h.db[email]
	if user == nil {
		return echo.NewHTTPError(http.StatusNotFound, "user not found")
	}
	return c.JSON(http.StatusOK, user)
}

handler_test.go:

package handler

import (
	"net/http"
	"net/http/httptest"
	"testing"

	"github.com/labstack/echo"
	"github.com/stretchr/testify/assert"
)

var (
	mockDB = map[string]*User{
		"[email protected]": &User{"Jon Snow", "[email protected]"},
	}
	userJSON = `{"name":"Jon Snow","email":"[email protected]"}`
)

func TestGetUser(t *testing.T) {
	// Setup
	e := echo.New()
	req := httptest.NewRequest(http.MethodGet, "/", nil)
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec)
	c.SetPath("/users/:email")
	c.SetParamNames("email")
	c.SetParamValues("[email protected]")
	h := &handler{mockDB}

	// Assertions
	if assert.NoError(t, h.getUser(c)) {
		assert.Equal(t, http.StatusOK, rec.Code)
		assert.Equal(t, userJSON, rec.Body.String())
	}
}

Version/commit

8d7f05e

[i82orbom]

Also facing the same issue here. MaxParams is not settable from an outside package, your tests are passing because in each test case you are manipulating the variable to have the correct array size at all times. This breaks the compatibility

[pratikmallya]

same issue. I believe 8d7f05e#diff-05a3aa0ab9e5687ff167e0ef3ae4c5a6R316 from this PR #1463 may be what is causing the problem

[yenskillah]

yes indeed, maxParams is unsettable outside fo the package.

[noamt]

I'm also experiencing this issue

[yenskillah]

@noamt there's a way to fix this #1463 (comment). It worked like a charm. Thanks @dlowe

[i82orbom]

@yenskillah I think the issue is not solved, the way of "fixing" this cannot be some trick... maxParamValues should be settable from outside or make the array dynamic.

There's even a PR open to address this: #1498

[ginnyedelstein]

i'm experiencing the same issue

[aquincum]

I agree that this is not solved. We have tests that depend on SetParamValues on a context used with a httptest.Recorder and a httptest.Request. We cannot access the maxParams since it's private and there is no way now to set our path parameters.

#1498 would be usable since we could at least manually set maxParam, but I'm not entirely sure why this breaking change was needed in the end.

[i82orbom]

Maybe @dlowe can shed some light. I don't see why the array can't be dynamic... the max param requirement note was introduced by him.

[dlowe]

I did not add the behavior or logic behind maxParam; I just fixed SetParamValues such that it wouldn't cause crashes in other parts of the code.

The comment I added was copied from https://github.com/labstack/echo/blob/master/context.go#L628.

As I said in #1463 (comment): I agree this isn't ideal, but I don't actually understand the point of maxParam, so I can't say what the best solution might be.

[i82orbom]

Maybe @vishr can tell us something

[aquincum]

The author of the issue whose fix broke functionality saying he doesn't "actually understand the point of maxParam" is definitely a red flag to escalate and make sure that the fix actually makes sense and doesn't introduce newer issues.

[dlowe]

The author of the issue whose fix broke functionality saying he doesn't "actually understand the point of maxParam" is definitely a red flag to escalate and make sure that the fix actually makes sense and doesn't introduce newer issues.

To clarify: What I don't understand is why maxParam is needed at all, in the context of the whole echo framework. Given that it exists, I'm certain that the previous behavior of SetParamValues (changing c.pvalues without regard for c.maxParam) was incorrect.

Someone with a better understanding of the intention behind maxParam might be able to make a change to SetParamValues that updates c.pvalues and c.maxParam safely.

[lammel]

@vishr Could we reopen this issue to ensure it is not breaking current test code.

echo.maxParam is currently using in the router to decide how many parameters are taken from the provided request. Only up the maxParam values are stored in the context.

So maxParam should be initialized with a sane value (e.g. 128) for now. My preferred solution would be to have a sane maxParam value and have a dynamically extending array of pvalues up to maxParam in size. This would allow for existing code to continue to work, keep memory requirements of the context at bay and still allow to have a limit defined to avoid eating to much memory for malicious requests.

Feedback from @vishr would be required here on his point of view.

[madeindra]

@vishr I agree this issue needs to be reopened. We can't use a trick to close the issue. Could we please reopen and solve this?

[likejehu]

i am facing the same issue...

[terev]

@likejehu I've worked around this issue with v4.1.14 and v4.1.15 by downgrading to v4.1.13 for now.

[likejehu]

@likejehu I've worked around this issue with v4.1.14 and v4.1.15 by downgrading to v4.1.13 for now.

thanks, I just started to learn programming in general, and work with golang and with this library in particular and I don’t understand very well what is happening and what to do in situation where I need to write unit tests. But im facing this issue and dont know can i crank some magic trick or do i need to wait until it gets fixed.

[eddwinpaz]

I took the change to modify the code where the error is taking place and monkey patch it. I tried to pull request it, but It fails for some reason on other side I cannot find yet.

labstack > echo > context.go

func (c *context) SetParamValues(values ...string) {
    for _, val := range values {
    c.pvalues = append(c.pvalues, val)
  }
}

[relax-admin]

@eddwinpaz Whether the priority of this issue should be raised. Since there is no alternative solution, all unit tests for echo are reported to be wrong. Especially with travis, it is difficult to control the echo version.

[178inaba]

I have the same issue.

I found that I set maxParam with the number of path names.

echo/router.go

Line 101 in 5ddc3a6

l := len(pnames)

So I modified to set maxParam when calling SetParamNames.
#1535
Can review this?

[hxt365]

So we've still had no solution yet?

[lammel]

Review started already for the PR#1535. Thanks @178inaba

[stiks]

Yay, 2 months and it's now finally sorted