-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
RCOCOA-2271: Collections in Mixed #8546
Conversation
b60f46f
to
3386d7a
Compare
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. |
ed91d6b
to
45a17bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A first pass over things.
RealmSwift/Map.swift
Outdated
@@ -779,6 +779,14 @@ public final class Map<Key: _MapKey, Value: RealmCollectionValue>: RLMSwiftColle | |||
} | |||
} | |||
|
|||
// MARK: - Hashable | |||
|
|||
extension Map: Hashable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, this is not at all a valid Hashable implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ObjectIdentifier given that we already conform to Equatable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can override the hash function on AnyRealmValue and return a constant value for the Map and List cases.
Realm/RLMAccessor.mm
Outdated
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would currentProperty
be nil
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still several new warnings. I'm not sure why the implicit casts from id to RLMArray are producing warnings here but not on master, but the other warnings seem straightforward.
RealmSwift/Query.swift
Outdated
} | ||
/// :nodoc: | ||
public subscript(key: String) -> Query<AnyRealmValue> { | ||
.init(appendKeyPath("['\(key)']", options: [.isPath])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do $0.anyCol["'"] == 0
to query on the dictionary key "'" this will produce invalid syntax.
3683365
to
98a39c3
Compare
98a39c3
to
2a3861f
Compare
@tgoyne solved PR comments and removed the warnings |
Realm/RLMValue.mm
Outdated
#pragma clang diagnostic push | ||
#pragma clang diagnostic ignored "-Wdeprecated-declarations" | ||
- (RLMPropertyType)rlm_valueType { | ||
REALM_UNREACHABLE(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning RLMPropertyTypeAny seems like the best option here.
REALM_UNREACHABLE() marks a place that should be logically impossible to hit and if we get there it always indicates a bug in our code.
RealmSwift/Query.swift
Outdated
} | ||
/// :nodoc: | ||
public subscript(key: String) -> Query<AnyRealmValue> { | ||
.init(appendKeyPath("['\(key)']", options: [.isPath])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what you mean. any["'"] == %@
(or any['\''] == %@
) is a perfectly valid query, which this currently fails to generate a valid string for.
RealmSwift/AnyRealmValue.swift
Outdated
public func hash(into hasher: inout Hasher) { | ||
switch self { | ||
case let .int(i): | ||
hasher.combine(i.hashValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should all be hasher.combine(i)
without the .hashValue
. This currently is hashing the values and then rehashing the hash.
Added support for storing nested collections (List and Map not ManagedSet) in a
AnyRealmValue
.Added new operators to Swift's Query API for supporting querying nested collections.
The
.all
operator allows looking up in all keys or indexes.