Skip to content

Commit a9c9b6c

Browse files
tgoynetgoyne
committed
Read the original data for PersistedMetadataStore::update_data() inside the write transaction
This avoids overwriting data with stale values in multiprocess scenarios.
1 parent c1632a4 commit a9c9b6c

File tree

4 files changed

+52
-39
lines changed

4 files changed

+52
-39
lines changed

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

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -715,12 +715,11 @@ void App::get_profile(const std::shared_ptr<User>& user,
715715
identities.push_back({get<std::string>(doc, "id"), get<std::string>(doc, "provider_type")});
716716
}
717717

718-
if (auto data = m_metadata_store->get_user(user->user_id())) {
719-
data->identities = std::move(identities);
720-
data->profile = UserProfile(get<BsonDocument>(profile_json, "data"));
721-
m_metadata_store->update_user(user->user_id(), *data);
722-
user->update_backing_data(std::move(data));
723-
}
718+
m_metadata_store->update_user(user->user_id(), [&](auto& data) {
719+
data.identities = std::move(identities);
720+
data.profile = UserProfile(get<BsonDocument>(profile_json, "data"));
721+
user->update_backing_data(data); // FIXME
722+
});
724723
}
725724
catch (const AppError& err) {
726725
return completion(nullptr, err);
@@ -812,12 +811,11 @@ void App::log_in_with_credentials(const AppCredentials& credentials, const std::
812811
try {
813812
auto json = parse<BsonDocument>(response.body);
814813
if (linking_user) {
815-
if (auto user_data = m_metadata_store->get_user(linking_user->user_id())) {
816-
user_data->access_token = RealmJWT(get<std::string>(json, "access_token"));
817-
// maybe a callback for this?
818-
m_metadata_store->update_user(linking_user->user_id(), *user_data);
819-
linking_user->update_backing_data(std::move(user_data));
820-
}
814+
m_metadata_store->update_user(linking_user->user_id(), [&](auto& data) {
815+
data.access_token = RealmJWT(get<std::string>(json, "access_token"));
816+
// FIXME: should be powered by callback
817+
linking_user->update_backing_data(data);
818+
});
821819
}
822820
else {
823821
auto user_id = get<std::string>(json, "user_id");
@@ -1338,11 +1336,10 @@ void App::refresh_access_token(const std::shared_ptr<User>& user, bool update_lo
13381336
try {
13391337
auto json = parse<BsonDocument>(response.body);
13401338
RealmJWT access_token{get<std::string>(json, "access_token")};
1341-
if (auto data = self->m_metadata_store->get_user(user->user_id())) {
1342-
data->access_token = access_token;
1343-
self->m_metadata_store->update_user(user->user_id(), *data);
1344-
user->update_backing_data(std::move(data));
1345-
}
1339+
self->m_metadata_store->update_user(user->user_id(), [&](auto& data) {
1340+
data.access_token = access_token;
1341+
user->update_backing_data(data);
1342+
});
13461343
}
13471344
catch (AppError& err) {
13481345
return completion(std::move(err));

‎src/realm/object-store/sync/impl/app_metadata.cpp‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -520,13 +520,21 @@ struct PersistedSyncMetadataManager : public app::MetadataStore {
520520
realm->commit_transaction();
521521
}
522522

523-
void update_user(std::string_view user_id, const UserData& data) override
523+
void update_user(std::string_view user_id, util::FunctionRef<void(UserData&)> update_fn) override
524524
{
525525
auto realm = get_realm();
526526
realm->begin_transaction();
527527
auto& schema = m_user_schema;
528528
Obj obj = find_user(*realm, user_id);
529-
REALM_ASSERT(obj);
529+
auto opt_data = read_user(obj);
530+
if (!opt_data) {
531+
realm->cancel_transaction();
532+
return;
533+
}
534+
535+
auto& data = *opt_data;
536+
update_fn(data);
537+
530538
obj.set(schema.state_col,
531539
int64_t(data.access_token ? SyncUser::State::LoggedIn : SyncUser::State::LoggedOut));
532540
obj.set<String>(schema.refresh_token_col, data.refresh_token.token);
@@ -794,12 +802,16 @@ class InMemoryMetadataStorage : public app::MetadataStore {
794802
}
795803
}
796804

797-
void update_user(std::string_view user_id, const UserData& data) override
805+
void update_user(std::string_view user_id, util::FunctionRef<void(UserData&)> update_fn) override
798806
{
799807
std::lock_guard lock(m_mutex);
800-
auto& user = m_users.find(user_id)->second;
801-
user = data;
802-
user.legacy_identities.clear();
808+
auto it = m_users.find(user_id);
809+
if (it == m_users.end()) {
810+
return;
811+
}
812+
813+
update_fn(it->second);
814+
it->second.legacy_identities.clear();
803815
}
804816

805817
void log_out(std::string_view user_id, SyncUser::State new_state) override

‎src/realm/object-store/sync/impl/app_metadata.hpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ class MetadataStore {
5656
std::string_view device_id) = 0;
5757

5858
// Update the stored data for an existing user
59-
virtual void update_user(std::string_view user_id, const UserData& data) = 0;
59+
virtual void update_user(std::string_view user_id, util::FunctionRef<void(UserData&)>) = 0;
6060

6161
// Discard the given user's tokens and set its state to the given one (LoggedOut or Removed).
6262
// If the user was the active user, a new active user is selected from the

‎test/object-store/sync/metadata.cpp‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -187,11 +187,12 @@ TEST_CASE("app metadata: common", "[sync][metadata]") {
187187

188188
SECTION("create_user() only updates the given fields and leaves the rest unchanged") {
189189
store->create_user(user_id, refresh_token, access_token, device_id);
190-
auto data = store->get_user(user_id);
191-
REQUIRE(data);
192-
data->profile = bson::BsonDocument{{"name", "user's name"}, {"email", "user's email"}};
193-
data->identities = {{"identity", "provider"}};
194-
store->update_user(user_id, *data);
190+
UserProfile profile = bson::BsonDocument{{"name", "user's name"}, {"email", "user's email"}};
191+
std::vector<UserIdentity> identities{{"identity", "provider"}};
192+
store->update_user(user_id, [&](UserData& data) {
193+
data.profile = profile;
194+
data.identities = identities;
195+
});
195196

196197
const auto access_token_2 = encode_fake_jwt("access_token_2", 123, 456);
197198
const auto refresh_token_2 = encode_fake_jwt("refresh_token_2", 123, 456);
@@ -203,8 +204,8 @@ TEST_CASE("app metadata: common", "[sync][metadata]") {
203204
CHECK(data2->refresh_token.token == refresh_token_2);
204205
CHECK(data2->legacy_identities.empty());
205206
CHECK(data2->device_id == "device id 2");
206-
CHECK(data2->identities == data->identities);
207-
CHECK(data2->profile.data() == data->profile.data());
207+
CHECK(data2->identities == identities);
208+
CHECK(data2->profile.data() == profile.data());
208209
}
209210

210211
SECTION("has_logged_in_user() is only true if user is present and valid") {
@@ -296,12 +297,15 @@ TEST_CASE("app metadata: common", "[sync][metadata]") {
296297
store->create_user("user 2", refresh_token, access_token, device_id);
297298
store->create_user("user 3", refresh_token, access_token, device_id);
298299

299-
auto data = store->get_user("user 3");
300-
data->access_token.token.clear();
301-
data->refresh_token.token.clear();
302-
store->update_user("user 3", *data);
300+
store->update_user("user 3", [](auto& data) {
301+
data.access_token.token.clear();
302+
data.refresh_token.token.clear();
303+
});
303304
CHECK(store->get_current_user() == "user 1");
304-
store->update_user("user 1", *data);
305+
store->update_user("user 1", [](auto& data) {
306+
data.access_token.token.clear();
307+
data.refresh_token.token.clear();
308+
});
305309
CHECK(store->get_current_user() == "user 2");
306310

307311
store->set_current_user("not a user");
@@ -334,10 +338,10 @@ TEST_CASE("app metadata: common", "[sync][metadata]") {
334338

335339
SECTION("update_user() does not set legacy identities") {
336340
store->create_user(user_id, refresh_token, access_token, device_id);
341+
store->update_user(user_id, [](auto& data) {
342+
data.legacy_identities.push_back("legacy uuid");
343+
});
337344
auto data = store->get_user(user_id);
338-
data->legacy_identities.push_back("legacy uuid");
339-
store->update_user(user_id, *data);
340-
data = store->get_user(user_id);
341345
REQUIRE(data->legacy_identities.empty());
342346
}
343347

0 commit comments

Comments
 (0)