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

RCORE-1973 Add role/permissions tests for new bootstrap feature #7675

Merged
merged 34 commits into from
Jul 4, 2024
Merged
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
67691ce
Moved role change tests to separate test file
Jun 11, 2024
2d304d5
Fixed building of new flx_role_change.cpp file
Jun 11, 2024
8d91a08
Added local changes w/role bootstrap test - fixed exception in subscr…
Jun 16, 2024
e8924dc
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jun 16, 2024
ac79b1f
Updated local change test to include valid offline writes during role…
Jun 17, 2024
b78283e
Added role change test during initial schema bootstrap
Jun 18, 2024
d350217
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jun 18, 2024
f9aec9b
Updated changelog
Jun 18, 2024
2d908be
Wrapped up role change during bootstrap tests
Jun 19, 2024
1878811
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jun 19, 2024
89b2c39
Removed debug statments to fix thread sanitizer
Jun 19, 2024
363581c
Updated sub state comments and reverted a minor change
Jun 19, 2024
990da9e
updates from review
Jun 19, 2024
fb91292
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jun 19, 2024
5ad76e9
Refactored role change tests and broke out into 2 separate test cases
Jun 19, 2024
2eb382e
Moved harness from a global to a static var in each test case
Jun 19, 2024
6092943
Reverted resetting the bootstrapping subscription state back to Pending
Jun 20, 2024
560cdb3
Updated baas to use protocol v14 and removed the feature flag for rol…
Jun 20, 2024
7951ef2
Removed left over code in statement...
Jun 20, 2024
304e658
Updated baasaas version to be a cached version
Jun 20, 2024
6994a85
Updated baasaas githash and reordered role change during bootstrap to…
Jun 20, 2024
579c50d
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jun 20, 2024
7983f9d
Minor updates to reuse the verify_records() fcn
Jun 25, 2024
3f17ce6
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jun 28, 2024
524261a
Updates from review
Jun 28, 2024
48497b1
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jul 1, 2024
2d67e5d
Updated changelog after release
Jul 1, 2024
2d27215
Fixed one more comment in changelog
Jul 1, 2024
7ee0ad0
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jul 1, 2024
75c9654
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jul 1, 2024
321b889
RCORE-2174 Bootstrap store is not being reset if initial subscription…
Jul 1, 2024
9f4b041
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jul 2, 2024
f869865
Updated changelog comments
Jul 3, 2024
2e5a010
Merge branch 'feature/role-change' of github.com:realm/realm-core int…
Jul 4, 2024
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
7 changes: 4 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@

### Enhancements
* <New feature description> (PR [#????](https://github.com/realm/realm-core/pull/????))
* None.
* Add support for server initiated bootstraps. ([PR #7440](https://github.com/realm/realm-core/pull/7440))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't this already done in a separate PR? Should this be under Internals? What is a user or SDK developer supposed to take away from this changelog entry? How will this effect them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this comment to the internal section and added a new comment to Enhancements:
Role and permissions changes no longer require a client reset to update the local realm.


### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* Fixed a change of mode from Strong to All when removing links from an embedded object that links to a tombstone. This affects sync apps that use embedded objects which have a `Lst<Mixed>` that contains a link to another top level object which has been deleted by another sync client (creating a tombstone locally). In this particular case, the switch would cause any remaining link removals to recursively delete the destination object if there were no other links to it. ([#7828](https://github.com/realm/realm-core/issues/7828), since 14.0.0-beta.0)
* Fixed removing backlinks from the wrong objects if the link came from a nested list, nested dictionary, top-level dictionary, or list of mixed, and the source table had more than 256 objects. This could manifest as `array_backlink.cpp:112: Assertion failed: int64_t(value >> 1) == key.value` when removing an object. ([#7594](https://github.com/realm/realm-core/issues/7594), since v11 for dictionaries)
* Fixed the collapse/rejoin of clusters which contained nested collections with links. This could manifest as `array.cpp:319: Array::move() Assertion failed: begin <= end [2, 1]` when removing an object. ([#7839](https://github.com/realm/realm-core/issues/7839), since the introduction of nested collections in v14.0.0-beta.0)
* wait_for_upload_completion() was inconsistent in how it handled commits which did not produce any changesets to upload. Previously it would sometimes complete immediately if all commits waiting to be uploaded were empty, and at other times it would wait for a server roundtrip. It will now always complete immediately. ([PR #7796](https://github.com/realm/realm-core/pull/7796)).
* If a sync session is interrupted by a disconnect or restart while downloading a bootstrap, stale data from the previous bootstrap may be included when the session reconnects and downloads the bootstrap. This can lead to objects stored in the database that do not match the actual state of the server and potentially leading to compensating writes. ([#7827](https://github.com/realm/realm-core/issues/7827), since v12.0.0)

### Breaking changes
* None.
Expand All @@ -22,6 +23,8 @@
### Internals
* Fixed `Table::remove_object_recursive` which wouldn't recursively follow links through a single `Mixed` property. This feature is exposed publicly on `Table` but no SDK currently uses it, so this is considered internal. ([#7829](https://github.com/realm/realm-core/issues/7829), likely since the introduction of Mixed)
* Upload completion is now tracked in a multiprocess-compatible manner ([PR #7796](https://github.com/realm/realm-core/pull/7796)).
* Protocol version has been updated to v14 to support server intiated bootstraps and role change updates without a client reset. ([PR #7440](https://github.com/realm/realm-core/pull/7440))
* Create additional role change tests to verify role change during initial schema and subscription bootstraps. ([PR #7675](https://github.com/realm/realm-core/pull/7675))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need a changelog entry for just adding new tests.


----------------------------------------------

Expand Down Expand Up @@ -54,7 +57,6 @@
* It is no longer an error to set a base url for an App with a trailing slash - for example, `https://services.cloud.mongodb.com/` instead of `https://services.cloud.mongodb.com` - before this change that would result in a 404 error from the server ([PR #7791](https://github.com/realm/realm-core/pull/7791)).
* Performance has been improved for range queries on integers and timestamps. Requires that you use the "BETWEEN" operation in MQL or the Query::between() method when you build the query. (PR [#7785](https://github.com/realm/realm-core/pull/7785))
* Expose `Obj::add_int()` in the bindgen spec. ([PR #7797](https://github.com/realm/realm-core/pull/7797)).
* Add support for server initiated bootstraps. ([PR #7440](https://github.com/realm/realm-core/pull/7440))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, you're handling merge conflicts so these are going to jump during each PR remaining for this project ☹️


### Fixed
* Valgrind could report a branch on an uninitialized read when opening something that is not an encrypted Realm file as an encrypted Realm file ([PR #7789](https://github.com/realm/realm-core/pull/7789), since v14.10.0).
Expand All @@ -72,7 +74,6 @@

### Internals
* Switch to building the Swift package and Cocoa binaries as C++20 ([PR #7802](https://github.com/realm/realm-core/pull/7802)).
* Protocol version has been updated to v14 to support server intiated bootstraps and role change updates without a client reset. ([PR #7440](https://github.com/realm/realm-core/pull/7440))

----------------------------------------------

Expand Down
4 changes: 2 additions & 2 deletions dependencies.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@ VERSION: 14.10.2
OPENSSL_VERSION: 3.2.0
ZLIB_VERSION: 1.2.13
# https://github.com/10gen/baas/commits
# 30c10fd is 2024 June 6
BAAS_VERSION: 30c10fd8e9400fc77e594340422d8b75c210e18d
# 458cf26 is 2024 June 20
BAAS_VERSION: 458cf268367cb5db1abba731b67ef1d0b9de1cd4
BAAS_VERSION_TYPE: githash
8 changes: 5 additions & 3 deletions src/realm/sync/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -836,7 +836,7 @@ bool SessionImpl::process_flx_bootstrap_message(const SyncProgress& progress, Do
}

try {
process_pending_flx_bootstrap();
process_pending_flx_bootstrap(); // throws
}
catch (const IntegrationException& e) {
on_integration_failure(e);
Expand All @@ -855,8 +855,6 @@ void SessionImpl::process_pending_flx_bootstrap()
if (!m_is_flx_sync_session || m_state != State::Active) {
return;
}
// Should never be called if session is not active
REALM_ASSERT_EX(m_state == SessionImpl::Active, m_state);
auto bootstrap_store = m_wrapper.get_flx_pending_bootstrap_store();
if (!bootstrap_store->has_pending()) {
return;
Expand Down Expand Up @@ -1185,6 +1183,10 @@ void SessionWrapper::on_flx_sync_progress(int64_t new_version, DownloadBatchStat
if (!has_flx_subscription_store()) {
return;
}
// Is this a server-initiated bootstrap? Skip notifying the subscription store
if (new_version == m_flx_active_version) {
return;
}
REALM_ASSERT(!m_finalized);
REALM_ASSERT(new_version >= m_flx_last_seen_version);
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't new_version always be either m_flx_last_seen_version, m_flx_active_version or m_flx_active_version+1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a query bootstrap is in progress when a server initiated bootstrap takes place (which cancels the current query bootstrap), m_flx_last_seen_version will be m_flx_active_version + 1 while new_version will be m_flx_active_version and this assertion will fail.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's still one of the cases I mentioned. I was proposing we update the assert, not keep the deleted one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the m_flx_last_seen_version can actually be greater than m_flx_active_version + 1 in the case of a QUERY_ERROR response and it always starts out at 0 when the session is started until the first download message is received. Since the current validation and checking for server-initiated bootstrap logic is valid, I am going to leave this as-is.

Copy link
Collaborator

@danieltabacaru danieltabacaru Jun 19, 2024

Choose a reason for hiding this comment

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

It seems that m_flx_last_seen_version should be updated when we get a query error then. m_flx_active_version is initialized though when the SessionWrapper is actualized. Either way, your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would update it, but this is a change that might affect progress notifications, which uses this value. I created RCORE-2173 to handle updating this separately if we want to update it.

REALM_ASSERT(new_version >= m_flx_active_version);
Expand Down
12 changes: 11 additions & 1 deletion src/realm/sync/noinst/client_impl_base.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1527,6 +1527,16 @@ void Session::cancel_resumption_delay()
if (unbind_process_complete())
initiate_rebind(); // Throws

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this different from #7831 ?

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 probably wasn't the best (or correct) way to do it, but PR #7831 was branched off of this PR branch, since it needed tests from this PR in order to validate the changes...

process_pending_flx_bootstrap(); // throws
}
catch (const IntegrationException& error) {
on_integration_failure(error);
}
catch (...) {
on_integration_failure(IntegrationException(exception_to_status()));
}

m_conn.one_more_active_unsuspended_session(); // Throws
if (m_try_again_activation_timer) {
m_try_again_activation_timer.reset();
Expand Down Expand Up @@ -1717,7 +1727,7 @@ void Session::activate()
m_conn.one_more_active_unsuspended_session(); // Throws

try {
process_pending_flx_bootstrap();
process_pending_flx_bootstrap(); // throws
}
catch (const IntegrationException& error) {
on_integration_failure(error);
Expand Down
1 change: 1 addition & 0 deletions test/object-store/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ if(REALM_ENABLE_SYNC)
sync/app.cpp
sync/client_reset.cpp
sync/flx_migration.cpp
sync/flx_role_change.cpp
sync/flx_schema_migration.cpp
sync/flx_sync.cpp
sync/metadata.cpp
Expand Down
Loading
Loading