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

Router not working with custom methods after parameter #2264

Closed
3 tasks done
dalmarcolucas opened this issue Sep 5, 2022 · 4 comments
Closed
3 tasks done

Router not working with custom methods after parameter #2264

dalmarcolucas opened this issue Sep 5, 2022 · 4 comments
Labels

Comments

@dalmarcolucas
Copy link

dalmarcolucas commented Sep 5, 2022

Issue Description

Router Find not working with "custom methods".
It seems that the fix made in issue #2046, does not deal with routes that have a custom method after parameter, (examples in the code below)

Checklist

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

Expected behaviour

Match routes with custom actions and parameters

Actual behaviour

Overwriting routes, when it should treat as a different route

Steps to reproduce

Code bellow

Working code to debug

package main

import (
	"strings"
	"testing"

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

func TestEscape(t *testing.T) {
	e := echo.New()

	router := echo.NewRouter(e)

	router.Add(strings.ToUpper("PUT"), "/api/users/:id", void)
	router.Add(strings.ToUpper("POST"), "/api/users/:id\\:customAction", void)
	router.Add(strings.ToUpper("POST"), "/api/users/:id\\:otherAction", void)

	echoContext := e.NewContext(nil, nil)

	router.Find("GET", "/api/users/123", echoContext)
	genericRoute := echoContext.Path()
	assert.Equal(t, "/api/users/:id", genericRoute)

	router.Find("POST", "/api/users/123:customAction", echoContext)
	genericRouteCustomAction := echoContext.Path()
	assert.Equal(t, "/api/users/:id:customAction", genericRouteCustomAction)

	router.Find("POST", "/api/users/123:otherAction", echoContext)
	genericRouteOtherAction := echoContext.Path()
	assert.Equal(t, "/api/users/:id:otherAction", genericRouteOtherAction)

      //All "Finds" calls return the "/api/users/:id\\:otherAction" route
}

func void(c echo.Context) error {
	return nil
}

Version/commit

v4.9.0

@aldas
Copy link
Contributor

aldas commented Sep 5, 2022

At the moment \\:verb only work when path part between slashes is static. Basically \\: means that : is not considered as start of path parameter when route is added. When there is a parameter i.e. :id between slashes its name is parsed always to the next slash or end of the route path. So in case of "/api/users/:id\\:otherAction" the parameter name that is registered for route is actually id\:otherAction.

Routes "/api/users/:id\\:customAction" and "/api/users/:id\\:otherAction" are parsed as /api/users/: path to be matched for router and their only difference is that parameter name is different - therefore latter overwrites former when you add those routes.

I would say this is working as intended - due to the limitations or design choices how parameters work.

@aldas aldas added the router label Sep 5, 2022
@dalmarcolucas
Copy link
Author

@aldas thanks! It makes sense, I agree its working as intended, but at the moment, there is maybe a workaround that would make the router match work for these routes now?
And will this match be possible in the future?

@aldas
Copy link
Contributor

aldas commented Sep 6, 2022

I think currently you need to resort to (maybe) creating POST route for path /api/users/:idverb and inside that handler split parameter idverb into two and then have a switch statement for each verb types.

something like that:

func otherAction(c echo.Context, id int64) error {
	return c.JSON(http.StatusOK, map[string]int64{"id": id})
}

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

	e.GET("/api/users/:idverb", func(c echo.Context) error {
		idStr, verb, ok := strings.Cut(c.Param("idverb"), ":")
		if !ok {
			return echo.NewHTTPError(http.StatusBadRequest, "missing verb")
		}
		id, err := strconv.ParseInt(idStr, 10, 64)
		if err != nil {
			return echo.NewHTTPError(http.StatusBadRequest, "invalid id value")
		}

		switch verb {
		case "otherAction":
			return otherAction(c, id)
		default:
			return echo.NewHTTPError(http.StatusBadRequest, "invalid verb value")
		}
	})

	if err := e.Start(":8080"); err != http.ErrServerClosed {
		e.Logger.Fatal(err)
	}
}

this is not that eloquent but should work.

@dalmarcolucas
Copy link
Author

dalmarcolucas commented Sep 22, 2022

Thanks again @aldas, but actually I have one permission service, where with base on the specific request, I need to find the generic route to check if the user has permission to access the endpoint. So this workarround wont work for me.

I changed to mux that works as expected with custom methods

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