-
Notifications
You must be signed in to change notification settings - Fork 18
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
RCPP-89 Add 301/308 redirection support for HTTP transport requests #242
base: main
Are you sure you want to change the base?
Conversation
virtual void send_request_to_server(::realm::networking::request &&request, | ||
std::function<void(const response &)> &&completion) = 0; |
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.
git clang-format
formatted these files differently than what is currently configured for realm-core. The .clang-format
says it was based off the CLion configuration.
include/cpprealm/networking/http.hpp
Outdated
@@ -115,7 +115,7 @@ namespace realm::networking { | |||
[[maybe_unused]] void set_http_client_factory(std::function<std::shared_ptr<http_transport_client>()>&&); | |||
|
|||
/// Built in HTTP transport client. | |||
struct default_http_transport : public http_transport_client { | |||
struct default_http_transport : public http_transport_client, public std::enable_shared_from_this<default_http_transport> { |
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.
why do we need to change the ownership/lifetime of this type from unique to shared?
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.
I was using to ensure the object was still valid in the async callbacks. Fortunately (or unfortunately) like you said, the service object created and run for each call of this function is essentially updating this operation to be completely synchronous and blocking until the response is received from the server...
This "threading" (or lack thereof) for the transport implementation should be revisited in a separate task to ensure it is working in an appropriate asynchronous manner.
@@ -243,45 +246,45 @@ namespace realm::networking { | |||
socket.ssl_stream->set_logger(logger.get()); | |||
} | |||
|
|||
realm::sync::HTTPHeaders headers; | |||
realm::sync::HTTPClient<DefaultSocket> m_http_client = realm::sync::HTTPClient<DefaultSocket>(socket, logger); |
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 can construct this directly like realm::sync::HTTPClient<DefaultSocket> m_http_client(socket, logger)
without any assignment.
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.
Updated - I hadn't changed it from how the original code was doing it below.
m_http_client.async_request(std::move(http_req), | ||
[self = weak_from_this(), orig_request = std::move(request), cb = std::move(completion_block), redirect_count](const realm::sync::HTTPResponse &resp, const std::error_code &ec) mutable { | ||
constexpr std::string_view location_header = "location"; | ||
auto transport = self.lock(); |
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.
even though this "async_request" appears to be done asynchronously, the network::Service is actually unique to each request and is run synchronously at the end of this function. we shouldn't have to do anything to keep any of this alive for the duration of the request.
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.
thinking about this a little more, does this mean that for each redirect we're creating a new network::Service and a new resolver thread?
}); | ||
// Pass along the original request so it can be resent to the redirected location URL if needed | ||
m_http_client.async_request(std::move(http_req), | ||
[self = weak_from_this(), orig_request = std::move(request), cb = std::move(completion_block), redirect_count](const realm::sync::HTTPResponse &resp, const std::error_code &ec) mutable { |
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.
i think the signature for the handler for HTTPClient::async_request is void(HTTPResponse, std::error_code)
. so we dont need to take these by const ref.
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.
Updated - this was from the original code, and it's actually better for resp
not to be a const ref since the response object is being std::move'd when calling this function.
// to perform redirections every time. | ||
std::string redirect_url; | ||
// Grab the new location from the 'Location' header and retry the request | ||
for (auto &[key, value]: resp.headers) { |
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.
the headers here is a std::map<std::string, std::string, HeterogeneousCaseInsensitiveCompare>
so you should be able to just query for the location
header by headers.find("location")
. did that not work for some reason?
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.
Updated - for some reason I was convinced this was a different type...
// Otherwise, resend the original request to the new redirect URL path | ||
// Perform the entire operation again, since the remote host is likely changing and | ||
// a newe socket will need to be opened, | ||
orig_request.url = redirect_url; |
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.
i believe in addition to updating the url we need to remove the Authorization header from the request. I think this is the standard for how to follow redirects in various browsers like webkit/chromium. Technically you're supposed to remove the authorization header if the new request's URL is at a new origin, but webkit has historically just removed the authorization header unconditionally.
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.
Yes - I wasn't sure of everything that was being updated as part of resending the packet after a redirect request, and the work to use redirection in the curl library showed that the Authorization header is removed.
Updated to remove this header before presenting the request for a redirect.
} | ||
::realm::networking::response res{static_cast<int>(resp.status), 0, {}, resp.body ? std::move(*resp.body) : "", std::nullopt}; | ||
// Copy over all the headers | ||
for (auto &[k, v]: resp.headers) { |
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 could also do res.headers = http_headers(resp.headers.begin(), resp.headers.end())
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.
Thanks for the review - I think updating the use of the service object by the default_http_transport
class should be revisited as a separate task, since these changes will likely need additional testing than what is currently provided in the sync tests.
@@ -243,45 +246,45 @@ namespace realm::networking { | |||
socket.ssl_stream->set_logger(logger.get()); | |||
} | |||
|
|||
realm::sync::HTTPHeaders headers; | |||
realm::sync::HTTPClient<DefaultSocket> m_http_client = realm::sync::HTTPClient<DefaultSocket>(socket, logger); |
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.
Updated - I hadn't changed it from how the original code was doing it below.
}); | ||
// Pass along the original request so it can be resent to the redirected location URL if needed | ||
m_http_client.async_request(std::move(http_req), | ||
[self = weak_from_this(), orig_request = std::move(request), cb = std::move(completion_block), redirect_count](const realm::sync::HTTPResponse &resp, const std::error_code &ec) mutable { |
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.
Updated - this was from the original code, and it's actually better for resp
not to be a const ref since the response object is being std::move'd when calling this function.
// to perform redirections every time. | ||
std::string redirect_url; | ||
// Grab the new location from the 'Location' header and retry the request | ||
for (auto &[key, value]: resp.headers) { |
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.
Updated - for some reason I was convinced this was a different type...
include/cpprealm/networking/http.hpp
Outdated
@@ -115,7 +115,7 @@ namespace realm::networking { | |||
[[maybe_unused]] void set_http_client_factory(std::function<std::shared_ptr<http_transport_client>()>&&); | |||
|
|||
/// Built in HTTP transport client. | |||
struct default_http_transport : public http_transport_client { | |||
struct default_http_transport : public http_transport_client, public std::enable_shared_from_this<default_http_transport> { |
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.
I was using to ensure the object was still valid in the async callbacks. Fortunately (or unfortunately) like you said, the service object created and run for each call of this function is essentially updating this operation to be completely synchronous and blocking until the response is received from the server...
This "threading" (or lack thereof) for the transport implementation should be revisited in a separate task to ensure it is working in an appropriate asynchronous manner.
// Otherwise, resend the original request to the new redirect URL path | ||
// Perform the entire operation again, since the remote host is likely changing and | ||
// a newe socket will need to be opened, | ||
orig_request.url = redirect_url; |
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.
Yes - I wasn't sure of everything that was being updated as part of resending the packet after a redirect request, and the work to use redirection in the curl library showed that the Authorization header is removed.
Updated to remove this header before presenting the request for a redirect.
What, How & Why?
Since the 301/308 http redirection support is being removed from Core via realm/realm-core#7996, these changes add support to the CPP SDK network transport to support redirections.
Fixes #239