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

Path parsing issue #366

Open
Seemone opened this issue Jan 21, 2025 · 14 comments · May be fixed by #371
Open

Path parsing issue #366

Seemone opened this issue Jan 21, 2025 · 14 comments · May be fixed by #371
Assignees
Labels
bug Something isn't working help wanted We need your opinion and suggestions! OD Boost Issues for the OnlyDust event

Comments

@Seemone
Copy link

Seemone commented Jan 21, 2025

To Reproduce
Steps to reproduce the behavior:

when creating routes like this

        bookGroup := fuego.Group(s, "/book")
	fuego.Get(bookGroup, "/", rs.getAllBook)
	fuego.Post(bookGroup, "/", rs.postBook)

	fuego.Get(bookGroup, "/{id}", rs.getBook)
	fuego.Put(bookGroup, "/{id}", rs.putBook)
	fuego.Delete(bookGroup, "/{id}", rs.deleteBook)

requests like

GET http://localhost:9999/book/43/

are routed tors.getAllBook instead of rs.getBook while requests like

GET http://localhost:9999/book/43

are correctly routed to rs.getBook

Versions
Go v1.23.5
Fuego v0.17.0

@Seemone Seemone added the bug Something isn't working label Jan 21, 2025
@dylanhitt
Copy link
Collaborator

hmmm. Maye you have a typo here?

I can't tell different between your to sample requests you provide.

GET http://localhost:9999/book/43/

and

GET http://localhost:9999/book/43/

Appear to be the same.

@Seemone
Copy link
Author

Seemone commented Jan 21, 2025

Apologies, typo indeed. Corrected

@EwenQuim
Copy link
Member

EwenQuim commented Jan 22, 2025

Good issue for people not coming from a Go background, we should definitely tackle this.

That is expected from the Go 1.22 default router. You can see by yourself on this Go playground I set up for you.

  • /book/43/ does NOT match /book/{id} (missing ending slash)
  • /book/{id} does match /book/ because of a special rule in Go : if it does not contains wildcard AND end with a trailing slash, it matches if the beginning matches

That being said, what can we do with Fuego?

  • 1️⃣ Strip ending slashes from requests (optionally ofc, what should be the default?)
  • 2️⃣ Strip ending slashes from route declarations (also optionally ofc, what should be the default?)
  • 3️⃣ Nothing and let the user make the correct request (current solution)

My suggestion : 3️⃣ I think that current behaviour is expected for gophers and make sense. I don't want to dive into routing, I want to rely heavily on Go's defaults

We can add an option for 1️⃣ (that should be deactivated by default to keep Fuego's philosophy in mind), a bit like chi's middleware RedirectSlashes or StripSlashes. But if we do 1️⃣, requests like /books/ will be rewritten as /books and will not match /books/ anymore, which implies to do 2️⃣ as well (or to let users declare routes like fuego.Get(s, "", myController) which looks weird)

@EwenQuim EwenQuim added help wanted We need your opinion and suggestions! OD Boost Issues for the OnlyDust event labels Jan 22, 2025
@GideonBature
Copy link

Could I take a shot at this?

@danielrobotz
Copy link

Could I try solving this?

@EwenQuim
Copy link
Member

Well, as spec is not really well defined, please state what you'll be doing first.

If you're going for what I've suggested in my previous comment, just know that Fuego's team might make a lot of comments and it might not be merged at all, depending on if we're happy with what is proposed :) So no guarantee of it being merged in the end :)

@sonkeydotcom
Copy link

Would love to tackle this! I’m an experienced full-stack developer with strong expertise in Golang and building robust APIs. I’d like to help resolve this routing issue by investigating how Fuego handles trailing slashes in route definitions. I will ensure that the behavior is consistent for routes like /book/{id} and /book/{id}/ to correctly map to the intended handlers. Looking forward to contributing and improving the framework!

@NueloSE
Copy link

NueloSE commented Jan 22, 2025

am a developer specializing in Golang API development, I'm eager to address the trailing slash routing challenge. This is a common issue in Go web frameworks, and I'll focus on implementing a consistent solution for Fuego's route handling. By examining how routes like /book/{id} are processed, we can ensure proper handler mapping regardless of trailing slashes. I'm committed to enhancing Fuego's routing functionality while maintaining its idiomatic Go approach

@fabrobles92
Copy link

Hello I am Fab a full stack engineer that has contributed to Go web3 projects in the past like https://github.com/concrete-eth/concrete-geth. I would love to address this issue by fixing the path parsing issue

@Seemone
Copy link
Author

Seemone commented Jan 22, 2025

to my knowledge a URL is equivalent to the same URL + "/" (this implies any number of trailing slashes).
The current implementation leads to ambiguous result and at least have an option to strip slashes would be very useful.

@EwenQuim
Copy link
Member

Is my solution with ideas 2️⃣ and 3️⃣ OK for you @Seemone ?

@Seemone
Copy link
Author

Seemone commented Jan 22, 2025

you mean 1 and 2? in that case yes. An option to strip trailing slashes (default false) seems sensible to me. Personally I don't find "" routes weird

@EwenQuim
Copy link
Member

Yes sorry, I meant 1 and 2.

I found that declaring /books handler does not match the requests ending in /books/, so we definitely need 1 and 2 simulteanously.

Proof of concept what do you think @dylanhitt?

@NueloSE you didn't reply and the issue is currently assigned to you, what is your personal opinion?

@NueloSE NueloSE linked a pull request Jan 23, 2025 that will close this issue
@dylanhitt
Copy link
Collaborator

Yes that is fine. I'm for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted We need your opinion and suggestions! OD Boost Issues for the OnlyDust event
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants