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.

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

Wrong parameter behaviour with specific path #1744

Closed
3 tasks done
ghost opened this issue Dec 31, 2020 · 3 comments
Closed
3 tasks done

Wrong parameter behaviour with specific path #1744

ghost opened this issue Dec 31, 2020 · 3 comments

Comments

@ghost
Copy link

ghost commented Dec 31, 2020

Issue Description

Route router.DELETE takes parameters from router.GET instead of his own parameters.

Checklist

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

Expected behaviour

DeleteTranslation handler receives echo.Context with id parameter without lang parameter

Actual behaviour

DeleteTranslation handler receives lang parameter instead of id parameter

Steps to reproduce

  1. Run server
  2. Request DELETE http://localhost:3333/translation/123

Working code to debug

package main

import (
	"github.com/labstack/echo/v4"
	"net/http"
)

var router = echo.New()

type resp struct {
	Name string
	Lang string
	Id string
}

func GetTranslation (c echo.Context) error {
	return c.JSON(http.StatusOK, resp{Name: "Get", Id: c.Param("id"), Lang: c.Param("lang")})
}

func DeleteTranslation (c echo.Context) error {
	return c.JSON(http.StatusOK, resp{Name: "Delete", Id: c.Param("id"), Lang: c.Param("lang")})
}

func main()  {
	router.GET("/translation/:lang", GetTranslation)
	router.DELETE("/translation/:id", DeleteTranslation)

	router.Start(":8000")
}

Version/commit

go 1.15
github.com/labstack/echo/v4 v4.1.17

@aldas
Copy link
Contributor

aldas commented Jan 1, 2021

Seems to be same as #479

Problem is that routes with same path (with path variables at same location) but with different method are grouped by path. Parameter name comes from firstly declared route that created routing node. Subsequent route declarations just add their method on that node but dont register their parameter names for the same place. So route for path /translation/: always sets parameter lang value.

echo/router.go

Line 204 in 6119aec

cn.addHandler(method, h)

so probably when method handler is added to Node we could add pNames also for that method (pull pnames down from node to near method handlers

echo/router.go

Line 32 in 6119aec

delete HandlerFunc

methodHandler struct {
...
  delete  struct {
    handler HandlerFunc
    pnames []string
  }
...
...

then every method on same router Node can have different name for parameter name at same place when match is made and we populate context with pnames/pvalues. Anyway @lammel and @pafuent should know that subject better.

@lammel
Copy link
Contributor

lammel commented Jan 1, 2021

This is currently not supported. You have to use consistent parameter naming for routes.
There were similar requests in the past though, so I guess it is time to tackle this issue.

The solution suggested by @aldas is one of the options, also a refactoring of the router to decouple routes and method handlers is an option.

@lammel
Copy link
Contributor

lammel commented Jan 1, 2021

Duplicate of #1726

@lammel lammel marked this as a duplicate of #1726 Jan 1, 2021
pafuent added a commit to pafuent/echo that referenced this issue Jan 29, 2021
From now, Echo panics if a route that was already added, is added again
but for a different HTTP method and the path params are different.
e.g.
GET /translation/:lang -> Added OK
PUT /translation/:lang -> Added OK
DELETE /translation/:id -> Panic

Partially Fixes labstack#1726 labstack#1744
@aldas aldas closed this as completed Mar 13, 2024
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

2 participants