*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[__NSArrayM insertObject:atIndex:]: object cannot be nil' crash when deleting non-persisted Core Data entities

[fatuhoku]

Synopsis

TPIPT's [updates performBatchUpdatesOnCollectionView:self.collectionView]; crashes because it's confused whenever unpersisted Core Data entities that use a specific identifierKeyPath gets deleted.

Reproduction

Have a view controller hooked up with a collection view, and implement the delegate methods as TLIPT intends.

• Create an entity "FooBar" with MagicalRecord's MR_createEntity. Do not save the entity.
• Delete the same entity with MR_deleteEntity (this essentially calls [context deleteObject:inContext]; behind the scenes. This is bog standard Core Data).

The Crash

2014-07-10 16:13:47.706 FooBarProject[11421:70b] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[__NSArrayM insertObject:atIndex:]: object cannot be nil'
*** First throw call stack:
(
    0   CoreFoundation                      0x018a61e4 __exceptionPreprocess + 180
    1   libobjc.A.dylib                     0x03b408e5 objc_exception_throw + 44
    2   CoreFoundation                      0x01858abc -[__NSArrayM insertObject:atIndex:] + 844
    3   CoreFoundation                      0x01858760 -[__NSArrayM addObject:] + 64
    4   BDDReactiveCocoa                    0x00379655 __69-[TLIndexPathUpdates performBatchUpdatesOnCollectionView:completion:]_block_invoke + 4293
    5   UIKit                               0x031345df -[UICollectionView performBatchUpdates:completion:] + 209
    6   BDDReactiveCocoa                    0x003784a6 -[TLIndexPathUpdates performBatchUpdatesOnCollectionView:completion:] + 1126
    7   BDDReactiveCocoa                    0x0037801c -[TLIndexPathUpdates performBatchUpdatesOnCollectionView:] + 108

The error is uninformative and leaves me with no leads. Perhaps TLIPT can improve on these error messages to help the user get back on their feet.

Offending code

The crash can be traced down into the code below in TLIndexPathUpdates.m:

        if (self.deletedItems.count) {
            NSMutableArray *indexPaths = [[NSMutableArray alloc] init];
            for (id item in self.deletedItems) {
                NSIndexPath *indexPath = [self.oldDataModel indexPathForItem:item]; // *** oldModel returns nil indexPath
                [indexPaths addObject:indexPath];
            }
            [collectionView deleteItemsAtIndexPaths:indexPaths];
        }

Extra information

I've defined my indexPathController like so:

    TLIndexPathController *indexPathController = [[TLIndexPathController alloc] initWithFetchRequest:fetchedRequest
                                          managedObjectContext:[NSManagedObjectContext MR_defaultContext]
                                            sectionNameKeyPath:nil
                                             identifierKeyPath:@"someID"  // someID is a field in FooBar.
                                                     cacheName:nil];

This function is pretty unsafe, because it's making the assumption that self.oldDataModel indexPathForItem will always succeed.

Below is some useful trace info I gathered when investigating the problem.

// Output from controller:willUpdateDataModel:withDataModel:

[DEBUG]   16:38:20.768 | > from model: Model(count=7, items=(
    "<FooBar: 0xb24b690> (entity: FooBar; id: 0xb24ce90 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p6> ; data: {...})",
    "<FooBar: 0xb24b4a0> (entity: FooBar; id: 0xb24bd30 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p8> ; data: {...})",
    "<FooBar: 0xb24b460> (entity: FooBar; id: 0xb24bd40 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p14> ; data: {...})",
    "<FooBar: 0xd0bbe80> (entity: FooBar; id: 0x144cfd20 <x-coredata:///FooBar/tE0DEE8FD-9BF1-4361-83D6-69FD3DA31B663> ; data: <fault>)",
    "<FooBar: 0xd0bcb80> (entity: FooBar; id: 0xd079380 <x-coredata:///FooBar/tE0DEE8FD-9BF1-4361-83D6-69FD3DA31B662> ; data: {...})",
    "<FooBar: 0xb24b3e0> (entity: FooBar; id: 0xb24bd60 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p7> ; data: {...})",
    "<FooBar: 0xb24b3a0> (entity: FooBar; id: 0xb24bd70 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p4> ; data: {...})"
))

2014-07-10 16:38:20.768 BDDReactiveCocoa[11798:70b] inserted: 0, deleted: 1, moved: 3, modified: 0
[DEBUG]   16:38:20.769 | > to model: Model(count=6, items=(
    "<FooBar: 0xb24b690> (entity: FooBar; id: 0xb24ce90 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p6> ; data: {...})",
    "<FooBar: 0xb24b4a0> (entity: FooBar; id: 0xb24bd30 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p8> ; data: {...})",
    "<FooBar: 0xb24b460> (entity: FooBar; id: 0xb24bd40 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p14> ; data: {...})",
    "<FooBar: 0xd0bcb80> (entity: FooBar; id: 0xd079380 <x-coredata:///FooBar/tE0DEE8FD-9BF1-4361-83D6-69FD3DA31B662> ; data: {...})",
    "<FooBar: 0xb24b3e0> (entity: FooBar; id: 0xb24bd60 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p7> ; data: {...})",
    "<FooBar: 0xb24b3a0> (entity: FooBar; id: 0xb24bd70 <x-coredata://55FB8417-7CBB-40CB-8633-CD9AE3447B4E/FooBar/p4> ; data: {...})"
))

It's obvious that in the old model, there's one item that has a data fault. This would cause [self.oldDataModel indexPathForItem:item]; to return nil, because it cannot access the identifier field someID of the deleted object, and therefore cannot look up the correct indexPath and therefore returns nil. This crashes the app.

Workaround

Persist the object (this is not what I want to do every time I create an object!)

[wtmoose]

The normal way to use TLIPT with Core Data is:

1. Don't specify an identifierKeyPath. By default, TLIPT identifies managed object by their objectID.
2. Call [NSManagedObjectContext obtainPermanentIDsForObjects:error:] on newly created objects to prevent the ID from changing when the object is saved.

Can you do that?

[fatuhoku]

Wowie. In short, yes, I could.

If in the eyes of TLIPT what I've done is somehow really stupid (which I don't think is, given that identifierKeyPath: is a part of the TLIPT API) then fair enough, I'll just stick to that opinionated use with Core Data.

The surprising crash behaviour is also seen in #22 — may be the thing that's more effective than investing any to fix the issue is just to provide more useful error messages.

I see TLIPT as a dependable component that connects data to my UI, and so to get crashes from it is really unsettling. When it works though it works beautifully, so thanks for that. =]

If only I had the time to delve into TLIPT's internals and philosophy + a workflow for submitting patches (from Cocoapods it's not trivial) I'd love to contribute back. For now I'm satisfied with the workaround.

That's not to say this issue doesn't need addressing!

[wtmoose]

TLIPT provides multiple ways to identify objects. From the API documentation:

TLIndexPathDataModel needs to be able to identify items in order to keep an internal
mapping between items and index paths and to track items across versions of the data
model. It does not assume that the item itself is a valid identifier (for example,
if the item doesn not implement NSCopying, it cannot be used as a dictionary key).
So the following set of rules are used to locate a valid identifier. Each rule
is tried in turn until a non-nil value is found:

1. If identifierKeyPath is specified (through an appropriate initializer),
   the data model attempts to use the item's value for this key path. If the key
   path is invalid for the given item, the next rule is tried.
2. If the item is an instance of TLIndexPathItem, the value of the identifier
   property is tried.
3. If the item is an instance of NSManagedObject, the objectID property is used.
4. If the item conforms to NSCopying, the item itself is used.
5. If all else fails, the item's memory address is returned as a string.

The default behavior works in for the most common cases, but ultimately, the onus is on you to select an appropriate identifier. If you choose to use identifierKeyPath then you must ensure that the value doesn't change throughout the object's lifecycle.

I'm not sure there's an issue to fix here other than touching up the documentation.

[fatuhoku]

The point is, TLIPT shouldn't crash and burn if those conditions are not satisfied. A more helpful error, rather than documentation, is the fix.

i.e. with respect I don't think leaving codepaths to error states in the form of NSInvalidArgumentException thrown from [__NSArrayM insertObject:atIndex:] inside of TLIPT is a good idea, when all the user really needs to get going again is a pointer back to the documentation, or indeed, #31, this discussion.

#22 is related — the difference there is that it's not a crash that happens in TLIPT, but an eventual consistency problem in CollectionView (just as ugly, really).

For this one, onus may be on the user to select the correct identifier but I think you'll agree it's the onus of TLIPT not do throw NSArray index out of bounds exceptions under any circumstances. That's the responsibility of Foundation ;)