Skip to content

Commit

Permalink
Updates from review
Browse files Browse the repository at this point in the history
  • Loading branch information
Michael Wilkerson-Barker committed Aug 29, 2024
1 parent 011cc91 commit c85aaf4
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 37 deletions.
10 changes: 5 additions & 5 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ NEXT_RELEASE Release notes (YYYY-MM-DD)
even if a sync configuration is not supplied.
* Added 301/308 http redirect response support to the default HTTP transport, since it has been
removed from Core and is the responsibility of the SDKs to handle the redirect operation. A
redirect should rarely be received from teh server, and typically happens after changing the
deployment server for the cloud app or the base URL is configured to connect to the wrong host.
redirect should rarely be received from the server, and typically happens after changing the
deployment model for the cloud app or the base URL is configured to connect to the wrong host.
If a custom HTTP transport is provided by the developer, it should also support handling
redirect responses with a status code of 301 or 308 and not change the request method from POST
to GET when resending the request.
Add `managed<std::map<std::string, T>>::contains_key` for conveniently checking if a managed map
redirect responses with a status code of 301 or 308, remove the Authorization header and
retain the original HTTP method when re-sending requests after a redirect.
* Add `managed<std::map<std::string, T>>::contains_key` for conveniently checking if a managed map
contains a given key. Use this method in the Type Safe Query API instead of `managed<std::map<std::string, T>>::find`.

### Compatibility
Expand Down
2 changes: 1 addition & 1 deletion include/cpprealm/networking/http.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, public std::enable_shared_from_this<default_http_transport> {
struct default_http_transport : public http_transport_client {
struct configuration {
/**
* Extra HTTP headers to be set on each request to Atlas Device Sync when using the internal HTTP client.
Expand Down
49 changes: 18 additions & 31 deletions src/cpprealm/internal/networking/network_transport.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ namespace realm::networking {
socket.ssl_stream->set_logger(logger.get());
}

realm::sync::HTTPClient<DefaultSocket> m_http_client = realm::sync::HTTPClient<DefaultSocket>(socket, logger);
realm::sync::HTTPClient<DefaultSocket> m_http_client(socket, logger);
auto convert_method = [](::realm::networking::http_method method) {
switch (method) {
case ::realm::networking::http_method::get:
Expand Down Expand Up @@ -300,53 +300,40 @@ namespace realm::networking {
if (ec.value() == 0) {
// 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 {
[this, orig_request = std::move(request), cb = std::move(completion_block), redirect_count](realm::sync::HTTPResponse resp, std::error_code ec) mutable {
constexpr std::string_view location_header = "location";
auto transport = self.lock();
constexpr std::string_view authorization_header = "authorization";
// If an error occurred or the transport has gone away, then send "operation aborted" to callback
if (ec || !transport) {
if (ec) {
cb({0, util::error::operation_aborted, {}, {}, std::nullopt});
return;
}
// Was a redirect response (301 or 308) received?
if (resp.status == realm::sync::HTTPStatus::PermanentRedirect || resp.status == realm::sync::HTTPStatus::MovedPermanently) {
auto max_redirects = transport->m_configuration.max_redirect_count;
auto max_redirects = m_configuration.max_redirect_count;
// Are redirects still allowed to continue?
if (max_redirects < 0 || ++redirect_count < max_redirects) {
// A possible future enhancement could be to cache the redirect URLs to prevent having
// 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) {
if (key.size() == location_header.size() &&
std::equal(key.begin(), key.end(), location_header.begin(), location_header.end(), [](char a, char b) {
return std::tolower(static_cast<unsigned char>(a)) == std::tolower(static_cast<unsigned char>(b));
})) {
// If the redirect URL path returned from the server was not empty, save it and exit the loop
if (!value.empty()) {
redirect_url = value;
break;
if (auto location = resp.headers.find(location_header); location != resp.headers.end()) {
if (!location->second.empty()) {
// Perform the entire operation again, since the remote host is likely changing and
// a new socket will need to be opened,
orig_request.url = location->second;
// Also remove the authorization header before forwarding the request to the new
// location, to prevent leaking the access token to an unauthorized server
if (auto authorization = resp.headers.find(authorization_header); authorization != resp.headers.end()) {
resp.headers.erase(authorization);
}
// Otherwise, keep looking, in case there's another 'location' entry
return send_request_to_server(std::move(orig_request), std::move(cb), redirect_count);
}
}
// If the redirect URL path wasn't found in headers, then send the response to the client...
if (!redirect_url.empty()) {
// 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;
return transport->send_request_to_server(std::move(orig_request), std::move(cb), redirect_count);
}
// If redirects disabled, max redirect reached or location was missing from response, then pass the
// redirect response to the callback function
}
// If redirects disabled, max redirect reached or location was missing from response, then pass the
// redirect response to the callback function
}
::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) {
res.headers[k] = v;
}
::realm::networking::response res{static_cast<int>(resp.status), 0, {resp.headers.begin(), resp.headers.end()}, resp.body ? std::move(*resp.body) : "", std::nullopt};
cb(res);
});
} else {
Expand Down

0 comments on commit c85aaf4

Please sign in to comment.