diff --git a/CHANGELOG.md b/CHANGELOG.md index 0147896c76..6bd7b4f5fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,8 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* Using an empty KeyPath in C API would result in no filtering being done ([#7805](https://github.com/realm/realm-core/issues/7805)) +* Using an empty KeyPath in C API would result in no filtering being done ([#7805](https://github.com/realm/realm-core/issues/7805), since 13.24.0) +* Filtering notifications with backlink columns as last element could sometimes give wrong results ([#7530](https://github.com/realm/realm-core/issues/7530), since 11.1.0) ### Breaking changes * None. diff --git a/src/realm/object-store/impl/deep_change_checker.cpp b/src/realm/object-store/impl/deep_change_checker.cpp index 7fd6d6c325..be4f0cd213 100644 --- a/src/realm/object-store/impl/deep_change_checker.cpp +++ b/src/realm/object-store/impl/deep_change_checker.cpp @@ -396,24 +396,6 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector& c if (depth >= key_path.size()) { // We've reached the end of the key path. - - // For the special case of having a backlink at the end of a key path we need to check this level too. - // Modifications to a backlink are found via the insertions on the origin table (which we are in right - // now). - auto last_key_path_element = key_path[key_path.size() - 1]; - auto last_column_key = last_key_path_element.second; - if (last_column_key.get_type() == col_type_BackLink) { - auto iterator = m_info.tables.find(table.get_key()); - auto table_has_changed = [iterator] { - return !iterator->second.insertions_empty() || !iterator->second.modifications_empty() || - !iterator->second.deletions_empty(); - }; - if (iterator != m_info.tables.end() && table_has_changed()) { - ColKey root_column_key = key_path[0].second; - changed_columns.push_back(root_column_key); - } - } - return; } @@ -421,15 +403,17 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector& c // Check for a change on the current depth level. auto iterator = m_info.tables.find(table_key); - if (iterator != m_info.tables.end() && (iterator->second.modifications_contains(object_key, {column_key}) || - iterator->second.insertions_contains(object_key))) { - // If an object linked to the root object was changed we only mark the - // property of the root objects as changed. - // This is also the reason why we can return right after doing so because we would only mark the same root - // property again in case we find another change deeper down the same path. - auto root_column_key = key_path[0].second; - changed_columns.push_back(root_column_key); - return; + if (iterator != m_info.tables.end()) { + auto& changes = iterator->second; + if (changes.modifications_contains(object_key, {column_key}) || changes.insertions_contains(object_key)) { + // If an object linked to the root object was changed we only mark the + // property of the root objects as changed. + // This is also the reason why we can return right after doing so because we would only mark the same root + // property again in case we find another change deeper down the same path. + auto root_column_key = key_path[0].second; + changed_columns.push_back(root_column_key); + return; + } } // Only continue for any kind of link. @@ -448,11 +432,12 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector& c auto target_table_key = mixed_object.get_link().get_table_key(); Group* group = table.get_parent_group(); auto target_table = group->get_table(target_table_key); - find_changed_columns(changed_columns, key_path, depth + 1, *target_table, object_key); + find_changed_columns(changed_columns, key_path, depth, *target_table, object_key); } }; // Advance one level deeper into the key path. + depth++; auto object = table.get_object(object_key); if (column_key.is_list()) { if (column_type == col_type_Mixed) { @@ -468,7 +453,7 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector& c auto target_table = table.get_link_target(column_key); for (size_t i = 0; i < list.size(); i++) { auto target_object = list.get(i); - find_changed_columns(changed_columns, key_path, depth + 1, *target_table, target_object); + find_changed_columns(changed_columns, key_path, depth, *target_table, target_object); } } } @@ -484,7 +469,7 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector& c auto set = object.get_linkset(column_key); auto target_table = table.get_link_target(column_key); for (auto& target_object : set) { - find_changed_columns(changed_columns, key_path, depth + 1, *target_table, target_object); + find_changed_columns(changed_columns, key_path, depth, *target_table, target_object); } } } @@ -505,7 +490,7 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector& c return; } auto target_table = table.get_link_target(column_key); - find_changed_columns(changed_columns, key_path, depth + 1, *target_table, target_object); + find_changed_columns(changed_columns, key_path, depth, *target_table, target_object); } else if (column_type == col_type_BackLink) { // A backlink can have multiple origin objects. We need to iterate over all of them. @@ -514,7 +499,23 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector& c size_t backlink_count = object.get_backlink_count(*origin_table, origin_column_key); for (size_t i = 0; i < backlink_count; i++) { auto origin_object = object.get_backlink(*origin_table, origin_column_key, i); - find_changed_columns(changed_columns, key_path, depth + 1, *origin_table, origin_object); + if (depth == key_path.size()) { + // For the special case of having a backlink at the end of a key path we need to check this level too. + // Modifications to a backlink are found via the insertions on the origin table (which we are in right + // now). + auto iterator = m_info.tables.find(origin_table->get_key()); + if (iterator != m_info.tables.end()) { + auto& changes = iterator->second; + if (changes.modifications_contains(origin_object, {origin_column_key}) || + changes.insertions_contains(origin_object)) { + ColKey root_column_key = key_path[0].second; + changed_columns.push_back(root_column_key); + } + } + } + else { + find_changed_columns(changed_columns, key_path, depth, *origin_table, origin_object); + } } } else { diff --git a/src/realm/object-store/shared_realm.cpp b/src/realm/object-store/shared_realm.cpp index 791efe2dab..6e4fcc47d9 100644 --- a/src/realm/object-store/shared_realm.cpp +++ b/src/realm/object-store/shared_realm.cpp @@ -1601,14 +1601,15 @@ bool KeyPathResolver::_resolve(PropId& current, const char* path) current.origin_prop->public_name, m_full_path)); } // Check if the rest of the path is stars. If not, we should exclude this property + auto tmp = path; do { - auto p = find_chr(path, '.'); - StringData property(path, p - path); - path = p; + auto p = find_chr(tmp, '.'); + StringData property(tmp, p - tmp); + tmp = p; if (property != "*") { return false; } - } while (*path++ == '.'); + } while (*tmp++ == '.'); return true; } // Target schema exists - proceed diff --git a/test/object-store/object.cpp b/test/object-store/object.cpp index 37ebc08ec7..e8782166e3 100644 --- a/test/object-store/object.cpp +++ b/test/object-store/object.cpp @@ -312,6 +312,7 @@ TEST_CASE("object") { }; SECTION("add_notification_callback()") { + bool first; auto table = r->read_group().get_table("class_table"); auto col_keys = table->get_column_keys(); std::vector pks = {3, 4, 7, 9, 10, 21, 24, 34, 42, 50}; @@ -345,7 +346,7 @@ TEST_CASE("object") { }; auto require_no_change = [&](Object& object, std::optional key_path_array = std::nullopt) { - bool first = true; + first = true; auto token = object.add_notification_callback( [&](CollectionChangeSet) { REQUIRE(first); @@ -817,6 +818,7 @@ TEST_CASE("object") { r->commit_transaction(); KeyPathArray kpa_to_depth_5 = r->create_key_path_array("table2", {"link2.link2.link2.link2.value"}); + KeyPathArray kpa_wildcard_wildcard = r->create_key_path_array("table2", {"*.*"}); KeyPathArray kpa_to_depth_6 = r->create_key_path_array("table2", {"link2.link2.link2.link2.link2.value"}); @@ -833,6 +835,16 @@ TEST_CASE("object") { REQUIRE_INDICES(change.columns[col_origin_link2.value], 0); } + SECTION("modifying table 'table2', property 'link2' 5 levels deep " + "while observing table 'table2', property '*.*'" + "-> DOES NOT send a notification") { + auto token = require_no_change(object_depth1, kpa_wildcard_wildcard); + + write([&] { + object_depth5.set_column_value("value", 555); + }); + } + SECTION("modifying table 'table2', property 'link2' 6 depths deep " "while observing table 'table2', property 'link2' 5 depths deep " "-> does NOT send a notification") {