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

Replace RedirectSlashes with smartRedirectSlashes #3385

Closed
wants to merge 1 commit into from

Conversation

tchssk
Copy link
Member

@tchssk tchssk commented Oct 6, 2023

Related to #3366

RedirectSlashes middleware was not suitable for Goa.

RedirectSlashes redirects a request with a trailing slash to the same path without slash.
It also works for patterns with a trailing slash, so they are no longer accesible.
(@ikawaha told me that. Thank you so much.)

I replaced the middleware with a new one.
It redirects both paths with and withohut a trailing slash according to defined patterns.

@tchssk tchssk marked this pull request as ready for review October 6, 2023 05:27
@ikawaha
Copy link
Contributor

ikawaha commented Oct 6, 2023

FYI: @tchssk

case 1: mux router

router := httptreemux.New()
router.GET("/about", pageHandler)
router.GET("/posts/", postIndexHandler)
router.POST("/posts", postFormHandler)

case 2: chi router

router := chi.NewRouter()
router.Get("/about", pageHandler)
router.Get("/posts/", postIndexHandler)
router.Post("/posts", postFormHandler)

case 3: chi + RedirectSlashes

router := chi.NewRouter()
router.Use(middleware.RedirectSlashes)
router.Get("/about", pageHandler)
router.Get("/posts/", postIndexHandler)
router.Post("/posts", postFormHandler)
mux GET:/about --- GET:/posts/ --- POST:/posts ---
access GET:/about GET:/about/ GET:/posts/ GET:/posts POST:/posts POST:/posts/
treemux /about /about /posts/ /posts/ /posts/ /posts/
chi /about 404 /posts/ 405 /posts 405
chi+RedirectSlashes /about /about 301 301 /posts /posts

What would be the accessibility of this chi+smrtRedirectSlashes combination?

@tchssk
Copy link
Member Author

tchssk commented Oct 6, 2023

If patterns are defined in design as

GET("/about")
GET("/posts/")
POST("/posts")

the routings would be as follows

Request GET /about GET /about/ GET /posts/ GET /posts POST /posts POST /posts/
httptreemux /about 301 (/about) /posts/ 301 (/posts/) 301 (/posts/) /posts/
chi /about 404 /posts/ 405 /posts 405
chi+RedirectSlashes /about 301 (/about) 301 (/posts) 405 /posts 301 (/posts)
chi+smartRedirectSlashes /about 301 (/about) /posts/ 301 (/posts/) /posts 301 (/posts)

POST /posts and POST /posts/ are not compatible between httptreemux and smartRedirectSlashes.

In httphtreemux, if a pattern with a trailing slash is added for a method , all other methods for that pattern will also be considered to have a trailing slash. This is not covered by smartRedirectSlashes because it's about routing and needs to be resolved when registering the handlers.

@ikawaha
Copy link
Contributor

ikawaha commented Oct 6, 2023

LGTM, if backward compatibility with treemux is important.
It may be an option not to use such a redirection at all, since chi's router does only strict routing.

@raphael
Copy link
Member

raphael commented Oct 13, 2023

Great discussion!

In httphtreemux, if a pattern with a trailing slash is added for a method , all other methods for that pattern will also be considered to have a trailing slash.

Maybe we have an opportunity to remove this implicit behavior as we move to chi. We do need to consider backwards compatibility though. Here are my thoughts:

  • We add the SmartRedirectSlashes as a public and well documented function in the Goa http package. In particular the header comment could explain that it exists to make the behavior similar to httptreemux and that the middleware should be mounted on the mux.
  • We do not mount that middleware by default, that is by default the Goa mux uses strict routing.

This way the new behavior is less surprising but users that rely on the implicit redirect can mount the middleware.

@tchssk
Copy link
Member Author

tchssk commented Oct 14, 2023

That makes sense.
I'll move the middleware under http package.

@tchssk
Copy link
Member Author

tchssk commented Oct 14, 2023

@tchssk tchssk closed this Oct 14, 2023
@tchssk tchssk deleted the http-mux-smartredirectslashes branch October 14, 2023 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants