radius-project / radius

Radius is a cloud-native, portable application platform that makes app development easier for teams building cloud-native apps.
https://radapp.io
Apache License 2.0
1.51k stars 97 forks source link

Gorilla Mux has weird behavior for trailing slashes #2303

Closed rynowak closed 1 year ago

rynowak commented 2 years ago

Found by @vinayada1 while working on UCP

Gorilla Mux is unfortunately sensitive to whether a trailing / is defined as part of the route, and none of their built-in behaviors make sense for us.

// Matches '/foo' but not '/foo/'
router.Path("/foo")

// Matches '/foo/' but not '/foo'
router.Path("/foo/")

They have a feature called strict slash but unfortunately this uses redirects to normalize the / instead of just matching both. Many API clients do not follow redirects and especially do not follow redirects correctly for non-idempotent verbs.

It's easy to test with az rest that ignoring a trailing / is the right before for ARM.

We need to standardized the correct behavior for ARM, and either find a solution to implement this with Gorilla Mux, or change to another library.

AB#6867

youngbupark commented 2 years ago

go-chi has the middleware to resolve the trailing / - https://github.com/go-chi/chi/blob/master/middleware/strip.go If we want to keep using gorilla mux, then we can implement similar middleware to strip /

Personally, I love go-chi because it is still actively developed and has many useful middleware with 100% compat go net/http lib. go-chi is faster than gorilla mux since it uses radix tree search whereas mux uses linear search. The other web frameworks such as gin, echo, fasthttp-router are incompatible with the standard net/http lib.

rynowak commented 2 years ago

I like the idea of switching to something more actively maintained. I noticed that gorilla mux has been looking for a new maintainer, which wasn't the case when I picked it a year ago 😆

I'll start reading about go-chi

We should get more eyeballs and this and avoid a rushed decision.

jkotalik commented 2 years ago

From my experience, I haven't had an amazing time with gorilla mux, especially around middleware handling and random path issues. If moving to go-chi is low cost relatively, I'm in favor of moving.

rynowak commented 2 years ago

From my experience, I haven't had an amazing time with gorilla mux, especially around middleware handling and random path issues. If moving to go-chi is low cost relatively, I'm in favor of moving.

I think anything we wanted to adopt anything that uses http.Handler or something similar it should be relatively easy.

shalabhms commented 1 year ago

@rynowak , is this issue still valid?

rynowak commented 1 year ago

Yes

rynowak commented 1 year ago

Not urgent though

rynowak commented 1 year ago

Actually it looks like gorilla-mux is officially unmaintained now https://github.com/gorilla#gorilla-toolkit

AaronCrawfis commented 1 year ago

@rynowak @vinayada1 @youngbupark can this be closed now that we've moved to go-chi?

rynowak commented 1 year ago

We should verify the current behavior. The goal is still for our API to work well.

youngbupark commented 1 year ago

@rynowak @vinayada1 @youngbupark can this be closed now that we've moved to go-chi?

go chi has out of box middleware to strip trailing slashes. - https://github.com/go-chi/chi/blob/99bc99e0bf0edc147e82765f2848167922a5c644/middleware/strip.go#L13

We can simply add this middleware to resolve this problem.

youngbupark commented 1 year ago

@rynowak @vinayada1 @youngbupark can this be closed now that we've moved to go-chi?

go chi has out of box middleware to strip trailing slashes. - https://github.com/go-chi/chi/blob/99bc99e0bf0edc147e82765f2848167922a5c644/middleware/strip.go#L13

We can simply add this middleware to resolve this problem.

I just confirm that it works without any additional changes. I am closing this issue.

image