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 2 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
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 ? "" : *subscription->name);
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 preserves the old behavior of never returning a "null" string. But perhaps it is the old behavior that is wrong and I should just return to_capi(subscription->name) here instead? Thoughts?

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 gave up on preserving this behavior. I think it was wrong anyway. The only way to get it is subscription->name ? to_capi(*subscription->name) : to_capi(""), and that doesn't seem worth it in order to get the broken old behavior. The returned string probably should be null if the subscription has no name.

Copy link
Member

Choose a reason for hiding this comment

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

If I recall this correctly, the problem was to determine whether the string was null because of some error or just because the subscription name was not set, probably, returning a simple bool to flag if the API was successful and pass some output parameter realm_string_t *name would suffice.
But it seems a lot of work for a simple getter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new code is just to_capi(subscription->name) this surprizingly works correctly via the to_capi(StringData) overload which in turn works because of the StringData(const optional<std::string>&) implicit conversion.

I don't see how the old code provided a way to distinguish errors from unset strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current version of this line is here fwiw https://github.com/realm/realm-core/pull/6065/files#diff-ba7e9e5a00e04033638ced899e7f7f613243e4e2685fbdf3c519798b4e711d35R516

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I think it is fine like it is... I would not check the pointer in there, the most important thing is that we don't end up hitting another ctor by mistake in the future and crash because of a subscription that does not have a 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