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

DeepChangeChecker: follow links in Mixed columns #4828

Merged
merged 8 commits into from
Jul 30, 2021
Merged

DeepChangeChecker: follow links in Mixed columns #4828

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
64d02a7
DeepChangeChecker: follow links in Mixed columns
fealebenpae Jul 27, 2021
1a9532b
Merge branch 'master' into yg/links-in-mixed-notifications
fealebenpae Jul 27, 2021
7a2f5b1
fix lint error
fealebenpae Jul 27, 2021
f91a9b1
fix more mixed and unresolved links in notifications
ironage Jul 27, 2021
fc9c24b
fix missing obj update causing crashes when REALM_MAX_BPNODE_SIZE=4
ironage Jul 29, 2021
72060c9
readability improvements from feedback
ironage Jul 29, 2021
746b83d
Merge branch 'master' into yg/links-in-mixed-notifications
ironage Jul 29, 2021
62cdcba
Refactored version of fix
jedelbo Jul 30, 2021
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
5 changes: 4 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@
* <How to hit and notice issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* Removing a change callback from a Results would sometimes block the calling thread while the query for that Results was running on the background worker thread (since v11.1.0).
* Object observers did not handle the object being deleted properly, which could result in assertion failures mentioning "m_table" in ObjectNotifier ([#4824](https://github.com/realm/realm-core/issues/4824), since v11.1.0).

* Fixed a crash when delivering notifications over a nested hierarchy of lists of Mixed that contain links. ([#4803](https://github.com/realm/realm-core/issues/4803), since v11.0.0)
* Fixed key path filtered notifications throwing on null links and asserting on unresolved links. ([#4828](https://github.com/realm/realm-core/pull/4828), since v11.1.0)
* Fixed a crash when an object which is linked to by a Mixed is invalidated (sync only). ([#4828](https://github.com/realm/realm-core/pull/4828), since v11.0.0)

### Breaking changes
* None.

Expand Down
21 changes: 15 additions & 6 deletions src/realm/obj.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2145,11 +2145,6 @@ void Obj::assign_pk_and_backlinks(const Obj& other)
set.insert(ObjLink{m_table->get_key(), get_key()});
}
}
else if (c.get_type() == col_type_Link) {
// Single link
REALM_ASSERT(!linking_obj.get<ObjKey>(c) || linking_obj.get<ObjKey>(c) == other.get_key());
linking_obj.set(c, get_key());
}
else if (c.is_list()) {
if (c.get_type() == col_type_Mixed) {
auto l = linking_obj.get_list<Mixed>(c);
Expand All @@ -2169,7 +2164,21 @@ void Obj::assign_pk_and_backlinks(const Obj& other)
}
}
else {
REALM_UNREACHABLE(); // missing type handling
REALM_ASSERT(!c.is_collection());
if (c.get_type() == col_type_Link) {
// Single link
REALM_ASSERT(!linking_obj.get<ObjKey>(c) || linking_obj.get<ObjKey>(c) == other.get_key());
linking_obj.set(c, get_key());
}
else if (c.get_type() == col_type_Mixed) {
// Mixed link
REALM_ASSERT(linking_obj.get_any(c).is_null() ||
linking_obj.get_any(c).get_link().get_obj_key() == other.get_key());
linking_obj.set(c, Mixed{ObjLink{m_table->get_key(), get_key()}});
}
else {
REALM_UNREACHABLE(); // missing type handling
}
}
}
return false;
Expand Down
65 changes: 42 additions & 23 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 @@ -276,10 +276,24 @@ bool DeepChangeChecker::check_outgoing_links(Table const& table, ObjKey obj_key,
return do_check_for_collection_modifications(obj, outgoing_link_column, filtered_columns, depth);
}

ObjKey dst_key = obj.get<ObjKey>(outgoing_link_column);
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();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!dst_key) // do not descend into a null or unresolved link
return false;
return check_row(*table.get_link_target(outgoing_link_column), dst_key.value, filtered_columns, depth + 1);
return check_row(*dst_table, dst_key.value, filtered_columns, depth + 1);
};

// Check the `links` of all `m_related_tables` and return true if any of them has a `linked_object_changed`.
Expand All @@ -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()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;
}

// First check if the object was modified directly. We skip this if we're
// looking at the root object because that check is done more efficiently
// in operator() before calling this.
Expand Down Expand Up @@ -384,6 +402,10 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<int64_t>&
return;
}

if (ObjKey(object_key_value).is_unresolved()) {
return;
}

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

// Check for a change on the current depth level.
Expand All @@ -409,6 +431,9 @@ void CollectionKeyPathChangeChecker::find_changed_columns(std::vector<int64_t>&
auto check_mixed_object = [&](const Mixed& mixed_object) {
if (mixed_object.is_type(type_Link, type_TypedLink)) {
auto object_key = mixed_object.get<ObjKey>();
if (object_key.is_unresolved()) {
return;
}
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);
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use range-based for here?

auto target_object = *it;
check_mixed_object(target_object);
}
}
else {
REALM_ASSERT(column_type == col_type_Link || column_type == col_type_LinkList);
auto set = object.get_linkset(column_key);
auto target_table = table.get_link_target(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) {
auto target_object = *it;
find_changed_columns(changed_columns, key_path, depth + 1, *target_table, target_object.value);
}
}
}
else if (column_key.is_dictionary()) {
if (column_type == col_type_Mixed) {
auto dictionary = object.get_dictionary(column_key);
for (size_t i = 0; i < dictionary.size(); i++) {
auto target_object = dictionary.get(dictionary.get_key(i));
check_mixed_object(target_object);
}
}
else {
REALM_ASSERT(column_type == col_type_Link || column_type == col_type_LinkList);
auto dictionary = object.get_dictionary(column_key);
auto linked_dictionary = std::make_unique<DictionaryLinkValues>(dictionary);
auto target_table = table.get_link_target(column_key);
for (size_t i = 0; i < linked_dictionary->size(); i++) {
auto target_object = linked_dictionary->get_key(i);
find_changed_columns(changed_columns, key_path, depth + 1, *target_table, target_object.value);
}
}
// a dictionary always stores mixed values
auto dictionary = object.get_dictionary(column_key);
dictionary.for_all_values([&](Mixed val) {
check_mixed_object(val);
});
}
else if (column_type == col_type_Mixed) {
check_mixed_object(object.get_any(column_key));
}
else if (column_type == col_type_Link) {
// A forward link will only have one target object.
auto target_object = object.get<ObjKey>(column_key);
if (!target_object || target_object.is_unresolved()) {
return;
}
auto target_table = table.get_link_target(column_key);
find_changed_columns(changed_columns, key_path, depth + 1, *target_table, target_object.value);
}
Expand Down
Loading