labstack / echo

High performance, minimalist Go web framework
https://echo.labstack.com
MIT License
29.01k stars 2.21k forks source link

Unclear behaviour of `*` in routes #2619

Open izaakschroeder opened 3 months ago

izaakschroeder commented 3 months ago

Issue Description

Checklist

Expected behaviour

Actual behaviour

Steps to reproduce

Trying to build router for: https://github.com/opencontainers/distribution-spec/blob/main/spec.md

handler := func (c echo.Context) err {
  fmt.Printf("%s\n", c.Path())
}

e := echo.New()
v2 := e.Group("/v2")

v2.DELETE("/*/blobs/:digest", handler)
v2.GET("/*/blobs/:digest", handler)
v2.HEAD("/*/blobs/:digest", handler)

v2.DELETE("/*/manifests/:ref", handler)
v2.GET("/*/manifests/:ref", handler)
v2.HEAD("/*/manifests/:ref", handler)
v2.PUT("/*/manifests/:ref", handler)

v2.GET("/*/tags/list", handler)

v2.GET("/*/blobs/uploads/:ref", handler)
v2.PATCH("/*/blobs/uploads/:ref", handler)
v2.POST("/*/blobs/uploads", handler)
v2.PUT("/*/blobs/uploads/:ref", handler)

v2.GET("", handler)

Run:

curl -ik  https://localhost:8080/v2/foo/bar/baz/tags/list

# /v2/*/blobs/uploads/:ref

Result outputs /v2/*/blobs/uploads/:ref when it should output /v2/*/tags/list.

Version/commit

v4.11.4

aldas commented 3 months ago

Current Router behavior/limitation with * is - if there is a * in registered route - it will mean that everything in that segment and after that becomes *. So "/*/blobs/uploads/:ref" is actually used as "/v2/*" and as /*/blobs/uploads/:ref was last router registered and it starts with prefix (/v2/*) it will be the route that Router will match.

TLDR: router does not have functionality to handle * routes with static suffixes i.e. */list. That path is used actually as *

izaakschroeder commented 3 months ago

Thanks for the fast reply @aldas 😄 Is there any plan to change this? I saw in the router test file there is something that looks a little similar: https://github.com/labstack/echo/blob/master/router_test.go#L924

aldas commented 3 months ago

This has been raised before. It seems that routes like that are popular lately. I would like Router to have that feature but I am not sure this will happen any time soon. At least 1 month time frame.

If you would like to delve into Router internals and maybe submit PR for it. I would gladly review it.

ps. I'll investigate this feature this weekend. maybe it is not that complex that fear it to be.

aldas commented 3 months ago

Linking recent similar issue https://github.com/labstack/echo/issues/2617

p.s. I proposed workaround in that case. It is not pretty but works for suffix matches.


This is side note for myself or any person reading this in the future.

Problem with matching parts of request URL to route params (including wildcard *) is that we need to start considering cases when there are multiple params in route.

Defining rules for suffixes

For example:

    e.GET("/*/list", handler)
    e.GET("/*/images", handler)

and request URL GET /gimp/images/jpg/list. is easy. both cases the param is gimp/images/jpg

now consider this case:

    e.GET("/*/list*", handler) // 1
    e.GET("/*/images*", handler)  // 2

and request URL GET /gimp/images/jpg/list/1. What rules we should apply? a) shortest url path match wins? i.e. /*/images* b) the order routes are added? i.e. /*/list*

I looked into different router implementation in Go ecosystem and there are different approaches. I personally do not think that the order in routes added should change how router matches but this would cost less CPU cycles and deciding which routes matches with shortest match means that we need to check more cases.

aldas commented 3 months ago

I'll update echo documentation with remark that * means that everything after that is considered as match. and multiple * in route does not work.

aldas commented 3 months ago

note to self: It would be worthwhile to investigate how Nginx/Apache do path matching. These should have solved same issues 15-20 years ago already. And we are about to reinvent the wheel here.

some other links:

izaakschroeder commented 3 months ago

Thanks for looking into this a bit more @aldas 😄 I will keep my eyes out for any developments! I'm not sure if I have the brains enough myself to go spelunking through the routing code to fix this one myself 😓