From 52093a7cd081582a68fc0ccd77fceeacc15b67fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Fri, 4 Dec 2020 15:12:36 +0100 Subject: [PATCH 1/4] Expose issue in test case --- test/object-store/results.cpp | 85 +++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/test/object-store/results.cpp b/test/object-store/results.cpp index 553bf25d95c..97d1c0c0020 100644 --- a/test/object-store/results.cpp +++ b/test/object-store/results.cpp @@ -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") { + _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); +} From 686144ebb4177311e884b14017ba11b8339ece40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Sat, 5 Dec 2020 15:32:39 +0100 Subject: [PATCH 2/4] Tentative solution --- src/realm/object-store/impl/results_notifier.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/realm/object-store/impl/results_notifier.cpp b/src/realm/object-store/impl/results_notifier.cpp index 13d045dd05a..09768e7da9e 100644 --- a/src/realm/object-store/impl/results_notifier.cpp +++ b/src/realm/object-store/impl/results_notifier.cpp @@ -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; + } + } + } + m_change = CollectionChangeBuilder::calculate(m_previous_rows, next_rows, get_modification_checker(*m_info, m_query->get_table()), m_target_is_in_table_order); From a671413346ef58f99bcedeee27faa8591520054e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Thu, 10 Dec 2020 09:36:03 +0100 Subject: [PATCH 3/4] Update after review --- src/realm/impl/transact_log.hpp | 9 ---- .../object-store/impl/results_notifier.cpp | 6 +-- .../impl/transact_log_handler.cpp | 16 ------- src/realm/object-store/object_changeset.cpp | 15 ------- src/realm/object-store/object_changeset.hpp | 8 +--- test/object-store/results.cpp | 42 +++++++++++++------ 6 files changed, 34 insertions(+), 62 deletions(-) diff --git a/src/realm/impl/transact_log.hpp b/src/realm/impl/transact_log.hpp index dd23ac4bb98..d169dec6285 100644 --- a/src/realm/impl/transact_log.hpp +++ b/src/realm/impl/transact_log.hpp @@ -1307,15 +1307,6 @@ class TransactReverser { return true; } - bool clear_table(size_t old_size) - { - while (old_size--) { - m_encoder.create_object(null_key); - append_instruction(); - } - return true; - } - bool set_link_type(ColKey key) { m_encoder.set_link_type(key); diff --git a/src/realm/object-store/impl/results_notifier.cpp b/src/realm/object-store/impl/results_notifier.cpp index 09768e7da9e..7155b3ae531 100644 --- a/src/realm/object-store/impl/results_notifier.cpp +++ b/src/realm/object-store/impl/results_notifier.cpp @@ -132,10 +132,10 @@ void ResultsNotifier::calculate_changes() 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]; + if (auto it = m_info->tables.find(table_key.value); it != m_info->tables.end()) { + auto& changes = it->second; for (auto& key_val : m_previous_rows) { - if (changes.get_deletions().count(key_val)) { + if (changes.deletions_contains(key_val)) { key_val = -1; } } diff --git a/src/realm/object-store/impl/transact_log_handler.cpp b/src/realm/object-store/impl/transact_log_handler.cpp index 7291bb901e7..0c41d6e3839 100644 --- a/src/realm/object-store/impl/transact_log_handler.cpp +++ b/src/realm/object-store/impl/transact_log_handler.cpp @@ -263,10 +263,6 @@ class TransactLogValidationMixin { { return true; } - bool clear_table(size_t = 0) noexcept - { - return true; - } bool list_set(size_t) { return true; @@ -491,18 +487,6 @@ class TransactLogObserver : public TransactLogValidationMixin { return true; } - bool clear_table(size_t old_size) - { - auto cur_table = current_table(); - if (m_active_table) - m_active_table->clear(old_size); - auto it = remove_if(begin(m_info.lists), end(m_info.lists), [&](auto const& lv) { - return lv.table_key == cur_table; - }); - m_info.lists.erase(it, end(m_info.lists)); - return true; - } - bool insert_column(ColKey) { m_info.schema_changed = true; diff --git a/src/realm/object-store/object_changeset.cpp b/src/realm/object-store/object_changeset.cpp index f6c7290cc15..bcaac92e000 100644 --- a/src/realm/object-store/object_changeset.cpp +++ b/src/realm/object-store/object_changeset.cpp @@ -42,15 +42,6 @@ void ObjectChangeSet::deletions_add(ObjectKeyType obj) } } -void ObjectChangeSet::clear(size_t old_size) -{ - static_cast(old_size); // unused - m_clear_did_occur = true; - m_insertions.clear(); - m_modifications.clear(); - m_deletions.clear(); -} - bool ObjectChangeSet::insertions_remove(ObjectKeyType obj) { return m_insertions.erase(obj) > 0; @@ -68,11 +59,6 @@ bool ObjectChangeSet::deletions_remove(ObjectKeyType obj) bool ObjectChangeSet::deletions_contains(ObjectKeyType obj) const { - if (m_clear_did_occur) { - // FIXME: what are the expected notifications when an object is deleted - // and then another object is inserted with the same key? - return m_insertions.count(obj) == 0; - } return m_deletions.count(obj) > 0; } @@ -103,7 +89,6 @@ void ObjectChangeSet::merge(ObjectChangeSet&& other) *this = std::move(other); return; } - m_clear_did_occur = m_clear_did_occur || other.m_clear_did_occur; verify(); other.verify(); diff --git a/src/realm/object-store/object_changeset.hpp b/src/realm/object-store/object_changeset.hpp index 4e63a862c4b..a7ec76b8ca1 100644 --- a/src/realm/object-store/object_changeset.hpp +++ b/src/realm/object-store/object_changeset.hpp @@ -46,7 +46,6 @@ class ObjectChangeSet { void insertions_add(ObjectKeyType obj); void modifications_add(ObjectKeyType obj, ColKeyType col); void deletions_add(ObjectKeyType obj); - void clear(size_t old_size); bool insertions_remove(ObjectKeyType obj); bool modifications_remove(ObjectKeyType obj); @@ -85,13 +84,9 @@ class ObjectChangeSet { return m_deletions.size(); } - bool clear_did_occur() const noexcept - { - return m_clear_did_occur; - } bool empty() const noexcept { - return m_deletions.empty() && m_insertions.empty() && m_modifications.empty() && !m_clear_did_occur; + return m_deletions.empty() && m_insertions.empty() && m_modifications.empty(); } void merge(ObjectChangeSet&& other); @@ -114,7 +109,6 @@ class ObjectChangeSet { ObjectSet m_deletions; ObjectSet m_insertions; ObjectMapToColumnSet m_modifications; - bool m_clear_did_occur = false; }; } // end namespace realm diff --git a/test/object-store/results.cpp b/test/object-store/results.cpp index 97d1c0c0020..7bf596043c7 100644 --- a/test/object-store/results.cpp +++ b/test/object-store/results.cpp @@ -3596,17 +3596,35 @@ TEST_CASE("notifications: objects with PK recreated") { 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(); + SECTION("objects removed") { + 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); + advance_and_notify(*r); + REQUIRE(calls1 == 2); + REQUIRE(calls2 == 2); + REQUIRE(calls3 == 2); + } + + SECTION("table cleared") { + r->begin_transaction(); + r->read_group().get_table("class_no_pk")->clear(); + r->read_group().get_table("class_int_pk")->clear(); + r->read_group().get_table("class_string_pk")->clear(); + 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); + } } From dbf943aafc297bba9d4b71b350efe799fbf44304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Edelbo?= Date: Wed, 16 Dec 2020 09:05:23 +0100 Subject: [PATCH 4/4] Unit test enhanced --- test/object-store/results.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/object-store/results.cpp b/test/object-store/results.cpp index 7bf596043c7..e9fd633b1a4 100644 --- a/test/object-store/results.cpp +++ b/test/object-store/results.cpp @@ -3607,6 +3607,12 @@ TEST_CASE("notifications: objects with PK recreated") { r->commit_transaction(); advance_and_notify(*r); + REQUIRE(changes1.insertions.count() == 1); + REQUIRE(changes1.deletions.count() == 1); + REQUIRE(changes2.insertions.count() == 1); + REQUIRE(changes2.deletions.count() == 1); + REQUIRE(changes3.insertions.count() == 1); + REQUIRE(changes3.deletions.count() == 1); REQUIRE(calls1 == 2); REQUIRE(calls2 == 2); REQUIRE(calls3 == 2); @@ -3623,6 +3629,12 @@ TEST_CASE("notifications: objects with PK recreated") { r->commit_transaction(); advance_and_notify(*r); + REQUIRE(changes1.insertions.count() == 1); + REQUIRE(changes1.deletions.count() == 1); + REQUIRE(changes2.insertions.count() == 1); + REQUIRE(changes2.deletions.count() == 1); + REQUIRE(changes3.insertions.count() == 1); + REQUIRE(changes3.deletions.count() == 1); REQUIRE(calls1 == 2); REQUIRE(calls2 == 2); REQUIRE(calls3 == 2);