-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
http: to_reply()
from redirect_exception
#2206
base: master
Are you sure you want to change the base?
http: to_reply()
from redirect_exception
#2206
Conversation
ac23cf0
to
6a41444
Compare
This function allows for the creation of an `http::reply` from a thrown `redirect_exception`, with a url for the `Location` header and an optional `Retry-After` timeout value.
6a41444
to
6196a68
Compare
@nyh , please review |
redirect_exception(const std::string& url, http::reply::status_type status = http::reply::status_type::moved_permanently) | ||
: base_exception("", status), url(url) { | ||
redirect_exception(const std::string& url, http::reply::status_type status = http::reply::status_type::moved_permanently, const std::optional<int>& retry_after = std::nullopt) | ||
: base_exception("", status), url(url), retry_after(retry_after) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should mention in the commit message (and perhaps even split the commit into two) that one of the things that this patch does is to add to add Retry-After header (see https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.3) support for redirections.
It is separate feature from the to_reply() function you are adding in this patch.
@@ -94,6 +93,9 @@ future<std::unique_ptr<http::reply> > routes::handle(const sstring& path, std::u | |||
handler->verify_mandatory_params(*req); | |||
auto r = handler->handle(path, std::move(req), std::move(rep)); | |||
return r.handle_exception(_general_handler); | |||
} catch (const redirect_exception& _e) { | |||
*rep = _e.to_reply(); | |||
rep->done("json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This catch was missing previously. It looks like it fixes a bug - can you explain in the commit message what bug this was? Can you perhaps write a unit test for it?
The commit message made it sound like your patch only added a new API function, but as I noted in the review above, it actually included two more separate changes:
|
This PR adds
redirect_exception::to_reply()
for construction of ahttp::reply
from a redirect case, with aLocation
header and optionally aRetry-After
header.