Throw if a table being written to has no subscriptions and FLX sync is enabled #5488

jbreams · 2022-05-16T21:33:57Z

What, How & Why?

This is one of the things we committed to for World in the FLX sync adaptive changes scope.

It seems an easy footgun for users is to do writes to a table where they haven't set any permissions yet. This adds a validation hook to the ClientReplication class where we can just check the latest subscription set for whether it has a subscription for the table being written to. It doesn't validate anything about the query - compensating writes on the server will do stricter validation than on the client.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)

tgoyne · 2022-05-27T20:44:18Z

Wouldn't it be both simpler and faster to have something like std::unordered_set<std::string> SubscriptionStore::get_subscribed_tables(Transaction& tr)? Checking if the table has subscriptions via SubscriptionSet would make sense if that's something that SubscriptionSet could easily answer, but it appears to be something that has zero overlap with what SubscriptionSet is currently doing.

jbreams · 2022-05-27T20:58:44Z

@tgoyne - not sure I follow - SubscriptionSet is the thing that knows what tables have which subscriptions. That's like the whole point of that type.

tgoyne · 2022-05-27T21:10:37Z

I don't understand how entirely new functionality can possibly be the point of a pre-existing type. The code which uses m_tables does not access any other functionality from SubscriptionSet, and the code which uses the other functionality from SubscriptionSet doesn't access m_tables.

jbreams · 2022-05-27T21:21:39Z

The point of the SubscriptionSet type is to hold and make it so you can query all the info about subscriptions, and by extension which tables they belong to. SubscriptionStore doesn't actually know/do anything with the data about subscriptions or tables or queries or anything, it just produces SubscriptionSets.

Is your suggestion here that SubscriptionStore should have some special side-logic to lookup just the table names for the latest SubscriptionSet without actually creating a SubscriptionSet and returning that as a set?

jbreams · 2022-05-27T21:40:27Z

@tgoyne I've put up what I think you're suggesting. Is this what you had in mind?

tgoyne · 2022-05-27T21:48:23Z

Yes. I think the primary advantage of that is it avoids introducing mutable state with additional invariants which have to be maintained, particularly when the mutable version isn't directly relevant (since we don't allow mixing updates to subscriptions and other writes).

It'll also be faster, but you'd probably need an absurd number of subscriptions for that to be a consideration even with it being something that has to happen on every write transaction, so that's not a real consideration.

(The "for_latest" part in get_tables_for_latest() is misleading, but I assume you just haven't made a final pass over the names)

ironage

I wasn't involved in the discussion around this, but it seems like a very disruptive change in functionality. If the server processes changes, the objects modified outside subscriptions are automatically reverted, but with this check in place on the client side, all changes in the same commit are lost. It seems like it could be a good feature to have on during development though, what do you think about making this configurable so devs can turn it off for production apps?

ironage · 2022-05-27T21:31:35Z

src/realm/group.hpp

        return table_name.substr(g_class_name_prefix_len);
    }
+
+    static bool table_name_is_class_name(StringData table_name)


my preference would be to overload table_is_public since they do the same thing but for different types

ironage · 2022-05-27T21:44:55Z

src/realm/object-store/impl/realm_coordinator.cpp

+                auto sub_mgr = weak_sub_mgr.lock();
+                if (!sub_mgr) {
+                    throw std::runtime_error("Subscription store was destroyed while user writes were on-going");
+                }


Why not move this weak -> strong pointer logic into the UniqueFunction? Then we can reduce the scope of strong pointers floating around.

It turns out we don't need to capture the sub_mgr at all. I think we still want to lock it and get the info out of it here so that we can cache the result across all the writes in this transaction.

jbreams · 2022-05-27T22:37:02Z

The scope of this work was defined and approved and signed off on in RPM-272. I think making this so you can control whether this validation is on/off within your Realm config would be a significant change in the agreed-upon scope. Also this apparently was enough of a footgun for developers during the beta period that just making it an error was decided to be worth it. cc @ianpward

ironage · 2022-05-27T22:43:59Z

Fair enough. But it does look like that decision was made when the consequence of this type of write was a client reset. I imagine it is much less of a problem with the advent of compensating writes.

tgoyne · 2022-05-27T23:48:07Z

I don't really have an opinion on if this needs an option, but adding an option would be trivial (just add bool strict_flx_sync_object_creation = true; to SyncConfig, change the check to m_config.sync_config && m_config.sync_config->flx_sync_requested && m_config.sync_config->strict_flx_sync_object_creation and add a test).

jbreams · 2022-05-28T00:14:50Z

Compensating writes was planned and in-progress when this feature was scoped. The idea from the scoping discussion was that this behavior is always a result of a programmer error and that it was very confusing for new users who would start doing writes and nothing would happen or they'd get an error message back asynchronously. From what I recall, it came up during user interviews @ianpward did. It was decided to just make the case where you're writing to a table without any subscriptions always an error. I was not the person pushing for this, so maybe @ianpward or @jsflax can correct me if I'm wrong about the history of this feature.

ianpward · 2022-05-28T21:23:47Z

Yes - we have had a clear and consistent feedback not only from our users, but shockingly our own realm engineers ran into this several times during the hackathon week.

ironage · 2022-05-30T17:27:44Z

Ok makes sense, I'm fine with this going ahead. We can always adjust later if needed. I will review again once the test failures are fixed.

jbreams · 2022-05-31T20:51:07Z

@ironage , I think the failures here are not caused by this work, can you review?

ironage

👍
Could use a note in the change log.

ironage · 2022-05-31T22:19:01Z

src/realm/object-store/impl/realm_coordinator.cpp

+
+                auto latest_sub_tables = sub_mgr->get_tables_for_latest(tr);
+                return [tables = std::move(latest_sub_tables)](const Table& table) {
+                    if (table.is_embedded()) {


heads up @danieltabacaru: we're going to need a similar early out for asymmetric tables depending on which PR gets merged first.

@jbreams since my pr went in first, you'll have to add the check

ironage · 2022-05-31T22:31:51Z

src/realm/sync/noinst/client_history_impl.hpp

@@ -447,12 +447,21 @@ class ClientReplication final : public SyncReplication {
    {
    }

+    // A write validator factory takes a read-able transaction and returns a UniqueFunction containing a
+    // SyncReplication::WriteValidator. The factory will get called at the start of a write transaction
+    // and the WriteValidator it returns will be re-used for all mutations wit`hin the transaction.


Suggested change

// and the WriteValidator it returns will be re-used for all mutations wit`hin the transaction.

// and the WriteValidator it returns will be re-used for all mutations within the transaction.

danieltabacaru · 2022-06-01T12:04:20Z

src/realm/object-store/impl/realm_coordinator.cpp

@@ -102,6 +103,30 @@ void RealmCoordinator::create_sync_session()
        return;

    m_sync_session = m_config.sync_config->user->sync_manager()->get_session(m_db, *m_config.sync_config);
+    if (m_config.sync_config && m_config.sync_config->flx_sync_requested) {
+        std::weak_ptr<sync::SubscriptionStore> weak_sub_mgr(m_sync_session->get_flx_subscription_store());
+        sync::ClientReplication& history = static_cast<sync::ClientReplication&>(*m_db->get_replication());


danieltabacaru · 2022-06-01T12:52:00Z

src/realm/sync/noinst/client_history_impl.hpp

@@ -447,12 +447,21 @@ class ClientReplication final : public SyncReplication {
    {
    }

+    // A write validator factory takes a read-able transaction and returns a UniqueFunction containing a


it sounds it is only used with a write transaction, right?

changed this to say "write transaction"

danieltabacaru

LGTM

…X sync is enabled (#5488)" This reverts commit 02adb14.

cla-bot bot added the cla: yes label May 16, 2022

jbreams force-pushed the jbr/throw_on_writes_without_a_subscription branch from b5b387c to 2c58d3a Compare May 27, 2022 19:46

jbreams marked this pull request as ready for review May 27, 2022 20:23

jbreams requested review from danieltabacaru, jedelbo and ironage May 27, 2022 20:23

ironage reviewed May 27, 2022

View reviewed changes

jbreams force-pushed the jbr/throw_on_writes_without_a_subscription branch 2 times, most recently from 9b05a5a to c56cfae Compare May 31, 2022 15:13

ironage approved these changes May 31, 2022

View reviewed changes

danieltabacaru reviewed Jun 1, 2022

View reviewed changes

jbreams added 6 commits June 1, 2022 08:48

Validate writes in flexible sync

ab1810c

testing and basic perf consideration

dbe1ff5

read from correct TR

0367244

simplify SubSet lifetime handling

cb8c9f8

make things more consistent

debc965

format

884a18d

jbreams added 10 commits June 1, 2022 08:49

wip

c1b7e46

undo wacky clang-format changes

392ea84

more touchups

7396106

do Thomas suggestion

7b59cea

cleanup

6f9df7c

adapt to compensating writes changes

ff58bfd

do not capture substore in validator

954b89d

fix table_name_is_class_name duplication

4ea56cb

fix audit tests

0a3dba8

rebase onto asymmetric sync

623070f

danieltabacaru reviewed Jun 1, 2022

View reviewed changes

danieltabacaru approved these changes Jun 1, 2022

View reviewed changes

update comments

88087e1

jbreams force-pushed the jbr/throw_on_writes_without_a_subscription branch from ccf398a to 88087e1 Compare June 1, 2022 13:01

jbreams added 3 commits June 1, 2022 09:03

changelog

2a9674c

fix asymmetric object test

a8b468d

remove number of checks for jørgen

274231e

jedelbo approved these changes Jun 1, 2022

View reviewed changes

jbreams merged commit 02adb14 into master Jun 1, 2022

jbreams deleted the jbr/throw_on_writes_without_a_subscription branch June 1, 2022 17:03

jedelbo added a commit that referenced this pull request Jun 2, 2022

Revert "Throw if a table being written to has no subscriptions and FL…

26a34fa

…X sync is enabled (#5488)" This reverts commit 02adb14.

jedelbo added a commit that referenced this pull request Jun 2, 2022

Revert "Throw if a table being written to has no subscriptions and FL…

fb980ac

…X sync is enabled (#5488)" This reverts commit 02adb14.

stoneyMDB linked an issue Jun 2, 2022 that may be closed by this pull request

Need error message for object creation before subscription #5423

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw if a table being written to has no subscriptions and FLX sync is enabled #5488

Throw if a table being written to has no subscriptions and FLX sync is enabled #5488

jbreams commented May 16, 2022 •

edited

Loading

tgoyne commented May 27, 2022

jbreams commented May 27, 2022

tgoyne commented May 27, 2022

jbreams commented May 27, 2022

jbreams commented May 27, 2022

tgoyne commented May 27, 2022

ironage left a comment

ironage May 27, 2022

jbreams May 31, 2022

ironage May 27, 2022

jbreams May 31, 2022

jbreams commented May 27, 2022

ironage commented May 27, 2022

tgoyne commented May 27, 2022

jbreams commented May 28, 2022

ianpward commented May 28, 2022

ironage commented May 30, 2022

jbreams commented May 31, 2022

ironage left a comment

ironage May 31, 2022

danieltabacaru Jun 1, 2022

ironage May 31, 2022

jbreams Jun 1, 2022

danieltabacaru Jun 1, 2022

jbreams Jun 1, 2022

danieltabacaru Jun 1, 2022

jbreams Jun 1, 2022

danieltabacaru left a comment

	// and the WriteValidator it returns will be re-used for all mutations wit`hin the transaction.
	// and the WriteValidator it returns will be re-used for all mutations within the transaction.

Throw if a table being written to has no subscriptions and FLX sync is enabled #5488

Throw if a table being written to has no subscriptions and FLX sync is enabled #5488

Conversation

jbreams commented May 16, 2022 • edited Loading

What, How & Why?

☑️ ToDos

tgoyne commented May 27, 2022

jbreams commented May 27, 2022

tgoyne commented May 27, 2022

jbreams commented May 27, 2022

jbreams commented May 27, 2022

tgoyne commented May 27, 2022

ironage 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

jbreams commented May 27, 2022

ironage commented May 27, 2022

tgoyne commented May 27, 2022

jbreams commented May 28, 2022

ianpward commented May 28, 2022

ironage commented May 30, 2022

jbreams commented May 31, 2022

ironage 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

danieltabacaru left a comment

Choose a reason for hiding this comment

jbreams commented May 16, 2022 •

edited

Loading