From 296baf5f854f57150ed12ca5bd8d3903db492734 Mon Sep 17 00:00:00 2001 From: Ryan Nystrom Date: Thu, 30 Nov 2017 12:35:41 -0800 Subject: [PATCH] Assert duplicate objects and check object types on map lookup Summary: Followup to make sure that object type mismatches can't happen, even if identifiers collide (which is discouraged). Add assert when duplicates are detected. Patched some unit tests while I'm in here. Reviewed By: calimarkus Differential Revision: D6439094 fbshipit-source-id: d669c01734e5ce9483e851051f548d9960b3087c --- CHANGELOG.md | 2 ++ .../Internal/IGListArrayUtilsInternal.h | 11 +++++--- Source/IGListAdapterUpdater.m | 7 ++--- Source/Internal/IGListDebugger.h | 2 ++ Source/Internal/IGListDebugger.m | 4 +++ Tests/IGListAdapterE2ETests.m | 27 ------------------- Tests/IGListAdapterStoryboardTests.m | 3 +-- Tests/IGListAdapterTests.m | 5 ++-- Tests/IGListAdapterUpdaterTests.m | 22 +++++++++++++-- Tests/IGListBindingSectionControllerTests.m | 2 +- Tests/IGListDebuggerTests.m | 3 +++ 11 files changed, 46 insertions(+), 42 deletions(-) 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];