Support assigning nested collections via templated API #7478

jedelbo · 2024-03-14T12:59:59Z

What, How & Why?

☑️ ToDos

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

coveralls-official · 2024-03-14T13:54:55Z

Pull Request Test Coverage Report for Build jorgen.edelbo_192

Details

464 of 474 (97.89%) changed or added relevant lines in 7 files are covered.
2064 unchanged lines in 77 files lost coverage.
Overall coverage increased (+0.08%) to 91.854%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
test/object-store/primitive_list.cpp	0	3	0.0%
src/realm/object-store/object_accessor.hpp	247	254	97.24%

Files with Coverage Reduction	New Missed Lines	%
src/realm/index_string.cpp	1	87.52%
src/realm/object-store/sync/mongo_database.cpp	1	50.0%
src/realm/object-store/sync/sync_session.hpp	1	94.12%
src/realm/sync/network/default_socket.hpp	1	94.12%
src/realm/sync/socket_provider.hpp	1	87.5%
src/realm/util/uri.cpp	1	99.47%
test/fuzz_tester.hpp	1	59.58%
test/object-store/sync/session/wait_for_completion.cpp	1	98.77%
test/test_index_string.cpp	1	94.63%
src/realm/object-store/c_api/realm.cpp	2	70.37%

Totals
Change from base Build 2227:	0.08%
Covered Lines:	244196
Relevant Lines:	265853

💛 - Coveralls

tgoyne · 2024-04-08T19:59:46Z

src/realm/object-store/dictionary.hpp

+} // namespace object_store
+} // namespace realm
+
+#include <realm/object-store/list.hpp>


Why is this include here?

Because of the mutural dependency between Dictionary and List, it must be included after the definition of Dictionary.

tgoyne · 2024-04-08T19:59:56Z

src/realm/object-store/collection.hpp

@@ -156,6 +156,13 @@ void Collection::validate_embedded(Context& ctx, T&& value, CreatePolicy policy)
        throw IllegalOperation(util::format("Cannot add an existing managed embedded object to a %1.", type_name()));
 }

+// Dummy implementation to satisfy StringifyingContext
+inline std::ostream& operator<<(std::ostream& out, const Collection&)


This should go in a test file.

tgoyne · 2024-04-08T20:00:51Z

src/realm/object-store/dictionary.hpp

@@ -178,6 +184,41 @@ inline Obj Dictionary::get<Obj>(StringData key) const
    return get_object(key);
 }

+namespace {


This is an ODR violation.

Switched bact to just using a lambda.

tgoyne · 2024-04-08T20:01:04Z

src/realm/object-store/dictionary.hpp

+                    attr_changed = !new_val.is_same_type(*old_val);
+                }
+
+                attr_changed = attr_changed || new_val != *old_val;


attr_changed is always false here.

How is that?

tgoyne · 2024-04-08T20:07:48Z

test/object-store/primitive_list.cpp

        sync_config.schema_version = 0;
+        server.start();


I don't see any reason to be delaying starting the server here?

I don't think this test has been run for a long time. I just tried to make it work, but did not really succeed. The test is still not run.

tgoyne · 2024-04-08T20:08:17Z

test/object-store/primitive_list.cpp

        }

+        std::this_thread::sleep_for(std::chrono::milliseconds(5));


This needs to be a proper wait rather than hoping that it's sleeping long enough.

This was just another attempt to get the test to work - but clearly not the right one.

tgoyne · 2024-04-08T20:09:12Z

src/realm/object-store/list.hpp

+    if (m_type == PropertyType::Mixed) {
+        Mixed new_val = ctx.template unbox<Mixed>(value, policy);
+        if (new_val.is_type(type_Dictionary)) {
+            set_collection(list_ndx, CollectionType::Dictionary);


There appears to be a bunch of duplicated code between List and Dictionary.

Similar, but not the same.

jedelbo · 2024-04-11T09:43:49Z

@tgoyne is anything blocking this now?

tgoyne · 2024-04-11T18:31:29Z

b56f162 eliminates the circular dependency that requires doing weird things with includes and extracts the easy-to-extract bits of duplicated logic.

jedelbo · 2024-04-12T12:58:57Z

@tgoyne Probably a better idea to collect all the templates in object_accessor.hpp - although they are collection accessors. I am not sure your changes works in all cases - I will have to make some more tests.

jedelbo · 2024-04-22T07:50:20Z

@tgoyne - your turn

dianaafanador3 · 2024-04-22T13:26:33Z

src/realm/object-store/results.hpp

 {
    return dispatch([&](auto t) {
-        return ctx.box(this->get<std::decay_t<decltype(*t)>>(row_ndx));
+        using U = std::decay_t<decltype(*t)>;


I don't know why this change here is causing a test to fail, in line 428 when the value is an object within an enumeration, with this error
realm::InvalidTableRef: null trying to call the Mixed constructor, I don't know why
Changing line 428 to return ctx.box(this->get<std::decay_t<decltype(*t)>>(ndx)); doesn't call the Mixed constructor and doesn't fails

dianaafanador3 · 2024-04-22T15:28:24Z

src/realm/object-store/object_accessor.hpp

+        if constexpr (std::is_same_v<U, Obj>) {
+            ObjKey old_key;
+            if (auto old_value = dict().try_get(key)) {
+                old_key = old_value->get_link().get_obj_key();


There is a crash when trying to add a Realm Object into a dictionary of realm objects. This tries to read the mixed values from the dictionary when is actually a dictionary of objects, this crash when trying to get the type of the mixed.

dianaafanador3

LGTM!, Changes are working fine with CIM implementation

Small optimization of CollectionBase::do_init_from_parent

ca1eeb6

cla-bot bot added the cla: yes label Mar 14, 2024

github-actions bot assigned jedelbo Mar 14, 2024

jedelbo force-pushed the je/nested-collection-accessor branch from eef764f to e6fae70 Compare March 14, 2024 13:14

Support assigning nested collections via templated API

9eaf6c3

jedelbo force-pushed the je/nested-collection-accessor branch from e6fae70 to 9eaf6c3 Compare March 14, 2024 14:44

Support getting nested collections via templated API

768ab8a

jedelbo force-pushed the je/nested-collection-accessor branch from f3563f3 to 768ab8a Compare March 18, 2024 09:18

jedelbo added 3 commits March 18, 2024 10:20

Merge branch 'master' into je/nested-collection-accessor

26a9307

Update release notes

05d453e

Merge branch 'master' into je/nested-collection-accessor

a5c6504

jedelbo marked this pull request as ready for review April 8, 2024 07:42

jedelbo requested a review from tgoyne April 8, 2024 07:42

tgoyne reviewed Apr 8, 2024

View reviewed changes

jedelbo added 3 commits April 9, 2024 15:59

Assigning unmanaged object to Mixed property

0dd6887

Move test

0039efc

Update after review

0f798cb

jedelbo force-pushed the je/nested-collection-accessor branch from f70bf10 to 0f798cb Compare April 9, 2024 14:00

jedelbo requested a review from tgoyne April 9, 2024 14:23

jedelbo added 2 commits April 16, 2024 09:32

Refactor and fix

03b4721

Make Results::get return proper nested collections

e71f412

jedelbo force-pushed the je/nested-collection-accessor branch from d37888b to e71f412 Compare April 16, 2024 07:32

dianaafanador3 mentioned this pull request Apr 22, 2024

RCOCOA-2271: Collections in Mixed realm/realm-swift#8546

Merged

dianaafanador3 reviewed Apr 22, 2024

View reviewed changes

Rearrange a bit

3c5a1b4

dianaafanador3 reviewed Apr 22, 2024

View reviewed changes

jedelbo added 2 commits April 22, 2024 17:29

Add check for NULL in Dictionary::insert<>()

06e71c4

Add some tests

8e3d77d

jedelbo requested a review from dianaafanador3 April 24, 2024 08:56

dianaafanador3 approved these changes Apr 24, 2024

View reviewed changes

jedelbo merged commit b2e42c8 into master Apr 25, 2024
8 of 34 checks passed

jedelbo deleted the je/nested-collection-accessor branch April 25, 2024 08:24

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support assigning nested collections via templated API #7478

Support assigning nested collections via templated API #7478

jedelbo commented Mar 14, 2024

coveralls-official bot commented Mar 14, 2024 •

edited

Loading

tgoyne Apr 8, 2024

jedelbo Apr 9, 2024

tgoyne Apr 8, 2024

jedelbo Apr 9, 2024

tgoyne Apr 8, 2024

jedelbo Apr 9, 2024

tgoyne Apr 8, 2024

jedelbo Apr 9, 2024

tgoyne Apr 8, 2024

jedelbo Apr 9, 2024

tgoyne Apr 8, 2024

jedelbo Apr 9, 2024

tgoyne Apr 8, 2024

jedelbo Apr 9, 2024

jedelbo commented Apr 11, 2024

tgoyne commented Apr 11, 2024

jedelbo commented Apr 12, 2024

jedelbo commented Apr 22, 2024

dianaafanador3 Apr 22, 2024

dianaafanador3 Apr 22, 2024

dianaafanador3 left a comment

Support assigning nested collections via templated API #7478

Support assigning nested collections via templated API #7478

Conversation

jedelbo commented Mar 14, 2024

What, How & Why?

☑️ ToDos

coveralls-official bot commented Mar 14, 2024 • edited Loading

Pull Request Test Coverage Report for Build jorgen.edelbo_192

Details

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedelbo commented Apr 11, 2024

tgoyne commented Apr 11, 2024

jedelbo commented Apr 12, 2024

jedelbo commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianaafanador3 left a comment

Choose a reason for hiding this comment

coveralls-official bot commented Mar 14, 2024 •

edited

Loading