contrib/dimfeld/httptreemux.v5: fix route and name for 30X redirects #2664
Conversation
The httptreemux router has a redirect behaviour that is invoked when a request URL and matched route only differs in a trailing slash. The default behaviour is to respond with a 301 (moved permanently) to redirect the client to the exact path of the matched handler. We previously patched one of the two scenarios where this occurs in DataDog#2332. The changes in this commit addresses the other scenario for the 301 redirect, but also the 307 and 308 redirect options. Several tests were included to cover the various scenarios. We also standardize the resource name in the event that there is a trailing slash missmatch between request URL and matched handler. We always truncate the trailing slash in the resource name. Fixes DataDog#2663
Simplied the unnecessarily verbose function comments of isSupportedRedirectStatus and routerRedirectEnabled.
The new tests added to assert the redirect behaviour for the four redirect modes supported by the router had redundant test cases. These test cases were reduced down to three scenarios per redirect mode: 1. path not found 2. path with parameterized URL excluding a trailing slash 3. path with parameterized URL inclduing a trailing slash
When handling the supported redirect modes of the router we were incorrectly trimming trailing slashes from the route path matched by the router. The previous behaviour is restored where we return the exact path matched by the router.
devillecodes
force-pushed
the
dv/2663/fix-redirect-handling
branch
from
ca7b437
to
4d3f72e
Compare
|
@rarguelloF I have addressed the feedback. Please take a look when you have a moment. Also let me know if there are any follow up actions you'd like me to look at. |
|
Hi folks. Happy Monday! Just checking in if there's anything required from me on this PR at this stage? |
|
@devillecodes No, it just needs to be merged. Allow us to take care of it during this week, before the beginning of |
|
@devillecodes To avoid blocking the start of the release train, we merged #2685 instead. It's a copy of your PR, with a small temporary fix for another issue unrelated to your PR (it's present in other PRs that we want to merge before the train starts). |
|
Amazing! Thanks folks! 🙌 Appreciate the efforts in getting this merged! 🎉 |
The httptreemux router has a redirect behaviour that is invoked when a request URL and matched route only differs in a trailing slash. The default behaviour is to respond with a 301 (moved permanently) to redirect the client to the exact path of the matched handler. We previously patched one of the two scenarios where this occurs in #2332. The changes in this commit addresses the other scenario for the 301 redirect, but also the 307 and 308 redirect options. Several tests were included to cover the various scenarios.
We also standardize the resource name in the event that there is a trailing slash missmatch between request URL and matched handler. We always truncate the trailing slash in the resource name.We no longer do this as part of these changes.Fixes #2663
What does this PR do?
There are two main changes in this PR:
Extensive tests have been added for the described changes.
Motivation
Fixes #2663.
Reviewer's Checklist
Unsure? Have a question? Request a review!