Skip to content

Incorrect path parameter names with some route registration orders #1221

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.

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

Closed
3 tasks
bwsalmon opened this issue Nov 16, 2018 · 1 comment
Closed
3 tasks

Incorrect path parameter names with some route registration orders #1221

bwsalmon opened this issue Nov 16, 2018 · 1 comment

Comments

@bwsalmon
Copy link

/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/
/ / / / / / / / / / / / / / / / / /

Please use forum https://forum.labstack.com to ask questions!

/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/_/
/ / / / / / / / / / / / / / / / / /

Issue Description

Depending on the ordering of my route registration, I am sometimes getting what I believe is incorrect path param behavior. This looks like the same issue as Issue 1052 from January, but it is happening on a much later version where it seems it may be a regression.

For example. If I register the following routes:

/v1/stream/:stream/foo/:id
/v1/stream/:id

In this order, the path params returned for a GET to /v1/stream/123 are stream=123 not id=123 as specified in the route.

If I reverse the order and register the shorter path first, everything works fine, the param in the /v1/stream/123 route is id=123.

I've attached a short unit test that illustrates the behavior.

Checklist

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

Expected behaviour

Path params bind to the names included in the route match path.

Actual behaviour

If longer paths using different variable names for the same slot are registered before shorter ones, the shorter ones will use the variable names from the longer paths, not the ones in their match pattern

Steps to reproduce

Working code to debug

package foo

import (
	"fmt"
	"net/http"
	"testing"

	"github.com/labstack/echo"
)

// Just print out the path parameters
func handler(c echo.Context) error {
	fmt.Println(c.Request().Method + " " + c.Path())
	fmt.Println("Names")
	fmt.Println(c.ParamNames())
	fmt.Println("Values")
	fmt.Println(c.ParamValues())
	fmt.Println("")
	return nil
}

// Register two servers, one with one order, the other with another.
func listenServer1() {
	server := echo.New()

	server.GET("/v1/stream/:id", handler)
	server.GET("/v1/stream/:stream/foo/:id", handler)

	server.Start(":1111")
}

// Register two servers, one with one order, the other with another.
func listenServer2() {
	server := echo.New()

	server.GET("/v1/stream/:stream/foo/:id", handler)
	server.GET("/v1/stream/:id", handler)

	server.Start(":2222")
}

func TestMatchPath(t *testing.T) {
	go listenServer1()
	go listenServer2()

        // Get the same paths on the two servers.
	http.Get("http://localhost:1111/v1/stream/123")
	http.Get("http://localhost:2222/v1/stream/123")
}

OUTPUT:

Note that the output for the first get is as expected, but the output for the second get looks incorrect.

   ____    __
  / __/___/ /  ___
 / _// __/ _ \/ _ \
/___/\__/_//_/\___/ v3.3.6
High performance, minimalist Go web framework
https://echo.labstack.com
____________________________________O/_______
                                    O\
⇨ http server started on [::]:1111

   ____    __
  / __/___/ /  ___
 / _// __/ _ \/ _ \
/___/\__/_//_/\___/ v3.3.6
High performance, minimalist Go web framework
https://echo.labstack.com
____________________________________O/_______
                                    O\
⇨ http server started on [::]:2222
GET /v1/stream/:id
Names
[id]
Values
[123]

GET /v1/stream/:id
Names
[stream]    <<<<<<<<<<<<<<<<<<<<<<<<<<<< INCORRECT?
Values
[123]

Version/commit

revision = "1abaa3049251d17932e4313c2d6165073fd07fd8"
version = "v3.3.6"

@bwsalmon
Copy link
Author

bwsalmon commented Nov 16, 2018

Hmm, oops. Looks like I actually have an earlier version that I had realized. Closing this out, since I believe it is a dup of #1052.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant