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.

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

Js/sectioned notification #5926

Merged
merged 4 commits into from
Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* None.
* Fixed an assertion failure when observing change notifications on a sectioned result, if the first modification was to a linked property that did not cause the state of the sections to change. ([#5912](https://github.com/realm/realm-core/issues/5912), since the introduction of sectioned results in v12.3.0)

### Breaking changes
* None.
Expand Down
8 changes: 8 additions & 0 deletions src/realm/object-store/sectioned_results.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,14 @@ void SectionedResults::calculate_sections()
m_row_to_index_path[i] = {section.index, section.indices.size() - 1};
}
}
if (!has_performed_initial_evalutation) {
m_previous_key_to_index_lookup.clear();
m_prev_section_index_to_key.clear();
Copy link
Member

Choose a reason for hiding this comment

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

I think it might make more sense to assert that these are empty rather than clearing them? If they're non-empty then something's gone wrong.

for (auto& [key, section] : m_sections) {
m_previous_key_to_index_lookup[key] = section.index;
m_prev_section_index_to_key[section.index] = section.key;
}
}
}

size_t SectionedResults::size()
Expand Down
64 changes: 64 additions & 0 deletions test/object-store/sectioned_results.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1554,6 +1554,70 @@ TEST_CASE("sectioned results", "[sectioned_results]") {
}
}

TEST_CASE("sectioned results link notification bug", "[sectioned_results]") {
_impl::RealmCoordinator::assert_no_open_realms();

InMemoryTestFile config;
config.automatic_change_notifications = false;

auto r = Realm::get_shared_realm(config);
r->update_schema(
{{"Transaction",
{{"_id", PropertyType::String, Property::IsPrimary{true}},
{"date", PropertyType::Date},
{"account", PropertyType::Object | PropertyType::Nullable, "Account"}}},
{"Account", {{"_id", PropertyType::String, Property::IsPrimary{true}}, {"name", PropertyType::String}}}});

auto coordinator = _impl::RealmCoordinator::get_coordinator(config.path);
auto transaction_table = r->read_group().get_table("class_Transaction");
transaction_table->get_column_key("date");
auto account_col = transaction_table->get_column_key("account");
auto account_table = r->read_group().get_table("class_Account");
auto account_name_col = account_table->get_column_key("name");

r->begin_transaction();
auto t1 = transaction_table->create_object_with_primary_key("t");
auto a1 = account_table->create_object_with_primary_key("a");
t1.set(account_col, a1.get_key());
r->commit_transaction();

Results results(r, transaction_table);
auto sorted = results.sort({{"date", false}});
auto sectioned_results = sorted.sectioned_results([](Mixed value, SharedRealm realm) {
auto obj = Object(realm, value.get_link());
auto ts = obj.get_column_value<Timestamp>("date");
auto tp = ts.get_time_point();
auto day = std::chrono::floor<std::chrono::hours>(tp);
return Timestamp{day};
});

REQUIRE(sectioned_results.size() == 1);
REQUIRE(sectioned_results[0].size() == 1);

SectionedResultsChangeSet changes;
size_t callback_count = 0;
auto token = sectioned_results.add_notification_callback([&](SectionedResultsChangeSet c) {
changes = c;
++callback_count;
});
coordinator->on_change();

REQUIRE(callback_count == 0);
Copy link
Member

Choose a reason for hiding this comment

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

Adding a advance_and_notify(*r); and expecting one call here makes this test a bit clearer IMO. I was briefly confused into thinking that it was checking the changeset from the initial notification rather than the second notification below.


r->begin_transaction();
a1.set(account_name_col, "a2");
r->commit_transaction();
advance_and_notify(*r);

REQUIRE(callback_count == 2);
REQUIRE(changes.sections_to_insert.empty());
REQUIRE(changes.sections_to_delete.empty());
REQUIRE(changes.insertions.size() == 0);
REQUIRE(changes.deletions.size() == 0);
REQUIRE(changes.modifications.size() == 1);
REQUIRE_INDICES(changes.modifications[0], 0);
}

namespace cf = realm::sectioned_results_fixtures;

TEMPLATE_TEST_CASE("sectioned results primitive types", "[sectioned_results]", cf::MixedVal, cf::Int, cf::Bool,
Expand Down