DeepChangeChecker: follow links in Mixed columns #4828

fealebenpae · 2021-07-27T17:10:52Z

What, How & Why?

When checking outgoing links the DeepChangeChecker always assumes that the column containing the outgoing link is always a Link column, but it's now also possible for this to be a Mixed columns with a value containing a TypedLink.

Fixes #4803

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)

tgoyne · 2021-07-27T17:24:19Z

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

+            Mixed value = obj.get<Mixed>(outgoing_link_column);
+            REALM_ASSERT(value.get_type() == type_TypedLink);
+            dst_key = value.get_link().get_obj_key();
+            dst_table = table.get_parent_group()->get_table(value.get_link().get_table_key());


This will crash if the Mixed column contains both links and non-links. I think the correct fix for this is just if (outgoing_link_column.get_type() == col_type_Mixed) { return do_check_mixed_for_link(...); }

ironage · 2021-07-27T23:30:56Z

test/object-store/transaction_log_parsing.cpp

+        bool did_run_section = false;
+
+        {
+            size_t col_ndx = GENERATE(1, 2, 3);


these tests are mostly the same as before, except that they use catch2's GENERATE to duplicate everything for each link column in the schema so that we can cover mixed as well

Neat, I didn't know that was a thing. It's nicer than injecting a template type just for this.

ironage · 2021-07-27T23:33:49Z

test/object-store/transaction_log_parsing.cpp

-            _impl::CollectionKeyPathChangeChecker checker(info, *table, related_tables, key_path_array_five_levels,
-                                                          false);
-            CHECK_THROWS(checker(19));
-            CHECK_THROWS(checker(18));


A functional change in the new test code is that calls like these for an object which has null links along the path to check for no longer throws an invalid object exception. I'm not sure why we would want to throw in that case so I changed it not to.

ironage · 2021-07-27T23:37:42Z

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

@@ -439,43 +464,37 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<int64_t>&
    else if (column_key.is_set()) {
        if (column_type == col_type_Mixed) {
            auto set = object.get_set<Mixed>(column_key);
-            for (size_t i = 0; i < set.size(); i++) {
-                auto target_object = set.get(i);
+            for (auto it = set.begin(); it != set.end(); ++it) {


the changes to use iterators in this section is just an optimization

Can we use range-based for here?

tgoyne · 2021-07-28T00:55:33Z

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

+        ConstTableRef dst_table;
+        ObjKey dst_key;
+        if (outgoing_link_column.get_type() == col_type_Link) {
+            dst_table = table.get_link_target(outgoing_link_column);
+            dst_key = obj.get<ObjKey>(outgoing_link_column);
+        }
+        else if (outgoing_link_column.get_type() == col_type_Mixed) {
+            TableRef no_cached;
+            Mixed value = obj.get<Mixed>(outgoing_link_column);
+            return do_check_mixed_for_link(*table.get_parent_group(), no_cached, value, filtered_columns, depth);
+        }
+        else {
+            REALM_UNREACHABLE();
+        }


This flow is pretty weird now. It declares variables, populates them in one branch of the if, but then doesn't use them in the other branch (and returns so the fact that they aren't used isn't important either). I think it'd be clearer with just if (outgoing_link_column.get_type() == col_type_Mixed) { ... } REALM_ASSERT(outgoing_link_column.get_type() == col_type_Link); and then all the non-mixed logic after the check for Mixed.

tgoyne · 2021-07-28T00:57:30Z

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

@@ -439,43 +464,37 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<int64_t>&
    else if (column_key.is_set()) {
        if (column_type == col_type_Mixed) {
            auto set = object.get_set<Mixed>(column_key);
-            for (size_t i = 0; i < set.size(); i++) {
-                auto target_object = set.get(i);
+            for (auto it = set.begin(); it != set.end(); ++it) {


Can we use range-based for here?

tgoyne · 2021-07-28T01:03:14Z

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

@@ -291,6 +305,10 @@ bool DeepChangeChecker::check_row(Table const& table, ObjKeyType object_key,
 {
    TableKey table_key = table.get_key();

+    if (ObjKey(object_key).is_unresolved()) {


How would we get an unresolved key here? Each of the callers is already checking for that.

The only way is to call the checker directly with an unresolved key. This shouldn't happen in from any of the notifiers in production, but some of the new test code I added calls the checker for all objects under test, including an unresolved one. I felt it was better to handle it here explicitly rather than not test it. I could add a comment about this, unless you'd prefer we not do it this way. I could also move this up a level if it detracts from the intended design.

This is very much a personal opinion thing, but I'd prefer an assert that the key isn't unresolved.

The general idea is that the more that a function's inputs are constrained, the easier it is to modify that function in the future. It's hard to imagine how it would happen in this specific case, but suppose that a future change made properly handling unresolved links here a lot more complicated. A future maintainer could then invest a bunch of effort into some code which isn't actually reachable in normal usage, or alternatively have to spend a while trying to figure out how this function could get an unresolved link so that they could solve the problem at a different layer. Asserting that the link isn't unresolved instead tells anyone reading this code that they don't have to consider that possibility, while still making it likely that changes in other places which accidentally result in this function being passed an unresolved link will be caught.

Even better would be to shove this off to the type system and have different types for possibly unresolved or null ObjKeys and definitely valid ObjKeys so that we could statically check that they're used properly, but that's also a lot more work than just slapping in some asserts.

I added the assert because I think that's the clearest option and could help us catch other mistakes. To accommodate the test code, I move the check up to the top level calls with a comment explaining why it is needed. Unresolved keys were intended to be a hidden detail and having to deal with them at this level is not ideal, so hopefully this is an acceptable compromise until we find a way to improve that situation.

tgoyne · 2021-07-28T01:04:51Z

test/object-store/transaction_log_parsing.cpp

+        bool did_run_section = false;
+
+        {
+            size_t col_ndx = GENERATE(1, 2, 3);


Neat, I didn't know that was a thing. It's nicer than injecting a template type just for this.

DeepChangeChecker: follow links in Mixed columns

64d02a7

fealebenpae requested review from tgoyne and ironage July 27, 2021 17:10

fealebenpae self-assigned this Jul 27, 2021

fealebenpae and others added 2 commits July 27, 2021 19:11

Merge branch 'master' into yg/links-in-mixed-notifications

1a9532b

fix lint error

7a2f5b1

tgoyne reviewed Jul 27, 2021

View reviewed changes

fix more mixed and unresolved links in notifications

f91a9b1

ironage reviewed Jul 27, 2021

View reviewed changes

ironage requested a review from jedelbo July 27, 2021 23:38

tgoyne reviewed Jul 28, 2021

View reviewed changes

ironage added 3 commits July 29, 2021 12:23

fix missing obj update causing crashes when REALM_MAX_BPNODE_SIZE=4

fc9c24b

readability improvements from feedback

72060c9

Merge branch 'master' into yg/links-in-mixed-notifications

746b83d

ironage requested a review from tgoyne July 29, 2021 19:50

tgoyne approved these changes Jul 29, 2021

View reviewed changes

Refactored version of fix

62cdcba

jedelbo force-pushed the yg/links-in-mixed-notifications branch from 40f453a to 62cdcba Compare July 30, 2021 08:31

ironage approved these changes Jul 30, 2021

View reviewed changes

ironage merged commit be03b65 into master Jul 30, 2021

ironage deleted the yg/links-in-mixed-notifications branch July 30, 2021 16:42

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

DeepChangeChecker: follow links in Mixed columns #4828

DeepChangeChecker: follow links in Mixed columns #4828

fealebenpae commented Jul 27, 2021 •

edited by ironage

Loading

tgoyne Jul 27, 2021

ironage Jul 27, 2021

tgoyne Jul 28, 2021

ironage Jul 27, 2021

ironage Jul 27, 2021

tgoyne Jul 28, 2021

tgoyne Jul 28, 2021

tgoyne Jul 28, 2021

tgoyne Jul 28, 2021

ironage Jul 28, 2021

tgoyne Jul 28, 2021

ironage Jul 29, 2021

tgoyne Jul 28, 2021

DeepChangeChecker: follow links in Mixed columns #4828

DeepChangeChecker: follow links in Mixed columns #4828

Conversation

fealebenpae commented Jul 27, 2021 • edited by ironage 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

fealebenpae commented Jul 27, 2021 •

edited by ironage

Loading