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-2168: Replicate clear instruction unconditionally for nested collections #7821

Merged
merged 6 commits into from
Jun 28, 2024

Conversation

jedelbo
Copy link
Contributor

@jedelbo jedelbo commented Jun 19, 2024

What, How & Why?

Fixes #7809

This change makes sure that a collection nested in a Mixed type always will contain the values that has been assigned to it by some client. Before this change you could end up having a combination of values assigned by different clients.

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed
  • bindgen/spec.yml, if public C++ API changed

Copy link

coveralls-official bot commented Jun 19, 2024

Pull Request Test Coverage Report for Build jorgen.edelbo_319

Details

  • 81 of 81 (100.0%) changed or added relevant lines in 3 files are covered.
  • 59 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+0.02%) to 90.982%

Files with Coverage Reduction New Missed Lines %
src/realm/array_mixed.cpp 1 91.94%
test/test_query2.cpp 1 98.73%
test/test_util_network_ssl.cpp 1 89.59%
src/realm/query_expression.hpp 2 93.81%
test/test_lang_bind_helper.cpp 2 93.2%
src/realm/sync/client.cpp 3 91.74%
src/realm/unicode.cpp 3 83.83%
src/realm/link_translator.cpp 4 76.92%
src/realm/bplustree.cpp 6 72.55%
src/realm/index_string.cpp 8 84.69%
Totals Coverage Status
Change from base Build 2432: 0.02%
Covered Lines: 214807
Relevant Lines: 236099

💛 - Coveralls

@danieltabacaru
Copy link
Collaborator

Can you add a summary in the description of the behavior these changes introduce?

@jedelbo
Copy link
Contributor Author

jedelbo commented Jun 21, 2024

Can you add a summary in the description of the behavior these changes introduce?

Done

test/test_sync.cpp Outdated Show resolved Hide resolved
@@ -6204,6 +6204,95 @@ TEST(Sync_DeleteCollectionInCollection)
}
}

TEST(Sync_NestedCollectionClear)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add the same test for dictionaries too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was hoping we can have the same schema with different scenarios as for lists (i.e, Dictionary vs Dictionary in Mixed)

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 am not sure what you mean. I have added a test case that fails without the fix that this PR provides.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I meant that for lists you use all three Mixed fields in the schema in tests.

Copy link

coveralls-official bot commented Jun 27, 2024

Pull Request Test Coverage Report for Build jorgen.edelbo_323

Details

  • 92 of 92 (100.0%) changed or added relevant lines in 3 files are covered.
  • 78 unchanged lines in 15 files lost coverage.
  • Overall coverage increased (+0.008%) to 90.974%

Files with Coverage Reduction New Missed Lines %
src/realm/array_mixed.cpp 1 91.94%
src/realm/sort_descriptor.cpp 1 94.06%
src/realm/sync/noinst/server/server_history.cpp 1 63.7%
test/test_dictionary.cpp 1 99.83%
test/test_query2.cpp 1 98.73%
src/realm/query_expression.hpp 2 93.81%
test/test_all.cpp 2 75.82%
src/realm/unicode.cpp 3 83.83%
src/realm/link_translator.cpp 4 76.92%
test/fuzz_group.cpp 4 47.85%
Totals Coverage Status
Change from base Build 2432: 0.008%
Covered Lines: 214793
Relevant Lines: 236103

💛 - Coveralls

@jedelbo jedelbo requested a review from danieltabacaru June 27, 2024 11:31
Copy link

coveralls-official bot commented Jun 27, 2024

Pull Request Test Coverage Report for Build jorgen.edelbo_324

Details

  • 92 of 92 (100.0%) changed or added relevant lines in 3 files are covered.
  • 77 unchanged lines in 15 files lost coverage.
  • Overall coverage increased (+0.005%) to 90.971%

Files with Coverage Reduction New Missed Lines %
src/realm/array_mixed.cpp 1 91.94%
src/realm/sync/noinst/client_impl_base.cpp 1 82.26%
test/test_index_string.cpp 1 93.48%
test/test_query2.cpp 1 98.73%
src/realm/query_expression.hpp 2 93.81%
test/test_all.cpp 2 75.82%
src/realm/sync/client.cpp 3 91.74%
src/realm/table.cpp 3 90.42%
src/realm/link_translator.cpp 4 76.92%
test/fuzz_group.cpp 4 50.08%
Totals Coverage Status
Change from base Build 2432: 0.005%
Covered Lines: 214792
Relevant Lines: 236110

💛 - Coveralls

test/test_sync.cpp Outdated Show resolved Hide resolved
@@ -6204,6 +6204,95 @@ TEST(Sync_DeleteCollectionInCollection)
}
}

TEST(Sync_NestedCollectionClear)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was hoping we can have the same schema with different scenarios as for lists (i.e, Dictionary vs Dictionary in Mixed)

@jedelbo jedelbo requested a review from danieltabacaru June 28, 2024 09:37
Copy link

coveralls-official bot commented Jun 28, 2024

Pull Request Test Coverage Report for Build jorgen.edelbo_329

Details

  • 91 of 91 (100.0%) changed or added relevant lines in 3 files are covered.
  • 57 unchanged lines in 13 files lost coverage.
  • Overall coverage increased (+0.01%) to 90.96%

Files with Coverage Reduction New Missed Lines %
src/realm/sync/noinst/client_impl_base.hpp 1 92.51%
src/realm/sync/noinst/server/server_history.cpp 1 63.7%
src/realm/cluster.cpp 2 75.6%
src/realm/sync/network/http.hpp 2 82.27%
src/realm/query_engine.cpp 3 92.36%
src/realm/sync/instruction_replication.cpp 3 91.48%
src/realm/table.cpp 3 90.42%
src/realm/unicode.cpp 3 83.83%
test/test_thread.cpp 3 66.14%
src/realm/sync/transform.cpp 4 60.92%
Totals Coverage Status
Change from base Build 2446: 0.01%
Covered Lines: 214857
Relevant Lines: 236211

💛 - Coveralls

@jedelbo jedelbo merged commit 3ccd33a into master Jun 28, 2024
13 of 14 checks passed
@jedelbo jedelbo deleted the je/nested-collection-clear branch June 28, 2024 14:00
@github-actions github-actions bot mentioned this pull request Jun 28, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assigning a mixed property with a collection should replace instead of merge
2 participants