RKEntityMapping with nested result objects crashes due to infinite recursion #1113

Closed
waltflanagan opened this Issue Dec 27, 2012 · 18 comments

Comments

Projects
None yet
3 participants
@waltflanagan

Recent changes have caused a previously working setup to now crash during visitMapping:atKeyPath:

I've tracked it down to this commit: 9f228e0

We have a nested mapping for reply objects coming back from our services, and replies can have replies, all the way down.

First we create a mapping for our root object (which contains simple property mappings)

   RKEntityMapping *rootMapping = [MappingFactory mappingForRootObject];

   [rootMapping addRelationshipMappingWithSourceKeyPath:@"RootReply" mapping:[MappingFactory replyMapping]];

The Reply mapping is constructed like this:

     RKEntityMapping* replyMapping = [RKEntityMapping mappingForEntityForName:@"Reply" inManagedObjectStore:[RKManagedObjectStore defaultStore]];

      [blurbMapping  addAttributeMappingsFromDictionary:@{
       @"Viewed": @"viewed",
       @"ReplyId":@"replyID",
       }];

      [replyMapping setSourceToDestinationKeyTransformationBlock:^NSString *(RKObjectMapping *mapping, NSString *sourceKey) {
         if ([sourceKey isEqualToString:@"Replies"])
         {
           return @"replies";
         };
         return sourceKey;
      }];

      [replyMapping addRelationshipMappingWithSourceKeyPath:@"Replies" mapping:replyMapping];

This used to work fine and map replies correctly, but now it explodes in a 1000+ call deep stack trace:
Screen Shot 2012-12-27 at 5 04 06 PM

I'm not sure if we're setting up our mappings incorrectly or if this commit broke this functionality.

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Dec 27, 2012

Owner

Yeah so the same thing was just reported under #1112. It looks like I missed testing a case to break recursion on the self-referential case. Will get a fix pushed to development this evening.

Owner

blakewatters commented Dec 27, 2012

Yeah so the same thing was just reported under #1112. It looks like I missed testing a case to break recursion on the self-referential case. Will get a fix pushed to development this evening.

@ghost ghost assigned blakewatters Dec 27, 2012

@waltflanagan

This comment has been minimized.

Show comment Hide comment
@waltflanagan

waltflanagan Dec 27, 2012

Thanks a bunch.

Thanks a bunch.

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Dec 28, 2012

Owner

Okay working on this now. Wish we had caught it before shipping pre5 :-(

I will probably immediately cut a pre6 version tomorrow with the fix. More once I have it pinned down.

Owner

blakewatters commented Dec 28, 2012

Okay working on this now. Wish we had caught it before shipping pre5 :-(

I will probably immediately cut a pre6 version tomorrow with the fix. More once I have it pinned down.

blakewatters added a commit that referenced this issue Dec 28, 2012

Implement Tarjan's algorithm to efficiently traverse the `RKResponseD…
…escriptors` within the `RKManagedObjectRequestOperation` and compute the key paths to all managed objects. refs #1113, refs #1112
@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Dec 28, 2012

Owner

Hi @waltflanagan and @rosem

I have pushed a much more sophisticated algorithm for the mapping traversal to the development branch with a test for the recursive case. All the tests are passing, so I believe its solid.

There may be some secondary work on this tomorrow with fresh eyes. I can probably enable efficient managed object deletion at arbitrary recursive depth now because I can trivially differentiate the key paths into the response JSON that are mapped recursively.

Take a look and let me know if things are looking good for you. Will likely push a very quick pre6 version including this fix since it was a nasty regression.

Owner

blakewatters commented Dec 28, 2012

Hi @waltflanagan and @rosem

I have pushed a much more sophisticated algorithm for the mapping traversal to the development branch with a test for the recursive case. All the tests are passing, so I believe its solid.

There may be some secondary work on this tomorrow with fresh eyes. I can probably enable efficient managed object deletion at arbitrary recursive depth now because I can trivially differentiate the key paths into the response JSON that are mapped recursively.

Take a look and let me know if things are looking good for you. Will likely push a very quick pre6 version including this fix since it was a nasty regression.

@rosem

This comment has been minimized.

Show comment Hide comment
@rosem

rosem Dec 28, 2012

Thanks!

It's not clear to me where you push the updates too — another branch?

Let me know when you push pre6 and I'll grab it.

On Dec 27, 2012, at 11:37 PM, Blake Watters wrote:

Hi @waltflanagan and @rosem

I have pushed a much more sophisticated algorithm for the mapping traversal to the development branch with a test for the recursive case. All the tests are passing, so I believe its solid.

There may be some secondary work on this tomorrow with fresh eyes. I can probably enable efficient managed object deletion at arbitrary recursive depth now because I can trivially differentiate the key paths into the response JSON that are mapped recursively.

Take a look and let me know if things are looking good for you. Will likely push a very quick pre6 version including this fix since it was a nasty regression.


Reply to this email directly or view it on GitHub.

rosem commented Dec 28, 2012

Thanks!

It's not clear to me where you push the updates too — another branch?

Let me know when you push pre6 and I'll grab it.

On Dec 27, 2012, at 11:37 PM, Blake Watters wrote:

Hi @waltflanagan and @rosem

I have pushed a much more sophisticated algorithm for the mapping traversal to the development branch with a test for the recursive case. All the tests are passing, so I believe its solid.

There may be some secondary work on this tomorrow with fresh eyes. I can probably enable efficient managed object deletion at arbitrary recursive depth now because I can trivially differentiate the key paths into the response JSON that are mapped recursively.

Take a look and let me know if things are looking good for you. Will likely push a very quick pre6 version including this fix since it was a nasty regression.


Reply to this email directly or view it on GitHub.

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Dec 28, 2012

Owner

I pushed to the development branch. If you installed as a submodule, you can do:

cd path/to/RestKit/Submodule
git fetch origin
git checkout development
git pull origin development

If installed via CocoaPods, you can add this to the Podfile:

pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :branch => 'development'

I am not cutting a pre6 build until we've gotten some testing on this.

Owner

blakewatters commented Dec 28, 2012

I pushed to the development branch. If you installed as a submodule, you can do:

cd path/to/RestKit/Submodule
git fetch origin
git checkout development
git pull origin development

If installed via CocoaPods, you can add this to the Podfile:

pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :branch => 'development'

I am not cutting a pre6 build until we've gotten some testing on this.

@waltflanagan

This comment has been minimized.

Show comment Hide comment
@waltflanagan

waltflanagan Dec 28, 2012

Just tested it and it's failing with an exception for adding nil to an NSSet in visitMapping:atKeyPath: because the response descriptor was set up with a nil keypath.

Here is the culprit:

- (void)visitMapping:(RKMapping *)mapping atKeyPath:(NSString *)keyPath
{
    NSValue *dictionaryKey = [NSValue valueWithNonretainedObject:mapping];
    if ([self.lowValues objectForKey:dictionaryKey]) {

        /* EXPLOSIONS IF keyPath == nil */
        if ([mapping isKindOfClass:[RKEntityMapping class]]) [self.mutableKeyPaths addObject:keyPath];

       return;
    }
...

Here is our code to set up the descriptor (previous submitted code is related):

   RKResponseDescriptor *rootResponseDescriptor = [RKResponseDescriptor responseDescriptorWithMapping: rootMapping
                                                                                                pathPattern:[PathFactory patternForRoots]
                                                                                                    keyPath:nil
                                                                                                statusCodes:RKStatusCodeIndexSetForClass(RKStatusCodeClassSuccessful)];
   [objectManager addResponseDescriptor:rootResponseDescriptor];

I'll continue to investigate whether inserting NSNull in the case where keypath is nil will work, but wanted to let you know here first.

Just tested it and it's failing with an exception for adding nil to an NSSet in visitMapping:atKeyPath: because the response descriptor was set up with a nil keypath.

Here is the culprit:

- (void)visitMapping:(RKMapping *)mapping atKeyPath:(NSString *)keyPath
{
    NSValue *dictionaryKey = [NSValue valueWithNonretainedObject:mapping];
    if ([self.lowValues objectForKey:dictionaryKey]) {

        /* EXPLOSIONS IF keyPath == nil */
        if ([mapping isKindOfClass:[RKEntityMapping class]]) [self.mutableKeyPaths addObject:keyPath];

       return;
    }
...

Here is our code to set up the descriptor (previous submitted code is related):

   RKResponseDescriptor *rootResponseDescriptor = [RKResponseDescriptor responseDescriptorWithMapping: rootMapping
                                                                                                pathPattern:[PathFactory patternForRoots]
                                                                                                    keyPath:nil
                                                                                                statusCodes:RKStatusCodeIndexSetForClass(RKStatusCodeClassSuccessful)];
   [objectManager addResponseDescriptor:rootResponseDescriptor];

I'll continue to investigate whether inserting NSNull in the case where keypath is nil will work, but wanted to let you know here first.

@waltflanagan

This comment has been minimized.

Show comment Hide comment
@waltflanagan

waltflanagan Dec 28, 2012

Adding either [NSNull null] to the self.mutableKeyPaths set or skipping adding keypath if it's nil both seem to work, but I'm unable to get the unit tests running on my machine to verify that this wouldnt break other tests.

Adding either [NSNull null] to the self.mutableKeyPaths set or skipping adding keypath if it's nil both seem to work, but I'm unable to get the unit tests running on my machine to verify that this wouldnt break other tests.

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Dec 28, 2012

Owner

The right thing to do there is keyPath ?: [NSNull null]

Thanks for the update, will fold that change into place. I need to whip out some additional test cases for this before declaring victory.

Owner

blakewatters commented Dec 28, 2012

The right thing to do there is keyPath ?: [NSNull null]

Thanks for the update, will fold that change into place. I need to whip out some additional test cases for this before declaring victory.

@rosem

This comment has been minimized.

Show comment Hide comment
@rosem

rosem Dec 28, 2012

Thanks! I pulled down the development branch and am now running into this new error:

http://d.pr/i/UMqu

Thanks again,
Mike

On Dec 28, 2012, at 6:57 AM, Blake Watters wrote:

I pushed to the development branch. If you installed as a submodule, you can do:

cd path/to/RestKit/Submodule
git fetch origin
git checkout development
git pull origin development
If installed via CocoaPods, you can add this to the Podfile:

pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :branch => 'development'
I am not cutting a pre6 build until we've gotten some testing on this.


Reply to this email directly or view it on GitHub.

rosem commented Dec 28, 2012

Thanks! I pulled down the development branch and am now running into this new error:

http://d.pr/i/UMqu

Thanks again,
Mike

On Dec 28, 2012, at 6:57 AM, Blake Watters wrote:

I pushed to the development branch. If you installed as a submodule, you can do:

cd path/to/RestKit/Submodule
git fetch origin
git checkout development
git pull origin development
If installed via CocoaPods, you can add this to the Podfile:

pod 'RestKit', :git => 'https://github.com/RestKit/RestKit.git', :branch => 'development'
I am not cutting a pre6 build until we've gotten some testing on this.


Reply to this email directly or view it on GitHub.

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Dec 28, 2012

Owner

Sit tight on this. Another round of significant changes and substantial test coverage additions is coming shortly.

Owner

blakewatters commented Dec 28, 2012

Sit tight on this. Another round of significant changes and substantial test coverage additions is coming shortly.

blakewatters added a commit that referenced this issue Dec 29, 2012

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Dec 29, 2012

Owner

Okay gents, give the new implementation a whirl. This should fix the crash and fully support cyclic mappings, including doing orphaned object deletion on recursive mappings.

Owner

blakewatters commented Dec 29, 2012

Okay gents, give the new implementation a whirl. This should fix the crash and fully support cyclic mappings, including doing orphaned object deletion on recursive mappings.

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Dec 29, 2012

Owner

@waltflanagan please reopen the issue if you are still seeing problems.

Owner

blakewatters commented Dec 29, 2012

@waltflanagan please reopen the issue if you are still seeing problems.

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Jan 1, 2013

Owner

Took another run at this yesterday and the new implementation should be both stable and correct. Will release pre6 shortly.

Owner

blakewatters commented Jan 1, 2013

Took another run at this yesterday and the new implementation should be both stable and correct. Will release pre6 shortly.

@rosem

This comment has been minimized.

Show comment Hide comment
@rosem

rosem Jan 2, 2013

Thanks! I'm on pre4 right now, so let me know when it's up and I'll test it out.

Did you get my tweet about not deleting orphan objects? Ideally I could mark them as "pending" and it wouldn't delete them, others would be removed if they are not on the server when synced. Any thoughts?

Thanks again Blake!

On Jan 1, 2013, at 12:21 PM, Blake Watters wrote:

Took another run at this yesterday and the new implementation should be both stable and correct. Will release pre6 shortly.


Reply to this email directly or view it on GitHub.

rosem commented Jan 2, 2013

Thanks! I'm on pre4 right now, so let me know when it's up and I'll test it out.

Did you get my tweet about not deleting orphan objects? Ideally I could mark them as "pending" and it wouldn't delete them, others would be removed if they are not on the server when synced. Any thoughts?

Thanks again Blake!

On Jan 1, 2013, at 12:21 PM, Blake Watters wrote:

Took another run at this yesterday and the new implementation should be both stable and correct. Will release pre6 shortly.


Reply to this email directly or view it on GitHub.

@waltflanagan

This comment has been minimized.

Show comment Hide comment
@waltflanagan

waltflanagan Jan 2, 2013

Development branch looks good from here blake. Thanks.

Development branch looks good from here blake. Thanks.

@waltflanagan

This comment has been minimized.

Show comment Hide comment
@waltflanagan

waltflanagan Jan 9, 2013

I may have spoken too soon @blakewatters

I missed this issue since I was out for the holidays.

We're seeing a problem where our reply objects from above have another relationship added to them that is not cyclic. This mapping is used for content of replies (in practice objects containing data about audio/video content hosted on the web). Our mapping is constructed as follows:

RKEntityMapping* replyMapping = [RKEntityMapping mappingForEntityForName:@"Reply" inManagedObjectStore:[RKManagedObjectStore defaultStore]];
      ... 
[replyMapping addRelationshipMappingWithSourceKeyPath:@"Replies" mapping:replyMapping];
[replyMapping addRelationshipMappingWithSourceKeyPath:@"Content" mapping:[MappingFactory contentMapping];

We're seeing an NSUnknownKeyException thrown inside RKAddObjectsInGraphWithCyclicKeyPathsToMutableSet at line 236: NSSet *objectsAtCyclicKeyPath = RKFlattenCollectionToSet([graph valueForKeyPath:cyclicKeyPath]);

The cyclicKeyPaths parameter passed into the method contains both 'Replies' and 'Content' but the current graph object in this conetxt is a Content object which does not have any cyclic relationships.

The mapping result in deleteLocalObjectsMissingFromMappingResult contains a mixed set of Content and Reply objects and those are iterated over and valueForKey'd for both keypaths that were assumed to be cyclic. This seems to be happening because the visition is marked as cyclic because Replies is a cyclic path, but the paths that are used come from mapping.relationshipMappings.destinationKeyPath.

Simply put, it looks like when a visitation is marked as cyclic, all relationships are then assumed to be cyclic when traversing the objects in the graph.

For now we're simply wrapping that line in a @try{} @catch{} but we'd like something a bit less messy to get around the issue.

Thanks again.

I may have spoken too soon @blakewatters

I missed this issue since I was out for the holidays.

We're seeing a problem where our reply objects from above have another relationship added to them that is not cyclic. This mapping is used for content of replies (in practice objects containing data about audio/video content hosted on the web). Our mapping is constructed as follows:

RKEntityMapping* replyMapping = [RKEntityMapping mappingForEntityForName:@"Reply" inManagedObjectStore:[RKManagedObjectStore defaultStore]];
      ... 
[replyMapping addRelationshipMappingWithSourceKeyPath:@"Replies" mapping:replyMapping];
[replyMapping addRelationshipMappingWithSourceKeyPath:@"Content" mapping:[MappingFactory contentMapping];

We're seeing an NSUnknownKeyException thrown inside RKAddObjectsInGraphWithCyclicKeyPathsToMutableSet at line 236: NSSet *objectsAtCyclicKeyPath = RKFlattenCollectionToSet([graph valueForKeyPath:cyclicKeyPath]);

The cyclicKeyPaths parameter passed into the method contains both 'Replies' and 'Content' but the current graph object in this conetxt is a Content object which does not have any cyclic relationships.

The mapping result in deleteLocalObjectsMissingFromMappingResult contains a mixed set of Content and Reply objects and those are iterated over and valueForKey'd for both keypaths that were assumed to be cyclic. This seems to be happening because the visition is marked as cyclic because Replies is a cyclic path, but the paths that are used come from mapping.relationshipMappings.destinationKeyPath.

Simply put, it looks like when a visitation is marked as cyclic, all relationships are then assumed to be cyclic when traversing the objects in the graph.

For now we're simply wrapping that line in a @try{} @catch{} but we'd like something a bit less messy to get around the issue.

Thanks again.

@blakewatters

This comment has been minimized.

Show comment Hide comment
@blakewatters

blakewatters Jan 10, 2013

Owner

@waltflanagan I have moved your last comment onto a fresh issue on #1142

Owner

blakewatters commented Jan 10, 2013

@waltflanagan I have moved your last comment onto a fresh issue on #1142

stojanovicigi added a commit to CenterDevice/RestKit that referenced this issue Dec 12, 2013

Implement Tarjan's algorithm to efficiently traverse the `RKResponseD…
…escriptors` within the `RKManagedObjectRequestOperation` and compute the key paths to all managed objects. refs #1113, refs #1112

stojanovicigi added a commit to CenterDevice/RestKit that referenced this issue Dec 12, 2013

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