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

RedBeard0531 · 2022-11-28T16:17:42Z

What, How & Why?

As discussed on slack.

☑️ ToDos

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

RedBeard0531 · 2022-11-28T16:18:37Z

src/realm/data_type.hpp

@@ -35,7 +35,6 @@ typedef float Float;
 typedef double Double;
 typedef realm::StringData String;
 typedef realm::BinaryData Binary;
-typedef realm::Timestamp Timestamp;


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

RedBeard0531 · 2022-11-28T16:19:34Z

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

 }

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


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?

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.

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.

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.

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

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 :-)

RedBeard0531 · 2022-11-28T16:21:09Z

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

@@ -762,7 +762,7 @@ RLM_API bool realm_sync_subscription_set_erase_by_id(realm_flx_sync_mutable_subs
    *erased = false;


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?

RedBeard0531 · 2022-11-28T16:24:17Z

src/realm/sync/subscriptions.cpp

@@ -370,7 +348,7 @@ std::pair<SubscriptionSet::iterator, bool> MutableSubscriptionSet::insert_or_ass
    auto table_name = Group::table_name_to_class_name(query.get_table()->get_name());
    auto query_str = query.get_description();
    auto it = std::find_if(begin(), end(), [&](const Subscription& sub) {
-        return (sub.name().empty() && sub.object_class_name() == table_name && sub.query_string() == query_str);


This used to do the wrong thing for m_name = "" (not sure if that is valid or not). I did not think that was worth preserving.

RedBeard0531 · 2022-11-30T15:42:01Z

src/realm/sync/subscriptions.cpp

@@ -492,6 +470,19 @@ util::Future<SubscriptionSet::State> SubscriptionSet::get_state_change_notificat
    return std::move(future);
 }

+void SubscriptionSet::get_state_change_notification(
+    State notify_when, util::UniqueFunction<void(util::Optional<State>, util::Optional<Status>)> cb) const


This matches our normal API for async callbacks of void(optional_or_nullable<ReturnType>, optional_or_nullable<ErrorType>). If you want, I can put this on Future instead.

This callback type.... I hope this isn't like this for forever. 😮‍💨 😭

If it makes you feel better no human will ever actually use this signature. The binding generator transforms it to function getStateChangeNotification(notify_when: SubscriptionSetState) -> Promise<SubscriptionSetState> (or whatever the native async signature would be in your language of choice) which is consumed via await set.getStateChangeNotification(blah). This signature is only used as part of the mechanical binding process.

If you are curious, the signature transform logic is here (this is language agnostic), and the asyncification happens here via the call from here (this is js-specific but shared by both engines). Interestingly, the actual generated C++ code doesn't care about the transform and just takes a normal callback from JS. This is part of why I didn't want to do a one-off for Future support just for this class because that strategy would need to change. I don't know if that is the implementation strategy all languages will choose, but it seems to work well here at least.

RedBeard0531 · 2022-12-02T11:56:03Z

src/realm/object-store/c_api/conversion.hpp

@@ -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.


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.

jbreams

Make sure you add a changelog entry

jbreams · 2022-12-02T19:23:48Z

src/realm/sync/subscriptions.cpp

@@ -492,6 +470,19 @@ util::Future<SubscriptionSet::State> SubscriptionSet::get_state_change_notificat
    return std::move(future);
 }

+void SubscriptionSet::get_state_change_notification(
+    State notify_when, util::UniqueFunction<void(util::Optional<State>, util::Optional<Status>)> cb) const


This callback type.... I hope this isn't like this for forever. 😮‍💨 😭

cla-bot bot added the cla: yes label Nov 28, 2022

RedBeard0531 marked this pull request as draft November 28, 2022 16:26

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

06e824a

RedBeard0531 force-pushed the ms/subscription_api branch from 8688d21 to 06e824a Compare November 30, 2022 15:19

remove std::move() before calls to commit()

c1faa6d

RedBeard0531 requested a review from jbreams November 30, 2022 15:53

RedBeard0531 commented Nov 30, 2022

View reviewed changes

RedBeard0531 marked this pull request as ready for review November 30, 2022 15:54

RedBeard0531 added 3 commits December 1, 2022 16:32

fix typo

9838837

to_capi(cond ? std::string(...) : "") returns a dangling pointer. FML

1d7aa7e

ban to_capi(temporary string)

e6f4a29

RedBeard0531 commented Dec 2, 2022

View reviewed changes

jbreams approved these changes Dec 2, 2022

View reviewed changes

RedBeard0531 added 3 commits December 5, 2022 14:36

changelog entry

78bca0b

use Mutable in changelog

59ecc8e

Merge branch 'master' into ms/subscription_api

f5379ef

RedBeard0531 merged commit 80d8790 into master Dec 5, 2022

RedBeard0531 deleted the ms/subscription_api branch December 5, 2022 14:55

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

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

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

RedBeard0531 commented Nov 28, 2022 •

edited

Loading

RedBeard0531 Nov 28, 2022

RedBeard0531 Nov 28, 2022

RedBeard0531 Dec 1, 2022

nicola-cab Dec 1, 2022

RedBeard0531 Dec 1, 2022

RedBeard0531 Dec 1, 2022

nicola-cab Dec 1, 2022

RedBeard0531 Nov 28, 2022

RedBeard0531 Nov 28, 2022

RedBeard0531 Nov 30, 2022

jbreams Dec 2, 2022

RedBeard0531 Dec 2, 2022

RedBeard0531 Dec 2, 2022

jbreams left a comment

jbreams Dec 2, 2022

		@@ -762,7 +762,7 @@ RLM_API bool realm_sync_subscription_set_erase_by_id(realm_flx_sync_mutable_subs
		*erased = false;

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

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

Conversation

RedBeard0531 commented Nov 28, 2022 • edited Loading

What, How & Why?

☑️ ToDos

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

jbreams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RedBeard0531 commented Nov 28, 2022 •

edited

Loading