Skip to content

Parameter name affects route registration ordering #1052

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 done
filet0fish opened this issue Jan 25, 2018 · 3 comments
Closed
3 tasks done

Parameter name affects route registration ordering #1052

filet0fish opened this issue Jan 25, 2018 · 3 comments
Assignees
Labels

Comments

@filet0fish
Copy link

Issue Description

Routes with identical parameter names can return the wrong value if not registered in a specific order.

Checklist

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

Expected behaviour

Routing documentation states "Routes can be written in any order."

Actual behaviour

Route order does matter in this edge case. If registered in the wrong order, the parameter names resolve to the wrong segment of the url pattern.

Steps to reproduce

Register a set of route patterns:

e.GET("/:a/:b/:c/:id", func(c echo.Context) error {
	return c.HTML(500, "FAILS :id returns value of :c")
})
e.GET("/:a/:id", func(c echo.Context) error {
	return c.HTML(200, "WORKS")
})
e.GET("/:a/:e/:id", func(c echo.Context) error {
	return c.HTML(200, "WORKS")
})

If the first route is moved to last position, all will return the correct value for c.Param("id"). If any one of the 3 routes is removed, the other two will return the correct value.

Working code to debug

package main

import "github.com/labstack/echo"

func main() {
	e := echo.New()

	e.GET("/:a/:b/:c/:id", func(c echo.Context) error {
		return c.HTML(500, "FAILS :id returns :c, ID == "+c.Param("id"))
	})
	e.GET("/:a/:id", func(c echo.Context) error {
		return c.HTML(200, "WORKS, ID == "+c.Param("id"))
	})
	e.GET("/:a/:e/:id", func(c echo.Context) error {
		return c.HTML(200, "WORKS, ID == "+c.Param("id"))
	})

	e.Logger.Fatal(e.Start(":8888"))
}

http://localhost:8888/foo/bar/baz/123 returns FAILS :id returns :c, ID == baz

Version/commit

version: ^3.1.0
version: b338075

@vishr vishr added the bug label Jan 31, 2018
@vishr vishr self-assigned this Jan 31, 2018
@vishr
Copy link
Member

vishr commented Jan 31, 2018

This came up from a commit fd9508c towards path param aliases. I am going to revert that change as it is buggy.

@vishr vishr closed this as completed in ec048ea Jan 31, 2018
@vishr
Copy link
Member

vishr commented Jan 31, 2018

@filet0fish please verify.

@filet0fish
Copy link
Author

Thanks, that fixed my test case.

We are still seeing some inconsistencies around registration order in our production app, but it appears to be unrelated. I will file another bug once we rule our code, have a good test case, and confirm the issue isn't a dupe of #875.

Our current workaround is sorting routes by complexity (number of params, then number of segments) and length before registering them.

Thanks again!

vishr pushed a commit that referenced this issue Mar 12, 2018
Enhancements:
    Implemented Response#After()
    Dynamically add/remove proxy targets
    Rewrite rules for proxy middleware
    Add ability to extract key from a form field
    Implemented rewrite middleware
    Adds a separate flag for the 'http/https server started on' message (#1043)
    Applied a little DRY to the redirect middleware (#1053) and tests (#1055)
    Simplify dep fetching (#1062)
    Add custom time stamp format #1046 (#1066)
    Update property name & default value & comment about custom logger
    Add X-Requested-With header constant
    Return error of context.File in c.contentDisposition
    Updated deps
    Updated README.md

Fixes:

    Fixed Response#Before()
    Fixed #990
    Fix href url from armor to echo (#1051)
    Fixed #1054
    Fixed #1052, dropped param alias feature
    Avoid redirect loop in HTTPSRedirect middleware (#1058)
    Fix deferring body close in middleware/compress test (#1063)
    Cleanup code (#1061)
    FIX - We must close gzip.Reader, only if no error (#1069)
    Fix formatting (#1071)
    Can be a fix for auto TLS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants