RCORE-2030 Add full support for automatic client reset for collections in mixed #7683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danieltabacaru
commented
May 8, 2024
Pull Request Test Coverage Report for Build daniel.tabacaru_829Details
💛 - Coveralls |
michael-wb
reviewed
May 8, 2024
| auto size_src = n_src_list->size(); | ||
| auto size_dst = n_dst_list->size(); | ||
| if (size_src != size_dst) | ||
| REALM_ASSERT(src_dictionary.contains(key) && dst_dictionary.contains(key)); |
There was a problem hiding this comment.
Should we just return false here instead of an assert?
There was a problem hiding this comment.
it's sort of a problem if the key (and the indexes in check_matching_list) is not in the dictionary. the caller ensures that (haven't really changed that). I added these to mostly catch unexpected behavior during development (i could turn it into debug asserts if it makes more sense).
michael-wb
reviewed
May 8, 2024
ironage
reviewed
May 8, 2024
ironage
reviewed
May 8, 2024
| @@ -626,52 +626,68 @@ void RecoverLocalChangesetsHandler::copy_lists_with_unrecoverable_changes() | |||
| bool RecoverLocalChangesetsHandler::resolve_path(ListPath& path, Obj remote_obj, Obj local_obj, | |||
There was a problem hiding this comment.
This method is one of the trickier changes in this PR, could you take a look at the coverage report and try to test some more of the uncovered corner cases?
ironage
reviewed
May 8, 2024
ironage
reviewed
May 8, 2024
ironage
reviewed
May 8, 2024
ironage
reviewed
May 8, 2024
ironage
reviewed
May 8, 2024
ironage
reviewed
May 8, 2024
jbreams
reviewed
May 9, 2024
| auto size_src = n_src_list->size(); | ||
| auto size_dst = n_dst_list->size(); | ||
| if (size_src != size_dst) | ||
| REALM_ASSERT(src_dictionary.contains(key) && dst_dictionary.contains(key)); |
| @@ -989,14 +1021,55 @@ void RecoverLocalChangesetsHandler::operator()(const Instruction::Clear& instr) | |||
| ClearResolver(RecoverLocalChangesetsHandler* applier, Instruction::Clear& instr) | |||
| : RecoveryResolver(applier, instr, "Clear") | |||
| { | |||
| switch (instr.collection_type) { | |||
| case Instruction::CollectionType::Single: | |||
| break; | |||
There was a problem hiding this comment.
I think this is actually REALM_UNREACHABLE ?
There was a problem hiding this comment.
we could be recovering instructions from a client that did not generate the collection type, so we play safe (same in the InstructionApplier)
| @@ -626,52 +626,68 @@ void RecoverLocalChangesetsHandler::copy_lists_with_unrecoverable_changes() | |||
| bool RecoverLocalChangesetsHandler::resolve_path(ListPath& path, Obj remote_obj, Obj local_obj, | |||
jbreams
approved these changes
May 9, 2024
ironage
approved these changes
May 9, 2024
michael-wb
approved these changes
May 10, 2024
danieltabacaru
deleted the
dt/client_reset_recovery_for_collections_in_mixed
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What, How & Why?
The recovery of local instructions operating on nested collections follows closely the recovery of operations on embedded objects (you can think of a nested dictionary or list as an embedded object).
Notably:
Additionally, I fixed a crash when recovering AddInteger instructions on a non-integer value (changed remotely).
Fixes #7490.
☑️ ToDos
[ ] C-API, if public C++ API changed[ ]bindgen/spec.yml, if public C++ API changed