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

Already on GitHub? Sign in to your account

RestKit saves scratch NSManagedObjectContext on POST error #1735

Closed
matej opened this Issue Jan 2, 2014 · 18 comments

Comments

Projects
None yet
3 participants
Contributor

matej commented Jan 2, 2014

When posting a new object I'm creating in a scratch NSManagedObjectContext (child context of moStore.mainQueueManagedObjectContext). If the POST operation succeeds RestKit saves the object's context, which pushes it to the main context and it appears in my UI. So far, so good.

Now, if the POST request fails, RestKit still saves the the object's context here, requiring me to manually delete it. Although it's pretty straightforward to do that, it can cause flicker on the UI side since NSManagedContext change notifications fire twice. It think it would be better if RestKit wouldn't save such scratch contexts if the request fails.

Thoughts?

NSManagedObjectContext *moc = [NSManagedObjectContext MR_contextWithParent:[NSManagedObjectContext MR_defaultContext]];
[moc performBlockAndWait:^{
    Photo *localPhoto = [Photo MR_createInContext:moc];
    [[RKObjectManager sharedManager] postObject:localPhoto path:nil parameters:nil success:nil failure:
     ^(RKObjectRequestOperation *operation, NSError *error) {
         // Could this be avoided?
         [localPhoto MR_deleteEntity];
         [moc MR_saveToPersistentStoreAndWait];
     }];
}];
Owner

segiddins commented Jan 3, 2014

@matej why are you creating the object in a scratch context?

Contributor

matej commented Jan 3, 2014

It's a pretty common pattern with Core Data. The main reasons here are to prevent the object from showing up in the UI before it's completely set up (which only happens after being processed on the back-end side) and to insulate the main object context from changes, if anything goes wrong (which doesn't work here, because of the behavior I described above). Usually I could just throw away the temporary context and all changes would be reverted. There's actually much more going on that what I posted in the snippet above, I'm also changing properties on other - related - objects that also need to be manually reverted if an error occurs.

Owner

blakewatters commented Jan 3, 2014

@matej is correct — we should not be doing the save operation if an error occurred while mapping the response. Need to add test coverage exposing the issue and fix.

@matej You can temporarily workaround this flickering by adding a predicate constraint on your UI. If you are using a fetched results controller, then adding primaryKeyAttribute != nil will prevent it from jumping into focus. Obviously a workaround, but it will prevent the flicker until we fix the bug.

@segiddins you want to try and put a bullet in this one?

Owner

segiddins commented Jan 3, 2014

Owner

segiddins commented Jan 4, 2014

@matej what kind of error is this still happening under?

Contributor

matej commented Jan 4, 2014

I was experimenting with 401 Not Authorized (I commented out the authorization token), but I assume it would be the same for other 4xx and 5xx errors.

Owner

blakewatters commented Jan 4, 2014

401 may be a curveball case because its response must include an authorization challenge and that has implications for NSURLConnection

Owner

segiddins commented Jan 4, 2014

The test I wrote is already passing on a 404. @blakewatters, any way to force a networking error or something?

Owner

blakewatters commented Jan 4, 2014

@matej are you sure that you are actually getting an error response? Previous authorization challenge credentials may be getting reused — I am not certain that just commenting out the auth token will do the trick if you have previously authenticated with the remote service. See NSURLCredential

@segiddins there should be some tests somewhere in there that use HTTP AUTH. A 401 response is the first leg of an HTTP AUTH challenge, which is typically followed by a secondary request that includes the Authorization header. If tests are passing for responses other than 401 then we are probably doing the right thing already. If I remember correctly, in an HTTP AUTH scenario if you decline to respond to the challenge then the secondary request will never be made and you’ll get an NSError back from the connection being failed. Like 302 responses, I believe the NSURL loading system seeks to hide these interactions from you so I am not sure that you can actually return a 401 response that will make it into our callbacks for processing.

This may be a red herring bug report.

Owner

segiddins commented Jan 4, 2014

@blakewatters it also passed with posts_with_invalid.json.

Owner

segiddins commented Jan 4, 2014

- (void)testThatManagedObjectContextIsNotSavedWhenOperationErrors
{
    RKManagedObjectStore *managedObjectStore = [RKTestFactory managedObjectStore];
    RKEntityMapping *entityMapping = [RKEntityMapping mappingForEntityForName:@"Human" inManagedObjectStore:managedObjectStore];
    [entityMapping addAttributeMappingsFromDictionary:@{ @"name": @"name" }];
    RKResponseDescriptor *responseDescriptor = [RKResponseDescriptor responseDescriptorWithMapping:entityMapping method:RKRequestMethodAny pathPattern:nil keyPath:@"human" statusCodes:RKStatusCodeIndexSetForClass(RKStatusCodeClassSuccessful)];

    NSManagedObjectContext *scratchContext = [[NSManagedObjectContext alloc] initWithConcurrencyType:NSPrivateQueueConcurrencyType];
    id mockContext = [OCMockObject partialMockForObject:scratchContext];
    [[mockContext reject] save:((NSError __autoreleasing **)[OCMArg anyPointer])];
    scratchContext.parentContext = [managedObjectStore mainQueueManagedObjectContext];

    RKHuman *human = [NSEntityDescription insertNewObjectForEntityForName:@"Human" inManagedObjectContext:mockContext];
    human.name = @"Blake";

    NSMutableURLRequest *request = [NSMutableURLRequest  requestWithURL:[NSURL URLWithString:@"/posts_with_invalid.json" relativeToURL:[RKTestFactory baseURL]]];
    request.HTTPMethod = @"POST";
    RKManagedObjectRequestOperation *managedObjectRequestOperation = [[RKManagedObjectRequestOperation alloc] initWithRequest:request responseDescriptors:@[ responseDescriptor ]];
    managedObjectRequestOperation.managedObjectContext = mockContext;
    managedObjectRequestOperation.targetObject = human;
    [managedObjectRequestOperation start];
    [managedObjectRequestOperation waitUntilFinished];

    expect(scratchContext.hasChanges).to.beTruthy();
    expect([managedObjectStore.mainQueueManagedObjectContext existingObjectWithID:human.objectID error:nil]).to.beNil();
    expect(scratchContext.insertedObjects).to.haveCountOf(1);
    [mockContext verify];
}
Contributor

matej commented Jan 4, 2014

I'm sure. We're using a custom auth. scheme by including a token as a query parameter. If I omit this, I get an error back from the server. The failure block gets invoked and and error is set.

To be sure I also forced a different error by removing a required property which gave me back a 422 Unprocessable Entity. The context was also saved and I was left with the locally created object.

Owner

segiddins commented Jan 4, 2014

@matej in that case could you give me a hand in crafting a failing unit test?

Contributor

matej commented Jan 4, 2014

Ok, I dug into this and as far as I can tell, this only occurs if I also have a custom error mapping set up (which I have in my app). Otherwise the aforementioned save in the response mapper didFinish block is not reached. Removing the error mapping actually causes my in-app example to work as expected.

Here's a modified test case that should illustrate this (note the added error mapping):

- (void)testThatManagedObjectContextIsNotSavedWhenOperationErrors
{
    RKManagedObjectStore *managedObjectStore = [RKTestFactory managedObjectStore];
    RKEntityMapping *entityMapping = [RKEntityMapping mappingForEntityForName:@"Human" inManagedObjectStore:managedObjectStore];
    [entityMapping addAttributeMappingsFromDictionary:@{ @"name": @"name" }];
    RKResponseDescriptor *responseDescriptor = [RKResponseDescriptor responseDescriptorWithMapping:entityMapping method:RKRequestMethodAny pathPattern:nil keyPath:@"human" statusCodes:RKStatusCodeIndexSetForClass(RKStatusCodeClassSuccessful)];

    RKObjectMapping *errorMapping = [RKObjectMapping mappingForClass:[RKErrorMessage class]];
    [errorMapping addAttributeMappingsFromDictionary:@{ @"self": @"errorMessage" }];
    NSMutableIndexSet *errorCodes = [[NSMutableIndexSet alloc] init];
    [errorCodes addIndexes:RKStatusCodeIndexSetForClass(RKStatusCodeClassClientError)];
    [errorCodes addIndexes:RKStatusCodeIndexSetForClass(RKStatusCodeClassServerError)];
    RKResponseDescriptor *errorDescriptor = [RKResponseDescriptor responseDescriptorWithMapping:errorMapping method:RKRequestMethodAny pathPattern:nil keyPath:@"error" statusCodes:errorCodes];

    NSManagedObjectContext *scratchContext = [[NSManagedObjectContext alloc] initWithConcurrencyType:NSPrivateQueueConcurrencyType];
    id mockContext = [OCMockObject partialMockForObject:scratchContext];
    [[mockContext reject] save:((NSError __autoreleasing **)[OCMArg anyPointer])];
    scratchContext.parentContext = [managedObjectStore mainQueueManagedObjectContext];

    RKHuman *human = [NSEntityDescription insertNewObjectForEntityForName:@"Human" inManagedObjectContext:mockContext];
    human.name = @"Blake";

    NSMutableURLRequest *request = [NSMutableURLRequest  requestWithURL:[NSURL URLWithString:@"/422" relativeToURL:[RKTestFactory baseURL]]];
    request.HTTPMethod = @"POST";
    RKManagedObjectRequestOperation *managedObjectRequestOperation = [[RKManagedObjectRequestOperation alloc] initWithRequest:request responseDescriptors:@[ responseDescriptor, errorDescriptor ]];
    managedObjectRequestOperation.managedObjectContext = mockContext;
    managedObjectRequestOperation.targetObject = human;
    [managedObjectRequestOperation start];
    [managedObjectRequestOperation waitUntilFinished];

    expect(scratchContext.hasChanges).to.beTruthy();
    expect([managedObjectStore.mainQueueManagedObjectContext existingObjectWithID:human.objectID error:nil]).to.beNil();
    expect(scratchContext.insertedObjects).to.haveCountOf(1);
    [mockContext verify];
}

I'm using the following on the server side (edited server.rb):

  post '/422' do
    status 422
    content_type 'application/json'
    { :error => "Unprocessable Entity." }.to_json
  end

Note that the test actually crashes for me, rather than failing, with the following exception message 2014-01-04 18:35:05.030 otest[8404:f03] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'OCPartialMockObject[NSManagedObjectContext]: explicitly disallowed method invoked: save:0xb00925d0'. I'm not sure if I'm doing something wrong here or if this is a OCMock thing, but the save is definitely hit in this case.

Owner

segiddins commented Jan 4, 2014

@matej @blakewatters I think the way around this is to check explicitly whether any of the mapped objects are actually managed

Contributor

matej commented Jan 4, 2014

That sounds reasonable to me. When a successful mapping of an error response is performed, it's most likely that we would just be dealing with transient error objects. If there are however also managed objects in the mix, we probably do want a save.

Owner

blakewatters commented Jan 4, 2014

Ah it makes sense now — its a combination of factors I hadn’t explicitly tested for in the past.

Owner

segiddins commented Jan 4, 2014

I'll bring this home later today

-Samuel E. Giddins

On Jan 4, 2014, at 1:47 PM, Blake Watters notifications@github.com wrote:

Ah it makes sense now — its a combination of factors I hadn’t explicitly tested for in the past.


Reply to this email directly or view it on GitHub.

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