Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IGListBindingSectionController updateAnimated have copy BUG #999

Closed
YasinZhou opened this issue Nov 1, 2017 · 3 comments
Closed

IGListBindingSectionController updateAnimated have copy BUG #999

YasinZhou opened this issue Nov 1, 2017 · 3 comments

Comments

@YasinZhou
Copy link

IGListBindingSectionController.m

  • (void)updateAnimated:(BOOL)animated completion:(void (^)(BOOL))completion{}
oldViewModels = self.viewModels;

and late

self.viewModels = [self.dataSource sectionController:self viewModelsForObject:object];
result = IGListDiff(oldViewModels, self.viewModels, IGListDiffEquality);

if self.viewModels is a NSMutableArray and just removeAllObjects and change the all value,
result = IGListDiff(oldViewModels, self.viewModels, IGListDiffEquality); is NOT right
so i think , oldViewModels need copy the self.viewModels([self.viewModels copy]) or self.viewModels's property replaces strong with copy.

and i think another updateAnimated have same BUG maybe

@rnystrom
Copy link
Contributor

rnystrom commented Nov 1, 2017

Thanks for investigating @YasinZhou! We deduplicate the models returned from the data source, so even if a mutable array is returned, it should be totally safe.

You made me double check everything though, and I did come across this, and you're right that if the data source returns an NSMutableArray, they could mutate the array and corrupt the models stored inside the binding section controller.

That'd be a really easy and useful fix if you want to submit a PR!

@rnystrom rnystrom added bug and removed question labels Nov 1, 2017
@rnystrom rnystrom added this to the 3.2.0 milestone Nov 1, 2017
@rnystrom rnystrom removed this from the 3.2.0 milestone Feb 6, 2018
@rnystrom
Copy link
Contributor

rnystrom commented Feb 6, 2018

Good bug, but not world-ending. Leaving as a great starter task for someone!

@KashishGoel
Copy link

@rnystrom I decided to take care of this. However, I'm not sure if this is something we need to write a test for. I don't think we need to write one for this change. Any thoughts on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants