diff --git a/CHANGELOG.md b/CHANGELOG.md index 02e3311b..986fd8be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,8 @@ The changelog for `IGListKit`. Also see the [releases](https://github.com/instag - Duplicate view models in `IGListBindingSectionController` gets filtered out. [Weyert de Boer](https://github.com/weyert) [(#916)](https://github.com/Instagram/IGListKit/pull/916) +- Check object type on lookup to prevent crossing types if different objects collide with their identifiers. [Ryan Nystrom](https://github.com/rnystrom) [(#tbd)](https://github.com/Instagram/IGListKit/pull/tbd) + 3.1.1 ----- diff --git a/Source/Common/Internal/IGListArrayUtilsInternal.h b/Source/Common/Internal/IGListArrayUtilsInternal.h index e32b7c30..ab786d69 100644 --- a/Source/Common/Internal/IGListArrayUtilsInternal.h +++ b/Source/Common/Internal/IGListArrayUtilsInternal.h @@ -10,21 +10,24 @@ #ifndef IGListArrayUtilsInternal_h #define IGListArrayUtilsInternal_h +#import + static NSArray *objectsWithDuplicateIdentifiersRemoved(NSArray> *objects) { if (objects == nil) { return nil; } - NSMutableSet *identifiers = [NSMutableSet new]; + NSMapTable *identifierMap = [NSMapTable strongToStrongObjectsMapTable]; NSMutableArray *uniqueObjects = [NSMutableArray new]; for (id object in objects) { id diffIdentifier = [object diffIdentifier]; + id previousObject = [identifierMap objectForKey:diffIdentifier]; if (diffIdentifier != nil - && ![identifiers containsObject:diffIdentifier]) { - [identifiers addObject:diffIdentifier]; + && previousObject == nil) { + [identifierMap setObject:object forKey:diffIdentifier]; [uniqueObjects addObject:object]; } else { - IGLKLog(@"WARNING: Diff identifier %@ for object %@ already appeared in objects array", diffIdentifier, object); + IGFailAssert(@"Duplicate identifier %@ for object %@ with object %@", diffIdentifier, object, previousObject); } } return uniqueObjects; diff --git a/Source/IGListAdapterUpdater.m b/Source/IGListAdapterUpdater.m index b633855d..46578664 100644 --- a/Source/IGListAdapterUpdater.m +++ b/Source/IGListAdapterUpdater.m @@ -384,9 +384,10 @@ void convertReloadToDeleteInsert(NSMutableIndexSet *reloads, #pragma mark - IGListUpdatingDelegate static BOOL IGListIsEqual(const void *a, const void *b, NSUInteger (*size)(const void *item)) { - const id left = (__bridge id)a; - const id right = (__bridge id)b; - return [[left diffIdentifier] isEqual:[right diffIdentifier]]; + const id left = (__bridge id)a; + const id right = (__bridge id)b; + return [left class] == [right class] + && [[left diffIdentifier] isEqual:[right diffIdentifier]]; } // since the diffing algo used in this updater keys items based on their -diffIdentifier, we must use a map table that diff --git a/Source/Internal/IGListDebugger.h b/Source/Internal/IGListDebugger.h index dcd2086e..895f78de 100644 --- a/Source/Internal/IGListDebugger.h +++ b/Source/Internal/IGListDebugger.h @@ -20,6 +20,8 @@ IGLK_SUBCLASSING_RESTRICTED + (NSArray *)adapterDescriptions; ++ (void)clear; + + (NSString *)dump; - (instancetype)init NS_UNAVAILABLE; diff --git a/Source/Internal/IGListDebugger.m b/Source/Internal/IGListDebugger.m index 4368aff5..dfc2d0b8 100644 --- a/Source/Internal/IGListDebugger.m +++ b/Source/Internal/IGListDebugger.m @@ -33,6 +33,10 @@ static NSHashTable *livingAdaptersTable = nil; return descriptions; } ++ (void)clear { + [livingAdaptersTable removeAllObjects]; +} + + (NSString *)dump { return [[self adapterDescriptions] componentsJoinedByString:@"\n"]; } diff --git a/Tests/IGListAdapterE2ETests.m b/Tests/IGListAdapterE2ETests.m index 1f488341..9b6ae0da 100644 --- a/Tests/IGListAdapterE2ETests.m +++ b/Tests/IGListAdapterE2ETests.m @@ -959,33 +959,6 @@ [self waitForExpectationsWithTimeout:30 handler:nil]; } -- (void)test_whenBatchUpdating_withDuplicateIdentifiers_thatHaveDifferentValues_thatCollectionViewWorks { - [self setupWithObjects:@[ - // using string values IGTestDelegateController always returns 1 cell - genTestObject(@1, @"a"), - genTestObject(@2, @"a"), - genTestObject(@3, @"a"), - genTestObject(@4, @"b"), // problem item w/ key 4, value "b" - ]]; - - self.dataSource.objects = @[ - genTestObject(@1, @"a"), - genTestObject(@5, @"a"), - genTestObject(@6, @"a"), - genTestObject(@7, @"a"), - genTestObject(@4, @"a"), // key 4 but value "a", so this needs reloaded - genTestObject(@8, @"a"), - genTestObject(@4, @"b"), // key 4 but value didn't change - ]; - XCTestExpectation *expectation = genExpectation; - - [self.adapter performUpdatesAnimated:YES completion:^(BOOL finished) { - [expectation fulfill]; - }]; - - [self waitForExpectationsWithTimeout:30 handler:nil]; -} - - (void)test_whenPerformingUpdates_withWorkingRange_thatAccessingCellDoesntCrash { [self setupWithObjects:@[ genTestObject(@1, @1), diff --git a/Tests/IGListAdapterStoryboardTests.m b/Tests/IGListAdapterStoryboardTests.m index 306f5d26..26220235 100644 --- a/Tests/IGListAdapterStoryboardTests.m +++ b/Tests/IGListAdapterStoryboardTests.m @@ -65,7 +65,7 @@ static const CGRect kStackTestFrame = (CGRect){{0.0, 0.0}, {100.0, 100.0}}; supplementarySource.collectionContext = self.adapter; supplementarySource.supportedElementKinds = @[UICollectionElementKindSectionHeader]; - IGListSectionController *controller = [self.adapter sectionControllerForObject:@1]; + IGListSectionController *controller = [self.adapter sectionControllerForObject:objects.firstObject]; controller.supplementaryViewSource = supplementarySource; supplementarySource.sectionController = controller; @@ -79,5 +79,4 @@ static const CGRect kStackTestFrame = (CGRect){{0.0, 0.0}, {100.0, 100.0}}; XCTAssertNotNil([self.collectionView supplementaryViewForElementKind:UICollectionElementKindSectionHeader atIndexPath:[NSIndexPath indexPathForItem:0 inSection:0]]); } - @end diff --git a/Tests/IGListAdapterTests.m b/Tests/IGListAdapterTests.m index 8b0025d4..f80655d1 100644 --- a/Tests/IGListAdapterTests.m +++ b/Tests/IGListAdapterTests.m @@ -1527,10 +1527,9 @@ XCTAssertEqual(collectionView1.dataSource, adapter2); } -- (void)test_whenPassingNonUniqueIdentifiers_adapterReloadShouldSkipDuplicates { +- (void)test_whenPassingNonUniqueIdentifiers_adapterReloadShouldThrow { self.dataSource.objects = @[@0, @1, @2, @0]; - [self.adapter reloadDataWithCompletion:nil]; - XCTAssertEqual(self.adapter.objects.count, 3); + XCTAssertThrows([self.adapter reloadDataWithCompletion:nil]); } - (void)test_whenPrefetchingEnabled_thatSetterDisables { diff --git a/Tests/IGListAdapterUpdaterTests.m b/Tests/IGListAdapterUpdaterTests.m index ec0de575..81bad5b2 100644 --- a/Tests/IGListAdapterUpdaterTests.m +++ b/Tests/IGListAdapterUpdaterTests.m @@ -14,6 +14,7 @@ #import "IGListAdapterUpdaterInternal.h" #import "IGListTestUICollectionViewDataSource.h" +#import "IGTestObject.h" #define genExpectation [self expectationWithDescription:NSStringFromSelector(_cmd)] #define waitExpectation [self waitForExpectationsWithTimeout:30 handler:nil] @@ -508,7 +509,7 @@ [mockDelegate verify]; } -- (void)test_ { +- (void)test_whenReloadBatchedWithUpdate_thatCompletionBlockStillExecuted { IGSectionObject *object = [IGSectionObject sectionWithObjects:@[@0, @1, @2]]; self.dataSource.sections = @[object]; @@ -538,7 +539,7 @@ [self waitForExpectationsWithTimeout:30 handler:nil]; } -- (void)test_2 { +- (void)test_whenNotInViewHierarchy_thatUpdatesStillExecuteBlocks { [self.collectionView removeFromSuperview]; IGSectionObject *object = [IGSectionObject sectionWithObjects:@[@0, @1, @2]]; @@ -579,4 +580,21 @@ [self waitForExpectationsWithTimeout:30 handler:nil]; } +- (void)test_whenObjectIdentifiersCollide_withDifferentTypes_thatLookupReturnsNil { + id testObject = [[IGTestObject alloc] initWithKey:@"foo" value:@"bar"]; + id collision = @"foo"; + XCTAssertEqual(collision, [testObject diffIdentifier]); + + IGListAdapterUpdater *updater = [IGListAdapterUpdater new]; + + // mimic internal map setup in IGListAdapter + NSPointerFunctions *keyFunctions = [updater objectLookupPointerFunctions]; + NSPointerFunctions *valueFunctions = [NSPointerFunctions pointerFunctionsWithOptions:NSPointerFunctionsStrongMemory]; + NSMapTable *table = [[NSMapTable alloc] initWithKeyPointerFunctions:keyFunctions valuePointerFunctions:valueFunctions capacity:0]; + + [table setObject:@1 forKey:testObject]; + XCTAssertNotNil([table objectForKey:testObject]); + XCTAssertNil([table objectForKey:collision]); +} + @end diff --git a/Tests/IGListBindingSectionControllerTests.m b/Tests/IGListBindingSectionControllerTests.m index 496e5135..c5e204b7 100644 --- a/Tests/IGListBindingSectionControllerTests.m +++ b/Tests/IGListBindingSectionControllerTests.m @@ -109,7 +109,7 @@ [self setupWithObjects:@[ [[IGTestDiffingObject alloc] initWithKey:@1 objects:@[@7, @"seven"]], ]]; - [self.adapter reloadObjects:@[[[IGTestDiffingObject alloc] initWithKey:@1 objects:@[@"four", @4, @"seven", @7, @"seven", @10]]]]; + [self.adapter reloadObjects:@[[[IGTestDiffingObject alloc] initWithKey:@1 objects:@[@"four", @4, @"seven", @7, @10]]]]; IGTestNumberBindableCell *cell00 = [self cellAtSection:0 item:0]; IGTestStringBindableCell *cell01 = [self cellAtSection:0 item:1]; diff --git a/Tests/IGListDebuggerTests.m b/Tests/IGListDebuggerTests.m index a687a89d..7bb1a40c 100644 --- a/Tests/IGListDebuggerTests.m +++ b/Tests/IGListDebuggerTests.m @@ -24,6 +24,9 @@ @implementation IGListDebuggerTests - (void)test_whenSearchingAdapterInstances_thatCorrectCountReturned { + // purge any leftover tracking + [IGListDebugger clear]; + UIViewController *controller = [UIViewController new]; UICollectionView *collectionView = [[UICollectionView alloc] initWithFrame:CGRectZero collectionViewLayout:[UICollectionViewFlowLayout new]]; IGListAdapterUpdater *updater = [IGListAdapterUpdater new];