Skip to content

Commit

Permalink
Fix handling backlink column as last element in KeyPath
Browse files Browse the repository at this point in the history
  • Loading branch information
@jedelbo
jedelbo committed Aug 16, 2024
1 parent b641e12 commit 13273f0
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 38 deletions.
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@

### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](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.
Expand Down
65 changes: 33 additions & 32 deletions src/realm/object-store/impl/deep_change_checker.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -396,40 +396,24 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<ColKey>& 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;
}

auto [table_key, column_key] = key_path.at(depth);

// 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.
Expand All @@ -448,11 +432,12 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<ColKey>& 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) {
Expand All @@ -468,7 +453,7 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<ColKey>& 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);
}
}
}
Expand All @@ -484,7 +469,7 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<ColKey>& 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);
}
}
}
Expand All @@ -505,7 +490,7 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<ColKey>& 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.
Expand All @@ -514,7 +499,23 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<ColKey>& 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 {
Expand Down
9 changes: 5 additions & 4 deletions src/realm/object-store/shared_realm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 13 additions & 1 deletion test/object-store/object.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<int64_t> pks = {3, 4, 7, 9, 10, 21, 24, 34, 42, 50};
Expand Down Expand Up @@ -345,7 +346,7 @@ TEST_CASE("object") {
};

auto require_no_change = [&](Object& object, std::optional<KeyPathArray> key_path_array = std::nullopt) {
bool first = true;
first = true;
auto token = object.add_notification_callback(
[&](CollectionChangeSet) {
REQUIRE(first);
Expand Down Expand Up @@ -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"});

Expand All @@ -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") {
Expand Down

0 comments on commit 13273f0

Please sign in to comment.