RCOCOA-2271: Collections in Mixed #8546

dianaafanador3 · 2024-04-12T16:14:02Z

Added support for storing nested collections (List and Map not ManagedSet) in a AnyRealmValue.

class MixedObject: Object {
  @Persisted var anyValue: AnyRealmValue
}

// You can build a AnyRealmValue from a Swift's Dictionary.
let dictionary: Dictionary<String, AnyRealmValue> = ["key1": .string("hello"), "key2": .bool(false)]

// You can build a AnyRealmValue from a Swift's Map
// and nest a collection within another collection.
let list: Array<AnyRealmValue> = [.int(12), .double(14.17), AnyRealmValue.fromDictionary(dictionary)]

let realm = realmWithTestPath()
try realm.write {
  let obj = MixedObject()
  obj.anyValue = AnyRealmValue.fromArray(list)
  realm.add(o)
}

Added new operators to Swift's Query API for supporting querying nested collections.

realm.objects(MixedObject.self).where { $0.anyValue[0][0][1] == .double(76.54) }

The .all operator allows looking up in all keys or indexes.

realm.objects(MixedObject.self).where { $0.anyValue["key"].all == .bool(false) }

dianaafanador3 · 2024-04-22T11:17:44Z

This is waiting for this PR to be merged to core. Also, there is one failing test which is working locally, taking a look at it, but putting this on review on the meantime.

tgoyne

A first pass over things.

Realm/RLMDictionary.h

Realm/RLMAccessor.mm

Realm/RLMArray_Private.hpp

Realm/RLMConstants.h

Realm/RLMAccessor.mm

RealmSwift/AnyRealmValue.swift

tgoyne · 2024-04-23T15:55:52Z

RealmSwift/List.swift

@@ -361,6 +361,14 @@ extension List: MutableCollection {
    }
 }

+// MARK: - Hashable
+
+extension List: Hashable {


This is not a valid definition of Hashable for List, and such a thing is probably not possible. Using a List as a dictionary key does not really make any sense too.

Same as Map

tgoyne · 2024-04-23T15:56:20Z

RealmSwift/Map.swift

@@ -779,6 +779,14 @@ public final class Map<Key: _MapKey, Value: RealmCollectionValue>: RLMSwiftColle
    }
 }

+// MARK: - Hashable
+
+extension Map: Hashable {


Similarly, this is not at all a valid Hashable implementation.

Using ObjectIdentifier given that we already conform to Equatable

ObjectIdentifier is not a valid definition either. Two managed dictionary objects which correspond to the same field are equal to each other but have different ObjectIdentifiers. All of our accessor types are fundamentally incompatible with Hashable due to that their identity changes when you promote an unmanaged object.

@tgoyne we do need to conform to Hashable to be able to include Map or List as a an associated value in AnyRealmValue if we want to go that approach.
I can think of a few approaches for this:

Use ObjectIdentifer and sacrifice equality for managed and unmanaged object

Have associated values as Swift's native types Array/Dictionary, this will clearly will go bad with nested collections which will need to be instantiated completely to convert it (Discarded)

Store some reference to the collection as the associated value and then and have some static function that returns the collection (Not need to conform to hashable, but worse developer experience).

I'm open to discuss different solutions

We can override the hash function on AnyRealmValue and return a constant value for the Map and List cases.

RealmSwift/Tests/ObjectCreationTests.swift

Realm/RLMAccessor.mm

leemaguire · 2024-04-23T14:01:30Z

Realm/RLMAccessor.mm

@@ -890,15 +914,17 @@ void RLMSetSwiftPropertyAny(__unsafe_unretained RLMObjectBase *const obj, uint16
 }

 id RLMAccessorContext::box(realm::Mixed v) {
-    return RLMMixedToObjc(v, _realm, &_info);
+    auto property = (currentProperty) ? currentProperty : _info.propertyForTableColumn(_colKey);


why would currentProperty be nil here?

For any other mixed wrapped types besides list and dictionary the accessor is constructed without the current property because it is not needed to box the native value

Realm/RLMCollection.mm

Realm/RLMValue.h

Realm/Tests/KVOTests.mm

Realm/Tests/NotificationTests.m

Realm/Tests/ObjectTests.m

RealmSwift/AnyRealmValue.swift

dianaafanador3 · 2024-04-24T14:58:44Z

@tgoyne and @leemaguire solved pr comments and there are some comments as well

elle-j

Nice to see the Swift implementation of this as well! 🙂

CHANGELOG.md

Package.swift

Realm/RLMUtil.mm

Realm/RLMValue.h

tgoyne · 2024-04-29T19:27:46Z

There are a bunch of new warnings here that need to be fixed.

Realm/ObjectServerTests/RealmServer.swift

Realm/RLMAccessor.mm

tgoyne · 2024-04-29T19:39:32Z

Realm/RLMManagedArray.mm

+        realm::Mixed value = _backingList.get_any(index);
+        RLMAccessorContext context(*_ownerInfo, *_objectInfo, _property);
+        if (value.is_type(realm::type_Dictionary)) {
+            return context.box(_backingList.get_dictionary(index));
+        }
+        else if (value.is_type(realm::type_List)) {
+            return context.box(_backingList.get_list(index));
+        }
+        else {
+            return context.box(value);
+        }


This is all done by _backingList.get().

I don't think get is getting nested levels, this are done by using get_dictionary and get_list, which is going an extra level within the collection https://github.com/realm/realm-core/blob/970e558fbe1b3439a5c6d8fbdecfee35815d04ba/src/realm/list.cpp#L585

I don't know if I'm missing something

List::get() calls get_dictionary() and get_list() as needed. As I have said a few times, all of the tests pass with this being just _backingList.get(). If you believe that there is some scenario where this doesn't work, you need to write a test for that scenario.

Realm/RLMQueryUtil.mm

tgoyne · 2024-04-29T19:58:42Z

Realm/RLMQueryUtil.mm

+
+// For nested subscripts in NSPredicate like `anyCol[0]['key'] we need to iterate through the nested NSExpression to get the paths of the query.
+void QueryBuilder::get_path_elements(std::vector<PathElement> &paths, NSExpression *expression) {
+    for (int i = 0; i < expression.arguments.count; i++) {


This function is still very aggressively confusing and adding a short description of what it's doing does not help. It's looping over the arguments, but then always recurring on the first argument regardless of where it finds a function expression? There's apparently non-constant expressions in the array, but they're just ignored?

Realm/RLMQueryUtil.mm

tgoyne · 2024-04-29T20:00:12Z

Realm/RLMQueryUtil.mm

+                    }
+                } else if ([value isKindOfClass:[NSString class]]) {
+                    NSString *key = (NSString *)value;
+                    if ([key isEqual:@"all"]) {


???

If you have a field named "all" it just completely ignores the field name and does a wildcard search instead?

Why is this using "all" as the wildcard search key?

RealmSwift/Object.swift

tgoyne · 2024-05-08T18:51:03Z

RealmSwift/AnyRealmValue.swift

+
+    // MARK: - Hashable
+
+    public func hash(into hasher: inout Hasher) {


This needs to actually hash all of the types.

tgoyne · 2024-05-08T18:55:04Z

Realm/RLMManagedArray.mm

+        realm::Mixed value = _backingList.get_any(index);
+        RLMAccessorContext context(*_ownerInfo, *_objectInfo, _property);
+        if (value.is_type(realm::type_Dictionary)) {
+            return context.box(_backingList.get_dictionary(index));
+        }
+        else if (value.is_type(realm::type_List)) {
+            return context.box(_backingList.get_list(index));
+        }
+        else {
+            return context.box(value);
+        }


List::get() calls get_dictionary() and get_list() as needed. As I have said a few times, all of the tests pass with this being just _backingList.get(). If you believe that there is some scenario where this doesn't work, you need to write a test for that scenario.

tgoyne · 2024-05-08T19:02:26Z

Realm/RLMCollection.mm

+            RLMAccessorContext ctx = RLMAccessorContext(*_info);
+            for (NSUInteger index = state->state; index < count && batchCount < len; ++index) {
+                _strongBuffer[batchCount] = _results->get(ctx, index);
+                batchCount++;
+            }
+        } else {
+            // This is used by Dicitonary and List for nested collections.
+            RLMAccessorContext ctx = RLMAccessorContext(*_parentInfo, *_info, _property);
+            for (NSUInteger index = state->state; index < count && batchCount < len; ++index) {
+                _strongBuffer[batchCount] = _results->get(ctx, index);
+                batchCount++;
+            }


Suggested change

RLMAccessorContext ctx = RLMAccessorContext(*_info);

for (NSUInteger index = state->state; index < count && batchCount < len; ++index) {

_strongBuffer[batchCount] = _results->get(ctx, index);

batchCount++;

}

} else {

// This is used by Dicitonary and List for nested collections.

RLMAccessorContext ctx = RLMAccessorContext(*_parentInfo, *_info, _property);

for (NSUInteger index = state->state; index < count && batchCount < len; ++index) {

_strongBuffer[batchCount] = _results->get(ctx, index);

batchCount++;

}

auto ctx = _parentInfo ? RLMAccessorContext(*_parentInfo, *_info, _property) :

RLMAccessorContext(*_info);

for (NSUInteger index = state->state; index < count && batchCount < len; ++index) {

_strongBuffer[batchCount] = _results->get(ctx, index);

batchCount++;

}

tgoyne · 2024-05-08T19:06:10Z

Realm/RLMManagedDictionary.mm

@@ -454,9 +483,9 @@ - (BOOL)isFrozen {
 - (instancetype)resolveInRealm:(RLMRealm *)realm {
    auto& parentInfo = _ownerInfo->resolve(realm);
    return translateErrors([&] {
-        return [[self.class alloc] initWithBackingCollection:_backingCollection.freeze(realm->_realm)
+        return [[RLMManagedDictionary alloc] initWithBackingCollection:_backingCollection.freeze(realm->_realm)


Things which produce a copy of the current object use self.class rather than explicitly spelling out the class name so that they work properly when called on subclasses. That's not directly relevant here, but it is the more generally correct pattern.

tgoyne · 2024-05-08T19:10:10Z

Realm/RLMQueryUtil.mm

+    // We consider only `NSKeyPathExpressionType` values to build the keypath.
+    for (unsigned long i = 0; i < pathElements.size(); i++) {
+        if (keyPathExpression.arguments[0].expressionType == NSKeyPathExpressionType) {
+            keyPath = [NSString stringWithFormat:@"%@", keyPathExpression.arguments[0]];


This appears to be a very roundabout way of writing keyPathExpression.arguments[0].description. I think the actually correct thing here is keyPathExpression.arguments[0].keyPath

tgoyne · 2024-05-08T19:16:27Z

RealmSwift/Object.swift

+        default:
+            fatalError()


These are not needed and will only lead to future bugs.

tgoyne · 2024-05-08T19:18:39Z

RealmSwift/ObjectiveCSupport+AnyRealmValue.swift

+/// :nodoc:
+extension RLMDictionary: RLMValue {
+    /// :nodoc:
+    public var rlm_anyValueType: RLMAnyValueType { .dictionary }


This is part of the obj-c API and it doesn't make any sense to be redefining it here.

tgoyne · 2024-05-08T19:21:14Z

Realm/RLMValue.mm

+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+- (RLMPropertyType)rlm_valueType {
+    REALM_UNREACHABLE();


This is not unreachable. rlm_valueType is part of the public API.

tgoyne · 2024-05-08T19:23:31Z

RealmSwift/Impl/ObjcBridgeable.swift

@@ -163,6 +163,27 @@ extension AnyRealmValue: BuiltInObjcBridgeable {
    }
 }

+
+extension RLMManagedDictionary: BuiltInObjcBridgeable {


These are not valid implementations of BuiltInObjcBridgeable, and declaring these types as BuiltInObjcBridgeable doesn't really make sense at all.

tgoyne · 2024-05-08T19:26:03Z

RealmSwift/Query.swift

+    }
+    /// :nodoc:
+    public subscript(key: String) -> Query<AnyRealmValue> {
+        .init(appendKeyPath("['\(key)']", options: [.isPath]))


This needs to handle key containing quotes.

cla-bot bot added the cla: yes label Apr 12, 2024

github-actions bot assigned dianaafanador3 Apr 12, 2024

dianaafanador3 closed this Apr 12, 2024

dianaafanador3 reopened this Apr 12, 2024

dianaafanador3 marked this pull request as draft April 12, 2024 16:16

dianaafanador3 force-pushed the dp/collections_in_mixed branch 8 times, most recently from b60f46f to 3386d7a Compare April 22, 2024 07:36

dianaafanador3 marked this pull request as ready for review April 22, 2024 11:15

dianaafanador3 requested review from tgoyne, nirinchev and leemaguire April 22, 2024 11:15

dianaafanador3 changed the title ~~Collections in Mixed~~ RCOCOA-2271: Collections in Mixed Apr 22, 2024

dianaafanador3 force-pushed the dp/collections_in_mixed branch 9 times, most recently from ed91d6b to 45a17bd Compare April 23, 2024 14:44

tgoyne reviewed Apr 23, 2024

View reviewed changes

leemaguire reviewed Apr 23, 2024

View reviewed changes

dianaafanador3 force-pushed the dp/collections_in_mixed branch from b0489ae to eec9244 Compare April 24, 2024 14:48

dianaafanador3 requested review from tgoyne and leemaguire April 24, 2024 14:58

elle-j reviewed Apr 25, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Package.swift Outdated Show resolved Hide resolved

Realm/RLMUtil.mm Show resolved Hide resolved

tgoyne reviewed Apr 25, 2024

View reviewed changes

Realm/RLMValue.h Outdated Show resolved Hide resolved

dianaafanador3 requested review from elle-j and tgoyne April 26, 2024 12:26

tgoyne reviewed Apr 29, 2024

View reviewed changes

dianaafanador3 added 5 commits May 7, 2024 00:29

Collections in mixed

bdc715f

Solved PR comments

4d639dd

Solve PR comments

5017352

Solve PR comments

237464e

Solve PR comments and use core version

36c05c8

dianaafanador3 force-pushed the dp/collections_in_mixed branch 3 times, most recently from 2e8b630 to cede8e2 Compare May 7, 2024 12:22

Fix CI issues and warnings

26d3366

dianaafanador3 force-pushed the dp/collections_in_mixed branch from cede8e2 to 26d3366 Compare May 7, 2024 15:05

tgoyne reviewed May 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RCOCOA-2271: Collections in Mixed #8546

RCOCOA-2271: Collections in Mixed #8546

dianaafanador3 commented Apr 12, 2024 •

edited

dianaafanador3 commented Apr 22, 2024

tgoyne left a comment

tgoyne Apr 23, 2024

dianaafanador3 Apr 24, 2024

tgoyne Apr 23, 2024

dianaafanador3 Apr 24, 2024

tgoyne Apr 25, 2024

dianaafanador3 Apr 26, 2024

tgoyne Apr 26, 2024

leemaguire Apr 23, 2024

dianaafanador3 Apr 24, 2024

dianaafanador3 commented Apr 24, 2024

elle-j left a comment

tgoyne commented Apr 29, 2024

tgoyne Apr 29, 2024

dianaafanador3 Apr 30, 2024

dianaafanador3 Apr 30, 2024

tgoyne May 8, 2024

tgoyne Apr 29, 2024

tgoyne Apr 29, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024

tgoyne May 8, 2024


		// MARK: - Hashable

		public func hash(into hasher: inout Hasher) {

RCOCOA-2271: Collections in Mixed #8546

Are you sure you want to change the base?

RCOCOA-2271: Collections in Mixed #8546

Conversation

dianaafanador3 commented Apr 12, 2024 • edited

dianaafanador3 commented Apr 22, 2024

tgoyne left a 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

dianaafanador3 commented Apr 24, 2024

elle-j left a comment

Choose a reason for hiding this comment

tgoyne commented Apr 29, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dianaafanador3 commented Apr 12, 2024 •

edited