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

Add support for wildcard paths with other children #329

Conversation

rw-access
Copy link

@rw-access rw-access commented Mar 21, 2021

I updated the search tree, so that non-wildcard paths are resolved before wildcard paths, but both can be valid children. For instance, now you can finally have /users/roles/:role and /users/:id at the same time without issue.

First, non-wildcard children are checked for matches. If those fail to match, then it tries the wildcard children routes. For existing compatible route configurations, there should be no (or negligible) performance impact. Many people prefer compact routes and expect a sort of precedence with httprouter, that mirrors this behavior.

This only works for param wildcards of the form :name and doesn't change the existing *

One thing to note, because of the prefix tree structure, if the path /books/fantasy/2007 would matched a /books/fantasy/... route because it takes priority over a /books/: category/:year template route, because exact prefixes take priority over wildcard prefixes. I think this is okay and is still a net positive. This can be merely a documentation issue.

Overall, I think the changes capture a pretty intuitive behavior, has little to no performance impact and is something many people expect from a router. Personally, I use gin and fizz and would love to have this behavior. So I played around with it and want to share and gather your thoughts @julienschmidt.

Related issues
Looks like there's a lot of issues in this repo, but mostly in Gin. I did my best to capture them, but I'm sure that I'm missing some.

Partially resolves #73 (doesn't work for catch-all wildcards)
Resolves #91
Resolves #183
Resolves #202
Resolves #203

Resolves gin-gonic/gin#136
Resolves gin-gonic/gin#205
Resolves gin-gonic/gin#360
Resolves gin-gonic/gin#388
Resolves gin-gonic/gin#574
Resolves gin-gonic/gin#957
Resolves gin-gonic/gin#1148
Resolves gin-gonic/gin#1065
Resolves gin-gonic/gin#1132
Resolves gin-gonic/gin#1301
Resolves gin-gonic/gin#1681
Resolves gin-gonic/gin#1730
Resolves gin-gonic/gin#1756
Resolves gin-gonic/gin#1990
Resolves gin-gonic/gin#1993
Resolves gin-gonic/gin#2005
Resolves gin-gonic/gin#2016
Resolves gin-gonic/gin#2062
Resolves gin-gonic/gin#2195
Resolves gin-gonic/gin#2289
Resolves gin-gonic/gin#2416
Resolves gin-gonic/gin#2465
Resolves gin-gonic/gin#2537

@rw-access
Copy link
Author

Hey @julienschmidt any thoughts on this?
I'll make this PR to gin, which is a dependency for a project of mine. I'm glad to do that, but want to give you a chance to share your thoughts first.

@julienschmidt
Copy link
Owner

julienschmidt commented Mar 27, 2021

Hi @rw-access, thanks a lot for this PR!
This has been on my to-do list for a long time.

Three comments:

  1. Unfortunately I won't find time to properly review this until next weekend.
  2. My original plan was to introduce two routing modes: The "original" strict mode and this new relaxed mode, where the relaxed mode would be the default. Most likely the actual routing algo can be the same, just the checks while building the tree would be stricter in the strict mode. This strict mode sometimes is desirable, as there is no other way to guarantee that there are no two possible routes for a path.
  3. As you already said, these short-comings definitely have to be documented.

P.S.: slightly embarrassing how many typos you found 👀

@rw-access
Copy link
Author

sounds good, thanks for responding. whenever you get around to it, just let me know what changes you would like. or if you think a follow up PR is better, I'm fine with that too

@ptesar-xyndata
Copy link

@julienschmidt @rw-access Hello guys, I know you have a lot of work, but how it looks with this PR? :) Thx a lot!

@darienmiller88
Copy link

Is it possible to know make Gin apps that no longer suffer from the issue of path prefix conflicts, i.e
example/:id
exampe/:id/example2
?

@ibraheemdev
Copy link

ibraheemdev commented May 13, 2021

For anyone following along, this PR was merged into gin and some issues emerged:

"/get/test/abc/" = value1
"/get/:param/abc/" = value2

GET /get/test/abc/ => value1
GET /get/xyz/abc/ => value2
GET /get/tt/abc/ => 404 (should be value2)

This is being adressed in gin-gonic/gin#2706.

@lallinger-arbeit
Copy link

This is being adressed in gin-gonic/gin#706.

I think you mean gin-gonic/gin#2706 ?

@ibraheemdev
Copy link

Oh, my bad, yes :)

@dkkb
Copy link

dkkb commented May 19, 2021

The number of issues is amazing.

@marshall-lee
Copy link

marshall-lee commented Aug 19, 2021

@rw-access hey why did you close your PR? is there anything we could help?

@rw-access
Copy link
Author

rw-access commented Aug 19, 2021

Because we found some bugs in the Gin implementation (I think they have been fixed) and also there's been no traction here, so I don't want the open PR to be misleading and appear done.

It's still a solvable problem and the code here can be updated to handle it, but it needs to be prioritized by the maintainer(s) here.

@pavelpatrin
Copy link

Guys maybe we can start some crowdfunding or just paid directly to author to get it released? @rw-access
I can't pay much, but f.e. 20$ for me are real.

similark pushed a commit to similarweb/httprouter that referenced this pull request May 9, 2023
Bumps [k8s.io/client-go](https://github.com/kubernetes/client-go) from 0.22.2 to 0.22.4.
- [Release notes](https://github.com/kubernetes/client-go/releases)
- [Changelog](https://github.com/kubernetes/client-go/blob/master/CHANGELOG.md)
- [Commits](kubernetes/client-go@v0.22.2...v0.22.4)

---
updated-dependencies:
- dependency-name: k8s.io/client-go
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment