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
base: master
Are you sure you want to change the base?
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/List.swift
Outdated
@@ -361,6 +361,14 @@ extension List: MutableCollection { | |||
} | |||
} | |||
|
|||
// MARK: - Hashable | |||
|
|||
extension List: 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.
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.
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.
Same as Map
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
b0489ae
to
eec9244
Compare
@tgoyne and @leemaguire solved pr comments and there are some comments as well |
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.
Nice to see the Swift implementation of this as well! 🙂
There are a bunch of new warnings here that need to be fixed. |
Realm/RLMManagedArray.mm
Outdated
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); | ||
} |
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.
This is all done by _backingList.get()
.
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 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
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 if I'm missing something
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.
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
Outdated
|
||
// 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++) { |
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.
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?
} | ||
} else if ([value isKindOfClass:[NSString class]]) { | ||
NSString *key = (NSString *)value; | ||
if ([key isEqual:@"all"]) { |
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 have a field named "all" it just completely ignores the field name and does a wildcard search instead?
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 is this using "all" as the wildcard search key?
2e8b630
to
cede8e2
Compare
cede8e2
to
26d3366
Compare
|
||
// MARK: - Hashable | ||
|
||
public func hash(into hasher: inout Hasher) { |
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.
This needs to actually hash all of the types.
Realm/RLMManagedArray.mm
Outdated
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); | ||
} |
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.
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.
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++; | ||
} |
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.
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++; | |
} |
@@ -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) |
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.
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.
// 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]]; |
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.
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
default: | ||
fatalError() |
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 are not needed and will only lead to future bugs.
/// :nodoc: | ||
extension RLMDictionary: RLMValue { | ||
/// :nodoc: | ||
public var rlm_anyValueType: RLMAnyValueType { .dictionary } |
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.
This is part of the obj-c API and it doesn't make any sense to be redefining it here.
#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.
This is not unreachable. rlm_valueType
is part of the public API.
@@ -163,6 +163,27 @@ extension AnyRealmValue: BuiltInObjcBridgeable { | |||
} | |||
} | |||
|
|||
|
|||
extension RLMManagedDictionary: BuiltInObjcBridgeable { |
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 are not valid implementations of BuiltInObjcBridgeable, and declaring these types as BuiltInObjcBridgeable doesn't really make sense at all.
} | ||
/// :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.
This needs to handle key
containing quotes.
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.