Add support for arrays of more types #5274

tgoyne · 2017-08-29T21:03:10Z

~~This is currently targeting a branch with #5267 and latest master merged into master-3.0 for the sake of showing something sensible for review.~~

All functionality of RLMArray and List should now be supported for all of the Realm-supported data types, except for queries. Currently trying to query primitive lists just silently ignores the predicate passed in. See RLMTestObjects.h for examples of how the models are declared in obj-c (as with RLMObject arrays, generic type parameters can be used but are insufficient). In Swift the expected normal things work (e.g. List<Int?> etc.).

The bulk of the obj-c changes are adjusting how validation is done. Notably unbox() no longer validates its input and instead we validate all values before passing them to the object store. This is needed both for providing sensible error messages without threading even more awful state tracking into RLMAccessorContext, and to avoid duplicating all of the logic for unmanaged objects. The other obj-c changes are just adding support in the KVC and aggregate functions.

Swift changes were simpler; all that was left to do was make more types conform to RealmCollectionValue and make realm collections conditionally define aggregate functions for Lists and Results of aggregatable types.

The tests are unfortunately something of a mess. For obj-c I had the idea of generating tests on each array type from a template using a simple python script. This did not turn out as well as I hoped, and unsurprisingly the python script turned into a terrible pile of hacks and the template file is awful. For Swift I attempted to take the same approach as the C++ objectstore tests, but ran into a lot of issues with the current limitations of Swift generics (most notably the lack of conditional conformances). Both of these probably need to be rethought, but I think that can wait; the tests do at least serve to test the code for now.

…lear the RLMArray

* tg/list-self-assignment: Fix more cases where assigning an RLMArray property to itself would clear the RLMArray Fix an issue causing permissions tests to fail Update objectstore Fix compilation error in Swift 4

These were skipped because prior to fine-grained notifications it was error-prone and hard to imagine a scenario where it'd be useful, but with fine-grained notifications it's potentially valuable.

austinzheng · 2017-08-29T22:26:25Z

Nothing stands out to me as being problematic, it looks good. I'll review it again tomorrow more carefully.

bdash · 2017-08-30T21:56:18Z

There's a linker error which suggests we'll need a core release including the fix from realm/realm-core#2823:

Undefined symbols for architecture i386:
  "unsigned long realm::Table::find_first<realm::BinaryData>(unsigned long, realm::BinaryData) const", referenced from:
      unsigned long realm::List::find<realm::BinaryData>(realm::BinaryData const&) const in libRealm.a(list.o)
      unsigned long realm::Results::index_of<realm::BinaryData>(realm::BinaryData const&) in libRealm.a(results.o)
ld: symbol(s) not found for architecture i386

bdash · 2017-08-30T22:08:50Z

Realm/RLMArray.h

+/**
+ Indicates whether the objects in the collection can be `nil`.
+ */
+@property (nonatomic, readonly) bool optional;


This should be BOOL, and should likely also have getter=isOptional to follow Cocoa's naming conventions.

bdash · 2017-08-30T22:11:57Z

Realm/RLMCollection.h

+/**
+ Indicates whether the objects in the collection can be `nil`.
+ */
+@property (nonatomic, readonly) bool optional;


Ditto for BOOL / getter=isOptional.

bdash · 2017-08-30T22:15:13Z

Realm/RLMManagedArray.mm

    return value ? @(*value) : nil;
 }

 - (void)deleteObjectsFromRealm {
+    if (_type != RLMPropertyTypeObject) {
+        @throw RLMException(@"Cannot delete objects from RLMArray<%@>: only RLMObjects can be deleted.", RLMTypeToString(_type));


Clearing the array seems close enough in terms of semantics to what happens with arrays containing objects that it may be simpler to do that rather than throwing here?

The public API that this is called by (-[RLMRealm deleteObjects:]) can't support deleting collections of non-objects (what would [realm deleteObjects:@[@0]] do?).

bdash · 2017-08-30T22:15:47Z

Realm/RLMManagedArray.mm

+                                    _realm.schema, _realm.group);
+    }
+    else {
+        // FIXME


Should we just thrown an exception for now?

bdash · 2017-08-30T22:17:28Z

Realm/RLMResults.h

@@ -64,6 +62,11 @@ typedef NS_ENUM(int32_t, RLMPropertyType);
 @property (nonatomic, readonly, assign) RLMPropertyType type;

 /**
+ Indicates whether the objects in the collection can be `nil`.
+ */
+@property (nonatomic, readonly) bool optional;


Ditto about BOOL / getter=isOptional.

bdash · 2017-08-30T22:18:45Z

Realm/RLMResults.mm

@@ -328,6 +336,7 @@ - (RLMResults *)objectsWithPredicate:(NSPredicate *)predicate {
        if (_results.get_mode() == Results::Mode::Empty) {
            return self;
        }
+        // FIXME: primitive array queries


Should we throw for now rather than silently giving incorrect results?

… of primitives There's a disagreement between Swift 3.1 and 3.2 as to whether `Element` or `ElementType` should be used in the constraints, and I've been unable to find one approach that works for both.

`-[NSData compare:]` doesn't exist, so resort to comparing `NSData` instances using `memcmp`. Using `-compare:` happened to work on macOS because a system framework provided an implementation in a category. No such luck on iOS, tvOS and watchOS though.

bdash · 2017-09-19T02:54:20Z

One problem the tests reveal with Xcode 9 / Swift 4 is:

Simultaneous accesses to 0x6040000de518, but modification requires exclusive access.
Previous access (a modification) started at RealmSwift Tests`implicit closure #1 in PrimitiveListTests.testReplaceRange() + 209 (0x120c8fa21).
Current access (a read) started at:
0    libswiftCore.dylib                 0x00000001219418e0 swift_beginAccess + 605
1    RealmSwift Tests                   0x0000000120c77a40 PrimitiveListTestsBase.array.getter + 68
2    RealmSwift Tests                   0x0000000120c8e0c0 PrimitiveListTests.testReplaceRange() + 1114
3    RealmSwift Tests                   0x0000000120c8ff20 @objc PrimitiveListTests.testReplaceRange() + 36

This error occurs during the evaluation of:

array.replaceSubrange(0..<0, with: [values[0]])

I think Swift's new exclusive access checking isn't safe against Objective-C / C++ exceptions being thrown, so the earlier:

assertThrows(array.replaceSubrange(0..<1, with: []),
             reason: "Index 0 is out of bounds")

leaves the access checking state in an inconsistent state.

I've disabled the problematic assertThrows in df38d3a for now.

`XCTestSuite.defaultTestSuite` changed from a function to a property in Swift 4.

Tagged pointers aren't enabled on all systems, and there's no guarantee when they'll be used either.

exclusive access checking.

Different versions of Swift have differing opinions as to whether these methods override methods with the same name from `TestCase`. Work around this by renaming the methods.

Swift 3.0 didn't allow bridging between `NSNumber` and sized integer types such as `Int8`. We need to use `dynamicBridgeCast` to get the correct bridging for those types.

`@testable` doesn't work with our Release configuration.

explicitly suppress documentation on a few internal methods.

bdash · 2017-09-20T04:48:05Z

All of the remaining CI failures are due to flakiness.

bdash · 2017-09-21T19:26:22Z

The work remaining here appears to be:

Change log entry
Another review pass from all involved to determine if there's any other remaining work

…ently failing

* origin/master-3.0: Remove `List` conformance to `RangeReplaceableCollection` Add compatibility for ROS 2 user info API

tgoyne · 2017-09-22T18:38:22Z

Configuration/RealmSwift/RealmSwift.xcconfig

@@ -10,3 +10,5 @@ DEFINES_MODULE = YES;
 INFOPLIST_FILE = RealmSwift/RealmSwift-Info.plist;
 PRODUCT_NAME = RealmSwift;
 PRODUCT_BUNDLE_IDENTIFIER = io.realm.RealmSwit;
+
+OTHER_SWIFT_FLAGS = -D BUILDING_REALM_SWIFT;


Doesn't this need to be defined in the podspec as well?

If that's difficult then inverting the condition would avoid that.

I assume it would. I'm quite surprised that the CocoaPods jobs succeeded without it!

It succeeded because the Swift compiler only emits a warning about the redundant import. I went ahead and inverted the condition in e1b6c8d anyway.

determining if it should import RealmSwift. This ensures it does the right thing when building via CocoaPods.

bdash · 2017-09-23T00:03:27Z

The failing Swift test has already been fixed on master-3.0.

* origin/master-3.0: Update ROS testing dependency, reenable tests (#5332) Update a test case now that a more precise error is being raised. Fix changelog Have +schemaVersionAtURL: do a better job of translating any exceptions that are thrown. Add a change log entry. Satisfy SwiftLint. Add Swift support. Add a test of offline client reset. Handle incompatible synced Realm exceptions.

tgoyne · 2017-09-23T00:31:14Z

Obj-C and Swift tests now pass for me locally on both macOS and on my phone, so I think any remaining failures are just CI flakiness.

tgoyne added 6 commits August 25, 2017 16:26

Fix more cases where assigning an RLMArray property to itself would c…

1b239e7

…lear the RLMArray

Merge branch 'tg/list-self-assignment' into master-3.0

9b58677

* tg/list-self-assignment: Fix more cases where assigning an RLMArray property to itself would clear the RLMArray Fix an issue causing permissions tests to fail Update objectstore Fix compilation error in Swift 4

Deliver notifications for deleted RLMArrays

fa96e2a

These were skipped because prior to fine-grained notifications it was error-prone and hard to imagine a scenario where it'd be useful, but with fine-grained notifications it's potentially valuable.

Remove a use of containsString:

ed6d9ac

Add support for arrays of more types

4b4f331

Delete some unreachable code

4301d21

tgoyne added the T-Feature label Aug 29, 2017

tgoyne self-assigned this Aug 29, 2017

tgoyne requested review from bdash and austinzheng August 29, 2017 21:03

bdash reviewed Aug 30, 2017

View reviewed changes

tgoyne added 3 commits September 7, 2017 16:36

Update objectstore

df5d94b

Update to core 3.1/sync 2.0-rc18

b87840d

Change the optional property on collections to BOOL

fb1e11f

andrewjl mentioned this pull request Sep 9, 2017

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

Closed

4 tasks

bdash mentioned this pull request Sep 12, 2017

Add documentation for arrays of primitives #5303

Closed

Merge remote-tracking branch 'origin/master-3.0' into tg/primitive-array

623348f

bdash changed the base branch from tg/primitive-array-base to master-3.0 September 18, 2017 23:32

bdash added 4 commits September 18, 2017 17:04

Duplicate the protocol RealmCollection extensions related to arrays…

bfee691

… of primitives There's a disagreement between Swift 3.1 and 3.2 as to whether `Element` or `ElementType` should be used in the constraints, and I've been unable to find one approach that works for both.

Remove some duplicated file references from the Xcode project.

7260ee4

Add missing @objc annotations to SwiftPrimitiveArrayObject.

93fb306

bdash added 3 commits September 18, 2017 20:04

Fix primitive list tests with Swift 4.

781b2c3

`XCTestSuite.defaultTestSuite` changed from a function to a property in Swift 4.

Don't hard-code the type of the string in the expected error.

6cb5edf

Tagged pointers aren't enabled on all systems, and there's no guarantee when they'll be used either.

Disable a portion of testReplaceRange that hits issues with Swift 4's

df38d3a

exclusive access checking.

bdash force-pushed the tg/primitive-array branch from 7c8059e to df38d3a Compare September 19, 2017 03:06

Rename PrimitiveListTestsBase.assertEqual.

a5ca2b5

Different versions of Swift have differing opinions as to whether these methods override methods with the same name from `TestCase`. Work around this by renaming the methods.

bdash added 6 commits September 18, 2017 21:26

Tweak some tests to be compatible with Swift 3.0.

1465f3e

Fix or suppress SwiftLint violations.

9557147

Fix tests with Swift 3.0.

aa4d784

Swift 3.0 didn't allow bridging between `NSNumber` and sized integer types such as `Int8`. We need to use `dynamicBridgeCast` to get the correct bridging for those types.

Compile Util.swift into RealmSwift Tests rather than using @testable.

454110c

`@testable` doesn't work with our Release configuration.

Increase the timeout for the interprocess tests.

6531f8d

Add documentation for extension methods on RealmCollection, and

699c298

explicitly suppress documentation on a few internal methods.

bdash force-pushed the tg/primitive-array branch from 4b38ceb to 699c298 Compare September 19, 2017 22:48

bdash added 2 commits September 19, 2017 15:52

Remove an unnecessary changelog section.

fac6ff9

Swap the order of SwiftLint and documentation suppression comments.

072d391

Fix some compiler warnings in PrimitiveListTests.swift.

39d5b7a

bdash added this to the RMP 2.0 milestone Sep 22, 2017

tgoyne added 3 commits September 22, 2017 11:22

Throw an exception when querying arrays of primitives rather than sil…

aab7f5f

…ently failing

Add a changelog entry

dc976ca

Merge remote-tracking branch 'origin/master-3.0' into tg/primitive-array

307946f

* origin/master-3.0: Remove `List` conformance to `RangeReplaceableCollection` Add compatibility for ROS 2 user info API

tgoyne commented Sep 22, 2017

View reviewed changes

Invert the sense of the define that Util.swift looks for when

e1b6c8d

determining if it should import RealmSwift. This ensures it does the right thing when building via CocoaPods.

bdash approved these changes Sep 22, 2017

View reviewed changes

tgoyne force-pushed the tg/primitive-array branch from a2b9afd to 4124acd Compare September 23, 2017 00:20

austinzheng approved these changes Sep 23, 2017

View reviewed changes

tgoyne merged commit 8b87751 into master-3.0 Sep 23, 2017

bmunkholm deleted the tg/primitive-array 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

Add support for arrays of more types #5274

Add support for arrays of more types #5274

tgoyne commented Aug 29, 2017 •

edited

Loading

austinzheng commented Aug 29, 2017

bdash commented Aug 30, 2017

bdash Aug 30, 2017

bdash Aug 30, 2017

bdash Aug 30, 2017

tgoyne Sep 7, 2017

bdash Aug 30, 2017

bdash Aug 30, 2017

bdash Aug 30, 2017

bdash commented Sep 19, 2017 •

edited

Loading

bdash commented Sep 20, 2017

bdash commented Sep 21, 2017

tgoyne Sep 22, 2017

bdash Sep 22, 2017 •

edited

Loading

bdash Sep 22, 2017

bdash commented Sep 23, 2017

tgoyne commented Sep 23, 2017

Add support for arrays of more types #5274

Add support for arrays of more types #5274

Conversation

tgoyne commented Aug 29, 2017 • edited Loading

austinzheng commented Aug 29, 2017

bdash commented Aug 30, 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

bdash commented Sep 19, 2017 • edited Loading

bdash commented Sep 20, 2017

bdash commented Sep 21, 2017

Choose a reason for hiding this comment

bdash Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdash commented Sep 23, 2017

tgoyne commented Sep 23, 2017

tgoyne commented Aug 29, 2017 •

edited

Loading

bdash commented Sep 19, 2017 •

edited

Loading

bdash Sep 22, 2017 •

edited

Loading