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

Je/rcore 368 #4167

Merged
merged 4 commits into from
Dec 18, 2020
Merged

Je/rcore 368 #4167

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52093a7
Expose issue in test case
jedelbo Dec 4, 2020
686144e
Tentative solution
jedelbo Dec 5, 2020
a671413
Update after review
jedelbo Dec 10, 2020
dbf943a
Unit test enhanced
jedelbo Dec 16, 2020
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
10 changes: 10 additions & 0 deletions src/realm/object-store/impl/results_notifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,16 @@ void ResultsNotifier::calculate_changes()
for (size_t i = 0; i < m_run_tv.size(); ++i)
next_rows.push_back(m_run_tv.get_key(i).value);

auto table_key = m_query->get_table()->get_key();
if (m_info->tables.count(table_key.value)) {
auto& changes = m_info->tables[table_key.value];
for (auto& key_val : m_previous_rows) {
if (changes.get_deletions().count(key_val)) {
key_val = -1;
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (m_info->tables.count(table_key.value)) {
auto& changes = m_info->tables[table_key.value];
for (auto& key_val : m_previous_rows) {
if (changes.get_deletions().count(key_val)) {
key_val = -1;
}
}
}
if (auto it = m_info->tables.find(table_key.value); it != m_info->tables.end()) {
auto& changes = *it;
for (auto& key_val : m_previous_rows) {
if (changes.deletions_contains(key_val)) {
key_val = -1;
}
}
}

Checking get_deletions() does the wrong thing for a table clear, and using find() eliminates a double lookup in the set (which probably doesn't matter much...). This does point out that we also need to test table clear followed by creating an object with one of the deleted PKs (and resolve the FIXME in deletions_contains() by changing the check on insertions to return true).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that "clear_table" is no longer issued. We will have a deletion instruction for every object. So that code is removed.


m_change = CollectionChangeBuilder::calculate(m_previous_rows, next_rows,
get_modification_checker(*m_info, m_query->get_table()),
m_target_is_in_table_order);
Expand Down
85 changes: 85 additions & 0 deletions test/object-store/results.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3525,3 +3525,88 @@ TEST_CASE("results: query helpers", "[include]") {
CHECK(includes.get_description(table) == "INCLUDE(@links.class_linking_object.link)");
}
}

TEST_CASE("notifications: objects with PK recreated") {
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 a lot of duplicated code. The test should just be another section in the existing test case rather than copying and pasting all the setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the setup is different.

_impl::RealmCoordinator::assert_no_open_realms();

InMemoryTestFile config;
config.cache = false;
config.automatic_change_notifications = false;

auto r = Realm::get_shared_realm(config);
r->update_schema({
{"no_pk",
{
{"id", PropertyType::Int},
{"value", PropertyType::Int},
}},
{"int_pk",
{
{"id", PropertyType::Int, Property::IsPrimary{true}},
{"value", PropertyType::Int},
}},
{"string_pk",
{
{"id", PropertyType::String, Property::IsPrimary{true}},
{"value", PropertyType::Int},
}},
});

auto add_callback = [](Results& results, int& calls, CollectionChangeSet& changes) {
return results.add_notification_callback([&](CollectionChangeSet c, std::exception_ptr err) {
REQUIRE_FALSE(err);
++calls;
changes = std::move(c);
});
};

auto coordinator = _impl::RealmCoordinator::get_existing_coordinator(config.path);
auto table1 = r->read_group().get_table("class_no_pk");
auto table2 = r->read_group().get_table("class_int_pk");
auto table3 = r->read_group().get_table("class_string_pk");

TestContext d(r);
auto create = [&](StringData type, util::Any&& value) {
return Object::create(d, r, *r->schema().find(type), value);
};

r->begin_transaction();
auto k1 = create("no_pk", AnyDict{{"id", INT64_C(123)}, {"value", INT64_C(100)}}).obj().get_key();
auto k2 = create("int_pk", AnyDict{{"id", INT64_C(456)}, {"value", INT64_C(100)}}).obj().get_key();
auto k3 = create("string_pk", AnyDict{{"id", std::string("hello")}, {"value", INT64_C(100)}}).obj().get_key();
r->commit_transaction();

Results results1(r, table1->where());
int calls1 = 0;
CollectionChangeSet changes1;
auto token1 = add_callback(results1, calls1, changes1);

Results results2(r, table2->where());
int calls2 = 0;
CollectionChangeSet changes2;
auto token2 = add_callback(results2, calls2, changes2);

Results results3(r, table3->where());
int calls3 = 0;
CollectionChangeSet changes3;
auto token3 = add_callback(results3, calls3, changes3);

advance_and_notify(*r);
REQUIRE(calls1 == 1);
REQUIRE(calls2 == 1);
REQUIRE(calls3 == 1);

r->begin_transaction();
r->read_group().get_table("class_no_pk")->remove_object(k1);
r->read_group().get_table("class_int_pk")->remove_object(k2);
r->read_group().get_table("class_string_pk")->remove_object(k3);
create("no_pk", AnyDict{{"id", INT64_C(123)}, {"value", INT64_C(200)}});
create("int_pk", AnyDict{{"id", INT64_C(456)}, {"value", INT64_C(200)}});
create("string_pk", AnyDict{{"id", std::string("hello")}, {"value", INT64_C(200)}});
r->commit_transaction();

advance_and_notify(*r);
REQUIRE(calls1 == 2);
REQUIRE(calls2 == 2);
REQUIRE(calls3 == 2);
}