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

Improve FLX/Subscriptions API to be more friendly to direct binding #6065

Merged
merged 8 commits into from
Dec 5, 2022
Merged

Improve FLX/Subscriptions API to be more friendly to direct binding #6065

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
06e824a
Improve FLX/Subscriptions API to be more friendly to direct binding
RedBeard0531 Nov 28, 2022
c1faa6d
remove std::move() before calls to commit()
RedBeard0531 Nov 30, 2022
9838837
fix typo
RedBeard0531 Dec 1, 2022
1d7aa7e
to_capi(cond ? std::string(...) : "") returns a dangling pointer. FML
RedBeard0531 Dec 1, 2022
e6f4a29
ban to_capi(temporary string)
RedBeard0531 Dec 2, 2022
78bca0b
changelog entry
RedBeard0531 Dec 5, 2022
59ecc8e
use Mutable in changelog
RedBeard0531 Dec 5, 2022
f5379ef
Merge branch 'master' into ms/subscription_api
RedBeard0531 Dec 5, 2022
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
1 change: 0 additions & 1 deletion src/realm/data_type.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ typedef float Float;
typedef double Double;
typedef realm::StringData String;
typedef realm::BinaryData Binary;
typedef realm::Timestamp Timestamp;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drive-by fix removes an unused header warning for timestamp.hpp in subscriptions.hpp which clearly uses Timestamp

typedef realm::Decimal128 Decimal;

struct ColumnType;
Expand Down
4 changes: 4 additions & 0 deletions src/realm/object-store/c_api/conversion.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ static inline realm_string_t to_capi(StringData data)
return realm_string_t{data.data(), data.size()};
}

// Because this is often used as `return to_capi(...);` it is dangerous to pass a temporary string here. If you really
// need to and know it is correct (eg passing to a C callback), you can explicitly create the StringData wrapper.
realm_string_t to_capi(const std::string&& str) = delete; // temporary std::string would dangle.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that this would catch my return to_capi(cond ? constStringRef : "") bug where that expression produced a temporary std::string. I usually don't love banning rvalue overloads because that prevents safe usage as much as it unsafe, but because A) to_capi() is often used in return position and B) it is easy to work around where it is known to be safe, it seems worth it here. If you disagree, I can revert the commit to add this overload.

The line comment is redundant with the comment above, however I like having it because it will show up in compiler error messages, while the above comment won't.


static inline realm_string_t to_capi(const std::string& str)
{
return to_capi(StringData{str});
Expand Down
47 changes: 19 additions & 28 deletions src/realm/object-store/c_api/sync.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,41 +507,41 @@ RLM_API void realm_sync_config_set_resync_mode(realm_sync_config_t* config,
RLM_API realm_object_id_t realm_sync_subscription_id(const realm_flx_sync_subscription_t* subscription) noexcept
{
REALM_ASSERT(subscription != nullptr);
return to_capi(subscription->id());
return to_capi(subscription->id);
}

RLM_API realm_string_t realm_sync_subscription_name(const realm_flx_sync_subscription_t* subscription) noexcept
{
REALM_ASSERT(subscription != nullptr);
return to_capi(subscription->name());
return to_capi(subscription->name);
}

RLM_API realm_string_t
realm_sync_subscription_object_class_name(const realm_flx_sync_subscription_t* subscription) noexcept
{
REALM_ASSERT(subscription != nullptr);
return to_capi(subscription->object_class_name());
return to_capi(subscription->object_class_name);
}

RLM_API realm_string_t
realm_sync_subscription_query_string(const realm_flx_sync_subscription_t* subscription) noexcept
{
REALM_ASSERT(subscription != nullptr);
return to_capi(subscription->query_string());
return to_capi(subscription->query_string);
}

RLM_API realm_timestamp_t
realm_sync_subscription_created_at(const realm_flx_sync_subscription_t* subscription) noexcept
{
REALM_ASSERT(subscription != nullptr);
return to_capi(subscription->created_at());
return to_capi(subscription->created_at);
}

RLM_API realm_timestamp_t
realm_sync_subscription_updated_at(const realm_flx_sync_subscription_t* subscription) noexcept
{
REALM_ASSERT(subscription != nullptr);
return to_capi(subscription->updated_at());
return to_capi(subscription->updated_at);
}

RLM_API void realm_sync_config_set_before_client_reset_handler(realm_sync_config_t* config,
Expand Down Expand Up @@ -654,10 +654,10 @@ realm_sync_find_subscription_by_name(const realm_flx_sync_subscription_set_t* su
const char* name) noexcept
{
REALM_ASSERT(subscription_set != nullptr);
auto it = subscription_set->find(name);
if (it == subscription_set->end())
auto ptr = subscription_set->find(name);
if (!ptr)
return nullptr;
return new realm_flx_sync_subscription_t(*it);
return new realm_flx_sync_subscription_t(*ptr);
}

RLM_API realm_flx_sync_subscription_t*
Expand All @@ -666,10 +666,10 @@ realm_sync_find_subscription_by_results(const realm_flx_sync_subscription_set_t*
{
REALM_ASSERT(subscription_set != nullptr);
auto realm_query = add_ordering_to_realm_query(results->get_query(), results->get_ordering());
auto it = subscription_set->find(realm_query);
if (it == subscription_set->end())
auto ptr = subscription_set->find(realm_query);
if (!ptr)
return nullptr;
return new realm_flx_sync_subscription_t{*it};
return new realm_flx_sync_subscription_t{*ptr};
}

RLM_API realm_flx_sync_subscription_t*
Expand All @@ -690,10 +690,10 @@ realm_sync_find_subscription_by_query(const realm_flx_sync_subscription_set_t* s
{
REALM_ASSERT(subscription_set != nullptr);
auto realm_query = add_ordering_to_realm_query(query->get_query(), query->get_ordering());
auto it = subscription_set->find(realm_query);
if (it == subscription_set->end())
auto ptr = subscription_set->find(realm_query);
if (!ptr)
return nullptr;
return new realm_flx_sync_subscription_t(*it);
return new realm_flx_sync_subscription_t(*ptr);
}

RLM_API bool realm_sync_subscription_set_refresh(realm_flx_sync_subscription_set_t* subscription_set)
Expand Down Expand Up @@ -762,7 +762,7 @@ RLM_API bool realm_sync_subscription_set_erase_by_id(realm_flx_sync_mutable_subs
*erased = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if I should delete these. They ensure that *erased is written even in the case of errors. Is that worth preserving?

return wrap_err([&] {
auto it = std::find_if(subscription_set->begin(), subscription_set->end(), [id](const Subscription& sub) {
return from_capi(*id) == sub.id();
return from_capi(*id) == sub.id;
});
if (it != subscription_set->end()) {
subscription_set->erase(it);
Expand All @@ -778,10 +778,7 @@ RLM_API bool realm_sync_subscription_set_erase_by_name(realm_flx_sync_mutable_su
REALM_ASSERT(subscription_set != nullptr && name != nullptr);
*erased = false;
return wrap_err([&]() {
if (auto it = subscription_set->find(name); it != subscription_set->end()) {
subscription_set->erase(it);
*erased = true;
}
*erased = subscription_set->erase(name);
return true;
});
}
Expand All @@ -793,10 +790,7 @@ RLM_API bool realm_sync_subscription_set_erase_by_query(realm_flx_sync_mutable_s
*erased = false;
return wrap_err([&]() {
auto realm_query = add_ordering_to_realm_query(query->get_query(), query->get_ordering());
if (auto it = subscription_set->find(realm_query); it != subscription_set->end()) {
subscription_set->erase(it);
*erased = true;
}
*erased = subscription_set->erase(realm_query);
return true;
});
}
Expand All @@ -808,10 +802,7 @@ RLM_API bool realm_sync_subscription_set_erase_by_results(realm_flx_sync_mutable
*erased = false;
return wrap_err([&]() {
auto realm_query = add_ordering_to_realm_query(results->get_query(), results->get_ordering());
if (auto it = subscription_set->find(realm_query); it != subscription_set->end()) {
subscription_set->erase(it);
*erased = true;
}
*erased = subscription_set->erase(realm_query);
return true;
});
}
Expand Down
6 changes: 3 additions & 3 deletions src/realm/sync/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1036,7 +1036,7 @@ void SessionWrapper::on_flx_sync_error(int64_t version, std::string_view err_msg

auto mut_subs = get_flx_subscription_store()->get_mutable_by_version(version);
mut_subs.update_state(SubscriptionSet::State::Error, err_msg);
std::move(mut_subs).commit();
mut_subs.commit();
}

void SessionWrapper::on_flx_sync_version_complete(int64_t version)
Expand Down Expand Up @@ -1085,7 +1085,7 @@ void SessionWrapper::on_flx_sync_progress(int64_t new_version, DownloadBatchStat

auto mut_subs = get_flx_subscription_store()->get_mutable_by_version(new_version);
mut_subs.update_state(new_state);
std::move(mut_subs).commit();
mut_subs.commit();
}

SubscriptionStore* SessionWrapper::get_flx_subscription_store()
Expand Down Expand Up @@ -1484,7 +1484,7 @@ void SessionWrapper::on_download_completion()
m_flx_pending_mark_version);
auto mutable_subs = m_flx_subscription_store->get_mutable_by_version(m_flx_pending_mark_version);
mutable_subs.update_state(SubscriptionSet::State::Complete);
std::move(mutable_subs).commit();
mutable_subs.commit();
m_flx_pending_mark_version = SubscriptionSet::EmptyVersion;
}

Expand Down
Loading