Skip to content
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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

RCPP-89 Add 301/308 redirection support for HTTP transport requests #242

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3db8f96
Added initial redirect support for CPP SDK
Aug 28, 2024
8918f13
Updated changelog
Aug 28, 2024
08490c0
Added max redirect support; reverted debug output that was left on
Aug 28, 2024
011cc91
Clang format
Aug 28, 2024
c85aaf4
Updates from review
Aug 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
.DS_Store
/.build
/build.debug
/Packages
/*.xcodeproj
.idea/
/.vscode
/.swiftpm
xcuserdata/
DerivedData/
Expand All @@ -10,5 +13,4 @@ Package.resolved
examples/*.pro.user
docs/html
docs/latex
.idea/
realm-core/src/realm/parser/generated/
realm-core/src/realm/parser/generated/
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,15 @@ NEXT_RELEASE Release notes (YYYY-MM-DD)
### Enhancements
* Add `realm::db_config::enable_forced_sync_history()` which allows you to open a synced Realm
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.
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
contains a given key. Use this method in the Type Safe Query API instead of `managed<std::map<std::string, T>>::find`.

### Compatibility
* Fileformat: Generates files with format v24. Reads and automatically upgrade from fileformat v10.
Expand Down
23 changes: 18 additions & 5 deletions include/cpprealm/networking/http.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,8 @@ namespace realm::networking {
// Interface for providing http transport
struct http_transport_client {
virtual ~http_transport_client() = default;
virtual void send_request_to_server(const request& request,
std::function<void(const response&)>&& completion) = 0;
virtual void send_request_to_server(::realm::networking::request &&request,
std::function<void(const response &)> &&completion) = 0;
Comment on lines +108 to +109
Copy link
Author

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.

};

/// Produces a http transport client from the factory.
Expand All @@ -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> {
Copy link

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?

Copy link
Author

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.

struct configuration {
/**
* Extra HTTP headers to be set on each request to Atlas Device Sync when using the internal HTTP client.
Expand Down Expand Up @@ -143,17 +143,30 @@ namespace realm::networking {
* is not set.
*/
std::function<SSLVerifyCallback> ssl_verify_callback;

/**
* Maximum number of subsequent redirect responses from the server to prevent getting stuck
* in a redirect loop indefinitely. Set to 0 to disable redirect support or -1 to allow
* redirecting indefinitely.
*/
int max_redirect_count = 30;
};

default_http_transport() = default;
default_http_transport(const configuration& c) : m_configuration(c) {}

~default_http_transport() = default;

void send_request_to_server(const ::realm::networking::request& request,
std::function<void(const ::realm::networking::response&)>&& completion);
void send_request_to_server(::realm::networking::request &&request,
std::function<void(const ::realm::networking::response &)> &&completion) {
send_request_to_server(std::move(request), std::move(completion), 0);
}

protected:
void send_request_to_server(::realm::networking::request &&request,
std::function<void(const ::realm::networking::response &)> &&completion,
int redirect_count);

configuration m_configuration;
};
}
Expand Down
2 changes: 1 addition & 1 deletion src/cpprealm/analytics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ namespace realm {
std::stringstream json_ss;
json_ss << post_data;
auto json_str = json_ss.str();
auto transport = std::make_unique<networking::default_http_transport>();
auto transport = std::make_shared<networking::default_http_transport>();

std::vector<char> buffer;
buffer.resize(5000);
Expand Down
136 changes: 86 additions & 50 deletions src/cpprealm/internal/networking/network_transport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
#include "realm/util/base64.hpp"
#include "realm/util/uri.hpp"

#include <algorithm>
#include <cctype>
#include <regex>

namespace realm::networking {
Expand Down Expand Up @@ -124,8 +126,9 @@ namespace realm::networking {
}
}

void default_http_transport::send_request_to_server(const ::realm::networking::request& request,
std::function<void(const ::realm::networking::response&)>&& completion_block) {
void default_http_transport::send_request_to_server(::realm::networking::request &&request,
std::function<void(const ::realm::networking::response &)> &&completion_block,
int redirect_count) {
const auto uri = realm::util::Uri(request.url);
std::string userinfo, host, port;
uri.get_auth(userinfo, host, port);
Expand Down Expand Up @@ -170,7 +173,7 @@ namespace realm::networking {
auto address = realm::sync::network::make_address(host, e);
ep = realm::sync::network::Endpoint(address, stoi(port));
} else {
auto resolved = resolver.resolve(sync::network::Resolver::Query(host, is_localhost ? "9090" : "443"));
auto resolved = resolver.resolve(sync::network::Resolver::Query(host, port));
ep = *resolved.begin();
}
}
Expand Down Expand Up @@ -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);
Copy link

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.

Copy link
Author

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.

auto convert_method = [](::realm::networking::http_method method) {
switch (method) {
case ::realm::networking::http_method::get:
return realm::sync::HTTPMethod::Get;
case ::realm::networking::http_method::put:
return realm::sync::HTTPMethod::Put;
case ::realm::networking::http_method::post:
return realm::sync::HTTPMethod::Post;
case ::realm::networking::http_method::patch:
return realm::sync::HTTPMethod::Patch;
case ::realm::networking::http_method::del:
return realm::sync::HTTPMethod::Delete;
default:
REALM_UNREACHABLE();
}
};

realm::sync::HTTPRequest http_req{
convert_method(request.method),
{},
request.url,
request.body.empty() ? std::nullopt : std::make_optional<std::string>(request.body)};
for (auto& [k, v] : request.headers) {
headers[k] = v;
http_req.headers[k] = v;
}
headers["Host"] = host;
headers["User-Agent"] = "Realm C++ SDK";
http_req.headers["Host"] = host;
http_req.headers["User-Agent"] = "Realm C++ SDK";

if (!request.body.empty()) {
headers["Content-Length"] = util::to_string(request.body.size());
http_req.headers["Content-Length"] = util::to_string(request.body.size());
}

if (m_configuration.custom_http_headers) {
for (auto& header : *m_configuration.custom_http_headers) {
headers.emplace(header);
http_req.headers.emplace(header);
}
}

realm::sync::HTTPClient<DefaultSocket> m_http_client = realm::sync::HTTPClient<DefaultSocket>(socket, logger);
realm::sync::HTTPMethod method;
switch (request.method) {
case ::realm::networking::http_method::get:
method = realm::sync::HTTPMethod::Get;
break;
case ::realm::networking::http_method::put:
method = realm::sync::HTTPMethod::Put;
break;
case ::realm::networking::http_method::post:
method = realm::sync::HTTPMethod::Post;
break;
case ::realm::networking::http_method::patch:
method = realm::sync::HTTPMethod::Patch;
break;
case ::realm::networking::http_method::del:
method = realm::sync::HTTPMethod::Delete;
break;
default:
REALM_UNREACHABLE();
}

/*
* Flow of events:
* 1. hostname is resolved from DNS
Expand All @@ -295,26 +298,59 @@ namespace realm::networking {
service.post([&](realm::Status&&){
auto handler = [&](std::error_code ec) {
if (ec.value() == 0) {
realm::sync::HTTPRequest req;
req.method = method;
req.headers = headers;
req.path = request.url;
req.body = request.body.empty() ? std::nullopt : std::optional<std::string>(request.body);

m_http_client.async_request(std::move(req), [cb = std::move(completion_block)](const realm::sync::HTTPResponse& r, const std::error_code&) {
::realm::networking::response res;
res.body = r.body ? *r.body : "";
for (auto& [k, v] : r.headers) {
res.headers[k] = v;
}
res.http_status_code = static_cast<int>(r.status);
res.custom_status_code = 0;
cb(res);
});
// 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 {
Copy link

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.

Copy link
Author

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.

constexpr std::string_view location_header = "location";
auto transport = self.lock();
Copy link

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.

Copy link

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?

// If an error occurred or the transport has gone away, then send "operation aborted" to callback
if (ec || !transport) {
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;
// 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) {
Copy link

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?

Copy link
Author

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...

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;
}
// Otherwise, keep looking, in case there's another 'location' entry
}
}
// 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;
Copy link

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.

Copy link
Author

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.

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
}
::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) {
Copy link

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())

res.headers[k] = v;
}
cb(res);
});
} else {
::realm::networking::response response;
response.custom_status_code = util::error::operation_aborted;
completion_block(std::move(response));
completion_block({0, util::error::operation_aborted, {}, {}, std::nullopt});
return;
}
};
Expand Down
Loading
Loading