Make breaking changes needed to support arrays of primitives #5149

tgoyne · 2017-07-20T21:20:30Z

Everything but the last commit is just pulling in latest master and #5148 on top of master-3.0.

This makes the two API changes needed for arrays of primitives: loosening the generic constraint to allow non-RLMObjects in RLMArray/RLMResults (but not yet actually supporting anything else), and changing RLMProperty/RLMPropertyType so that arrays of things other than objects can be expressed.

bdash · 2017-07-21T17:13:32Z

Shouldn't we pull in the changes from master (and #5148, once it's merged to master) via a merge commit, rather than rebasing as you appear to have done here?

tgoyne · 2017-07-21T17:18:12Z

Yes, but that merge should be from master so it needs #5148 to be merged first.

jpsim · 2017-07-31T21:38:46Z

Realm/RLMArray.mm

-    if (![array->_objectClassName isEqualToString:object->_objectSchema.className]) {
-        @throw RLMException(@"Object type '%@' does not match RLMArray type '%@'.",
-                            object->_objectSchema.className, array->_objectClassName);
+- (RLMResults *)sortedResultsUsingProperty:(NSString *)property ascending:(BOOL)ascending {


I think this can be completely removed for 3.0.

jpsim · 2017-07-31T21:45:26Z

Realm/RLMArray.mm

@@ -387,7 +435,7 @@ - (id)maxOfProperty:(NSString *)property {

 - (id)sumOfProperty:(NSString *)property {
    [self validateAggregateProperty:property method:_cmd allowDate:false];
-    return [_backingArray valueForKeyPath:[@"@sum." stringByAppendingString:property]];
+    return [_backingArray valueForKeyPath:[@"@sum." stringByAppendingString:property]] ?: @0;


If Foundation's @sum returns nil for an empty array, shouldn't we match that?

We decided not to match Foundation's behavior because Foundation is wrong; the sum of an empty set isn't undefined.

I vaguely remember this discussion, but not the outcome. thanks for clarifying.

jpsim · 2017-07-31T21:56:27Z

Realm/RLMProperty.mm

        // get object class from type string - @"RLMArray<objectClassName>"
        _objectClassName = [[NSString alloc] initWithBytes:code + arrayPrefixLen
                                                    length:strlen(code + arrayPrefixLen) - 2 // drop trailing >"
                                                  encoding:NSUTF8StringEncoding];

        if ([RLMSchema classForString:_objectClassName]) {
            _optional = false;
-            _type = RLMPropertyTypeArray;
+            _type = RLMPropertyTypeObject;
            return YES;
        }
        @throw RLMException(@"Property '%@' is of type 'RLMArray<%@>' which is not a supported RLMArray object type. "


This exception message is incorrect now "RLMArrays can only contain instances of RLMObject subclasses."

jpsim · 2017-07-31T21:59:14Z

Realm/RLMQueryUtil.mm


-        if (property.type == RLMPropertyTypeArray || property.type == RLMPropertyTypeLinkingObjects)
+        if (property.array || property.type == RLMPropertyTypeLinkingObjects)


RLMPropertyTypeLinkingObjects properties have array set to true, so this check can just be if (property.array)

jpsim · 2017-07-31T22:03:04Z

Realm/RLMUtil.mm

        return YES;
    }
+    if (array) {
+        if (RLMArray *array = RLMDynamicCast<RLMArray>(value)) {
+            return [array.objectClassName isEqualToString:objectClassName];


Doesn't this need to consider primitive types?

No, this PR does not add support for arrays of primitives.

jpsim · 2017-07-31T22:03:17Z

Realm/RLMUtil.mm

+            return [array.objectClassName isEqualToString:objectClassName];
+        }
+        if (RLMListBase *list = RLMDynamicCast<RLMListBase>(value)) {
+            return [list._rlmArray.objectClassName isEqualToString:objectClassName];


Doesn't this need to consider primitive types?

bdash · 2017-08-22T02:11:36Z

RealmSwift/List.swift

-    public override func value(forKey key: String) -> Any? {
-        return value(forKeyPath: key)
+     */
+    @nonobjc public func value(forKey key: String) -> [AnyObject] {


Does @nonobjc mean that this cannot be invoked from Obj-C? I'd think that would give unexpected behavior from calls to -valueForKeyPath: with a key path that traverses into a List but where the List isn't the root object.

Invoking it from obj-c calls the one defined on the parent class which does the exact same thing. This function just refines the return type when calling it from from Swift to make the code using it a little less awkward.

bdash · 2017-08-22T02:15:20Z

RealmSwift/RealmCollection.swift

@@ -123,6 +122,11 @@ private func forceCast<A, U>(_ from: A, to type: U.Type) -> U {
    return from as! U
 }

+/// A type which can be stored in a Realm List or Results
+public protocol RealmCollectionValue {
+    static func className() -> String


What will this return for primitive types?

This function has actually gone away on the primitive arrays branch because different functions were needed.

It looks like CI is failing due to this function being undocumented:

Undocumented Realm swift declarations: { "warnings": [ { "file": "/Users/realm/workspace/objc_pr/configuration/Release/target/docs/xcode_version/8.3.3/RealmSwift/RealmCollection.swift", "line": 127, "symbol": "RealmCollectionValue.className()", "symbol_kind": "source.lang.swift.decl.function.method.static", "warning": "undocumented" } ],

bdash · 2017-08-22T02:21:44Z

RealmSwift/Tests/ListTests.swift

-        assertThrows(array.swap(index1: 0, -1))
-        assertThrows(array.swap(index1: 1000, 0))
-        assertThrows(array.swap(index1: 0, 1000))
+        assertThrows(array.swap(-1, 0))


In Swift 4, the equivalent method on Array appears to be called swapAt.

bdash · 2017-08-22T02:22:38Z

RealmSwift/Tests/ObjectSchemaTests.swift

        // swiftlint:disable:next line_length
-        XCTAssertEqual(objectSchema.description, "SwiftObject {\n\tboolCol {\n\t\ttype = bool;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tintCol {\n\t\ttype = int;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tfloatCol {\n\t\ttype = float;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tdoubleCol {\n\t\ttype = double;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tstringCol {\n\t\ttype = string;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tbinaryCol {\n\t\ttype = data;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tdateCol {\n\t\ttype = date;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tobjectCol {\n\t\ttype = object;\n\t\tobjectClassName = SwiftBoolObject;\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = YES;\n\t}\n\tarrayCol {\n\t\ttype = array;\n\t\tobjectClassName = SwiftBoolObject;\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n}")
+//        XCTAssertEqual(objectSchema.description, "SwiftObject {\n\tboolCol {\n\t\ttype = bool;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tintCol {\n\t\ttype = int;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tfloatCol {\n\t\ttype = float;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tdoubleCol {\n\t\ttype = double;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tstringCol {\n\t\ttype = string;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tbinaryCol {\n\t\ttype = data;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tdateCol {\n\t\ttype = date;\n\t\tobjectClassName = (null);\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n\tobjectCol {\n\t\ttype = object;\n\t\tobjectClassName = SwiftBoolObject;\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = YES;\n\t}\n\tarrayCol {\n\t\ttype = array;\n\t\tobjectClassName = SwiftBoolObject;\n\t\tlinkOriginPropertyName = (null);\n\t\tindexed = NO;\n\t\tisPrimary = NO;\n\t\toptional = NO;\n\t}\n}")


I assume this needs to be updated?

austinzheng

It looks good to me. I have no feedback of any real significance.

austinzheng · 2017-08-22T19:30:56Z

CHANGELOG.md

@@ -99,6 +99,13 @@ x.x.x Release notes (yyyy-MM-dd)
  correctly nil out nullable properties when updating an existing
  object when the `value` argument specifies nil or `NSNull` for
  the property value.
+* Rename `List.remove(objectAtIndex:)` to `List.remove(at:)` to match the named


austinzheng · 2017-08-22T19:39:48Z

Realm/RLMObjectBase.mm

@@ -78,34 +78,28 @@ - (void)dealloc {
    _observationInfo = nullptr;
 }

+static id cooerceToObjectType(id obj, Class cls, RLMSchema *schema) {


austinzheng · 2017-08-22T19:54:00Z

RealmSwift/List.swift

@@ -314,14 +314,16 @@ public final class List<T: Object>: ListBase {

     - parameter index: The index at which to remove the object.
     */
-    public func remove(objectAtIndex index: Int) {
+    public func remove(at index: Int) {


The Swift standard library version of this method returns the removed element. I don't know if this is something we should replicate.

It's sort of gratuitous overhead for something that's at best vaguely convenient, but matching the standard library is probably the way to go.

It looks like Array returns the value that was there, but Collection/RangeReplaceableCollection don't and there's alternative functions on Collection for remove and return old (popLast() etc.), so I'm inclined to leave it as a void return.

bdash · 2017-08-24T01:11:09Z

The changelog entries will need to be moved into the section for the upcoming section now that v3.0.0-beta.3 has been released.

tgoyne · 2017-08-24T02:48:54Z

Failures are both CI things that passed before updating the changelog.

tgoyne added the S:In Progress label Jul 20, 2017

tgoyne self-assigned this Jul 20, 2017

bdash mentioned this pull request Jul 21, 2017

Make non-breaking changes to support arrays of primitives #5148

Merged

tgoyne force-pushed the tg/relax-collection-constraints branch from 2a254d7 to 59a7340 Compare July 28, 2017 18:56

jpsim reviewed Jul 31, 2017

View reviewed changes

tgoyne force-pushed the tg/relax-collection-constraints branch 2 times, most recently from f4664c2 to db1e356 Compare August 2, 2017 18:26

tgoyne force-pushed the tg/relax-collection-constraints branch from db1e356 to f80f7fb Compare August 10, 2017 23:23

andrewjl mentioned this pull request Aug 16, 2017

Support storing arrays of primitives (string, int, etc.) as RLMObject properties #1120

Closed

4 tasks

bmunkholm removed the S:In Progress label Aug 21, 2017

tgoyne force-pushed the tg/relax-collection-constraints branch from f80f7fb to 4943c8f Compare August 21, 2017 22:12

bdash reviewed Aug 22, 2017

View reviewed changes

austinzheng reviewed Aug 22, 2017

View reviewed changes

tgoyne force-pushed the tg/relax-collection-constraints branch 3 times, most recently from ba37482 to 215909c Compare August 22, 2017 22:08

bdash approved these changes Aug 24, 2017

View reviewed changes

austinzheng approved these changes Aug 24, 2017

View reviewed changes

tgoyne added 7 commits August 23, 2017 18:31

Make breaking changes needed to support arrays of primitives

407f048

Add array to RLMProperty's description

5492319

Fix typos

eee0a74

Rename swap() to swapAt() to match the name used by Array

a25b469

Fix Xcode 9 compilation

d2c6f80

Mark RealmCollectionValue.className as nodoc

bbb365b

Move changelog entries to the correct section

719ded8

tgoyne force-pushed the tg/relax-collection-constraints branch from 2caba93 to 719ded8 Compare August 24, 2017 01:32

tgoyne merged commit 7d9f20e into master-3.0 Aug 24, 2017

bmunkholm deleted the tg/relax-collection-constraints branch March 12, 2019 13:05

github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make breaking changes needed to support arrays of primitives #5149

Make breaking changes needed to support arrays of primitives #5149

tgoyne commented Jul 20, 2017

bdash commented Jul 21, 2017

tgoyne commented Jul 21, 2017

jpsim Jul 31, 2017

jpsim Jul 31, 2017

tgoyne Jul 31, 2017

jpsim Jul 31, 2017

jpsim Jul 31, 2017

jpsim Jul 31, 2017

jpsim Jul 31, 2017

tgoyne Jul 31, 2017

jpsim Jul 31, 2017

bdash Aug 22, 2017

tgoyne Aug 22, 2017

bdash Aug 22, 2017

tgoyne Aug 22, 2017

bdash Aug 24, 2017

bdash Aug 22, 2017

bdash Aug 22, 2017

austinzheng left a comment

austinzheng Aug 22, 2017

austinzheng Aug 22, 2017

austinzheng Aug 22, 2017

tgoyne Aug 22, 2017

tgoyne Aug 22, 2017

bdash commented Aug 24, 2017

tgoyne commented Aug 24, 2017


		if (property.type == RLMPropertyTypeArray \|\| property.type == RLMPropertyTypeLinkingObjects)
		if (property.array \|\| property.type == RLMPropertyTypeLinkingObjects)

Make breaking changes needed to support arrays of primitives #5149

Make breaking changes needed to support arrays of primitives #5149

Conversation

tgoyne commented Jul 20, 2017

bdash commented Jul 21, 2017

tgoyne commented Jul 21, 2017

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

austinzheng 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

bdash commented Aug 24, 2017

tgoyne commented Aug 24, 2017