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 1 commit
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/
10 changes: 5 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 @@ -150,8 +150,8 @@ namespace realm::networking {

~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);

protected:
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
126 changes: 76 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,8 @@ 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) {
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 +172,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 +245,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 +297,50 @@ 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)](const realm::sync::HTTPResponse &resp, const std::error_code &ec) mutable {
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) {
// 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));
})) {
// Get the redirect URL path returned from the server and exit the loop
redirect_url = value;
break;
}
}
// 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));
}
}
::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