Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RCORE-2061: Fix handling backlink column as last element in KeyPath #7987

Merged
merged 1 commit into from
Aug 16, 2024
Merged

RCORE-2061: Fix handling backlink column as last element in KeyPath #7987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading