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/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.
Nice to see the Swift implementation of this as well! 🙂
There are a bunch of new warnings here that need to be fixed. |
cede8e2
to
26d3366
Compare
#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.
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 This is a very particular case because we are deprecating the declaration but also changing the type to be used. You are right because this is part of the public, the user can ask for rlm_valueType
, but because we don't want to add any new types to property type we are in some sort of limbo here.
Solutions
We can return RLMPropertyTypeAny like we do for Null, and this is actually true because this indeed a colection of mixed?
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.
} | ||
/// :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.
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
Do you mean because of this ?
Note
The expressions you write inside parentheses within an interpolated string can’t contain an unescaped backslash (\), a carriage return, or a line feed. However, they can contain other string literals.
https://docs.swift.org/swift-book/documentation/the-swift-programming-language/stringsandcharacters/
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.
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.
$0.anyCol["'"]
is not allowed for AnyRealmValue subscripts or 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.
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.
f92fe16
to
760f3e5
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.
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.
} | ||
/// :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 |
#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.
} | ||
/// :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.
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.