Keep arguments in redirections #2800
Conversation
github-actions
bot
added
the
S-waiting-on-review
There was a problem hiding this comment.
we probably also should update the rustdoc_redirector_handler to forward the query args.
Also we should see if the trailing-slash-redirect we're using also forwards the query (not sure if we should spend time on fixing it if it doesn't).
|
Only using the |
did you intentionally leave out the redirector handler? |
|
There was only one case which needed it as most of them are already using |
| format!("/crate/{crate_name}/{}", matched_release.req_version), | ||
| EscapedURI::new( | ||
| &format!("/crate/{crate_name}/{}", matched_release.req_version), | ||
| uri.query(), |
There was a problem hiding this comment.
I'm missing a test for this updated behaviour here.
There was a problem hiding this comment.
I can't figure out what's the URL for it. I tried /crate/sysinfo/0.29.0?search=q and /crate/sysinfo/0.40.0?search=q and both have the query arguments even before this change. Any idea?
There was a problem hiding this comment.
the redirector is behind URLs like /sysinfo
There was a problem hiding this comment.
ah wait, you're searching for an example for this specific else part
There was a problem hiding this comment.
you will only reach it when rustdoc_status is false.
That being said, should we update the query_pairs in the redirect_to_doc function in here too to your new EscapedURI?
There was a problem hiding this comment.
Not sure it's worth it considering we need to overload the search query if it exists and then we make use of axum_parse_uri_with_params.
There was a problem hiding this comment.
you will only reach it when
rustdoc_statusisfalse.
Can't find a way to write a test with this case. Do we have a test with this situation by any chance?
|
generally: with all the github noise I typically work with the personal "review requests" tab quite much, and check these more often than notifications in general, so re-requesting a review when it's ready again really helps me. ( I'll see the other notifications later too, but not as fast) |
Noted! |
Fixes #2786.
I created a new type to ensure that the escaping was done only once.