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

Crash in -[MMRecordResponse recordsFromObjectGraph] #91

Open
jonbrooks opened this issue Aug 12, 2014 · 3 comments
Open

Crash in -[MMRecordResponse recordsFromObjectGraph] #91

jonbrooks opened this issue Aug 12, 2014 · 3 comments
Milestone

Comments

@jonbrooks
Copy link

We've encountered a crash in MMRecord in our app that we have yet to reproduce in house. The crash occurred in [MMRecordResponse recordsFromObjectGraph] attempting to add a nil record to the records array in this line:

for (MMRecordProtoRecord *protoRecord in self.objectGraph) {
    [records addObject:protoRecord.record];
}

After some careful analysis of the code path in question, I believe we could only have gotten here if the top level protoRecord wasn't added to a responseGroup. This seems impossible, yet our app got there somehow. I wonder if there should be some error handling in MMRecordResponse to prevent adding a protoRecord to the objectGraph if it did not have a responseGroup.

I will continue to try to reproduce. I would love to hear any thoughs on where things could be going wrong.

@jonbrooks
Copy link
Author

I should mention that it's possible we were having backend server issues during the time we encountered these crashes, but I can't come up with any plausible scenario that would lead to this crash. We're using AFMMRecordResponseSerializer.

@jonbrooks
Copy link
Author

I have diagnosed the problem: We were using a primaryKeyInjectionBlock for our top-level objects. In the crash case, we were returning objects that would create the same primaryKey value. In the injection block case, current code doesn't detect this when building the protoRecords. So duplicate protoRecords were created (and added to the objectGraph), but not added to the responseGroups, which prevent adding of dupes. Then the response groups are iterated filling out the records, leaving these dupes unfilled. Then at the end, we crash when we assume there's a filled record in all of the top level objects.

Here's how I fixed it locally - there's probably a slightly cleaner way - basically use the injectionBlock for looking for the existing object - not just when creating the new object.

Starting with line 224 of MMRecordResponse.m:

id primaryValue = [representation primaryKeyValueFromDictionary:recordResponseObject]; 
//Use the injection block if appropriate 
if (primaryValue == nil) {
    if (self.options.entityPrimaryKeyInjectionBlock != nil) {
        primaryValue = self.options.entityPrimaryKeyInjectionBlock(entity,
                                                                      recordResponseObject,
                                                                      parentProtoRecord);
    }
}

MMRecordProtoRecord *proto = [recordResponseGroup protoRecordForPrimaryKeyValue:primaryValue];

@cnstoll
Copy link
Contributor

cnstoll commented Aug 18, 2014

Sorry for the delay getting back to you Jon. That sounds entirely reasonable to me. The only change I would make along with what you've got there is to make it so that the block would not get called again a few lines below when creating a new proto record. There might need to be a bit of additional refactoring that happens there.

MMRecord 2.0 has already begun development, and there is a branch under the same name. We could make this change as part of that, or you can feel free to submit a PR to the current version if you want to. I'm expecting the 2.0 version to be complete sometime after iOS 8 ships.

Thanks,

  • Conrad

@cnstoll cnstoll added this to the 1.4.2 milestone Aug 18, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants