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

Unclear behaviour of * in routes #2619

Open
3 tasks done
izaakschroeder opened this issue Apr 5, 2024 · 8 comments
Open
3 tasks done

Unclear behaviour of * in routes #2619

izaakschroeder opened this issue Apr 5, 2024 · 8 comments
Assignees

Comments

@izaakschroeder
Copy link

Issue Description

Checklist

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

Expected behaviour

  • If I have static text in a route that static text is guaranteed to show up in c.Path() or c.Request().RequestURI
  • Multiple instances of * in routes play nicely with each other

Actual behaviour

  • If I have static text in a route, another route with * may "absorb it"
  • Multiple instances of * in routes do NOT seem to play nicely with each other

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
Copy link
Contributor

aldas commented Apr 5, 2024

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
Copy link
Author

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
Copy link
Contributor

aldas commented Apr 5, 2024

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
Copy link
Contributor

aldas commented Apr 6, 2024

Linking recent similar issue #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
Copy link
Contributor

aldas commented Apr 6, 2024

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
Copy link
Contributor

aldas commented Apr 6, 2024

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
Copy link
Author

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 😓

@iamgoroot
Copy link

#2657
Created PR to address this issue. Would like to hear some feedback (especially from people who worked with that code)

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

No branches or pull requests

3 participants