Reconstruct app url after a migration #5743

michael-wb · 2022-08-13T05:04:50Z

What, How & Why?

Updated Realm::App::do_request() to handle redirect (301) responses from the server to update the server url to the hostname provided in the "Location" header string.

This change required updating the callback argument for GenericNetworkTransport::send_request_to_server() to receive both the original Request and Response objects (SDK breaking change). The C-API was not affected since the completion_data pointer is now a struct that contains both the original Request and the callback function.

This change required numerous updates to the tests and all tests were passing before committing (on MacOS).

Added tests to verify redirect response handling.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

jbreams

First pass of comments. Looks like a good start!

src/realm/object-store/c_api/http.cpp

src/realm/object-store/sync/app.cpp

jbreams

Still looking through the tests, but here are some minor comments from a first pass. Good start!

src/realm/object-store/c_api/http.cpp

src/realm/object-store/sync/generic_network_transport.hpp

src/realm/object-store/sync/app.cpp

src/realm/object-store/sync/app_utils.cpp

src/realm/object-store/sync/generic_network_transport.cpp

…ruct-app-url

jbreams

Looking good. Just a few more comments/questions/suggestions.

jbreams · 2022-09-15T21:34:24Z

CHANGELOG.md

 * Syncing of a Decimal128 with big significand could result in a crash. ([#5728](https://github.com/realm/realm-core/issues/5728))
 * Recovery/discardLocal client reset modes will now wait for FLX sync realms to be fully synchronized before beginning recovery operations ([#5705](https://github.com/realm/realm-core/issues/5705))

 ### Breaking changes
 * None.
-


can you put this line back?

I don't know how it got deleted... must've done it by accident

jbreams · 2022-09-15T21:40:26Z

src/realm/object-store/sync/generic_network_transport.hpp

+    /**
+     * A recursion counter to prevent too many redirects
+     */
+    util::Optional<size_t> max_redirects;


rename this something like redirect_count? max_redirects sounds like this is the limit we're working towards rather than how many redirects we've processed so far.

also, i think this can just be a regular int rather than a util::Optional<size_t>, and we can set its default value to zero. a redirect count of zero seems to have the same semantic meaning as an empty counter (at least in my opinion).

jbreams · 2022-09-15T23:12:11Z

test/object-store/sync/app.cpp

+        sc_config.metadata_mode = realm::SyncManager::MetadataMode::NoEncryption;
+
+        // initialize app and sync client
+        std::shared_ptr<realm::app::App> redir_app = app::App::get_uncached_app(app_config, sc_config);


you can just do auto redir_app = here.

jbreams · 2022-09-15T23:15:48Z

test/object-store/sync/app.cpp

+        util::try_make_dir(base_file_path);
+        SyncClientConfig sc_config;
+        sc_config.base_file_path = base_file_path;
+        sc_config.log_level = TEST_ENABLE_SYNC_LOGGING ? util::Logger::Level::all : util::Logger::Level::off;


You can just use this define https://github.com/realm/realm-core/blob/master/test/object-store/util/test_file.hpp#L228 to setup the log level if you've included test_file.hpp already.

jbreams · 2022-09-15T23:16:03Z

test/object-store/sync/app.cpp

 TEST_CASE("app: sync integration", "[sync][app]") {
+    auto logger = std::make_unique<util::StderrLogger>();
+#if TEST_ENABLE_SYNC_LOGGING
+    logger->set_level_threshold(util::Logger::Level::all);


You can just use this define https://github.com/realm/realm-core/blob/master/test/object-store/util/test_file.hpp#L228 to setup the log level if you've included test_file.hpp already.

jbreams · 2022-09-15T23:18:19Z

test/object-store/sync/app.cpp

@@ -2062,7 +2241,7 @@ TEST_CASE("app: sync integration", "[sync][app]") {
        // This assumes that we make an http request for the new token while
        // already in the WaitingForAccessToken state.
        bool seen_waiting_for_access_token = false;
-        transport->request_hook = [&](Request&) {
+        transport->request_hook = [&](Request&) -> bool {


you don't have to change this, but if a lambda returns a single type - i.e. you just add a return true to the end - you don't need to make the return type explicit.

jbreams · 2022-09-15T23:19:33Z

src/realm/object-store/sync/app.cpp

+
+            update_hostname(m_sync_manager->app_metadata());
+        }
+        catch (const AppError&) {


just curious what kind of AppError's we could catch here?

The parse and get functions throw an AppError with a JSONErrorCode if the operation fails. Since the only interface to the caller is a callback that takes a Response object, the failure will be determined (again) when the callback is run.

jbreams · 2022-09-15T23:21:23Z

src/realm/object-store/sync/app.cpp

+        return completion(std::move(error)); // early return
+    }
+
+    if (request.max_redirects) {


if max_redirects was just an int that defaults to zero, you could do

if (++request.redirect_count > max_http_redirects) { ... handle the error ... }

Thanks - I like that better

jbreams · 2022-09-15T23:22:49Z

src/realm/object-store/sync/app.cpp

+    constexpr size_t https_len = std::char_traits<char>::length("https://");
+    auto new_url = location->second;
+    size_t split = new_url.find_first_of("/#?", https_len);
+    if (split != std::string::npos) {


you could do

if (auto split = new_url.find_first_of("/#?", https_len); split != std::string::npos) { new_url.remove_suffix(new_url.size() - split); }

jbreams · 2022-09-15T23:29:40Z

src/realm/object-store/sync/app.cpp

-    });
+    // Update the metadata using the new location and replay the request with the updated hostname
+    // Trim off any trailing path/anchor/query string
+    constexpr size_t https_len = std::char_traits<char>::length("https://");


do we want to make sure the URL starts with https:// before we use that? Maybe do:

std::string_view new_url = location->second; if (auto scheme_end = new_url.find("://"); scheme_end != std::string_view::npos) { new_url.remove_prefix(scheme_end + std::char_traits<char>::length("://")); }

Then new_url would be the url minus the initial scheme.

I wanted to skip past the scheme and look for the first /, #, or ? after the hostname and trim just that part until the end.

…ruct-app-url

michael-wb requested review from jbreams and danieltabacaru August 13, 2022 05:04

michael-wb self-assigned this Aug 13, 2022

cla-bot bot added the cla: yes label Aug 13, 2022

sync-by-unito bot mentioned this pull request Aug 15, 2022

Reconstruct app url after a migration #5648

Closed

michael-wb linked an issue Aug 15, 2022 that may be closed by this pull request

Reconstruct app url after a migration #5648

Closed

michael-wb force-pushed the mwb/reconstruct-app-url branch from 4a4bab8 to 583c473 Compare August 15, 2022 16:43

michael-wb changed the title ~~Handle redirect (301) server responses to update the server URL and resend the request to the new host~~ Reconstruct app url after a migration Aug 15, 2022

jbreams reviewed Aug 16, 2022

View reviewed changes

michael-wb force-pushed the mwb/reconstruct-app-url branch 2 times, most recently from d4e7fa8 to 72038c8 Compare August 24, 2022 14:30

Michael Wilkerson-Barker added 14 commits August 25, 2022 12:48

Added handling and retry mechanism for redirect response

9606fc7

Updated tests after redirect changes

c7b435f

Updated flow to fix missing return

b0b0a38

Updated changelog

f9a706e

Updated std::optional to util::Optional

c610d0a

Addressed linter issue

442d3ef

linter updates

c183010

Fixed warning on linux builds

99fdc73

Updated changelog to include Issue #

8dcfa67

Updated per review and added tests

9c8eeab

clang format

bfe149b

Removed line at start of function

ed19663

Fixed warning (again) on Windows builds

4aa6305

Removed extra 'using' for TransportCallback struct

403b5bf

michael-wb force-pushed the mwb/reconstruct-app-url branch from 72038c8 to 403b5bf Compare August 25, 2022 16:50

michael-wb requested a review from jbreams August 26, 2022 13:56

jbreams reviewed Sep 1, 2022

View reviewed changes

Michael Wilkerson-Barker added 2 commits September 1, 2022 12:55

Updates from review and clang-format

7541db0

Merge branch 'master' of github.com:realm/realm-core into mwb/reconst…

05cdcb2

…ruct-app-url

Michael Wilkerson-Barker added 4 commits September 1, 2022 13:55

Fixed clang format changes

fd2211d

Merge branch 'master' of github.com:realm/realm-core into mwb/reconst…

99d43f0

…ruct-app-url

Merge branch 'master' of github.com:realm/realm-core into mwb/reconst…

0c3326f

…ruct-app-url

Merge branch 'master' of github.com:realm/realm-core into mwb/reconst…

70402e8

…ruct-app-url

michael-wb requested a review from jbreams September 12, 2022 21:02

Merge branch 'master' of github.com:realm/realm-core into mwb/reconst…

c6855ef

…ruct-app-url

jbreams reviewed Sep 15, 2022

View reviewed changes

Michael Wilkerson-Barker added 2 commits September 16, 2022 00:54

Updates from review

461bdb4

Merge branch 'master' of github.com:realm/realm-core into mwb/reconst…

9760d51

…ruct-app-url

michael-wb requested a review from jbreams September 16, 2022 13:10

Michael Wilkerson-Barker added 3 commits September 19, 2022 20:19

Use stringview and added MaintenanceInProgress error

516e06b

Merge branch 'master' of github.com:realm/realm-core into mwb/reconst…

411bf6f

…ruct-app-url

Fixed clang format issue ih app.cpp test file

6b50cf4

jbreams approved these changes Sep 20, 2022

View reviewed changes

michael-wb merged commit 53f292a into master Sep 20, 2022

michael-wb deleted the mwb/reconstruct-app-url branch September 20, 2022 20:04

nirinchev mentioned this pull request Sep 26, 2022

[C-API] Http transport doesn't correctly preserve the request body #5890

Closed

tgoyne mentioned this pull request Sep 26, 2022

Install util/http.hpp #5893

Merged

michael-wb mentioned this pull request Sep 27, 2022

Revisit GenericNetworkTransport interface updated for PR 5743 #5898

Merged

3 tasks

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reconstruct app url after a migration #5743

Reconstruct app url after a migration #5743

michael-wb commented Aug 13, 2022 •

edited

Loading

jbreams left a comment

jbreams left a comment

jbreams left a comment

jbreams Sep 15, 2022

michael-wb Sep 16, 2022

jbreams Sep 15, 2022

jbreams Sep 15, 2022

jbreams Sep 15, 2022

jbreams Sep 15, 2022

jbreams Sep 15, 2022

jbreams Sep 15, 2022

michael-wb Sep 16, 2022

jbreams Sep 15, 2022

michael-wb Sep 16, 2022

jbreams Sep 15, 2022

jbreams Sep 15, 2022

michael-wb Sep 16, 2022

Reconstruct app url after a migration #5743

Reconstruct app url after a migration #5743

Conversation

michael-wb commented Aug 13, 2022 • edited Loading

What, How & Why?

☑️ ToDos

jbreams left a comment

Choose a reason for hiding this comment

jbreams left a comment

Choose a reason for hiding this comment

jbreams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-wb commented Aug 13, 2022 •

edited

Loading